RFR(s): 8235913: ThreadStop should be a handshake

[Robbin Ehn]

Hi David,

On 12/18/19 5:57 AM, David Holmes wrote:
> Incremental changes look good. I guess we will see how this works out. :)

Thanks, yes.

(did a t1-5 on inc also)

/Robbin

> 
> Thanks,
> David
> 
> On 17/12/2019 11:57 pm, Robbin Ehn wrote:
>> Hi David,
>>
>> On 12/17/19 12:39 AM, David Holmes wrote:
>>>> I even think send_thread_stop is less error-prone when 'this' is the same as 
>>>> Thread::current().
>>>
>>> I remain concerned when we suddenly will start executing code in threads that 
>>> have never before executed this code.
>>
>> This code is executed when running nsk_jvmti.
>>
>>>
>>>>>
>>>>> Do we actually have tests that exercise that particular code path? I'd also 
>>>>> check that we test JVM TI stopping of the current thread. (JVM_ThreadStop 
>>>>> is not an issue as it already special cases the current thread).
>>>>
>>>> We have several test that do:
>>>> jvmti->StopThread(thread, exception)
>>>
>>> Yes but the issue is with stopping the current thread.
>>>
>>> I'm mostly concerned that the refill_ic_stubs() code path may not be getting 
>>> tested, or even executed at all given how rarely async exceptions are used in 
>>> practice.
>>
>> If I run nsk_jvmti I hit it like 30 times.
>>
>>>
>>> When a thread executes:
>>>
>>> Handshake::execute(&vm_stop, target);
>>>
>>> does that perform any thread-state transitions or related checks that would 
>>> install the pending-async-exception as an actual pending exception? If so 
>>> that breaks the refill_ic_stubs code.
>>
>> Both VMThread::execute() and Handshake::execute() adds a VM_op on to VMThread 
>> queue and current thread goes to blocked.
>> If there is an issue, it should already be present.
>> But I think you are correct that there is an issue.
>> And I think Martin was correct, we should just re-installed it with:
>> set_pending_async_exception
>>
>> It very hard to hit code path, getting an exception in the safepoint for ic stub
>> refill.
>>
>> Inc:
>> http://cr.openjdk.java.net/~rehn/8235913/v2/inc/webrev/
>> Full:
>> http://cr.openjdk.java.net/~rehn/8235913/v2/full/webrev/
>>
>> Thanks, Robbin
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> And test like StopAtExit.java which do the java version.
>>>>
>>>> Thanks, Robbin
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Passes t1-7.
>>>>>>
>>>>>> Thanks, Robbin