8281096: Flags introduced by configure script are not passed to ADLC build #357

gnu-andrew · 2023-08-23T03:58:10Z

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

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
JDK-8281096 needs maintainer approval
Commit message must refer to an issue

Issue

JDK-8281096: Flags introduced by configure script are not passed to ADLC build (Bug - P4 - Approved)

Reviewers

Thomas Stuefe (@tstuefe - Reviewer)
Zdenek Zambersky (@zzambers - Committer)
Severin Gehwolf (@jerboaa - Reviewer)

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

Link to Webrev Comment

bridgekeeper · 2023-08-23T04:00:36Z

👋 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.

mlbridge · 2023-08-23T09:22:08Z

Webrevs

tstuefe

Looks good and makes sense. Why not passing LFLAGS to solaris?

deepa181 · 2023-09-04T12:59:57Z

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.

bridgekeeper · 2023-10-02T17:30:32Z

@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!

bridgekeeper · 2023-10-30T18:11:19Z

@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 · 2023-11-20T18:16:02Z

/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)

openjdk · 2023-11-20T18:18:34Z

@gnu-andrew This pull request is now open

bridgekeeper · 2023-12-19T02:53:56Z

@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!

bridgekeeper · 2024-01-16T10:56:38Z

@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 · 2024-01-22T19:27:08Z

/open

openjdk · 2024-01-22T19:27:24Z

@gnu-andrew This pull request is now open

…build

openjdk · 2024-01-24T01:35:51Z

@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.

gnu-andrew · 2024-01-24T01:36:48Z

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.

gnu-andrew · 2024-02-19T16:58:41Z

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.

mlbridge · 2024-02-20T11:11:53Z

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 · 2024-05-23T00:37:06Z

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.

tstuefe

Mostly good. Weird test errors in GHA, though.

tstuefe · 2024-06-01T05:58:30Z

.github/workflows/submit.yml

+        run: |
+          cat build/linux-${{ matrix.gnu-arch }}-hotspot/spec.gmk
+          cat build/linux-${{ matrix.gnu-arch }}-hotspot/hotspot-spec.gmk
+          make CONF_NAME=linux-${{ matrix.gnu-arch }}-hotspot LOG_LEVEL=debug hotspot


What are the cats for? debug output? If yes, still needed?

#357 (comment)

Yeah. They are not needed, but I see no harm in leaving them in. The output is small in comparison to the whole log and it's useful to have if the build does fail. We've had similar in the RPM spec files for a long time.

gnu-andrew · 2024-06-05T00:57:21Z

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.

tstuefe

@gnu-andrew Okay, if you say the GHA failures are unrelated to this patch, this is okay for me.

openjdk · 2024-06-05T06:42:33Z

⚠️ @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.

gnu-andrew · 2024-06-24T13:44:04Z

/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.

openjdk · 2024-06-24T13:44:29Z

@gnu-andrew
8281096: The approval request has been created successfully.

gnu-andrew · 2024-06-24T17:29:09Z

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)

zzambers · 2024-06-25T10:42:40Z

@gnu-andrew I think remaining failure should get fixed by: #497

gnu-andrew · 2024-06-25T11:53:00Z

@gnu-andrew I think remaining failure should get fixed by: #497

Ah, thanks for spotting that one and getting it sponsored.

zzambers · 2024-06-25T17:22:22Z

I have done some additional testing:

