On Feb 24 2014, at 06:09 , Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> wrote:
> On 2014-02-21 00:43, Mike Duigou wrote: >> Hello all; >> >> This issue is a followon to JDK-8030350 >> (https://bugs.openjdk.java.net/browse/JDK-8030350) which enhanced the >> compiler warnings used for compiling native code. The proposed changes >> principally impact the Linux platform. >> >> While 8030350 was focused on compiler warnings which did not impact code >> generation, this changeset will, for some configurations, change the native >> code generated and likely change performance. These proposed option changes >> prevent specific types of relocation table, stack and heap memory corruption >> in native code. Preventing these types of memory corruption may be useful >> for finding certain kinds of bugs though and do provide some minimal >> additional protections against malicious attacks. They aren't, by any means, >> a substitute for following appropriate secure coding guidelines. >> >> The rationale behind the implementation is as follows. For release builds >> during the initial phase of JDK 9 I would like to enable only compile time >> checks. This ends up being similar to the warnings in JDK-8030350. These >> options have no runtime impact on footprint or performance and very minimal >> additional compile time cost while providing value. **Release builds are not >> expected to see any performance or footprint change as a result of this >> changeset** >> >> For fast debug builds we can enable linker protections (relro) and static >> compile time bounds checks (FORTIFY_SOURCE=1). FORTIFY_SOURCE=1 might be >> moved to the production builds as well because it has no runtime cost or >> executable size cost. >> >> For slow debug builds we can enable full linker protection (at a potential >> cost in startup time), runtime bounds checks and stack protection >> (FORTIFY_SOURCE=2 -fprotect-stack-all). We will likely enable >> -fprotect-stack-strong when available in GCC 4.9 >> >> The basis for enabling the additional protections in debug builds is that it >> will help us find bugs in our native code and we aren't as concerned in >> debug builds with footprint and performance. Since many developers already >> do their personal builds in fastdebug or slowdebug mode for testing this >> will provide good opportunity to shake out any problems with the options >> while not impacting release builds. Should we find that any of the options >> provide significant value for their cost we can move them to fastdebug or >> release. If any of the options prove too costly they can be demoted or >> removed entirely. >> >> https://bugs.openjdk.java.net/browse/JDK-8032045 >> http://cr.openjdk.java.net/~mduigou/JDK-8032045/2/webrev/ >> >> Additional to enabling the various compiler options I attempted to >> rationalize some of the skew between the various >> hotspot/make/{platform}/makefiles/gcc.make files while avoiding changing >> existing behaviour. I have also introduced the new -Og "optimize for >> debugging" option and there are now an explicit C{XX}_O_FLAG_DEBUG >> definitions to complement the C{XX}_O_FLAG_{DEBUG|NORM|HI|HIGHEST|NONE} >> optimization options. > > Mike, > > I have only looked at toolchain.m4. > > First of all, I like the $C_O_FLAG_DEBUG logic. > > Some questions/comments: > * Have you tested the -qnoopt option on xlc? No, though I did see another version of the same flag in the hotspot makefiles. I tried to use the canonical flag names rather than the shorthand versions. I will find a reference if I can or remove it. > * This code and comment is not really aligned. Is the comment overly broad, > or is it a mistake in C_O_FLAG_HIGHEST? > # Disable optimization > C_O_FLAG_HIGHEST="$C_O_FLAG_NORM" > C_O_FLAG_HI="$C_O_FLAG_DEBUG" It looks like an error. > * Maybe it's worth considering to extract the duplication of > "-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-all --param > ssp-buffer-size=1" into a separate variable, something like > CFLAGS_EXTRA_CHECKS or whatever. The same argument is also valid for the > fastdebug extra switches, I think, even the amount of code duplication is not > as high. Hm. Actually. In fact, from what I can tell this change will add > these extra flags on all compilers, even though they are GCC only (?). So > breaking out these flags into a variable, that can be set to empty if not > using gcc, is probably not just a matter of style but a matter of correctness. Sounds reasonable. > * I'd also have a preference for moving the "compiler supports -Og" check > outside the actual flag setting. Maybe you can do the checking first and set > a variable indicating the availablility of this flag, e.g. > GCC_SUPPORTS_OG_FLAG, and then just check on that. The point here is that > it's hard as it is to see the pattern between different optimization levels > and compilers, but the more the code resembles a simple structured assignment > matrix, the easier it is to see it. With checking code like this in the > midst, it's easier to get lost. Understood. Perhaps do the GCC_SUPPORTS_OG_FLAG in compiler setup?
