----- 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. > 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. 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.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, > > -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
