8370198: Test gc/arguments/TestShrinkHeapInSteps.java crashed: assert(left >= right) failed: avoid underflow

[albertnetymk]

Add an early-return for outside-heap address in CollectedHeap::is_in API.

While investigating this failure (JDK-8370198), I realized that some java-threads (compiler-threads) in native state can invoke CollectedHeap APIs. Since heap-resizing occurs inside safepoint but java-threads in native state just ignore safepoint, I have added some assert to catch such dangerous uses, where the return value might not be stable.

Test: tie1-5; can't reproduce the JDK-8370198 with or without this patch for >8000 runs.

---

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-8370198: Test gc/arguments/TestShrinkHeapInSteps.java crashed: assert(left >= right) failed: avoid underflow (Bug - P2)(⚠️ The fixVersion in this issue is [26] but the fixVersion in .jcheck/conf is 27, a new backport will be created when this pr is integrated.)

Reviewers

• Stefan Karlsson (@stefank - Reviewer)
• Thomas Schatzl (@tschatzl - Reviewer) Review applies to 1a7bbae0

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28393

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8370198: Test gc/arguments/TestShrinkHeapInSteps.java crashed:  assert(left >= right) failed: avoid underflow

Reviewed-by: stefank, tschatzl

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

• 674cc3e: 8042054: JTree.updateUI uses out-of-date item size information
• 7e91d34: 8365732: RISC-V: implement AES CTR intrinsics
• 15f2538: 8373056: Shenandoah: Remove unnecessary use of ShenandoahAllocRequest.type()
• ... and 18 more: https://git.openjdk.org/jdk/compare/16699a394d4d6c2b8a21e7de3c3d344c5a3309b4...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]

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

• hotspot-gc

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

• 04: Full - Incremental (6f4f258d)
• 03: Full - Incremental (40ee3859)
• 02: Full - Incremental (1a7bbae0)
• 01: Full - Incremental (515c739e)
• 00: Full (6f435555)

[stefank]

This is quite conspicuous. Could this the addition be added to a function (or macro) so that the check becomes a one-liner. We have a similar function inside the ZGC runtime barriers. See:

template <typename ZBarrierSlowPath>
inline zaddress ZBarrier::barrier(ZBarrierFastPath fast_path, ZBarrierSlowPath slow_path, ZBarrierColor color, volatile zpointer* p, zpointer o, bool allow_null) {
  z_verify_safepoints_are_blocked();
... < rest of the function >

I don't understand why we would want to put this check between the two range changes. Is there a reason why the added check couldn't be done at the beginning of the function?

[albertnetymk]

Could this the addition be added to a function (or macro) so that the check becomes a one-liner.

I can't come up with a good name to abstract its semantics -- its correct invocation requires quite specific calling context, so I think it's best to "inline" it to the caller.

Is there a reason why the added check couldn't be done at the beginning of the function?

MacroAssembler::set_narrow_klass is an example that will fail the assert, if placed at the beginning of the method.

The thing is that CollectedHeap::is_in need to take heap-resizing into account for input address that lie within heap-reserved space, which places some extra constraint for callers. OTOH, for outside-heap-reserved addresses, heap-resizing is orthogonal to the result, so calling constraint can be relaxed.

[stefank]

Could this the addition be added to a function (or macro) so that the check becomes a one-liner.

I can't come up with a good name to abstract its semantics -- its correct invocation requires quite specific calling context, so I think it's best to "inline" it to the caller.

That doesn't instill confidence. I think we might have to figure out what semantics we want the check to have. Maybe it can be fixed by the suggestion below:

Is there a reason why the added check couldn't be done at the beginning of the function?

MacroAssembler::set_narrow_klass is an example that will fail the assert, if placed at the beginning of the method.

The thing is that CollectedHeap::is_in need to take heap-resizing into account for input address that lie within heap-reserved space, which places some extra constraint for callers. OTOH, for outside-heap-reserved addresses, heap-resizing is orthogonal to the result, so calling constraint can be relaxed.

This also means that this calling code can only use the negative version of that check:

  assert(! Universe::heap()->is_in(k), "should not be an oop");

A check like:

  assert(Universe::heap()->is_in(obj), "should be an oop");

would trigger the introduced assert that we are calling this from native.

I suggest that we remove this assert. It was needed when we had PermGen, but now we have other safeguards. Just a few lines below we have CompressedKlassPointers::encode(k);, which I think performs enough sanity check on the provided Klass*

[albertnetymk]

There are three related but distinct parts in this problem/solution:

1. Asserting that an address is outside the heap-reserved space when the caller is in native state.

   For example, the assertion in MacroAssembler::set_narrow_klass.

2. Adding an early return to is_in for addresses outside the heap-reserved space.

   This is always correct and almost trivial.

3. Adding an assertion inside is_in to catch potentially racy or unsafe uses of this API for addresses that fall inside the heap-reserved range.

---

Analysis

To resolve the specific crash observed once in JDK-8370198, removing the unnecessary assertion in set_narrow_klass is sufficient. However, because the reproduction rate is extremely low, I suspect there may be other call sites with similar usage patterns -- that is, calling is_in with addresses outside the heap-reserved range. For this reason, part (2) is beneficial.

Additionally, there may be other problematic uses of this API that became unsafe after the recent heap/young-generation resizing support. Part (3) aims to uncover such issues.

[albertnetymk]

After some offline discussion with Stefan, it's suggested that CollectedHeap::is_in should probably require callers to be in good state (e.g. not java-threads-in-native).

After removing the asserts in MacroAssembler::set_narrow_klass, I don't any crashes with placing such state-verification at the beginning of is_in. Therefore, it's probably best to make the assumption explicit (as a precondition).

[openjdk]

@albertnetymk hotspot has been added to this pull request based on files touched in new commit(s).

[stefank]

Please extract the verification into a helper function / macro

[tschatzl]

lgtm; I think verify_not_in_native_if_java_thread could be made a NOT_DEBUG_RETURN method which I tend to prefer to have less macros in the code. Either is fine to me though.

[albertnetymk]

Thanks for review.

/integrate

[openjdk]

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

• 811591c: 8373262: Parallel: gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java fails
• 355755d: 8366671: Refactor Thread::SpinAcquire and Thread::SpinRelease
• ac81ce5: 8372555: Test com/sun/jdi/ExceptionEvents.java failed: ObjectCollectedException
• ... and 38 more: https://git.openjdk.org/jdk/compare/16699a394d4d6c2b8a21e7de3c3d344c5a3309b4...master

Your commit was automatically rebased without conflicts.

[openjdk]

@albertnetymk Pushed as commit d34ef19.

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

[albertnetymk]

/backport :jdk26

[openjdk]

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

The commit being backported was authored by Albert Mingkun Yang on 8 Dec 2025 and was reviewed by Stefan Karlsson and Thomas Schatzl.

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/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-albertnetymk-d34ef196-jdk26:backport-albertnetymk-d34ef196-jdk26
$ git checkout backport-albertnetymk-d34ef196-jdk26
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-albertnetymk-d34ef196-jdk26