8360936: Test compiler/onSpinWait/TestOnSpinWaitAArch64.java fails after JDK-8359435 #26072

eastig · 2025-07-01T15:29:10Z

Test compiler/onSpinWait/TestOnSpinWaitAArch64.java needs debug info to identify a position of spin wait instructions in generated code. The test switched to use XX:CompileCommand=print instead of XX:+PrintAssembly to have assembly only for a tested Java method. In release builds XX:+PrintAssembly prints out debug info but XX:CompileCommand=print does not.

This PR reimplements the test to parse instructions and to check them. The test does not rely on debug info anymore.

Tested on Linux and MacOS with and without hsdis:

Fastdebug: test passed
Slowdebug: test passed.
Release: test passed.

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-8360936: Test compiler/onSpinWait/TestOnSpinWaitAArch64.java fails after JDK-8359435 (Bug - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Andrew Haley (@theRealAph - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26072

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…ter JDK-8359435

bridgekeeper · 2025-07-01T15:29:57Z

👋 Welcome back eastigeevich! 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-07-01T15:30:04Z

@eastig 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:

8360936: Test compiler/onSpinWait/TestOnSpinWaitAArch64.java fails after JDK-8359435

Reviewed-by: shade, aph

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

76442f3: 8361494: [IR Framework] Escape too much in replacement of placeholder
445e5ec: 8361342: Shenandoah: Evacuation may assert on invalid mirror object after JDK-8340297
3ffc5b9: 8359344: C2: Malformed control flow after intrinsic bailout
... and 25 more: https://git.openjdk.org/jdk/compare/a41d35073ee6da0dde4dd731c1ab4c25245d075a...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-07-01T15:30:34Z

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

hotspot-compiler

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-07-01T15:33:52Z

Webrevs

shipilev

Looks okay, but I am confused why the test did not fail before JDK-8359435?

shipilev · 2025-07-01T15:36:40Z

test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitAArch64.java

@@ -29,6 +29,7 @@
 *
 * @requires vm.flagless
 * @requires os.arch=="aarch64"
+ * @requires vm.debug==true


Can be just @requires vm.debug.

eastig · 2025-07-01T16:40:27Z

Looks okay, but I am confused why the test did not fail before JDK-8359435?

Just checked. It's not because of JDK-8359435. There were some changes which disabled printing debug info in release build.

eastig · 2025-07-01T16:41:04Z

The test started failing after I had updated my branch.

shipilev · 2025-07-01T17:11:19Z

OK, are you able to bisect which change? This fix to only do debug VM needs to be correctly linked to the actual cause, IMO.

eastig · 2025-07-02T07:37:40Z

I finished bisecting. This is my changes in the test which made it failing:

@@ -56,7 +58,6 @@ public static void main(String[] args) throws Exception {
         command.add("-showversion");
         command.add("-XX:-BackgroundCompilation");
         command.add("-XX:+UnlockDiagnosticVMOptions");
-        command.add("-XX:+PrintAssembly");
         if (compiler.equals("c2")) {
             command.add("-XX:-TieredCompilation");
         } else if (compiler.equals("c1")) {
@@ -69,13 +70,17 @@ public static void main(String[] args) throws Exception {
         command.add("-XX:OnSpinWaitInst=" + spinWaitInst);
         command.add("-XX:OnSpinWaitInstCount=" + spinWaitInstCount);
         command.add("-XX:CompileCommand=compileonly," + Launcher.class.getName() + "::" + "test");
+        command.add("-XX:CompileCommand=print," + Launcher.class.getName() + "::" + "test");
         command.add(Launcher.class.getName());

It looks like XX:+PrintAssembly prints out debug info in release builds but XX:CompileCommand=print does not.
I am switching back to XX:+PrintAssembly.

theRealAph · 2025-07-02T08:03:31Z

It looks like XX:+PrintAssembly prints out debug info in release builds but XX:CompileCommand=print does not. I am switching back to XX:+PrintAssembly.

That's not great. What info do you need, exactly?

eastig · 2025-07-02T08:45:23Z

OK, are you able to bisect which change? This fix to only do debug VM needs to be correctly linked to the actual cause, IMO.

It looks like XX:+PrintAssembly prints out debug info in release builds but XX:CompileCommand=print does not. I am switching back to XX:+PrintAssembly.

That's not great. What info do you need, exactly?

  # {method} {0x0000ffff50400378} 'test' '()V' in 'compiler/onSpinWait/TestOnSpinWaitAArch64$Launcher'
  #           [sp+0x20]  (sp of caller)
  0x0000ffff985731c0: ff83 00d1 | fd7b 01a9 | 2803 0018 | 8923 40b9 | 1f01 09eb

  0x0000ffff985731d4: ;*synchronization entry
                      ; - compiler.onSpinWait.TestOnSpinWaitAArch64$Launcher::test@-1 (line 224)
  0x0000ffff985731d4: 2102 0054 | 1f20 03d5 | 1f20 03d5 | 1f20 03d5 | 1f20 03d5 | 1f20 03d5 | 1f20 03d5

  0x0000ffff985731f0: ;*invokestatic onSpinWait {reexecute=0 rethrow=0 return_oop=0}
                      ; - compiler.onSpinWait.TestOnSpinWaitAArch64$Launcher::test@0 (line 224)
  0x0000ffff985731f0: 1f20 03d5 | fd7b 41a9 | ff83 0091

  0x0000ffff985731fc: ;   {poll_return}
  0x0000ffff985731fc: 8817 40f9 | ff63 28eb | 4800 0054 | c003 5fd6

  0x0000ffff9857320c: ;   {internal_word}
  0x0000ffff9857320c: 88ff ff10 | 88a3 02f9

  0x0000ffff98573214: ;   {runtime_call SafepointBlob}
  0x0000ffff98573214: 5bc3 fe17

  0x0000ffff98573218: ;   {runtime_call Stub::method_entry_barrier}
  0x0000ffff98573218: 0850 96d2 | 480a b3f2 | e8ff dff2 | 0001 3fd6 | ecff ff17

The test searches for - compiler.onSpinWait.TestOnSpinWaitAArch64$Launcher::test@0 and invokestatic onSpinWait. They identify the place where to search instructions.

Assembly from all builds always has {poll_return}. I can use it as a search point.

theRealAph · 2025-07-02T08:49:45Z

The test searches for - compiler.onSpinWait.TestOnSpinWaitAArch64$Launcher::test@0 and invokestatic onSpinWait. They identify the place where to search instructions.

That's not great. C2 is free to move stuff around, so it's not certain this test will keep working. If you just want to make sure that the pattern is used, a block_comment() would be more reliable.

eastig · 2025-07-03T08:14:06Z

I have rewritten the test not to use debug info at all. The test works with instructions instead.

eastig · 2025-07-07T15:05:24Z

@shipilev, @theRealAph
Any comments on the new version?

shipilev · 2025-07-07T15:23:50Z

test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitAArch64.java

+    private static String retInst = "ret";
+    private static String neededAddInst = "addsp,sp,#0x20";
+    private static String neededLdpInst = "ldpx29,x30,[sp,#16]";


Move these default inits down to analyzer.contains("[MachCode]") block, since it looks like it selects between two options based on [MachCode] presence. Something like:

boolean disassembly = analyzer.contains("[MachCode]"); retInst = disassembly ? "ret" : "c0035fd6"; ...

shipilev · 2025-07-07T15:31:49Z

test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitAArch64.java

+          String s = instrReverseIter.previous();
+          instrReverseIter.next();
+            if (instrReverseIter.previous().startsWith(neededAddInst)) {


Multiple issues here:

Confusing: what's the use of s, did you mean to use it for startsWith?

Indenting is off

Overall, I think searching for this multi-instruction stencil gets fairly hairy with iterators. Would a more straight-forward looping work?

int found = 0; for (int c = 0; c < instrs.size() - 2; c++) { if (instrs.get(c).startsWith(spinWaitInst) && instrs.get(c+1).startsWith(neededLdpInst) && instrs.get(c+2).startsWith(neededAddInst)) { found++; } }

shipilev · 2025-07-07T15:45:16Z

Then again, it does not address a central point: the test keeps relying on particular instruction scheduling, which is not reliable. Why do we even need to search for ldp+add anchor? Can we just blindly search for spin-wait-looking instructions? I would expect block_comment to be even more reliable, as @theRealAph suggested.

eastig · 2025-07-07T18:16:03Z

I would expect block_comment to be even more reliable

I have rewrote the test to use block_comment.

theRealAph · 2025-07-08T08:28:33Z

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

@@ -6803,6 +6803,7 @@ void MacroAssembler::verify_cross_modify_fence_not_required() {
 #endif

 void MacroAssembler::spin_wait() {
+  block_comment("spin_wait");


You need something unique. e.g.

Suggested change

block_comment("spin_wait");

block_comment("spin_wait_ohthoo8H");

Erm, I don't see why?

Erm, I don't see why?

To make it unique? I don't see why you don't see that we need to ensure that the string is unique.

@theRealAph, what do you think to have it in the format: spin_wait_%INST-COUNT%_%INST_NAME%?
For example: spin_wait_1_isb

But this is a generic macroAssembler block comment. It does not make sense to me to have a block comment that looks like a memory corrupted string, just to satisfy a single test. There should be a middle-ground here, e.g. spin_wait {, which looks reasonably enough as the block comment, and not anything else. Would also match nicely when we emit the closing } at the end of the instruction block.

I like the idea of having {, }.
IMO this should be informative:

;; spin_wait_3_nop { nop nop nop ;; }

Putting _3_nop might be convenient, but I think that would introduce too much hassle in macroAssembler? Just this looks okay to me:

;; spin_wait { nop nop nop ;; }

@theRealAph, are you okay with the latest variant Aleksey proposes?

But this is a generic macroAssembler block comment. It does not make sense to me to have a block comment that looks like a memory corrupted string, just to satisfy a single test. There should be a middle-ground here, e.g. spin_wait {, which looks reasonably enough as the block comment, and not anything else. Would also match nicely when we emit the closing } at the end of the instruction block.

OK

I update the PR to use spin_wait { ... }

eastig · 2025-07-09T15:51:39Z

Two MacOS tests failed due to time out. This is not related to my change.

shipilev

Good patch. I propose a few cosmetics: 8360936-cosmetics-1.patch.txt -- easier to express them as patch. Untested, see if it makes sense?

eastig · 2025-07-09T19:52:11Z

Good patch. I propose a few cosmetics: 8360936-cosmetics-1.patch.txt -- easier to express them as patch. Untested, see if it makes sense?

Thank you! It make sense.

eastig · 2025-07-09T20:11:01Z

Good patch. I propose a few cosmetics: 8360936-cosmetics-1.patch.txt -- easier to express them as patch. Untested, see if it makes sense?

Thank you! It make sense.

I tested it. It works as expected.

shipilev

Looks good to me, thanks.

shipilev · 2025-07-10T08:41:58Z

test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitAArch64.java

-            if (nextLine.contains(substring)) {
-                return nextLine;
+            String line = iter.next().trim();
+            if (line.startsWith(";;}")) {


Oh, apologies, I made a little mistake here when playing around with trims. Should be:

Suggested change

if (line.startsWith(";;}")) {

if (line.startsWith(";; }")) {

The test probably does not fail because it meets no instructions beyond the spin_wait block. Cleaner to fix it anyway.

I fixed this.
I have also added a specialized lambda function to count expected instructions:

for disassembled code, just check a line.

for hex code, split and count.

Not sure lambdas make this cleaner, TBH.

Ok, the lambda is removed.

… count instructions

eastig · 2025-07-11T15:24:37Z

/integrate

openjdk · 2025-07-11T15:25:25Z

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

76442f3: 8361494: [IR Framework] Escape too much in replacement of placeholder
445e5ec: 8361342: Shenandoah: Evacuation may assert on invalid mirror object after JDK-8340297
3ffc5b9: 8359344: C2: Malformed control flow after intrinsic bailout
... and 25 more: https://git.openjdk.org/jdk/compare/a41d35073ee6da0dde4dd731c1ab4c25245d075a...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-07-11T15:25:38Z

@eastig Pushed as commit a86dd56.

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

8360936: Test compiler/onSpinWait/TestOnSpinWaitAArch64.java fails af…

b2ba0a9

…ter JDK-8359435

openjdk bot added the rfr Pull request is ready for review label Jul 1, 2025

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jul 1, 2025

shipilev reviewed Jul 1, 2025

View reviewed changes

Simplify requirement for debug build

e91036b

Reimplement checking algo without using debug info

0b3320e

shipilev reviewed Jul 7, 2025

View reviewed changes

Implement using block_comment

2a20921

openjdk bot removed the rfr Pull request is ready for review label Jul 7, 2025

Fix whitespace error

e3163c9

openjdk bot added the rfr Pull request is ready for review label Jul 7, 2025

theRealAph reviewed Jul 8, 2025

View reviewed changes

Update spin_wait block comment

6d60fb4

Merge branch 'master' into JDK-8360936

8554242

shipilev reviewed Jul 9, 2025

View reviewed changes

Apply 8360936-cosmetics-1.patch.txt from PR

1b4d81b

shipilev approved these changes Jul 10, 2025

View reviewed changes

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

shipilev approved these changes Jul 10, 2025

View reviewed changes

Fix detection of block comment end; Use specilized lambda function to…

92a20a2

… count instructions

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

Remove lambda

22644d5

shipilev approved these changes Jul 10, 2025

View reviewed changes

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

theRealAph approved these changes Jul 11, 2025

View reviewed changes

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

openjdk bot closed this Jul 11, 2025

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

	block_comment("spin_wait");
	block_comment("spin_wait_ohthoo8H");

8360936: Test compiler/onSpinWait/TestOnSpinWaitAArch64.java fails after JDK-8359435 #26072

8360936: Test compiler/onSpinWait/TestOnSpinWaitAArch64.java fails after JDK-8359435 #26072

Conversation

eastig commented Jul 1, 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 Jul 1, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Jul 1, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eastig commented Jul 1, 2025

Uh oh!

eastig commented Jul 1, 2025

Uh oh!

shipilev commented Jul 1, 2025

Uh oh!

eastig commented Jul 2, 2025

Uh oh!

theRealAph commented Jul 2, 2025

Uh oh!

eastig commented Jul 2, 2025

Uh oh!

theRealAph commented Jul 2, 2025

Uh oh!

eastig commented Jul 3, 2025

Uh oh!

eastig commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shipilev commented Jul 7, 2025

Uh oh!

eastig commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shipilev Jul 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eastig commented Jul 9, 2025

Uh oh!

eastig commented Jul 1, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 1, 2025 •

edited

Loading

mlbridge bot commented Jul 1, 2025 •

edited

Loading

shipilev Jul 8, 2025 •

edited

Loading