jdk8u-dev
jdk8u-dev copied to clipboard
openjdk
8281096: Flags introduced by configure script are not passed to ADLC build
The 8u configure script defines compiler flags in EXTRA_CFLAGS, EXTRA_LDFLAGS and EXTRA_ASFLAGS. Some are added by configure tests, while others are taken directly from corresponding options passed by the user.
8u still use the legacy HotSpot build system which is not fully integrated with the autoconf system. Variables defined by configure thus have to be explicitly passed down to the separate HotSpot build.
ADLC is a tool used at build-time and so the flags it uses don't impact on the end product. So, for a long time, it has been ignoring these flags defined by configure and using just its own minimal set.
However, with newer compilers, this means that the code is compiled to a newer version of the C++ standard, as the default has changed in GCC 6 and later (see JDK-8151841). With the latest versions of GCC (11 and 12), this actually leads to build failures due to the use of 'register' (GCC 11) and the way comments are used (GCC 12) in the code.
We should fix the ADLC build to use the same flags as the rest of the build. The impact should be negligible, given the same flags are already used in the code that is actually shipped.
This does not affect 9+ where HotSpot's build system has been replaced with full integration in the autoconf system.
With this change, 8u can be built with GCC 11 on GNU/Linux. I'd appreciate testing on other platforms, particularly those not covered by GHA (Solaris, AIX - @adamfarley, @sxa & @deepa181 who have provided previous fixes for these platforms)
During testing, we found we could not use the EXTRA_ prefixed variables as they include flags for the target compiler on cross-compilation builds. To fix this, we split out new HOST_ prefixed variables which include all the flags of the EXTRA_ prefixed variables, bar the --sysroot options.
Progress
- [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [ ] JDK-8281096 needs maintainer approval
- [x] Commit message must refer to an issue
Issue
- JDK-8281096: Flags introduced by configure script are not passed to ADLC build (Bug - P4 - Requested)
Reviewers
- Thomas Stuefe (@tstuefe - Reviewer)
- Zdenek Zambersky (@zzambers - Committer)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/357/head:pull/357
$ git checkout pull/357
Update a local copy of the PR:
$ git checkout pull/357
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/357/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 357
View PR using the GUI difftool:
$ git pr show -t 357
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/357.diff
Webrev
:wave: Welcome back andrew! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
Webrevs
- 06: Full - Incremental (febff626)
- 05: Full - Incremental (123e74d7)
- 04: Full - Incremental (4e1e5613)
- 03: Full - Incremental (22f17651)
- 02: Full - Incremental (85a507ca)
- 01: Full - Incremental (d1c15a55)
- 00: Full (c6dbc0b9)
Hi @gnu-andrew , It successfully built on jdk8u-dev on AIX. This is a positive sign that the changes are effective. Please let me know for further assistance. I would be happy to check. Thank You.
@gnu-andrew This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@gnu-andrew This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
/open
@deepa181 thanks for the feedback. It's good to know it doesn't break AIX.
@tstuefe good catch. I need to find out why LFLAGS is missing on Solaris (assuming at the moment it's not used generally) and also check why it seemed to be failing on cross-compiles (we don't use them ourselves so that is untested prior to this)
@gnu-andrew This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@gnu-andrew This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
@gnu-andrew Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.
Looks good and makes sense. Why not passing LFLAGS to solaris?
It seems this matches what Solaris does in vm.make. These new lines are effectively just copied over from vm.make which handles the rest of the build, and for some reason, Solaris does not set LFLAGS for the VM build either.
I think I know what's wrong here now with the cross-compilation builds. The EXTRA_CFLAGS that HotSpot gets end up including the --sysroot options on cross-compile builds, which are really destined for the target compiler, not a build compiler which is compiling a build tool that is going to be run during the build.
I can fix this, but it's going to make this fix a bit more involved than I initially thought.
Mailing list message from Thorsten Glaser on jdk8u-dev:
On Mon, 19 Feb 2024, Andrew John Hughes wrote:
I think I know what's wrong here now with the cross-compilation builds. The `EXTRA_CFLAGS` that HotSpot gets end up including the `--sysroot` options on cross-compile builds, which are really destined for the target compiler, not a build compiler which is compiling a build tool that is going to be run during the build.
Ouch. So no clean distinction between CFLAGS and HOSTCFLAGS?
I can fix this, but it's going to make this fix a bit more involved than I initially thought.
Yocto has an ugly workaround: https://github.com/ostroproject/ostro-os/blob/master/meta-java/recipes-core/openjdk/patches-openjdk-8/openjdk8-fix-adlc-flags.patch
bye, //mirabilos -- Infrastrukturexperte ? Qvest Digital AG Am Dickobskreuz 10, D-53121 Bonn ? https://www.qvest-digital.com/ Telephon +49 228 54881-393 ? Fax: +49 228 54881-235 HRB AG Bonn 18196 ? USt-ID (VAT): DE274355441 Vorstand: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg Vorsitzender Aufsichtsrat: Peter N?then
@gnu-andrew This change now passes all automated pre-integration checks.
After integration, the commit message for the final commit will be:
8281096: Flags introduced by configure script are not passed to ADLC build
Reviewed-by: stuefe, zzambers, sgehwolf
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 27 new commits pushed to the master branch:
- 0e7148dbfab4b01bdcb615eb2e8d217a47d6874e: Merge
- 18b3ca58d72881d3b6a89c6f299e26635fd149c4: 8325600: Better symbol storage
- dfbb2cfbc4887184487206224f0503a7ccd2b3f6: 8324559: Improve 2D image handling
- bffe2842ea81ed73668e72bc1009f21ac4c2023f: 8323231: Improve array management
- 172214bbc8e1a8c9f2ff5e87151afb38d9092902: 8323390: Enhance mask blit functionality
- e81aa94e18397ccd55c39efac857f33088d18bf3: 8322106: Enhance Pack 200 loading
- 42c9f651c9ce12413fc0fd8a83858260bad8c2e8: 8320097: Improve Image transformations
- 45b83c2c56bdd5bb2b66a40fbada147c01928130: 8320548: Improved loop handling
- 3dd1095e9aac868078aaaa8b6d1da51873545a4e: 8319859: Better symbol storage
- 48e19d8467c7cd6f2594093c42f9292df8bf5e96: 8314794: Improve UTF8 String supports
- ... and 17 more: https://git.openjdk.org/jdk8u-dev/compare/b1e2ea81f45d9b144e1de8d16b2b698f1788d4d7...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
@gnu-andrew This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@gnu-andrew Finally got around to looking at this (and this comment will stop the bot trying to close it!) and performed an initial test on Linux/aarch64 before trying elsewhere. I can build ok with GCC 10.3 but not 11.2 due to this error:
/home/sxa/temurin-build/build-farm/workspace/build/src/hotspot/src/share/vm/opto/type.cpp:2556:71: error: 'this' pointer is null [-Werror=nonnull]
Solaris build of this branch using our default Sun Studio compiler seems to build ok (x64 and SPARC) 👍🏻
@gnu-andrew Finally got around to looking at this (and this comment will stop the bot trying to close it!) and performed an initial test on Linux/aarch64 before trying elsewhere. I can build ok with GCC 10.3 but not 11.2 due to this error:
/home/sxa/temurin-build/build-farm/workspace/build/src/hotspot/src/share/vm/opto/type.cpp:2556:71: error: 'this' pointer is null [-Werror=nonnull]
Thanks for verifying this. Once we can get this change in, I'll look and replicating other failures and resolving those. I think the type.cpp one has been raised in a few other places, but I've not seen it myself (maybe it requires warnings as errors)
Mailing list message from Thorsten Glaser on jdk8u-dev:
On Mon, 19 Feb 2024, Andrew John Hughes wrote:
I think I know what's wrong here now with the cross-compilation builds. The
EXTRA_CFLAGSthat HotSpot gets end up including the--sysrootoptions on cross-compile builds, which are really destined for the target compiler, not a build compiler which is compiling a build tool that is going to be run during the build.Ouch. So no clean distinction between CFLAGS and HOSTCFLAGS?
Yes, this is the conclusion I've come to as well.
I can fix this, but it's going to make this fix a bit more involved than I initially thought.
Yocto has an ugly workaround: https://github.com/ostroproject/ostro-os/blob/master/meta-java/recipes-core/openjdk/patches-openjdk-8/openjdk8-fix-adlc-flags.patch
Ah good to see someone else has encountered this. I think we can resolve it in a similar way, but by defining a new flag in configure rather than trying to filter things apart later.
bye, //mirabilos -- Infrastrukturexperte ? Qvest Digital AG Am Dickobskreuz 10, D-53121 Bonn ? https://www.qvest-digital.com/ Telephon +49 228 54881-393 ? Fax: +49 228 54881-235 HRB AG Bonn 18196 ? USt-ID (VAT): DE274355441 Vorstand: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg Vorsitzender Aufsichtsrat: Peter N?then
I've rewritten the patch so it now uses HOST_CFLAGS and HOST_LDFLAGS in the ADLC build. These are created in configure by not including the sysroot arguments.
The below shows the old and new flags in the aarch64 cross-compile, which now passes:
# Retain EXTRA_{CFLAGS,CXXFLAGS,LDFLAGS,ASFLAGS} for the target flags to
# maintain compatibility with the existing Makefiles
EXTRA_CFLAGS= --sysroot="/home/runner/sysroot-arm64/" -fstack-protector $(NO_DELETE_NULL_POINTER_CHECKS_CFLAG) \
$(NO_LIFETIME_DSE_CFLAG) $(CXXSTD_CXXFLAG)
EXTRA_CXXFLAGS= --sysroot="/home/runner/sysroot-arm64/" -fstack-protector
EXTRA_LDFLAGS= --sysroot="/home/runner/sysroot-arm64/" -Wl,-z,relro
EXTRA_ASFLAGS=
# Define an equivalent set for the host flags (i.e. without sysroot options)
HOST_CFLAGS= -fstack-protector $(NO_DELETE_NULL_POINTER_CHECKS_CFLAG) \
$(NO_LIFETIME_DSE_CFLAG) $(CXXSTD_CXXFLAG)
HOST_CXXFLAGS= -fstack-protector
HOST_LDFLAGS= -Wl,-z,relro
HOST_ASFLAGS=
I'll remove the verbose build log now, but leave the printing of the spec files as I think it does no harm and may be useful for future debugging.
The remaining failures are due to download issues (Debian Buster release on some architectures, Windows VS2010) and the cacerts tests, both of which are not caused by this patch. It may be worth trying to bump the cross-compile to a later Debian release in another PR, as Buster is about to go EOL.
I'll remove the verbose build log now, but leave the printing of the spec files as I think it does no harm and may be useful for future debugging.
Actually looks like the main build has had LOG_LEVEL=debug since inception so I'll leave this in for cross-compiles. It makes things easier to debug.
Mostly good. Weird test errors in GHA, though.
Thanks for looking over it again. Which ones do you mean? The download issues and cacert failures aren't related to this change. I seem them on other PRs.
⚠️ @gnu-andrew This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.
/approval request Fixes ADLC build with GCC 11+ by passing through compiler flags used for the rest of the HotSpot build. Later versions of OpenJDK are not affected, due to the HotSpot build being integrated into the new build system there.
Merged with latest jdk8u-dev to bring in @zzambers ' changes to the Windows & alt-Linux builds. All builds now pass and the only test failure seems unrelated (com/sun/jdi/JdbExprTest.sh)
@gnu-andrew I think remaining failure should get fixed by: https://github.com/openjdk/jdk8u-dev/pull/497
@gnu-andrew I think remaining failure should get fixed by: #497
Ah, thanks for spotting that one and getting it sponsored.