RFR: 8151841: Build needs additional flags to compile with GCC 6
Erik Joelsson
erik.joelsson at oracle.com
Wed Mar 16 09:01:51 UTC 2016
On 2016-03-16 05:25, Andrew Hughes wrote:
> ----- Original Message -----
>> Hello,
>>
>> As representative for the build-infra group creating the new Hotspot
>> build, I appreciate that the changes are made in configure. That will at
>> least automatically force me to merge them correctly when they hit the
>> build-infra forest and will make the merge easier. The idea is to start
>> getting the new Hotspot build in soon after Jigsaw M3 is integrated. If
>> the GCC 6 changes get in soon enough in jdk9/dev I will be able to merge
>> and convert in time.
>>
> When is M3 due to happen? I couldn't see anything regarding the milestones
> on http://openjdk.java.net/projects/jdk9/. Hopefully we can get this change
> in this week.
See http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-March/003877.html
If you get in this week, it should be fine.
>> I like the refactoring of the FLAGS_COMPILER_CHECK_ARGUMENTS. Disabling
>> optimizations that have obviously worked fine for a long time doesn't
>> seem like a good idea though. I would prefer putting a conditional on
>> the GCC version in those cases, but still keep the proposed flag check
>> as well. There should be a toolchain version variable to compare
>> against.
> I agree that will lower the potential impact of this change. I've
> added:
>
> + TOOLCHAIN_CHECK_COMPILER_VERSION(VERSION: 6, IF_AT_LEAST: FLAGS_SETUP_GCC6_COMPILER_FLAGS)
>
> and moved the -fno-lifetime-dse and -fno-delete-null-pointer-checks
> into this new macro, FLAGS_SETUP_GCC6_COMPILER_FLAGS. I'll post the
> revised version once I've done more testing with GCC 6. With GCC 5.3,
> I can confirm the tests and additions are gone with this change.
Nice, I haven't kept up to date with the recent improvements Magnus made
to configure here. That macro made the change very simple.
/Erik
> I would also prefer if you could break the rather long line in
>> hotspot-spec.gmk.in.
>>
> Done. I've also updated its copyright line as requested.
>
>> If you would like to try out the new build in its current state, feel
>> free to clone build-infra/jdk9.
>>
> Thanks. I'll try and take a look.
>
>> /Erik
>>
>> On 2016-03-15 05:18, 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