8327743: JVM crash in hotspot/share/runtime/javaThread.cpp - failed: held monitor count should be equal to jni: 0 != 1

[dholmes-ora]

The crux of the problem here is that the virtual thread code was not keeping the held-monitor-count and jni-monitor-count in sync under all conditions. So if a vthread acquired a monitor via JNI but failed to unlock it before terminating, the underlying platform thread's counts were out of sync and if it terminated we would trigger the assertion that checks for such things.

The actual fix is very simple: we zero the platform thread's jni-monitor-count in continuation_enter_cleanup the same way we zero the held-monitor-count. In addition we apply the same CheckJNICalls check for this unbalanced locking and issue a warning in the virtual thread case. That fact this happens in asm code complicates matters.

The existing JNIMonitor.java test is greatly expanded to test these scenarios and check the unified logging output.

Other minor changes involve expanding some of the other assertions relating to the two counts so we can detected a mismatch earlier without a need for the thread to terminate. And the test that original uncovered the problem (GetOwnedMonitorInfoTest.java) has some minor adjustments to enhance diagnostics.

I've provided the fix for all architectures that support continuations: x64, aarch64, riscv and ppc. The latter both build okay in GHA but I can't actually test them with the updated test. So some assistance from RISCV folk (@robehn ?) and PPC folk (??) would be appreciated (otherwise any issues will have to be handled as follow up fixes

The changes are structured so that there is no extra code executed in product builds unless CheckJNICalls is set. This means that product builds will not keep the JNI count in sync with the held count, unless CheckJNICalls is set. This could trip up a future logging entry or explicit check of the JNI count, but it is expected that these counts will be removed once ObjectMonitor usage will not force virtual thread pinning.

Testing:

• regression test 10x on all x64 and aarch64 platforms
• tiers 1-4
• GHA

Thanks to @pchilano for help working out the best form of the fix and the initial asm for x64.

Thanks to @fbredber for the Aarch64 and RISCV asm code.

Thanks

---

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-8327743: JVM crash in hotspot/share/runtime/javaThread.cpp - failed: held monitor count should be equal to jni: 0 != 1 (Bug - P3)

Reviewers

• Robbin Ehn (@robehn - Reviewer) ⚠️ Review applies to 70f43301
• Fredrik Bredberg (@fbredber - Committer)
• Patricio Chilano Mateo (@pchilano - Reviewer) ⚠️ Review applies to 5891800b
• Richard Reingruber (@reinrich - Reviewer)

Contributors

• Fredrik Bredberg <fbredberg@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18445

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@dholmes-ora 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:

8327743: JVM crash in hotspot/share/runtime/javaThread.cpp - failed: held monitor count should be equal to jni: 0 != 1

Co-authored-by: Fredrik Bredberg <fbredberg@openjdk.org>
Reviewed-by: rehn, fbredberg, pchilanomate, rrich

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

• 7df4926: 8327741: JVM crash in hotspot/share/opto/compile.cpp - failed: missing inlining msg
• 2ceeb6c: 8328822: C2: "negative trip count?" assert failure in profile predicate code
• b04b304: 8329254: optimize integral reverse operations on x86 GFNI target.
• d9c84e7: 8329656: assertion failed in MAP_ARCHIVE_MMAP_FAILURE path: Invalid immediate -5 0
• 5e544f1: 8329491: GetThreadListStackTraces function should use JvmtiHandshake
• 643dd48: 8329432: PopFrame and ForceEarlyReturn functions should use JvmtiHandshake
• 70944ca: 8330033: com/sun/net/httpserver/bugs/B6431193.java fails in AssertionError after JDK-8326568
• 279ed0d: 8329961: Buffer overflow in os::Linux::kernel_version
• 9731b1c: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel
• c5150c7: 8309751: Duplicate constant pool entries added during default method processing
• ... and 56 more: https://git.openjdk.org/jdk/compare/51b0abc87439014c4c5022c0057614f99a741ddd...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]

@dholmes-ora The following label will be automatically applied to this pull request:

• hotspot

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.

[dholmes-ora]

/contributor add @fbredber

[openjdk]

@dholmes-ora
Contributor Fredrik Bredberg <fbredberg@openjdk.org> successfully added.

[mlbridge]

Webrevs

• 05: Full - Incremental (4a9fc71f)
• 04: Full - Incremental (70f43301)
• 03: Full - Incremental (5891800b)
• 02: Full - Incremental (59d93389)
• 01: Full - Incremental (bfe62751)
• 00: Full (32a62a27)

[dholmes-ora]

Thus implement what this comment says we are missing,

@robehn the comment isn't saying that we are missing anything. Only JNI DetachThread specifies that it will release all held monitors. When a thread terminates (platform or virtual) there is no specification to say it should also release any still held monitors - so we don't. We find all locked monitors from the in-use monitor list, we don't rely on the counters for that.

Thanks for the testing and review. I just pushed a small update for RISC

[dean-long]

Isn't continuation_enter_cleanup() also used by yield()? How does monitorEnter(); Thread.yield(); monitorExit() not result in the JNI lock count going to -1?

[dholmes-ora]

Isn't continuation_enter_cleanup() also used by yield()?

@dean-long In general yes but in this case the virtual thread is pinned so the cleanup is skipped. These monitor counts only exist to force pinning.

[dean-long]

The tests expect an exit value of 0, but for some reason why I run it they are returning 1.

[dean-long]

The tests expect an exit value of 0, but for some reason why I run it they are returning 1.

Nevermind, user error.

[fbredber]

I've done basic smoke testing on PowerPC using QEMU.
JAVA_OPTIONS=-XX:+CheckJNICalls TEST=test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java passes ok. But it would be nice if @TheRealMDoerr or @reinrich could take it for a spin on real hardware.

[reinrich]

I've done basic smoke testing on PowerPC using QEMU. JAVA_OPTIONS=-XX:+CheckJNICalls TEST=test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java passes ok. But it would be nice if @TheRealMDoerr or @reinrich could take it for a spin on real hardware.

Thanks for the pin. We will do that.

[pchilano]

Looks good to me, thanks for working this one out David.

[dholmes-ora]

Thanks for the review @pchilano .

[reinrich]

Testing didn't bring up any issue. I've only got a few minor comments.
Thanks, Richard.

[dholmes-ora]

Thanks for the review comments and testing of PPC @reinrich !

[fbredber]

Looks good to me.

[reinrich]

Thanks for fixing this and taking care of ppc.

[dholmes-ora]

Thanks for all the reviews.

/integrate

[openjdk]

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

• 140f567: 8323220: Reassociate loop invariants involved in Cmps and Add/Subs
• a293bdf: 8330197: Make javac/diags/example release agnostic
• da75e01: 8330196: Make java/lang/invoke/defineHiddenClass/BasicTest release agnostic
• ddc3921: 8330103: Compiler memory statistics should keep separate records for C1 and C2
• 273df62: 8328792: Parallel: Refactor PSParallelCompact::summary_phase
• a3fecdb: 8329764: G1: Handle null references during verification first
• 60d88b7: 8330176: Typo in Linker javadoc
• 3f1d9c4: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc
• 5404b4e: 8330105: SharedRuntime::resolve* should respect interpreter-only mode
• d22d560: 8329864: TestLibGraal.java still crashes with assert(_stack_base != nullptr)
• ... and 101 more: https://git.openjdk.org/jdk/compare/51b0abc87439014c4c5022c0057614f99a741ddd...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dholmes-ora Pushed as commit 274c805.

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