8369947: Bytecode rewriting causes Java heap corruption on RISC-V

[feilongjiang]

As discussed in #27748 (review), the same issue occurs with the RISC-V port.

Testing:

• tier1 - tier4 linux-riscv64 fastdebug

---

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-8369947: Bytecode rewriting causes Java heap corruption on RISC-V (Bug - P2)

Reviewers

• Andrew Haley (@theRealAph - Reviewer)
• Justin King (@jcking - Committer)
• Fei Yang (@RealFYang - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27850

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8369947: Bytecode rewriting causes Java heap corruption on RISC-V

Reviewed-by: aph, jcking, 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 37 new commits pushed to the master branch:

• 1816570: 8321687: Test vmTestbase/nsk/jvmti/scenarios/contention/TC03/tc03t002/TestDescription.java failed: JVMTI_ERROR_THREAD_NOT_ALIVE
• 926f61f: 8369447: GenShen: Regulator thread may observe inconsistent states
• 49b17dd: 8356575: Test order in which jpackage fills app image
• ... and 34 more: https://git.openjdk.org/jdk/compare/303eb1096ccaf06106aa080b9ea0553c0f6912dd...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]

@feilongjiang 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]

Webrevs

• 00: Full (bed47b4f)

[RealFYang]

Hi, I am having some difficulty in understanding the issue.
@shipilev @theRealAph : For the aarch64 counterpart, shouldn't the ldarb at [1] prevent the reordering of STR of PBC and STLR of RFE? It's a load instruction with acquire semantics.

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/templateTable_aarch64.cpp#L200

[shipilev]

@shipilev @theRealAph : For the aarch64 counterpart, shouldn't the ldarb at [1] prevent the reordering of STR of PBC and STLR of RFE? It's a load instruction with acquire semantics.

Yes, I was confused about this myself. The key thing for this particular issue: the reader we need to sync up with is not patch_bytecode, it is the thread that executes the patched bytecode. In other words, the writer is patch_bytecode, and reader is executing thread.

So acquire barrier in patch_bytecode does not help this case, because it is a write path, it needs release, which aarch64 fix did. The read path needs some other synchronization for acquire-like semantics; in aarch64 we reasoned the control dependency on bytecode itself and the barrier in RFE resolution is already enough to do this. See my writeup here: https://bugs.openjdk.org/browse/JDK-8369506?focusedId=14824157#comment-14824157 -- and the comments after it.

[theRealAph]

@shipilev @theRealAph : For the aarch64 counterpart, shouldn't the ldarb at [1] prevent the reordering of STR of PBC and STLR of RFE? It's a load instruction with acquire semantics.

Yes, I was confused about this myself. The key thing for this particular issue: the reader we need to sync up with is not patch_bytecode, it is the thread that executes the patched bytecode. In other words, the writer is patch_bytecode, and reader is executing thread.

So acquire barrier in patch_bytecode does not help this case, because it is a write path, it needs release, which aarch64 fix did. The read path needs some other synchronization for acquire-like semantics; in aarch64 we reasoned the control dependency on bytecode itself and the barrier in RFE resolution is already enough to do this.

RISCV is good on the read side, we just need this patch to fix the write:

void InterpreterMacroAssembler::load_field_entry(Register cache, Register index, int bcp_offset) {
...
  // Get address of field entries array
  ld(cache, Address(xcpool, ConstantPoolCache::field_entries_offset()));
  addi(cache, cache, Array<ResolvedIndyEntry>::base_offset_in_bytes());
  add(cache, cache, index);
  // Prevents stale data from being read after the bytecode is patched to the fast bytecode
  membar(MacroAssembler::LoadLoad);
}

[TheRealMDoerr]

@shipilev @theRealAph : For the aarch64 counterpart, shouldn't the ldarb at [1] prevent the reordering of STR of PBC and STLR of RFE? It's a load instruction with acquire semantics.

Yes, I was confused about this myself. The key thing for this particular issue: the reader we need to sync up with is not patch_bytecode, it is the thread that executes the patched bytecode. In other words, the writer is patch_bytecode, and reader is executing thread.
So acquire barrier in patch_bytecode does not help this case, because it is a write path, it needs release, which aarch64 fix did. The read path needs some other synchronization for acquire-like semantics; in aarch64 we reasoned the control dependency on bytecode itself and the barrier in RFE resolution is already enough to do this.

RISCV is good on the read side, we just need this patch to fix the write:

