RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake

[David Holmes]

On 23/07/2020 1:13 pm, Yasumasa Suenaga wrote:
> Hi David,
> 
> Thanks for your comment!
> 
> On 2020/07/23 11:01, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 23/07/2020 12:38 am, Yasumasa Suenaga wrote:
>>> Hi all,
>>>
>>> Please review this change:
>>>
>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8248362
>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8248362/webrev.00/
>>>
>>> Migrate JVMTI frame operations to Thread-Local Handshakes from VM 
>>> Operations.
>>>
>>>      - VM_GetFrameCount
>>>      - VM_GetFrameLocation
>>>
>>> They affects both GetFrameCount() and GetFrameLocation() JVMTI 
>>> functions.
>>
>> Your changes all seem fine.
>>
>> But I'm a bit confused about the existing code. In 
>> JvmtiEnv::GetFrameLocation we have:
>>
>>     if (java_thread == JavaThread::current()) {
>>       err = get_frame_location(java_thread, depth, method_ptr, 
>> location_ptr);
>>
>> but then in get_frame_location we have:
>>
>>     assert((SafepointSynchronize::is_at_safepoint() ||
>>             java_thread->is_thread_fully_suspended(false, &debug_bits)),
>>             "at safepoint or target thread is suspended");
>>
>> and that assert must surely fire if called by the current thread for 
>> itself! ???
> 
> is_thread_fully_suspended() returns true when it calles from current 
> thread, so this assert would not fire.

I had not realized it was defined that way. Seems like a bit of a kludge 
to avoid making the call conditional on a check of the current thread. :(

Cheers,
David

> 
> Yasumasa
> 
> 
>> Thanks,
>> David
>>
>>> This change has passed all tests on submit repo 
>>> (mach5-one-ysuenaga-JDK-8248362-20200722-1249-12859056), and 
>>> vmTestbase/nsk/{jdi,jdw,jvmti}, serviceability/{jdwp,jvmti} .
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa