8303276: Secondary assertion failure in AdapterHandlerLibrary::contains during crash reporting

[coleenp]

Don't check lock ownership if an error has been reported.
Ran tier1-4 tests which include some intentionally crashing tests.

---

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-8303276: Secondary assertion failure in AdapterHandlerLibrary::contains during crash reporting

Reviewers

• Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to e9810ef3
• Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to e9810ef3

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13500

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@coleenp 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

• 02: Full - Incremental (5ac4fc95)
• 01: Full - Incremental (e9810ef3)
• 00: Full (1dbb2161)

[dholmes-ora]

Not sure I agree with this. If there is code that gets called on the error-handler path and it can't be called due to issues like this then we need to know about that and try to fix the code. Ignoring the need for locking or safepoints could just lead to different secondary crashes.

[dholmes-ora]

More specifically I think we need to evaluate on a case by case basis - so the check for is_error_reported() would be in the function with the assert_locked_or_safepoint. This is a bit messy syntactically.

\#ifdef ASSERT
if (!VM_Error::is_error_reported()) assert_locked_or_safepoint(...);
\#endif

[coleenp]

I disagree. In almost all cases of error handling, we should not let a locked check prevent error reporting. Playing whack-a-mole with the locations is not sustainable. And noisy in the code.

[tstuefe]

I disagree. In almost all cases of error handling, we should not let a locked check prevent error reporting. Playing whack-a-mole with the locations is not sustainable. And noisy in the code.

I agree with @coleenp. This is a read access from the error handler, where we are guarded against secondary crashes. Disabling the assert means we trade a guaranteed secondary error against a very unlikely one.

[coleenp]

For this change, JDK-8210754, there was specific test for ClassLoaderDataGraph_lock which I removed. This category of assert should be disabled for error reporting as it is for debugging.

[dholmes-ora]

we trade a guaranteed secondary error against a very unlikely one

I would say we trade a very obvious secondary error for a potentially very obscure one. If you don't hold the lock, nor are at a safepoint, then the access is inherently unsafe. You may not crash you may just get bad or misleading data which could then send you off on a wild goose chase.

Time will tell ...

[iklam]

LGTM

[openjdk]

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

8303276: Secondary assertion failure in AdapterHandlerLibrary::contains during crash reporting

Reviewed-by: iklam, stuefe

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

• 4b23bef: 8301377: adjust timeout for JLI GetObjectSizeIntrinsicsTest.java subtest again
• f32adaf: 8304836: Make MALLOC_MIN4 macro more robust
• 2763cf1: 8304896: Update to use jtreg 7.2
• b2ccc97: 8306444: Don't leak memory in PhaseChaitin::PhaseChaitin
• d980cb4: 8306658: GHA: MSVC installation could be optional since it might already be pre-installed
• 62acc88: 8306476: CDS ArchiveHeapTestClass.java test asserts when vm_exit is called on VM thread

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

[tstuefe]

LGTM

(but the aarch64 build errors may require you to re-merge head and build again)

[coleenp]

Thanks Thomas and Ioi. I remerged mainline and the aarch64 failures in GHA went away.
/integrate

[openjdk]

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

• 0dc03c9: 8305922: [aix,linux] Avoid comparing 'this' to nullptr
• 328e512: 8306780: ProblemList java/lang/Thread/virtual/HoldsLock.java#id0 in Xcomp
• 937dd34: 8306775: Problemlist runtime/Monitor/GuaranteedAsyncDeflationIntervalTest.java
• 314db55: 8304818: Prune HttpURLConnection cache when corresponding Authenticator is garbage collected
• 6b81342: 8305994: Guarantee eventual async monitor deflation
• 41d6be4: 8301065: Handle control characters in java_lang_String::print
• 4b23bef: 8301377: adjust timeout for JLI GetObjectSizeIntrinsicsTest.java subtest again
• f32adaf: 8304836: Make MALLOC_MIN4 macro more robust
• 2763cf1: 8304896: Update to use jtreg 7.2
• b2ccc97: 8306444: Don't leak memory in PhaseChaitin::PhaseChaitin
• ... and 2 more: https://git.openjdk.org/jdk/compare/7400aff3b8a0294dcbb6e89e9d8aad984f29fe92...master

Your commit was automatically rebased without conflicts.

[openjdk]

@coleenp Pushed as commit 2ea62c1.

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