8310340: assert(_thread->is_interp_only_mode() || stub_caller) failed: expected a stub-caller #22931

pchilano · 2025-01-06T16:41:21Z

Please review the following fix. In method JvmtiEventControllerPrivate::recompute_thread_enabled(), we are missing to call leave_interp_only_mode() for the case where should_be_interp is computed as false and state->is_pending_interp_only_mode() is true. I added the full trace leading to the crash in the bug comments.
In JDK-8338383 I removed this assert because the branch condition changed and it became sort of a redundant check. But given that it was able to find this issue I added it back.
I was able to reproduce the crash easily by adding an extra delay before the assert. I verified the crash doesn’t reproduce anymore with this fix. I also run the patch through mach5 tiers 1-7.

Thanks,
Patricio

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-8310340: assert(_thread->is_interp_only_mode() || stub_caller) failed: expected a stub-caller (Bug - P3)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Alex Menkov (@alexmenkov - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22931

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-06T16:41:57Z

👋 Welcome back pchilanomate! 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 · 2025-01-06T16:42:53Z

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

8310340: assert(_thread->is_interp_only_mode() || stub_caller) failed: expected a stub-caller

Reviewed-by: dholmes, amenkov, sspitsyn

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:

bcefab5: 8342468: Improve API documentation for java.lang.classfile.constantpool
40f0a39: 8343342: java/io/File/GetXSpace.java fails on Windows with CD-ROM drive
021c476: 8347148: [BACKOUT] AccessFlags can be u2 in metadata
ddb5881: 8329549: Remove FORMAT64_MODIFIER
098afc8: 8339113: AccessFlags can be u2 in metadata
e413fc6: 8347127: CTW fails to build after JDK-8334733
9702acc: 8175709: DateTimeFormatterBuilder.appendZoneId() has misleading JavaDoc
030149f: 8334644: Automate javax/print/attribute/PageRangesException.java
c8a9dd3: 8346609: Improve MemorySegment.toString
4d8fb80: 8347038: [JMH] jdk.incubator.vector.SpiltReplicate fails NoClassDefFoundError
... and 27 more: https://git.openjdk.org/jdk/compare/07c9f7138affdf0d42ecdc30adcb854515569985...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 · 2025-01-06T16:43:26Z

@pchilano The following labels will be automatically applied to this pull request:

hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-01-06T17:42:08Z

Webrevs

00: Full (762c0d02)

dholmes-ora

Looks good to me. Thanks for the detailed analysis.

sspitsyn · 2025-01-07T20:19:04Z

Patricio, please, let me double check something before your push.

sspitsyn

The fix looks good. It is a great and important finding - thanks!
What was bothering me is that the EnterInterpOnlyModeClosure is used in the JvmtiEventControllerPrivate::enter_interp_only_mode() but no HandshakeClosure is used in the JvmtiEventControllerPrivate::leave_interp_only_mode(). The handshake use for the enter_interp_only_mode() looks like a paranoid overkill to me. It can be I just forgot the exact reason why it is used there.

pchilano · 2025-01-08T17:33:09Z

The fix looks good. It is a great and important finding - thanks! What was bothering me is that the EnterInterpOnlyModeClosure is used in the JvmtiEventControllerPrivate::enter_interp_only_mode() but no HandshakeClosure is used in the JvmtiEventControllerPrivate::leave_interp_only_mode(). The handshake use for the enter_interp_only_mode() looks like a paranoid overkill to me. It can be I just forgot the exact reason why it is used there.

I see. Yes, we still need the handshake in JvmtiEventControllerPrivate::enter_interp_only_mode to safely walk the stack of the target to deoptimize the frames. When leaving interpreter only mode we just decrement _interp_only_mode/_saved_interp_only_mode so I don’t think we need anything else, other than making sure the change is atomic, which I think we guarantee by holding JvmtiThreadState_lock.

pchilano · 2025-01-08T21:50:27Z

Thanks for the reviews @dholmes-ora, @alexmenkov and @sspitsyn!

pchilano · 2025-01-08T21:50:37Z

/integrate

openjdk · 2025-01-08T21:51:05Z

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

2e00816: 8346671: java/nio/file/Files/probeContentType/Basic.java fails on Windows 2025
88fa3b2: 8346998: Test nsk/jvmti/ResourceExhausted/resexhausted003 fails with java.lang.OutOfMemoryError when CDS is off
6ee2bd2: 8347147: [REDO] AccessFlags can be u2 in metadata
a641932: 8346310: Duplicate !HAS_PENDING_EXCEPTION check in DynamicArchive::dump_at_exit
49ee4df: 8166983: Remove old/legacy unused tzdata files
3fe0818: 8346099: JFR: Query for 'jfr view' can't handle wildcard with multiple event types
55bcf4c: 8346047: JFR: Incorrect percentile value in 'jfr view'
ae3fc46: 8345580: Remove const from Node::_idx which is modified
4d18e5a: 8346872: tools/jpackage/windows/WinLongPathTest.java fails
92ad8a1: 8346052: JFR: Incorrect average value in 'jfr view'
... and 44 more: https://git.openjdk.org/jdk/compare/07c9f7138affdf0d42ecdc30adcb854515569985...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-08T21:51:11Z

@pchilano Pushed as commit ea49537.

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

sspitsyn · 2025-01-09T02:12:49Z

I see. Yes, we still need the handshake in JvmtiEventControllerPrivate::enter_interp_only_mode to safely walk the stack of the target to deoptimize the frames. When leaving interpreter only mode we just decrement _interp_only_mode/_saved_interp_only_mode so I don’t think we need anything else, other than making sure the change is atomic, which I think we guarantee by holding JvmtiThreadState_lock.

Agreed.

pchilano · 2025-01-09T14:16:20Z

/backport :jdk24

openjdk · 2025-01-09T14:17:03Z

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

The commit being backported was authored by Patricio Chilano Mateo on 8 Jan 2025 and was reviewed by David Holmes, Alex Menkov and Serguei Spitsyn.

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-pchilano-ea495377-jdk24:backport-pchilano-ea495377-jdk24
$ git checkout backport-pchilano-ea495377-jdk24
# 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-pchilano-ea495377-jdk24

v1

762c0d0

openjdk bot added serviceability [email protected] hotspot [email protected] labels Jan 6, 2025

pchilano marked this pull request as ready for review January 6, 2025 17:38

openjdk bot added the rfr Pull request is ready for review label Jan 6, 2025

dholmes-ora approved these changes Jan 6, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 6, 2025

alexmenkov approved these changes Jan 7, 2025

View reviewed changes

sspitsyn approved these changes Jan 8, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 8, 2025

openjdk bot closed this Jan 8, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8310340: assert(_thread->is_interp_only_mode() || stub_caller) failed: expected a stub-caller #22931

8310340: assert(_thread->is_interp_only_mode() || stub_caller) failed: expected a stub-caller #22931

pchilano commented Jan 6, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 6, 2025

openjdk bot commented Jan 6, 2025 •

edited

Loading

openjdk bot commented Jan 6, 2025

mlbridge bot commented Jan 6, 2025

dholmes-ora left a comment

sspitsyn commented Jan 7, 2025

sspitsyn left a comment •

edited

Loading

pchilano commented Jan 8, 2025

pchilano commented Jan 8, 2025

pchilano commented Jan 8, 2025

openjdk bot commented Jan 8, 2025

openjdk bot commented Jan 8, 2025

sspitsyn commented Jan 9, 2025

pchilano commented Jan 9, 2025

openjdk bot commented Jan 9, 2025

8310340: assert(_thread->is_interp_only_mode() || stub_caller) failed: expected a stub-caller #22931

8310340: assert(_thread->is_interp_only_mode() || stub_caller) failed: expected a stub-caller #22931

Conversation

pchilano commented Jan 6, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 6, 2025

openjdk bot commented Jan 6, 2025 • edited Loading

openjdk bot commented Jan 6, 2025

mlbridge bot commented Jan 6, 2025

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

sspitsyn commented Jan 7, 2025

sspitsyn left a comment • edited Loading

Choose a reason for hiding this comment

pchilano commented Jan 8, 2025

pchilano commented Jan 8, 2025

pchilano commented Jan 8, 2025

openjdk bot commented Jan 8, 2025

openjdk bot commented Jan 8, 2025

sspitsyn commented Jan 9, 2025

pchilano commented Jan 9, 2025

openjdk bot commented Jan 9, 2025

pchilano commented Jan 6, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 6, 2025 •

edited

Loading

sspitsyn left a comment •

edited

Loading