8351997: AArch64: Interpreter volatile reference stores with G1 are not sequentially consistent

[shipilev]

Fixes AArch64 memory ordering problem. Readily manifests in new jcstress tests; there are sightings of related problems in JSR 133 tests.

The patch is not clean, because JDK-8301996 is in the way, and it is IMO too risky to backport. I picked up the JDK 21u backport version (https://git.openjdk.org/jdk21u-dev/commit/729f10053eab4b8fe8a857e14ce0f96a82c721b8), where I resolved some hunks to match the intended behavior, after checking do_oop_store clobbers the same r3 as in mainline.

Additional testing:

• Linux AArch64 server release, jcstress RefDekker test now passes
• Linux AArch64 server release, jcstress seqcst.volatiles.ref tests now pass
• Linux AArch64 server release, jcstress all tests now pass
• Linux AArch64 server fastdebug, all

---

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
• JDK-8351997 needs maintainer approval

Issue

• JDK-8351997: AArch64: Interpreter volatile reference stores with G1 are not sequentially consistent (Bug - P2 - Approved)

Reviewers

• Erik Österlund (@fisk - Committer)
• Andrew Dinn (@adinn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/3697/head:pull/3697
$ git checkout pull/3697

Update a local copy of the PR:
$ git checkout pull/3697
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/3697/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3697

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/3697.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back shade! 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]

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

8351997: AArch64: Interpreter volatile reference stores with G1 are not sequentially consistent

Reviewed-by: eosterlund, adinn

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

• 2f8944c: 8352946: SEGV_BND signal code of SIGSEGV missing from our signal-code table
• ca47598: 8226919: attach in linux hangs due to permission denied accessing /proc/pid/root
• ffa7dd5: 8299338: AssertionError in ResponseSubscribers$HttpResponseInputStream::onSubscribe
• ... and 1 more: https://git.openjdk.org/jdk17u-dev/compare/ee991b7607b8011599e6d6d0334afb4199f42328...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]

This backport pull request has now been updated with issue from the original commit.

[mlbridge]

Webrevs

• 00: Full (3ba2d271)

[shipilev]

/approval request Same reason as for 21u. Same level of testing is performed; jcstress now passes. Similar risk to 21u: while the patch is relatively new, it is well understood.

[openjdk]

@shipilev
8351997: Approval has already been requested and approved.

[shipilev]

@shipilev 8351997: Approval has already been requested and approved.

Well, that's odd. Looks like @theRealAph approved the backport before I asked for the approval :) Was that intentional, Andrew? I'll put jdk17u-fix-request manually for better records, anyhow.

[theRealAph]

@shipilev 8351997: Approval has already been requested and approved.

Well, that's odd. Looks like @theRealAph approved the backport before I asked for the approval :) Was that intentional, Andrew? I'll put jdk17u-fix-request manually for better records, anyhow.

Oh, sorry. I was going to review it, and then I thought to myself that maintainers are rarer than approvers, so it's a better bottleneck to do the approval. I never thought to check you'd asked. I'm not sure that requiring an ask before approval is granted is more than process for its own sake anyway.

[shipilev]

Oh, sorry. I was going to review it, and then I thought to myself that maintainers are rarer than approvers, so it's a better bottleneck to do the approval. I never thought to check you'd asked.

Yeah, I followed the process: submit the PR, let people look at it while testing is running, wait until testing is complete, use test results as the justification for backport. So there is a natural sequence of events here :)

Anyhow, I need a formal Review in this PR, as Erik's "committer" review formally does not count.

[adinn]

Yep, nod it through . . .

[shipilev]

Thanks, here we go.

/integrate

[openjdk]

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

• 2f8944c: 8352946: SEGV_BND signal code of SIGSEGV missing from our signal-code table
• ca47598: 8226919: attach in linux hangs due to permission denied accessing /proc/pid/root
• ffa7dd5: 8299338: AssertionError in ResponseSubscribers$HttpResponseInputStream::onSubscribe
• ... and 1 more: https://git.openjdk.org/jdk17u-dev/compare/ee991b7607b8011599e6d6d0334afb4199f42328...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit 0e9b1df.

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