On Wed, 7 Jun 2023 21:52:33 GMT, Serguei Spitsyn <sspit...@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
