8360775: Fix Shenandoah GC test failures when APX is enabled #26009

vamsi-parasa · 2025-06-27T00:02:08Z

This PR fixes the test failures seen in many JTreg tests related to Shenandoah GC (test/hotspot/jtreg/gc/shenandoah/) with UseAPX. The issues were root caused to:

Higher band registers are not saved and restored in Shenandoah load_reference_barrier.
Pusha/Popa implementation using push2p/pop2p does not restore the contents of rax.

Both the issues are fixed in this PR.

Progress

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

Issue

JDK-8360775: Fix Shenandoah GC test failures when APX is enabled (Bug - P3)

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer)
Jatin Bhateja (@jatin-bhateja - Reviewer)
Emanuel Peter (@eme64 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26009

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-27T00:02:58Z

👋 Welcome back sparasa! 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-06-27T00:03:26Z

@vamsi-parasa 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:

8360775: Fix Shenandoah GC test failures when APX is enabled

Reviewed-by: sviswanathan, jbhateja, epeter

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

21f2e9a: 8344332: (bf) Migrate DirectByteBuffer away from jdk.internal.ref.Cleaner
854de8c: 8336147: Clarify CDS documentation about static vs dynamic archive
16af473: 8361115: javax/swing/JComboBox/bug4276920.java unnecessarily throws Error instead of RuntimeException
... and 81 more: https://git.openjdk.org/jdk/compare/20e0055e202e523b40e8b066e2f71c21d8cc5ea9...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@sviswa7, @jatin-bhateja, @eme64) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-06-27T00:03:52Z

@vamsi-parasa The following labels will be automatically applied to this pull request:

hotspot
shenandoah

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

mlbridge · 2025-06-27T16:34:11Z

Webrevs

01: Full - Incremental (de7c373f)
00: Full (2dba6e3f)

jatin-bhateja · 2025-06-30T09:26:17Z

src/hotspot/cpu/x86/assembler_x86.cpp

@@ -15672,32 +15672,41 @@ void Assembler::pusha() { // 64bit
 // The slot for rsp just contains an arbitrary value.
 void Assembler::pusha_uncached() { // 64bit
  if (UseAPX) {
-    // Data being pushed by PUSH2 must be 16B-aligned on the stack, for this push rax upfront


Hi @vamsi-parasa ,

PUSHA / POPA assembler is agnostic to the use of hardcoded registers in calling context, e.g. in following line of code
https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/gc/shenandoah/shenandoahBarrierSetAssembler_x86.cpp#L495

If dst and tmp1 are RAX then we endup currpting it since RAX is used as a scratch register for stack alignment, and in case RAX holds an oop pointer then we may see random crashes. Such idioms are limited to GC barreirs currently, and we have recently fixed one such issue in #25351

While the instruction sequence of PUSHA/ POPA with PPX hints is correct, Do you think for the time being we should limit the scope of this fix to save_machine_state and restor_machine_state routines rather than making generic fix in pusha/popa ?

I have tried it and it's working.

@jatin-bhateja Pusha is not expected to change any registers. The inadvertent change of registers is very hard to debug. So in my thoughts it is better to have a conservative implementation currently which doesn't change RAX register.

Please see the updated code which fixes the issue by restoring the contents of RAX. The tests are passing with this update.

Thanks @vamsi-parasa , yes this looks fine to me now, I had a similar thought to fix it.

https://github.com/openjdk/jdk/pull/25351/files#diff-d5d721ebf93346ba66e81257e4f6c5e6268d59774313c61e97353c0dfbf686a5R93

sviswa7

Looks good to me.

jatin-bhateja

LGTM.

Best Regards,
Jatin

jatin-bhateja · 2025-07-01T06:04:18Z

src/hotspot/cpu/x86/gc/shenandoah/shenandoahBarrierSetAssembler_x86.cpp

+    __ movptr(Address(rsp, (--slot) * wordSize), r16);
+    __ movptr(Address(rsp, (--slot) * wordSize), r17);
+    __ movptr(Address(rsp, (--slot) * wordSize), r18);
+    __ movptr(Address(rsp, (--slot) * wordSize), r19);
+    __ movptr(Address(rsp, (--slot) * wordSize), r20);
+    __ movptr(Address(rsp, (--slot) * wordSize), r21);
+    __ movptr(Address(rsp, (--slot) * wordSize), r22);
+    __ movptr(Address(rsp, (--slot) * wordSize), r23);
+    __ movptr(Address(rsp, (--slot) * wordSize), r24);
+    __ movptr(Address(rsp, (--slot) * wordSize), r25);
+    __ movptr(Address(rsp, (--slot) * wordSize), r26);
+    __ movptr(Address(rsp, (--slot) * wordSize), r27);
+    __ movptr(Address(rsp, (--slot) * wordSize), r28);
+    __ movptr(Address(rsp, (--slot) * wordSize), r29);
+    __ movptr(Address(rsp, (--slot) * wordSize), r30);
+    __ movptr(Address(rsp, (--slot) * wordSize), r31);


I think you should use pushp2 / pop2p for these instructions also , maybe it can be handled along with
#25889

Thanks Jatin (@jatin-bhateja) for the review and approval! This modification will be pursued in another PR (say #25889).

vamsi-parasa · 2025-07-01T21:24:43Z

Hi Emanuel (@eme64),

This PR is also ready for integration. Would it be possible for you to run the tests?

Thanks,
Vamsi

eme64 · 2025-07-02T08:20:55Z

Submitted testing for commit 2 / v01 :)

However: I can only test on SSE/AVX machines, so you will have to make sure it runs fine on APX and other architectures that may be impacted.

vamsi-parasa · 2025-07-02T15:27:02Z

Submitted testing for commit 2 / v01 :)

However: I can only test on SSE/AVX machines, so you will have to make sure it runs fine on APX and other architectures that may be impacted.

Thank you, Emanuel!
Yes, this was tested using Intel SDE to emulate the APX features.

vamsi-parasa · 2025-07-03T15:42:42Z

Submitted testing for commit 2 / v01 :)

Hi Emanuel (@eme64),
If the testing has passed, could you pls let me know?

Thanks,
Vamsi

eme64

Our internal testing passes. Fix looks reasonable. Thanks for the work!

vamsi-parasa · 2025-07-04T06:16:13Z

Our internal testing passes. Fix looks reasonable. Thanks for the work!

Thank you, Emanuel! :)

vamsi-parasa · 2025-07-04T06:16:21Z

/integrate

openjdk · 2025-07-04T06:16:57Z

@vamsi-parasa
Your change (at version de7c373) is now ready to be sponsored by a Committer.

sviswa7 · 2025-07-04T15:07:41Z

/sponsor

openjdk · 2025-07-04T15:09:01Z

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

f153e41: 8361253: CommandLineOptionTest library should report observed values on failure
fba74f7: 8361306: jdk.compiler-gendata needs to depend on java.base-launchers
56ebb8c: 8359110: Log accumulated GC and process CPU time upon VM exit
... and 85 more: https://git.openjdk.org/jdk/compare/20e0055e202e523b40e8b066e2f71c21d8cc5ea9...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-07-04T15:09:16Z

@sviswa7 @vamsi-parasa Pushed as commit 1c56072.

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

8360775: Fix Shenandoah GC test failures when APX is enabled

2dba6e3

vamsi-parasa marked this pull request as draft June 27, 2025 00:02

openjdk bot added hotspot [email protected] shenandoah [email protected] labels Jun 27, 2025

vamsi-parasa marked this pull request as ready for review June 27, 2025 16:29

openjdk bot added the rfr Pull request is ready for review label Jun 27, 2025

jatin-bhateja reviewed Jun 30, 2025

View reviewed changes

restore the orginal contents of rax

de7c373

sviswa7 approved these changes Jul 1, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 1, 2025

jatin-bhateja approved these changes Jul 1, 2025

View reviewed changes

jatin-bhateja reviewed Jul 1, 2025

View reviewed changes

eme64 approved these changes Jul 4, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 4, 2025

openjdk bot added the integrated Pull request has been integrated label Jul 4, 2025

openjdk bot closed this Jul 4, 2025

openjdk bot removed the ready Pull request is ready to be integrated label Jul 4, 2025

openjdk bot removed rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jul 4, 2025

8360775: Fix Shenandoah GC test failures when APX is enabled #26009

8360775: Fix Shenandoah GC test failures when APX is enabled #26009

Conversation

vamsi-parasa commented Jun 27, 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 Jun 27, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Jun 27, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

jatin-bhateja Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

sviswa7 Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

vamsi-parasa Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja Jul 1, 2025 • 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!

sviswa7 left a comment

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja left a comment

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

vamsi-parasa Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

vamsi-parasa commented Jul 1, 2025

Uh oh!

eme64 commented Jul 2, 2025

Uh oh!

vamsi-parasa commented Jul 2, 2025

Uh oh!

vamsi-parasa commented Jul 3, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

vamsi-parasa commented Jul 4, 2025

Uh oh!

vamsi-parasa commented Jul 4, 2025

Uh oh!

openjdk bot commented Jul 4, 2025

Uh oh!

sviswa7 commented Jul 4, 2025

Uh oh!

openjdk bot commented Jul 4, 2025

Uh oh!

openjdk bot commented Jul 4, 2025

Uh oh!

Uh oh!

vamsi-parasa commented Jun 27, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 27, 2025 •

edited

Loading

mlbridge bot commented Jun 27, 2025 •

edited

Loading

jatin-bhateja Jul 1, 2025 •

edited

Loading