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

[Robbin Ehn]

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