jdk 8341273: JVMTI is not properly hiding some continuation related methods

This fixes a problem in the VTMS (Virtual Thread Mount State) transition frames hiding mechanism. Please, see a fix description in the first comment.

Testing:

Verified with new test vthread/CheckHiddenFrames
Mach5 tiers 1-6 are passed

Progress

[x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue

Issue

JDK-8341273: JVMTI is not properly hiding some continuation related methods (Bug - P3)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21397

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

Using diff file

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

Webrev

Link to Webrev Comment

Oct 07 '24 22:10 sspitsyn

:wave: Welcome back sspitsyn! 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.

Oct 07 '24 22:10 bridgekeeper[bot]

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

8341273: JVMTI is not properly hiding some continuation related methods

Reviewed-by: alanb, amenkov

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

9cfb0f7f7ad31081c917be1eb0e39e2552e45382: 8341527: AVX-512 intrinsic for SHA3
4ce19ca110b6e1eeed7483a1ec7c75fbc1d1b773: 8343190: GHA: Try building JTReg several times
7c800e6bae388dd87986f366787398fe99b4e2ee: 8343026: JFR: Index into fields in the topFrame
d8b3685d36873904248e9701f66459e074a4a8ab: 8342607: Enhance register printing on x86_64 platforms
d8430efb5e159b8e08d2cac66b46cb4ff1112927: 8339573: Update CodeCacheSegmentSize and CodeEntryAlignment for ARM
6332e258f91789cf50d07a6929f32ff3aaef1a92: 8343183: [s390x]: Problemlist runtime/Monitor/SyncOnValueBasedClassTest.java Failure
79a07ad726f4e4b0502a22a55832960aa1561911: 8343149: Cleanup os::print_tos_pc on AIX
beff8bfe2a5334823b67cb748bc8652dc6a3f3d4: 8342823: Ubsan: ciEnv.cpp:1614:65: runtime error: member call on null pointer of type 'struct CompileTask'
e389f82b1b2365a43fef744936b222328d71494b: 8343137: C2: VerifyLoopOptimizations fails with "Was reachable in only one"
0abfa3ba8f72538f62be838c1ebac8cfbdd14cdf: 8304824: NMT should not use ThreadCritical
... and 73 more: https://git.openjdk.org/jdk/compare/d6eddcdaf92f2352266ba519608879141997cd63...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.

Oct 07 '24 22:10 openjdk[bot]

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

core-libs
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.

Oct 07 '24 22:10 openjdk[bot]

The frames which are in VTMS transition should not be visible to the JVMTI agents including debug agent because the thread identity can be incorrect. The JVMTI events are not posted when java_thread->is_in_VTMS_transition() == true. All the JVMTI functions returning stack related info do skip frames that are in transition. The hiding mechanism is using the annotation @JvmtiMountTransition to mark the notifyJvmti* methods and the bit java_thread->is_in_VTMS_transition(). It occurred that the methods yield() and yield0() can be present in stack trace of an unmounted virtual threads. The bit java_thread->is_in_VTMS_transition() is not set in such a case. The fix is to add the annotation @JvmtiMountTransition to the methods yield() and yield0() and correct the frames skipping mechanism to account for such frames as well. The update also includes:

fix in one of the JvmtiHandshake::execute() functions which is needed for better stability and safety
tweak in the test which expects frames with yield() and yield0() methods to be present at the top

Oct 07 '24 22:10 sspitsyn

Webrevs

Oct 07 '24 22:10 mlbridge[bot]

I've pushed two updates. The first one addresses the suggestions from Leonid:

minor tweaks in new test
moved skipping hidden methods yield() and yield0() from check_and_skip_hidden_frames() to a separate function
explained in a comment of JvmtiHandshake::execute() why all VTMS transitions are disabled for platform thread
added extra safety/stability checks and a couple of asserts around calls to check_and_skip_hidden_frames()

Second update fixes one more NotifyFramePop issue with the frames at the virtual thread start:

enter(), enter0, VThreadContinuation$1.run() and VirtualThread.run()

This the bottom of stack trace from debugger:

  0: java/lang/VirtualThread: run(Ljava/lang/Runnable;)V
  1: java/lang/VirtualThread$VThreadContinuation$1: run()V
  2: jdk/internal/vm/Continuation: enter0()V
  3: jdk/internal/vm/Continuation: enter(Ljdk/internal/vm/Continuation;Z)V

The fix includes three parts:

The notifyJvmtiStart()/notifyJvmtiEnd() notification calls are moved from VirtualThread.run() to the VThreadContinuation$1.run()
The annotation @JvmtiMountTransition has been added to the VThreadContinuation$1.run()
The NotifyFramePop is changed to return JVMTI_ERRO_OPAQUE_FRAME for frames with enter() and methods annotated with @JvmtiMountTransition. The JVMTI_ERRO_OPAQUE_FRAME is already returned for native methods like enter0().

The latest update is to avoid any virtual thread events before VirtualThreadEntry and after VirtualThreadExit events are posted. The tweak in NotifyFramePop implementation is to disallow FramePop requests for such methods.

Oct 10 '24 10:10 sspitsyn

I believe, I've resolved almost all review comments. Please, let me know if anything is missed or there are some issues in my latest updates. The mach5 tiers 1-6 are good.

Oct 17 '24 15:10 sspitsyn

@sspitsyn this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout b40
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Oct 22 '24 19:10 openjdk[bot]

Latest update to JvmtiMountTransition looks okay. I assume wait until Alex Menkov and Leonid are finished their reviews.

Oct 24 '24 09:10 AlanBateman

Alan and Alex, thank you for review!

Oct 29 '24 19:10 sspitsyn

/integrate

Oct 29 '24 19:10 sspitsyn

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

520ddac97053be669d9678375266ccfd6724e3e1: 8331861: [PPC64] Implement load / store assembler functions which take an Address object
9cfb0f7f7ad31081c917be1eb0e39e2552e45382: 8341527: AVX-512 intrinsic for SHA3
4ce19ca110b6e1eeed7483a1ec7c75fbc1d1b773: 8343190: GHA: Try building JTReg several times
7c800e6bae388dd87986f366787398fe99b4e2ee: 8343026: JFR: Index into fields in the topFrame
d8b3685d36873904248e9701f66459e074a4a8ab: 8342607: Enhance register printing on x86_64 platforms
d8430efb5e159b8e08d2cac66b46cb4ff1112927: 8339573: Update CodeCacheSegmentSize and CodeEntryAlignment for ARM
6332e258f91789cf50d07a6929f32ff3aaef1a92: 8343183: [s390x]: Problemlist runtime/Monitor/SyncOnValueBasedClassTest.java Failure
79a07ad726f4e4b0502a22a55832960aa1561911: 8343149: Cleanup os::print_tos_pc on AIX
beff8bfe2a5334823b67cb748bc8652dc6a3f3d4: 8342823: Ubsan: ciEnv.cpp:1614:65: runtime error: member call on null pointer of type 'struct CompileTask'
e389f82b1b2365a43fef744936b222328d71494b: 8343137: C2: VerifyLoopOptimizations fails with "Was reachable in only one"
... and 74 more: https://git.openjdk.org/jdk/compare/d6eddcdaf92f2352266ba519608879141997cd63...master

Your commit was automatically rebased without conflicts.

Oct 29 '24 19:10 openjdk[bot]

@sspitsyn Pushed as commit 60364ef0010bde2933c22bf581ff8b3700c4afd6.

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

Oct 29 '24 20:10 openjdk[bot]