8350258: AArch64: Client build fails after JDK-8347917 #23682

dchuyko · 2025-02-18T22:42:18Z

The location for rfp should be set in in the register map. In particular, it wasn't set in frame::sender_for_interpreter_frame() if neither C2 nor JVMCI were included.

COMPILER1_OR_COMPILER2 condition is used instead of COMPILER2_OR_JVMCI, which also covers INCLUDE_JVMCI case.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8350258: AArch64: Client build fails after JDK-8347917 (Bug - P3)

Reviewers

Dean Long (@dean-long - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23682/head:pull/23682
$ git checkout pull/23682

Update a local copy of the PR:
$ git checkout pull/23682
$ git pull https://git.openjdk.org/jdk.git pull/23682/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23682

View PR using the GUI difftool:
$ git pr show -t 23682

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23682.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-18T22:43:09Z

👋 Welcome back dchuyko! 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.

openjdk · 2025-02-18T22:43:17Z

@dchuyko This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8350258: AArch64: Client build fails after JDK-8347917

Reviewed-by: dlong, kvn

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 42 new commits pushed to the master branch:

9d9d7a1: 8349759: Add unit test for CertificateBuilder and SimpleOCSPServer test utilities
b45c32c: 8350287: Cleanup SA's support for CodeBlob subclasses
bd8ad30: 8350285: Shenandoah: Regression caused by ShenandoahLock under extreme contention
24b5573: 8350444: Check for verifer error in StackMapReader::check_offset()
dfcd0df: 8350019: HttpClient: DelegatingExecutor should resort to the fallback executor only on RejectedExecutionException
1b6281d: 8321003: RISC-V: C2 MulReductionVI
c73fead: 8350383: Test: add more test case for string compare (UL case)
16033ea: 8342238: Test javax/crypto/CryptoPermissions/InconsistentEntries.java writes files in tested JDK dir
f979f72: 8337723: Remove redundant tests from com/sun/security/sasl/gsskerb
1eb0db3: 8350151: Support requires property to filter tests incompatible with --enable-preview
... and 32 more: https://git.openjdk.org/jdk/compare/2d03bd007895b139b027947852c8b5ad8eab49b6...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.

openjdk · 2025-02-18T22:43:50Z

@dchuyko The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-02-18T22:47:09Z

Webrevs

01: Full - Incremental (d157893c)
00: Full (8c273575)

dean-long · 2025-02-19T00:46:58Z

src/hotspot/cpu/aarch64/frame_aarch64.cpp

-#if COMPILER2_OR_JVMCI
+#if COMPILER1_OR_COMPILER2
  if (map->update_map()) {
    update_map_with_saved_link(map, (intptr_t**) addr_at(link_offset));


Is it correct that this is only needed when PreserveFramePointer is false, and it's harmless to do when PreserveFramePointer is true?

Yes. I'd say rfp always has a location, but it only can contain oop if PreserveFramePointer is false.
See MacroAssembler::build_frame()/remove_frame()

jdk/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

Line 5710 in 57f4c30

assert(framesize >= 2 * wordSize, "framesize must include space for FP/LR");

and frame::update_map_with_saved_link()

jdk/src/hotspot/cpu/aarch64/frame_aarch64.inline.hpp

Line 451 in 57f4c30

// The interpreter and compiler(s) always save FP in a known

dchuyko · 2025-02-19T07:45:24Z

The original COMPILER2 guard comes from the 'Initial load' commit for x86 https://github.com/openjdk/jdk/blame/bb2c21a0252d12dc9edef3b676a12051caf7643e/hotspot/src/cpu/x86/vm/frame_x86.cpp#L407
In this patch it is just extended to cover C1 for aarch64 (and jmvci is included only if either C1 or C2 is included).

dean-long · 2025-02-19T22:06:51Z

/label hotspot-compiler
/reviewers 2

openjdk · 2025-02-19T22:07:32Z

@dean-long
The hotspot-compiler label was successfully added.

openjdk · 2025-02-19T22:07:56Z

@dean-long
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

dean-long

LGTM

vnkozlov

COMPILER1_OR_COMPILER2 condition is used instead of COMPILER2_OR_JVMCI, which also covers INCLUDE_JVMCI case.

Where you got this? Client VM will have only C1 and your guard will pass.

vnkozlov · 2025-02-19T22:46:02Z

Is it from here?: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvm.cpp#L379

dean-long

I think @vnkozlov is right. I don't see where COMPILER1_OR_COMPILER2 is true for JVMCI. Should we use COMPILER1 || COMPILER2_OR_JVMCI, or remove the #if and instead guard with !PreserveFramePointer?

dchuyko · 2025-02-20T05:53:36Z

Is it from here?: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvm.cpp#L379

Yes, I mean this check.

dchuyko · 2025-02-20T08:52:04Z

remove the #if and instead guard with !PreserveFramePointer?

It doesn't seem necessary to change the current behavior for Int->C2, especially only for a single platform.

vnkozlov · 2025-02-20T22:40:21Z

Is it from here?: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvm.cpp#L379

Yes, I mean this check.

Configure also prevent to to build VM with JVMCI without C1 or C2: jvm-features.m4#L517

vnkozlov · 2025-02-20T22:46:27Z

I was about suggest to add comment to avoid confusion but then I thought what @dean-long suggested is better and don't need comment:

#if defined(COMPILER1) || COMPILER2_OR_JVMCI

We already use such condition: threads.cpp#L727

dchuyko · 2025-02-21T08:48:30Z

OK, changed to a full condition.

vnkozlov

Good.

dchuyko · 2025-02-21T21:43:13Z

Dean, Vladimir, thanks for reviews.

dchuyko · 2025-02-21T21:43:23Z

/integrate

openjdk · 2025-02-21T21:43:55Z

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

825ab20: 8350456: Test javax/crypto/CryptoPermissions/InconsistentEntries.java crashed: EXCEPTION_ACCESS_VIOLATION
9d9d7a1: 8349759: Add unit test for CertificateBuilder and SimpleOCSPServer test utilities
b45c32c: 8350287: Cleanup SA's support for CodeBlob subclasses
bd8ad30: 8350285: Shenandoah: Regression caused by ShenandoahLock under extreme contention
24b5573: 8350444: Check for verifer error in StackMapReader::check_offset()
dfcd0df: 8350019: HttpClient: DelegatingExecutor should resort to the fallback executor only on RejectedExecutionException
1b6281d: 8321003: RISC-V: C2 MulReductionVI
c73fead: 8350383: Test: add more test case for string compare (UL case)
16033ea: 8342238: Test javax/crypto/CryptoPermissions/InconsistentEntries.java writes files in tested JDK dir
f979f72: 8337723: Remove redundant tests from com/sun/security/sasl/gsskerb
... and 33 more: https://git.openjdk.org/jdk/compare/2d03bd007895b139b027947852c8b5ad8eab49b6...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-21T21:44:03Z

@dchuyko Pushed as commit 25322aa.

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

Perform update_map_with_saved_link for C1 as well

8c27357

openjdk bot changed the title ~~JDK-8350258~~ 8350258: AArch64: Client build fails after JDK-8347917 Feb 18, 2025

openjdk bot added the rfr Pull request is ready for review label Feb 18, 2025

openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 18, 2025

dean-long reviewed Feb 19, 2025

View reviewed changes

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 19, 2025

dean-long approved these changes Feb 19, 2025

View reviewed changes

vnkozlov suggested changes Feb 19, 2025

View reviewed changes

dean-long suggested changes Feb 19, 2025

View reviewed changes

Full #if condition

d157893

dean-long approved these changes Feb 21, 2025

View reviewed changes

vnkozlov approved these changes Feb 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 21, 2025

openjdk bot added the integrated Pull request has been integrated label Feb 21, 2025

openjdk bot closed this Feb 21, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 21, 2025

8350258: AArch64: Client build fails after JDK-8347917 #23682

8350258: AArch64: Client build fails after JDK-8347917 #23682

Uh oh!

Conversation

dchuyko commented Feb 18, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 18, 2025

Uh oh!

openjdk bot commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 18, 2025

Uh oh!

mlbridge bot commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dean-long Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

dchuyko Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

dchuyko commented Feb 19, 2025

Uh oh!

dean-long commented Feb 19, 2025

Uh oh!

openjdk bot commented Feb 19, 2025

Uh oh!

openjdk bot commented Feb 19, 2025

Uh oh!

dean-long left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Feb 19, 2025

Uh oh!

dean-long left a comment

Choose a reason for hiding this comment

Uh oh!

dchuyko commented Feb 20, 2025

Uh oh!

dchuyko commented Feb 20, 2025

Uh oh!

vnkozlov commented Feb 20, 2025

Uh oh!

vnkozlov commented Feb 20, 2025

Uh oh!

dchuyko commented Feb 21, 2025

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

dchuyko commented Feb 21, 2025

Uh oh!

dchuyko commented Feb 21, 2025

Uh oh!

openjdk bot commented Feb 21, 2025

Uh oh!

openjdk bot commented Feb 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

dchuyko commented Feb 18, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 18, 2025 •

edited

Loading

mlbridge bot commented Feb 18, 2025 •

edited

Loading

vnkozlov left a comment •

edited

Loading