RFR: 8341273: JVMTI is not properly hiding some continuation related methods [v3]

[Alex Menkov]

On Wed, 9 Oct 2024 22:58:33 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> 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
>
> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Disallow NotifyFramePop for enter/enter0/VirtualThread.run/VThreadContinuation.run

src/hotspot/share/prims/jvmtiEnvBase.cpp line 692:

> 690:   if (jt->is_in_VTMS_transition()) {
> 691:     jvf = check_and_skip_hidden_frames(jt->is_in_VTMS_transition(), jvf);
> 692:   } else if (is_virtual) { // filter out pure continuations

Not sure I follow the logic here.
As far as I understand yield/yield need to be filtered out only for unmounted virtual threads. But `is_virtual` here means we have mounted VT?
Looks like almost all callers call this method only if `jt->is_in_JTMS_transilition()` is true
Exception is a call from `get_vthread_jvf()` when vthread is mounted.

src/hotspot/share/prims/jvmtiEnvBase.cpp line 703:

> 701:   if (java_lang_VirtualThread::is_instance(vthread)) { // paranoid check for safety
> 702:     if (java_lang_Thread::is_in_VTMS_transition(vthread)) {
> 703:       jvf = check_and_skip_hidden_frames(java_lang_Thread::is_in_VTMS_transition(vthread), jvf);

it's just checked that `java_lang_Thread::is_in_VTMS_transition(vthread)` is true
Suggestion:

      jvf = check_and_skip_hidden_frames(true, jvf);

src/hotspot/share/prims/jvmtiEnvBase.cpp line 2025:

> 2023:   // - it is a good invariant when a thread's handshake can't be impacted by a VTMS transition
> 2024:   // - there is no mechanism to disable transitions of a specific carrier thread yet
> 2025:   JvmtiVTMSTransitionDisabler disabler(is_virtual ? target : nullptr); // nullptr is to disable all

We have a number of places with the same issue - `JvmtiVTMSTransitionDisabler disabler(target)` when target thread can be virtual or platform.
I think they need to be fixed all together (and most likely as a separate issue).
Maybe it would be better to fix disabler itself (check if the thread provided is platform and disable transitions for all threads in the case). Then there is no need to update all this places when (if) disabling for single platform thread is implemented

src/hotspot/share/prims/jvmtiExport.cpp line 1814:

> 1812:   }
> 1813:   // pure continuations have no VTMS transitions but they use methods annotated with JvmtiMountTransition
> 1814:   if ((mh->jvmti_mount_transition() && state->is_virtual()) || thread->is_in_any_VTMS_transition()) {

Could you please explain the change (and other similar changes in jvmtiExport.cpp)
Before events were not posted for any `@JvmtiMountTransition` methods, and now only for virtual threads? I.e. events for `@JvmtiMountTransition` methods of carrier threads are posted?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1801933054
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1801953309
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1801992736
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1802034030