RFR: 8316397: StackTrace/Suspended/GetStackTraceSuspendedStressTest.java failed with: SingleStep event is NOT expected [v8]

[David Holmes]

On Tue, 8 Apr 2025 03:20:17 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> The `get_jvmti_thread_state()` function is called from `JvmtiExport::at_single_stepping_point()`.  It can block for virtual threads. Then the `SingleStep` events can be enabled at that point. The incorrect behavior is that the `SingleStep` events will be posted even though the virtual thread has been suspended with the JVMTI `SuspendThread`, `SuspendThreadList`, or `SuspendAllVirtualThreads`. The fix is to add a suspend point for virtual threads to the `get_jvmti_thread_state()` function.
>> 
>> Testing:
>>  - Ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains nine additional commits since the last revision:
> 
>  - Merge
>  - review: add workaround for post_dynamic_code_generated_while_holding_locks
>  - Merge
>  - Merge
>  - review: move suspend point from post_single_step() to get_jvmti_thread_state()
>  - Merge
>  - removed obsolete fragment that was not removed in last update
>  - review: re-fixed the issue as initial fix was wrong
>  - 8316397: StackTrace/Suspended/GetStackTraceSuspendedStressTest.java failed with: SingleStep event is NOT expected

Okay as a point fix what has been proposed seems reasonable. I would like some addtional comments please.

But I would really like to see some high-level design documents for all the interactions here because I can't get complete sense of the big picture here. It seems we are forever playing whack-a-mole when it comes to virtual threads and JVMTI.

Thanks

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

> 426:       // suspend here if there is a suspend request
> 427:       ThreadBlockInVM tbivm(thread, true /* allow suspend */);
> 428:     }

We need some kind of comment before `thread_started` to make it clear why we need to check suspension only on this path. Something like:

// Within thread_started we could block on a VM mutex which would allow this thread to be marked as suspended,
// so in some cases we need to honor that suspend request before procedding.

src/hotspot/share/prims/jvmtiExport.hpp line 312:

> 310:   // is enabled globally then it is created.
> 311:   // Otherwise, the thread->jvmti_thread_state() is returned.
> 312:   static JvmtiThreadState* get_jvmti_thread_state(JavaThread *thread, bool allow_suspend = true);

Please add a detailed comment on what the `allow_suspend` parameter means and when it should be true and false.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23490#pullrequestreview-2751600747
PR Review Comment: https://git.openjdk.org/jdk/pull/23490#discussion_r2034158938
PR Review Comment: https://git.openjdk.org/jdk/pull/23490#discussion_r2034159526