RFR: 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING [v4]

[Alex Menkov]

On Wed, 7 Jun 2023 21:52:33 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 765:
>> 
>>> 763:   if (is_thread_carrying_vthread(jt, thread_oop)) {
>>> 764:     state &= ~JVMTI_THREAD_STATE_RUNNABLE;
>>> 765:     state |= JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY;
>> 
>> This does not look correct.
>> GetThreadState spec provides hierarchical set of questions to interpret thread state value.
>> JVMTI_THREAD_STATE_ALIVE | JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY is only one branch and I'd expect all other bits are not set for this state.
>> Need to decide what do we want to report as carrier thread state for all possible values returned by get_thread_state_base().
>
> Good concern.
> There are two bits (and the related RUNNABLE bit) that we care in this sub-tree of state bits: `SUSPENDED` and `INTERRUPTED`. This update clones these two bits. The RUNNABLE bit must be cleared.
> A thread carrying a virtual thread can not be in native, blocked, parked, sleeping or waiting on some object.
> The state returned by the `get_thread_state_base` is based on the call:
> `    state = (jint)java_lang_Thread::get_thread_status(thread_oop);`
> and addition of the derived from JavaThread bits: `SUSPENDED`, `INTERRUPTED` and `IN_NATIVE`.
> The three bits derived from the JavaThread are not relevant.
> This call has to be made directly:
> `   state = (jint)java_lang_Thread::get_thread_status(thread_oop);`
> The SUSPEND bit has to be based on the call:
> `    jt->is_carrier_thread_suspended();`
> 
> The function `get_thread_state` will look as below:
> 
>   if (is_thread_carrying_vthread(jt, thread_oop)) {
>     jint state = (jint)java_lang_Thread::get_thread_status(thread_oop);
>     if (jt->is_carrier_thread_suspended()) {
>       state |= JVMTI_THREAD_STATE_SUSPENDED;
>     }
>     // It's okay for the JVMTI state to be reported as WAITING when waiting
>     // for something other than an Object.wait. So, we treat a thread carrying
>     // a virtual thread as waiting indefinitely which is not runnable.
>     // It is why the RUNNABLE bit is cleared and the WAITING bits are added.
>     state &= ~JVMTI_THREAD_STATE_RUNNABLE;
>     state |= JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY;
>     return state;
>   } else { 
>     return get_thread_state_base(thread_oop, jt);
>   }

Do you need to check jt->is_interrupted(false) and set INTERRUPTED bit?
It looks like java_lang_Thread::get_thread_status(thread_oop) can only return RUNNABLE in the case and we clear it, so the call is not needed:

    if (is_thread_carrying_vthread(jt, thread_oop)) {
      jint state = JVMTI_THREAD_STATE_WAITING | JVMTI_THREAD_STATE_WAITING_INDEFINITELY;
      if (jt->is_carrier_thread_suspended()) {
        state |= JVMTI_THREAD_STATE_SUSPENDED;
      }
      if (jt->is_interrupted(false)) {
        state |= JVMTI_THREAD_STATE_INTERRUPTED;
      }
      return state;
    } else ...

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14366#discussion_r1222252628