RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake

[David Holmes]

<trimming>

On 26/06/2020 11:31 am, Yasumasa Suenaga wrote:
> On 2020/06/25 21:48, David Holmes wrote:
>>>> You are not checking the return value of Handshake::execute_direct 
>>>> and so are missing the possibility that the target thread has 
>>>> terminated before you got to do the operation on it. It isn't clear 
>>>> to me under what other circumstances execute_direct can also return 
>>>> false.
>>>
>>> I will add it. According to Handshake::execute_direct() and 
>>> HandshakeOperation::do_handshake(), it seems to return false if the 
>>> target thread has terminated as you said.
>>
>> Yes, but also if the handshake is not executed - but I don't know 
>> under what conditions that can occur.
>>
>>>
>>>> You don't seem to have these checks anymore in some places:
>>>>
>>>>    && !_java_thread->is_exiting() && _java_thread->threadObj() != NULL)
>>>>
>>>> why not?
>>>
>>> I thought the thread which enters handshake is always alive and it 
>>> has threadObj.
>>
>> As far as I can see we can still engage in a handshake with a thread 
>> after it has marked itself as exiting.
> 
> I think the handshake should not be run if its state is exiting because 
> we can deem it as "dead".
> What do you think?

The thread is marked as exiting fairly early in its termination path and 
can still interact with oops after that point so we must continue to 
obey all safety protocols in relation to handshakes and safepoints. I 
think it is up to the handshake operation to check that the target 
thread is in a suitable state for processing.

> 
>> The threadObj() can only be null while a thread is attaching, which 
>> means it would have to checked in the general case, but for these JVM 
>> TI operations if we already have a jthread reference to the target 
>> thread then it must be beyond that point. Mind you that same logic 
>> applies to the existing code so ...
>>
>>> I will recover their conditions.
>>> (I also should recover them for GetOwnedMonitorInfoClosure and 
>>> GetCurrentContendedMonitorClosure - I removed them in JDK-8242425)
>>
>> I think so - and we need to check the return value of execute_direct 
>> to determine when to report JVMTI_ERROR_THREAD_NOT_ALIVE.
> 
> I will file it to JBS.
> We can get the result from result() in their Closures. 
> JVMTI_ERROR_THREAD_NOT_ALIVE is set by default in 
> GetCurrentContendedMonitorClosure, so we can get this error if the 
> handshake is not completed.
> Should I check result of execute_direct() even if that?
> (Of course, we should fix GetOwnedMonitorInfoClosure and threadObj() check)

I think it is a little bit too subtle to rely on a default setting for 
the result (and begs the question why GetOwnedMonitorInfoClosure doesn't 
also set result to JVMTI_ERROR_THREAD_NOT_ALIVE?). I think we should be 
establishing a common pattern for writing these Handshake closures and 
the related operation, in a clear, correct way.

> 
>>>> It is not clear that all the code that previously could execute at a 
>>>> safepoint, due to being called from a VM_Operation, is still 
>>>> executable at a safepoint e.g. JvmtiThreadState::count_frames()
>>>>
>>>>> GetThreadListStackTrace() uses direct handshake if thread count == 
>>>>> 1. In other case (thread count > 1), it would be performed as VM 
>>>>> operation (VM_GetThreadListStackTraces).
>>>>
>>>> This introduces a large chunk of duplicated code for the frame fill 
>>>> in and final allocation. Can you not reuse the existing logic that 
>>>> does this - and in the process do away with the the use of 
>>>> _needs_thread_state? I really wanted to see simpler code after this 
>>>> conversion.
>>>>
>>>> I'm also wondering whether we can hide all this logic in the 
>>>> closure, as was done with the VM_Operation i.e.
>>>>
>>>> *stack_info_ptr = op.stack_info();
>>>
>>> I will try to refactor this change.
>>>
>>>
>>>>> Caller of VM_GetCurrentLocation 
>>>>> (JvmtiEnvThreadState::reset_current_location()) might be called at 
>>>>> safepoint. So I added safepoint check in its caller.
>>>>
>>>> I could not figure out what you were referring to here.
>>>
>>> I guess following callpath is available:
>>>
>>> VM_GetCurrentLocation
>>>    JvmtiEnvThreadState::reset_current_location()
>>>      JvmtiEventControllerPrivate::recompute_env_thread_enabled()
>>>        JvmtiEventControllerPrivate::recompute_thread_enabled()
>>>          JvmtiEventControllerPrivate::set_frame_pop()
>>>            JvmtiEventController::set_frame_pop()
>>>              JvmtiEnvThreadState::set_frame_pop()
>>>                VM_SetFramePop::doit()
>>>
>>> However, VM_SetFramePop seems not to allow nested VM operations.
>>
>> It is the outer operation that has to allow nesting but 
>> VM_GetCurrentLocation doesn't allow it either. So if this path is 
>> possible then something is broken.
> 
> I'm not sure this path would be happen. However following comments are 
> left in the code:
> 
> ```
>        // The java thread stack may not be walkable for a running thread
>        // so get current location at safepoint.
>        VM_GetCurrentLocation op(_thread);
> ```

I'm not sure what point is being made with all this. It's not safe to 
ask a running thread for the current location, so it must be done via a 
safepoint VM operation, or now a direct handshake operation with the thread.

Thanks,
David
-----

> 
> Thanks,
> 
> Yasumasa