void InterpreterMacroAssembler::load_field_entry(Register cache, Register index, int bcp_offset) {
...
  // Get address of field entries array
  ld(cache, Address(xcpool, ConstantPoolCache::field_entries_offset()));
  addi(cache, cache, Array<ResolvedIndyEntry>::base_offset_in_bytes());
  add(cache, cache, index);
  // Prevents stale data from being read after the bytecode is patched to the fast bytecode
  membar(MacroAssembler::LoadLoad);
}

I just made a similar proposal for PPC64. But, I only use a barrier for fast Bytecodes:
https://github.com/TheRealMDoerr/jdk/blob/138df669209ae58676e0559cf825d0a0cc81ee1b/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp#L491

[RealFYang]

@shipilev @theRealAph : For the aarch64 counterpart, shouldn't the ldarb at [1] prevent the reordering of STR of PBC and STLR of RFE? It's a load instruction with acquire semantics.

Yes, I was confused about this myself. The key thing for this particular issue: the reader we need to sync up with is not patch_bytecode, it is the thread that executes the patched bytecode. In other words, the writer is patch_bytecode, and reader is executing thread.
So acquire barrier in patch_bytecode does not help this case, because it is a write path, it needs release, which aarch64 fix did. The read path needs some other synchronization for acquire-like semantics; in aarch64 we reasoned the control dependency on bytecode itself and the barrier in RFE resolution is already enough to do this.

Nice analysis! I read it several times as well and I think I know what's going on now. Thanks.

RISCV is good on the read side, we just need this patch to fix the write:

void InterpreterMacroAssembler::load_field_entry(Register cache, Register index, int bcp_offset) {
...
  // Get address of field entries array
  ld(cache, Address(xcpool, ConstantPoolCache::field_entries_offset()));
  addi(cache, cache, Array<ResolvedIndyEntry>::base_offset_in_bytes());
  add(cache, cache, index);
  // Prevents stale data from being read after the bytecode is patched to the fast bytecode
  membar(MacroAssembler::LoadLoad);
}

Yes, we only need the necessary barrier on the write side.

[feilongjiang]

Thanks!

/integrate

[openjdk]

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

• 1816570: 8321687: Test vmTestbase/nsk/jvmti/scenarios/contention/TC03/tc03t002/TestDescription.java failed: JVMTI_ERROR_THREAD_NOT_ALIVE
• 926f61f: 8369447: GenShen: Regulator thread may observe inconsistent states
• 49b17dd: 8356575: Test order in which jpackage fills app image
• ... and 34 more: https://git.openjdk.org/jdk/compare/303eb1096ccaf06106aa080b9ea0553c0f6912dd...master

Your commit was automatically rebased without conflicts.

[openjdk]

@feilongjiang Pushed as commit 4625199.

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

[feilongjiang]

/backport jdk25u

[openjdk]

@feilongjiang the backport was successfully created on the branch backport-feilongjiang-46251993-master in my personal fork of openjdk/jdk25u. To create a pull request with this backport targeting openjdk/jdk25u: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 46251993 from the openjdk/jdk repository.

The commit being backported was authored by Feilong Jiang on 18 Oct 2025 and was reviewed by Andrew Haley, Justin King 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/jdk25u:

$ git fetch https://github.com/openjdk-bots/jdk25u.git backport-feilongjiang-46251993-master:backport-feilongjiang-46251993-master
$ git checkout backport-feilongjiang-46251993-master
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk25u.git backport-feilongjiang-46251993-master

[feilongjiang]

/backport jdk21u-dev

[openjdk]

@feilongjiang Could not automatically backport 46251993 to openjdk/jdk21u-dev due to conflicts in the following files:

• src/hotspot/cpu/riscv/templateTable_riscv.cpp

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk21u-dev. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk21u-dev.git master:master

# Check out the target branch and create your own branch to backport
$ git checkout master
$ git checkout -b backport-feilongjiang-46251993-master

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git 462519935827e25475f2fb35746ad81a14bc5da7

# Backport the commit
$ git cherry-pick --no-commit 462519935827e25475f2fb35746ad81a14bc5da7
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport 462519935827e25475f2fb35746ad81a14bc5da7'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk21u-dev with the title Backport 462519935827e25475f2fb35746ad81a14bc5da7.

Below you can find a suggestion for the pull request body:

Hi all,

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

The commit being backported was authored by Feilong Jiang on 18 Oct 2025 and was reviewed by Andrew Haley, Justin King and Fei Yang.

Thanks!