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

[David Holmes]

On 16/12/2019 11:28 pm, Robbin Ehn wrote:
> Hi David,
> 
> On 12/16/19 12:28 PM, David Holmes wrote:
>> Hi Robbin,
>>
>> On 16/12/2019 7:51 pm, Robbin Ehn wrote:
>>> Hi all, please review:
>>>
>>> Making VM_ThreadStop into a handshake instead.
>>>
>>> Issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8235913
>>>
>>> Code:
>>> http://cr.openjdk.java.net/~rehn/8235913/v1/webrev/index.html
>>
>> It isn't obvious to me that the code in send_thread_stop that was 
>> previously always executed by the VMThread can be safely executed by 
>> the target JavaThread itself. In particular when executing 
>> InlineCacheBuffer::refill_ic_stubs() what state is the JavaThread in?
> 
> To run VMThread::execute() you must be in _thread_in_vm. (we assert this)

Okay that is good to know.

> 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.

>>
>> 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.

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.

Thanks,
David

> And test like StopAtExit.java which do the java version.
> 
> Thanks, Robbin
> 
>>
>> Thanks,
>> David
>>
>>> Passes t1-7.
>>>
>>> Thanks, Robbin