RFR: 8151841: Build needs additional flags to compile with GCC 6
David Holmes
david.holmes at oracle.com
Tue Mar 15 05:10:24 UTC 2016
Just a couple of FYIs:
1. hotspot groups require copyright updates on pushes to hotspot
forests; other groups do not. Hence many non-hotspot sources have out of
date copyrights - these will eventually be fixed en-masse.
2. The CXXFLAGS situation, as alluded, arose out of confusion with the
old CPPFLAGS variable. CPP was intended to mean C pre-processor, but
then it was mistakenly assumed to be C++ compiler versus CFLAGS for the
C-compiler, and that was carried into the CXX name change. Quite a mess
that the new build will hopefully rectify. Hotspot doesn't generally use
a plain C compiler but always a C++ compiler (the ADLC tool may be an
exception there).
3. Given the new build for hotspot will soon be in 9 I think there is
adequate justification to fix this separately in 8u (first if desired).
David
-----
On 15/03/2016 2:18 PM, Andrew Hughes wrote:
> ----- Original Message -----
>>> On Mar 14, 2016, at 3:17 PM, Andrew Hughes <gnu.andrew at redhat.com> wrote:
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8151841
>>> Webrev: http://cr.openjdk.java.net/~andrew/8151841/webrev.01/
>>>
>>> A number of additional flags need to be passed to the C and C++ compiler
>>> for OpenJDK to compile with GCC 6:
>>
>> This might not be the best time to be making interesting changes to
>> the Hotspot build system in order to support a not yet released
>> compiler, since there is a new Hotspot build system coming soon:
>> https://bugs.openjdk.java.net/browse/JDK-8076052,
>> http://openjdk.java.net/jeps/284. But I'll leave that up to the folks
>> in charge of the build infrastructure.
>
> I'm not sure I'd refer to this as 'interesting changes'. GCC 6 is in final
> regression testing at the moment and will be appearing in the upcoming releases
> of a number of GNU/Linux distributions. It's pretty necessary to get this fix
> in now, so that it can also be fixed in 8u and get out into released versions
> which are being packaged for these distributions. It may not be released right
> now, but, by the time it's worked its way through the system, people will already
> be experiencing build failures with GCC 6.
>
> I briefly saw a post about these HotSpot changes when I was about to post this,
> and I was wondering when this was going to be finally cleaned up when I was working
> on the patch. It's good to see it being done. I don't think it changes the need
> for this patch though, as these flags also need to be added to the JDK build.
> At best, it will simplify the HotSpot part of this patch, which at the moment,
> is pretty ugly. Hopefully, this will mean that HotSpot is then actually using
> the C++ flags rather than the C ones! If you'd like some further testing of
> this new build, I'd be happy to take a look.
>
> My primary target for this though is 8u, so I'd like to see this in sooner rather
> than later. If we wait for the HotSpot build system to change, we're going to have
> a lot of broken 8u reports.
>
>>
>> That said, here are some specific comments on the webrev.
>>
>> ------------------------------------------------------------------------------
>> common/autoconf/hotspot-spec.gmk.in
>> Needs copyright update.
>
> It looks like it did years ago. My feelings on these are that they are best
> done in bulk in their own changeset. Including them with other changes makes
> it hard to then backport the patch cleanly. But I can add that if it's really
> necessary.
>
>>
>> ------------------------------------------------------------------------------
>> common/autoconf/flags.m4
>> 631 # These flags are required for GCC 6 builds but may not be available
>> on earlier versions
>> 632 NO_NULL_POINTER_CHECK_CFLAG="-fno-delete-null-pointer-checks"
>>
>> According to gcc documentation, this option goes back at least into
>> the 3.x series. So this seems somewhat overkill.
>
> Possibly. I've tended to err on the side of caution; the option could equally be
> removed or renamed in some future release.
>
>>
>> ------------------------------------------------------------------------------
>> common/autoconf/flags.m4
>> 631 # These flags are required for GCC 6 builds but may not be available
>> on earlier versions
>> ...
>> 636 NO_LIFETIME_DSE_CFLAG="-fno-lifetime-dse"
>>
>> This one does seem to be relatively new; I think it's introduced in
>> gcc4.9. However, there are other places where version
>> conditionalization of options is performed, such as
>> hotspot/make/linux/makefiles/gcc.make, where the addition of some
>> options is dependent on the version. Here it's done based on behavior
>> rather than simply acceptance by the compiler being used. So, for
>> example, -Wuninitialized isn't turned on until gcc4.8, because earlier
>> versions apparently report false positives.
>>
>> Of course, there's the annoying fact that there are multiple gcc.make
>> files that might need to be modified for this. But I'm not sure the
>> simple "the compiler accepts this option" is the right test here.
>
> Yes, there seems to be a lot of logic duplicated in various places. The
> reason we're doing this at the root level is these options are also
> being applied to the JDK build. It seems like this is also more likely
> to survive the HotSpot build refactoring this way.
>
> Do you have an idea as to what the right test might be? I tend to
> find that checking an option is accepted is better than assuming
> it's accepted because we're using version x; changes can occasionally
> be backported to older versions at a later date. I'm not sure
> if that's what you were getting at or not. As this is an optimisation
> being disabled, rather than a warning, the behaviour is not as easily
> testable.
>
> I did consider restricting the additions to GCC >= 6, but my understanding
> is that these optimisations are problematic because of some of the OpenJDK
> code itself, and it's more simple luck that they haven't caused crashes before.
> I'm willing to defer to someone else more familiar with the code on this one
> though, and we can restrict it if it's known to be safe on earlier versions.
>
>>
>> ------------------------------------------------------------------------------
>> common/autoconf/flags.m4
>> 588 elif test "x$TOOLCHAIN_TYPE" = xgcc; then
>> 589 CXXSTD_CXXFLAG="-std=gnu++98"
>> 590 FLAGS_CXX_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [$CXXSTD_CXXFLAG
>> -Werror],
>> 591 IF_FALSE:
>> [CXXSTD_CXXFLAG=""])
>>
>> Checking for acceptance of this option seems like overkill.
>>
>
> Again, possibly. I know it fails with the C compiler. configure is not
> particularly performance critical and it's better to fail early there than
> halfway through the build.
>
>> ------------------------------------------------------------------------------
>> common/autoconf/flags.m4
>>
>> If the new option checks weren't being made (as discussed above), the
>> changes to the checker wouldn't be needed.
>
> Agreed, it could be much simpler. I think the changes are generally beneficial
> though.
>
>>
>> ------------------------------------------------------------------------------
>> common/autoconf/hotspot-spec.gmk.in
>>
>> 113 EXTRA_CFLAGS=@LEGACY_EXTRA_CFLAGS@ $(CFLAGS_CCACHE)
>> $(NO_NULL_POINTER_CHECK_FLAG) $(NO_LIFETIME_DSE_CFLAG) $(CXXSTD_CXXFLAG)
>>
>> It seems strange to only include $(NO_NULL_POINTER_CHECK_FLAG) and
>> $(NO_LIFETIME_DSE_FLAG) in EXTRA_CFLAGS, but not EXTRA_CXXFLAGS.
>>
>> And it seems strange to include $(CXXSTD_CXXFLAG) in EXTRA_CFLAGS at
>> all, rather than in EXTRA_CXXFLAGS.
>
> It is strange, but it's because HotSpot currently completely ignores EXTRA_CXXFLAGS.
> I found this out the hard way :-)
>
> When we were testing this by passing --with-extra-cflags and --with-extra-cxxflags
> to the build, just putting the -std option in cxxflags didn't fix the HotSpot build.
> There's actually no way of doing it via the command-line options such that
> that the JDK C compiler doesn't get given the -std option and issue a warning
> as a result.
>
> hotspot/make/linux/makefiles/rules.make:CC_COMPILE = $(CC) $(CXXFLAGS) $(CFLAGS)
> hotspot/make/linux/makefiles/rules.make:CXX_COMPILE = $(CXX) $(CXXFLAGS) $(CFLAGS)
>
> EXTRA_CFLAGS are added to CFLAGS, but CXXFLAGS is only ever given -D and -I options.
> Essentially it's being used to mean the pre-processor, not the C++ compiler in the
> HotSpot build. The C compiler is hardly used at all in the HotSpot build.
>
>>
>> ------------------------------------------------------------------------------
>>
>>> 2. A number of optimisations in GCC 6 lead to a broken JVM. We need to
>>> add -fno-delete-null-pointer-checks and -fno-lifetime-dse to get a
>>> working JVM.
>>
>> That's very disturbing!
>>
>> -fdelete-null-pointer-checks is the default in much earlier versions
>> than gcc6 (since 4.5?), and much longer than that at higher
>> optimization levels (since somewhere in the 3.x series?). But maybe
>> gcc6 is doing a better job of recognizing the relevant situations. Or
>> maybe there's a bug in gcc6, which is not a released version yet.
>>
>> One specific gcc6 change that could be relevant is:
>> Value range propagation now assumes that the this pointer of C++
>> member functions is non-null. This eliminates common null pointer
>> checks but also breaks some non-conforming code-bases (such as Qt-5,
>> Chromium, KDevelop). As a temporary work-around
>> -fno-delete-null-pointer-checks can be used. Wrong code can be
>> identified by using -fsanitize=undefined.
>>
>> There's also a new warning option in gcc6 that might help find
>> places where -fdelete-null-pointer-checks is leading to problems:
>> -Wnull-dereference.
>>
>> Do you have any information as to where things are being broken by
>> this option? I think I would prefer an attempt to track down this
>> class of problem rather than hiding it via
>> -fno-delete-null-pointer-checks.
>>
>> I don't have any suggestions for why gcc6 might be having problems
>> because of -flifetime-dse, or how to find them. Do you? This seems
>> to be a relatively new option, having been introduced in gcc4.9(?),
>> and seems to have always been on by default since being introduced.
>> Again, this could be a matter of gcc6 doing a better job of
>> recognizing relevant situations, or a bug in that not-yet-released
>> version.
>>
>>
>>
>
> Andrew Haley (CCed) has more details on the need for these options,
> as he diagnosed the reason for the crash, with the help of the GCC
> developers. From what I understand of it, it is a case of more
> aggressive optimisations in the new GCC running into problems with
> code that doesn't strictly conform to the specification and exhibit
> undefined behaviour. The need for -flifetime-dse is covered in
> comment #47 of the bug for this [0]; "an object field [is] being
> accessed before the object is constructed, in breach of
> C++98 [class.cdtor]".
>
> I concur with you that the real solution here is to fix this
> undefined behaviour, but that's quite an involved job (as is
> converting the code to work with C++14) and one which may not be
> able to be backported when done. What I'm
> proposing here is a workaround to keep OpenJDK building in
> the mean-time. Getting a stable OpenJDK build on GCC 6 with
> these optimisations is a more involved task.
>
> We could limit disabling these options to GCC 6 and above.
> With this initial version of the patch, I've generally
> taken the safest route; although builds with earlier versions
> may not exhibit crashes, they still perform optimisations
> where the basis for these optimisations is being broken
> by some of the code in OpenJDK.
>
> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1306558#c47
>
> Thanks for the extensive feedback,
>
More information about the build-dev
mailing list