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

[David Holmes]

On Thu, 3 Apr 2025 17:55:16 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> David, we had an internal Zoom meeting with Patricio and agreed on a couple of points (I hope, Patricio will fix me if needed).
>> #1:
>> The main (biggest) problem is the call to `JvmtiEventController::thread_started()` in the function `get_jvmti_thread_state()`. The function `JvmtiThreadState::state_for()` is called there if a mounted virtual thread is the current thread and any thread-filtered event is enabled globally. The function `JvmtiThreadState::state_for()` has this line: `MutexLocker mu(JvmtiThreadState_lock);`
>> 
>> The `JvmtiThreadState_lock` allows safepointing. It is where a JVMTI suspend request for this thread has been processed/registered. The `SingleStep` events for this thread are also enabled after the suspend request was processed/registered. However, suspends are not allowed by this `MutexLocker`.
>> This is very common code path for any thread-filtered event (e.g. the event can be enabled for a specific thread).
>> 
>> #2:
>> This PR fixes this exact issue with this update:
>> 
>> JvmtiThreadState* JvmtiExport::get_jvmti_thread_state(JavaThread *thread) {
>>   assert(thread == JavaThread::current(), "must be current thread");
>>   if (thread->is_vthread_mounted() && thread->jvmti_thread_state() == nullptr) {
>>     JvmtiEventController::thread_started(thread);
>> +    if (thread->is_suspended()) {
>> +      // suspend here if there is a suspend request
>> +      ThreadBlockInVM tbivm(thread);
>> +    }
>>   }
>>   . . .
>> 
>> 
>> But we also agreed to add a tweak for the function `post_dynamic_code_generated_while_holding_locks()` to disallow suspension in this context. Then it will look like this:
>> 
>> +JvmtiThreadState* JvmtiExport::get_jvmti_thread_state(JavaThread *thread, bool allow_suspend) {
>>   assert(thread == JavaThread::current(), "must be current thread");
>>   if (thread->is_vthread_mounted() && thread->jvmti_thread_state() == nullptr) {
>>     JvmtiEventController::thread_started(thread);
>> +    if (allow_suspend && thread->is_suspended()) {
>> +      // suspend here if there is a suspend request
>> +      ThreadBlockInVM tbivm(thread, true /* allow suspend */);
>> +    }
>>   }
>>   . . .
>> 
>> void JvmtiExport::post_dynamic_code_generated_while_holding_locks(const char* name,
>>                                                                   address code_begin, address code_end)
>> {
>>   . . .
>>   JvmtiThreadState *state = get_jvmti_thread_state(thread, false /* allow_suspend */);
>>   . . .
>> 
>> 
>> I have already pushed the minor tweak above....
>
> Thanks for the summary Serguei. I would only add that IMO, given that it’s already clear there could be many suspend points before even reaching one of this `JvmtiExport::post_*` methods (like the one shown in #3), I would just address all these cases here too my moving the suspend check outside of the `thread->is_vthread_mounted() && thread->jvmti_thread_state() == nullptr` conditional. But since this PR has already been blocked for too long I’m okay if we only want to address the specific suspend point in #1.

I thought we already guarded/handled suspension whilst acquiring a mutex, as a general case. ??? Now I am getting really confused about the state of the world.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23490#discussion_r2034142625