I tested this, with GHA workflows updated to ubuntu-22.04 on top (+ fix for jdi test). This is because, I have recently seen build failures on some archs, when I tried to update GHA testing to ubuntu-22.04, while looking at another issue. There are no build failures with this changeset (1 test failure/error on linux x86 seems unrelated).
I have also tried to do build in rhel-9 VM (gcc-11.4.1). It passed ADLC build (unlike unpatched version). Build later failed on another (separate) error in hotspot/src/share/vm/opto/type.cpp (see 8329826: GCC 12 reports some compiler error when building jdk8 #479 (comment)). If I also modified type.cpp (based on that PR), build passed.

Change looks good. (works for me)

bridgekeeper · 2024-07-23T19:18:06Z

@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 · 2024-08-14T15:48:09Z

I have done some additional testing:

* I  [tested](https://github.com/zzambers/jdk8u-dev/actions/runs/9664993178)  this, with GHA workflows updated to ubuntu-22.04 on top (+ [fix for jdi test](https://github.com/openjdk/jdk8u-dev/pull/497)). This is because, I have recently seen build failures on some archs, when I [tried to update GHA testing to ubuntu-22.04](https://github.com/openjdk/jdk8u-dev/pull/491#issue-2281128235), while looking at another issue. There are no build failures with this changeset (1 test failure/error on linux x86 seems unrelated).

* I have also tried to do build in rhel-9 VM (gcc-11.4.1). It passed ADLC build (unlike unpatched version). Build later failed on another (separate) error in `hotspot/src/share/vm/opto/type.cpp` (see [8329826: GCC 12 reports some compiler error when building jdk8 #479 (comment)](https://github.com/openjdk/jdk8u-dev/pull/479#issuecomment-2058078166)). If I also modified `type.cpp` (based on that PR), build passed.

Change looks good. (works for me)

Thanks for testing. Can you give me more details on what kind of build failed in type.cpp (e.g. release/slowdebug/fastdebug)? With just this patch, it builds for me on GCC 13.

gnu-andrew · 2024-08-14T15:48:31Z

Thanks for the approval, @jerboaa
/integrate

openjdk · 2024-08-14T15:50:23Z

Going to push as commit 4106121.
Since your change was applied there have been 31 commits pushed to the master branch:

b4fc8e7: 8337312: [8u] Windows x86 VS2010 build broken by JDK-8320097
09bab6e: 8145919: sun/management/jmxremote/bootstrap/RmiSslBootstrapTest failed with Connection failed for no credentials
d4adbe3: 8152207: Perform array bound checks while getting a length of bytecode instructions
047b08a: 8324723: GHA: Upgrade some actions to avoid deprecated Node 16
0e7148d: Merge
18b3ca5: 8325600: Better symbol storage
dfbb2cf: 8324559: Improve 2D image handling
bffe284: 8323231: Improve array management
172214b: 8323390: Enhance mask blit functionality
e81aa94: 8322106: Enhance Pack 200 loading
... and 21 more: https://git.openjdk.org/jdk8u-dev/compare/b1e2ea81f45d9b144e1de8d16b2b698f1788d4d7...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-14T15:50:32Z

@gnu-andrew Pushed as commit 4106121.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

zzambers · 2024-08-14T16:23:26Z

I have done some additional testing:

* I  [tested](https://github.com/zzambers/jdk8u-dev/actions/runs/9664993178)  this, with GHA workflows updated to ubuntu-22.04 on top (+ [fix for jdi test](https://github.com/openjdk/jdk8u-dev/pull/497)). This is because, I have recently seen build failures on some archs, when I [tried to update GHA testing to ubuntu-22.04](https://github.com/openjdk/jdk8u-dev/pull/491#issue-2281128235), while looking at another issue. There are no build failures with this changeset (1 test failure/error on linux x86 seems unrelated).

* I have also tried to do build in rhel-9 VM (gcc-11.4.1). It passed ADLC build (unlike unpatched version). Build later failed on another (separate) error in `hotspot/src/share/vm/opto/type.cpp` (see [8329826: GCC 12 reports some compiler error when building jdk8 #479 (comment)](https://github.com/openjdk/jdk8u-dev/pull/479#issuecomment-2058078166)). If I also modified `type.cpp` (based on that PR), build passed.

Change looks good. (works for me)

Thanks for testing. Can you give me more details on what kind of build failed in type.cpp (e.g. release/slowdebug/fastdebug)? With just this patch, it builds for me on GCC 13.

NP :)
I can still reproduce issue with type.cpp on rhel-9, with just most default build (just boot jdk set). Gcc version is 11.4.1 (system one).

Errors look following way (see jdk8-type.cpp-error.log for full configure/make log):

In constructor ‘TypeOopPtr::TypeOopPtr(Type::TYPES, TypePtr::PTR, ciKlass*, bool, ciObject*, int, int, const TypeOopPtr*, int)’,
    inlined from ‘TypeAryPtr::TypeAryPtr(TypePtr::PTR, ciObject*, const TypeAry*, ciKlass*, bool, int, int, bool, const TypeOopPtr*, int)’ at /home/tester/jdk8u-dev/hotspot/src/share/vm/opto/type.hpp:1094:39,
    inlined from ‘static const TypeAryPtr* TypeAryPtr::make(TypePtr::PTR, const TypeAry*, ciKlass*, bool, int, int, const TypeOopPtr*, int)’ at /home/tester/jdk8u-dev/hotspot/src/share/vm/opto/type.cpp:3745:115:
/home/tester/jdk8u-dev/hotspot/src/share/vm/opto/type.cpp:2556:71: error: ‘this’ pointer is null [-Werror=nonnull]
 2556 |           ciInstanceKlass* k = o->as_instance()->java_lang_Class_klass()->as_instance_klass();
      |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
In file included from /home/tester/jdk8u-dev/hotspot/src/share/vm/ci/ciCallSite.hpp:28,
                 from /home/tester/jdk8u-dev/hotspot/src/share/vm/code/dependencies.hpp:28,
                 from /home/tester/jdk8u-dev/hotspot/src/share/vm/ci/ciEnv.hpp:32,
                 from /home/tester/jdk8u-dev/hotspot/src/share/vm/ci/ciUtilities.hpp:28,
                 from /home/tester/jdk8u-dev/hotspot/src/share/vm/ci/ciNullObject.hpp:30,
                 from /home/tester/jdk8u-dev/hotspot/src/share/vm/ci/ciConstant.hpp:29,
                 from /home/tester/jdk8u-dev/hotspot/src/share/vm/ci/ciArray.hpp:29,
                 from /home/tester/jdk8u-dev/hotspot/src/share/vm/precompiled/precompiled.hpp:33:
/home/tester/jdk8u-dev/hotspot/src/share/vm/ci/ciInstance.hpp: In function ‘static const TypeAryPtr* TypeAryPtr::make(TypePtr::PTR, const TypeAry*, ciKlass*, bool, int, int, const TypeOopPtr*, int)’:
/home/tester/jdk8u-dev/hotspot/src/share/vm/ci/ciInstance.hpp:68:12: note: in a call to non-static member function ‘ciKlass* ciInstance::java_lang_Class_klass()’
   68 |   ciKlass* java_lang_Class_klass();
      |            ^~~~~~~~~~~~~~~~~~~~~

sxa · 2024-08-16T15:17:49Z

I can still reproduce issue with type.cpp on rhel-9, with just most default build (just boot jdk set). Gcc version is 11.4.1 (system one).

@zzambers Looks similar to what I saw with GCC 11.2/aarch64
Ref: adoptium/temurin-build#3758 (comment) which links to the full log in our jenkins instance.

Based on your earlier comment do we need to re-ignite the other PR which has been closed due to inactivity and try and get that in too?

zzambers · 2024-08-19T10:32:01Z

@gnu-andrew Actually I have found that that there are backports to fix type.cpp (they also include tests):
#536
and this one (see discussion in higher PR):
#552
(so compiler warnings seem to be symptom of real issue)

Issue, I referenced earlier actually seems like duplicate:
#479
(I commented there)

openjdk bot added the rfr Pull request is ready for review label Aug 23, 2023

tstuefe reviewed Aug 25, 2023

View reviewed changes

gnu-andrew mentioned this pull request Sep 1, 2023

8315506: C99 compatibility issue in LinuxNativeDispatcher #362

Closed

3 tasks

bridgekeeper bot closed this Oct 30, 2023

openjdk bot reopened this Nov 20, 2023

bridgekeeper bot closed this Jan 16, 2024

openjdk bot reopened this Jan 22, 2024

8281096: Flags introduced by configure script are not passed to ADLC …

0f50681

…build

gnu-andrew force-pushed the JDK-8281096 branch from c6dbc0b to 0f50681 Compare January 24, 2024 01:34

Temporarily enable debug logging for cross-compile build

d1c15a5

gnu-andrew mentioned this pull request Feb 16, 2024

JDK-8324530: Build error with gcc 10 #424

Closed

4 tasks

gnu-andrew added 4 commits February 17, 2024 18:05

Print the spec.gmk and hotspot-spec.gmk files

20afddf

Merge remote-tracking branch 'dev/master' into JDK-8281096

85a507c

Fix whitespace in yaml file

22f1765

Print spec files in right workflow and use path to build directory

4e1e561

zzambers mentioned this pull request May 27, 2024

8207011: Remove uses of the register storage class specifier #508

Closed

4 tasks

tstuefe reviewed Jun 1, 2024

View reviewed changes

tstuefe approved these changes Jun 5, 2024

View reviewed changes

openjdk bot added the approval label Jun 24, 2024

Merge remote-tracking branch 'dev/master' into JDK-8281096

febff62

zzambers approved these changes Jun 25, 2024

View reviewed changes

jerboaa approved these changes Aug 13, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed approval labels Aug 13, 2024

openjdk bot added the integrated Pull request has been integrated label Aug 14, 2024

openjdk bot closed this Aug 14, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 14, 2024

8281096: Flags introduced by configure script are not passed to ADLC build #357

8281096: Flags introduced by configure script are not passed to ADLC build #357

Conversation

gnu-andrew commented Aug 23, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 23, 2023

mlbridge bot commented Aug 23, 2023 • edited Loading

Webrevs

tstuefe left a comment

Choose a reason for hiding this comment

deepa181 commented Sep 4, 2023

bridgekeeper bot commented Oct 2, 2023

bridgekeeper bot commented Oct 30, 2023

gnu-andrew commented Nov 20, 2023

openjdk bot commented Nov 20, 2023

bridgekeeper bot commented Dec 19, 2023

bridgekeeper bot commented Jan 16, 2024

gnu-andrew commented Jan 22, 2024

openjdk bot commented Jan 22, 2024

openjdk bot commented Jan 24, 2024

gnu-andrew commented Jan 24, 2024

gnu-andrew commented Feb 19, 2024

mlbridge bot commented Feb 20, 2024

gnu-andrew commented May 23, 2024

tstuefe left a comment • edited Loading

Choose a reason for hiding this comment

tstuefe Jun 1, 2024

Choose a reason for hiding this comment

gnu-andrew Jun 5, 2024

Choose a reason for hiding this comment

gnu-andrew commented Jun 5, 2024

tstuefe left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 5, 2024

gnu-andrew commented Jun 24, 2024

openjdk bot commented Jun 24, 2024

gnu-andrew commented Jun 24, 2024

zzambers commented Jun 25, 2024

gnu-andrew commented Jun 25, 2024

zzambers commented Jun 25, 2024

bridgekeeper bot commented Jul 23, 2024

gnu-andrew commented Aug 14, 2024

gnu-andrew commented Aug 14, 2024

openjdk bot commented Aug 14, 2024

openjdk bot commented Aug 14, 2024

zzambers commented Aug 14, 2024 • edited Loading

sxa commented Aug 16, 2024 • edited Loading

zzambers commented Aug 19, 2024 • edited Loading

gnu-andrew commented Aug 23, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 23, 2023 •

edited

Loading

tstuefe left a comment •

edited

Loading

zzambers commented Aug 14, 2024 •

edited

Loading

sxa commented Aug 16, 2024 •

edited

Loading

zzambers commented Aug 19, 2024 •

edited

Loading