----- Original Message -----
On Mar 14, 2016, at 3:17 PM, Andrew Hughes <gnu.and...@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,
