8316566: RISC-V: Zero extended narrow oop passed to Atomic::cmpxchg

[robehn]

Hi, please consider!

There is bug in gcc < 12 where __synch_synchronize() in some corner-cases don't enforce the compiler barrier.
This causes some code to be placed after the __synch_synchronize(), and in this case causing a word to to be not be sign extended as a collateral issue of the bug.
You can see the 'bad' assembly in JBS, where a branch is moved over the compiler barrier.

Trying to get information from gcc folks.

It seems like either adding a extra compiler barrier, or use __atomic_thread_fence(__ATOMIC_SEQ_CST) fixes it.

Tested https://bugs.openjdk.org/browse/JDK-8316186 with this fix.
Manually verified assembly, with this fix we generate the same as gcc 12.

---

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-8316566: RISC-V: Zero extended narrow oop passed to Atomic::cmpxchg (Bug - P4)

Reviewers

• Ludovic Henry (@luhenry - Committer)
• Fei Yang (@RealFYang - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15917

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

• hotspot-runtime

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]

Webrevs

• 00: Full (0ccbded8)

[robehn]

Thank you @luhenry !

[RealFYang]

Good catch! But seems to me that it is not necessarily be a missing compiler barrier issue here.
The 'bne' branch instruction is supposed to be there in the critical section which should be similar as our inline asm [1].
I guess there might be some other gcc optimization bug which erroneously deleted the necessary 32-bit sign-extension instruction.
So better to hear what the gcc folks say I think.

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/linux_riscv/atomic_linux_riscv.hpp#L116

[robehn]

   0x00000040034f089a <+94>: ld a4,0(a1)
   0x00000040034f089c <+96>: lw a2,8(a1)
   0x00000040034f089e <+98>: fence 0x00000040034f089e <+98>: fence
   0x00000040034f08a2 <+102>: sub a5,s6,a4
   0x00000040034f08a6 <+106>: srl a5,a5,a2
   0x00000040034f08aa <+110>: sext.w a5,a5
   0x00000040034f08ac <+112>: beqz s1,0x40034f0a0c <_ZN14AccessInternal19PostRuntimeDispatchIN20ShenandoahBarrierSet13AccessBarrierILm402470ES1_EELNS_11BarrierTypeE5ELm402470EE18oop_access_barrierEP7oopDesclS7_S7_+464>
   0x00000040034f08b0 <+116>: sub a4,s1,a4
   0x00000040034f08b4 <+120>: srl a4,a4,a2
=> 0x00000040034f08b8 <+124>: lr.w a2,(s2)
   0x00000040034f08bc <+128>: bne a2,a4,0x40034f08c6 <_ZN14AccessInternal19PostRuntimeDispatchIN20ShenandoahBarrierSet13AccessBarrierILm402470ES1_EELNS_11BarrierTypeE5ELm402470EE18oop_access_barrierEP7oopDesclS7_S7_+138>
   0x00000040034f08c0 <+132>: sc.w a0,a5,(s2)
   0x00000040034f08c4 <+136>: bnez a0,0x40034f08b8 <_ZN14AccessInternal19PostRuntimeDispatchIN20ShenandoahBarrierSet13AccessBarrierILm402470ES1_EELNS_11BarrierTypeE5ELm402470EE18oop_access_barrierEP7oopDesclS7_S7_+124>
   0x00000040034f08c6 <+138>: sext.w a5,a2
   0x00000040034f08ca <+142>: fence

Here there are three branches, so compiler moved unexpectedly some the oop access barrier code over compiler barrier.
I think this move causes collateral bugs.

[RealFYang]

OK. I think this is a good workaround for now. It achieves the same functionality with the same cost. Thanks for looking into this issue.

[openjdk]

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

8316566: RISC-V: Zero extended narrow oop passed to Atomic::cmpxchg

Reviewed-by: luhenry, fyang

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

• fee9d33: 8293176: SSLEngine handshaker does not send an alert after a bad parameters
• fd52be2: 8316895: SeenThread::print_action_queue called on a null pointer
• e39197e: 8316933: RISC-V: compiler/vectorapi/VectorCastShape128Test.java fails when using RVV
• b659e03: 8316897: tools/jpackage/junit tests fail on AIX after JDK-8316547
• 83806ab: 6450193: After the first Serialization, JTableHeader does not uninstall its UI
• 2f311d5: 8316211: Open source several manual applet tests
• 65227a3: 8316389: Open source few AWT applet tests
• 788e6e1: 8316218: Open some swing tests 5
• ee9776f: 8304839: Move TestScaffold.main() to the separate class DebugeeWrapper
• 36ac839: 8073061: (fs) Files.copy(foo, bar, REPLACE_EXISTING) deletes bar even if foo is not readable
• ... and 6 more: https://git.openjdk.org/jdk/compare/52983ed529182901db4e33857bfeab2727e235df...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.

[robehn]

Thanks @RealFYang !

I'll stall a day or two and see if compiler can give some more hints before push.

[robehn]

The bug is not known, no new insights. I'm going to report it upstreams.

I'll back port this to 21 and 17, since we don't know if there are other cases when we use FULL_MEM_BARRIER and compiler does this.

/integrate

[openjdk]

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

• 42924ed: 8316946: jtreg failure handler pages are mislabelling the jcmd/thread/dump_to_file results.
• 384d2ea: 8316125: Windows call_stub unnecessarily saves xmm16-31 when UseAVX>=3
• 84390dd: 8316658: serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java fails intermittently
• 52073b2: 8316994: Avoid modifying ClassLoader and Module objects during -Xshare:dump
• 83c0e45: 8306630: Bump minimum boot jdk to JDK 21
• d3a79b5: 8316371: Open some swing tests 6
• 750da00: 8316743: RISC-V: Change UseVectorizedMismatchIntrinsic option result to warning
• b8cec48: 8315848: G1: Rename rs_ prefix to card_rs in analytics
• 5350fd6: 8299560: Assertion failed: currentQueryIndex >= 0 && currentQueryIndex < numberOfJavaProcessesAtInitialization
• edcc559: 8316661: CompilerThread leaks CodeBlob memory when dynamically stopping compiler thread in non-product
• ... and 22 more: https://git.openjdk.org/jdk/compare/52983ed529182901db4e33857bfeab2727e235df...master

Your commit was automatically rebased without conflicts.

[openjdk]

@robehn Pushed as commit 2d154fc.

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

[robehn]

/backport jdk21u

[openjdk]

@robehn the backport was successfully created on the branch robehn-backport-2d154fcd in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 2d154fcd from the openjdk/jdk repository.

The commit being backported was authored by Robbin Ehn on 28 Sep 2023 and was reviewed by Ludovic Henry and Fei Yang.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u:

$ git fetch https://github.com/openjdk-bots/jdk21u.git robehn-backport-2d154fcd:robehn-backport-2d154fcd
$ git checkout robehn-backport-2d154fcd
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u.git robehn-backport-2d154fcd

[robehn]

/backport jdk17u

[openjdk]

@robehn the backport was successfully created on the branch robehn-backport-2d154fcd in my personal fork of openjdk/jdk17u. To create a pull request with this backport targeting openjdk/jdk17u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 2d154fcd from the openjdk/jdk repository.

The commit being backported was authored by Robbin Ehn on 28 Sep 2023 and was reviewed by Ludovic Henry and Fei Yang.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk17u:

$ git fetch https://github.com/openjdk-bots/jdk17u.git robehn-backport-2d154fcd:robehn-backport-2d154fcd
$ git checkout robehn-backport-2d154fcd
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk17u.git robehn-backport-2d154fcd