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

[David Holmes]

On 20/12/2019 5:20 am, coleen.phillimore at oracle.com wrote:
> 
> I see you're already pushed this, which is fine.  I have a question 
> though.  Why is this a special case?  Why don't we have to do this after 
> all VMThread::execute(&vmop) ?   I was going to see if you could fix the 
> typo here, but too late, so never mind.
> 
>   161   VM_ICBufferFull ibf;
>   162   VMThread::execute(&ibf);
>   163   // We could potential get an async. exception at this point.
>   164   // In that case we will rethrow it to ourselvs.
>   165   if (HAS_PENDING_EXCEPTION) {
>   166     oop exception = PENDING_EXCEPTION;
>   167     CLEAR_PENDING_EXCEPTION;
> 168 JavaThread::current()->set_pending_async_exception(exception);
>   169   }

This code shifts the exception from being a pending exception (that a 
CHECK macro would respond to) to be a "pending async exception" (which 
will only get promoted to being a proper pending exception when specific 
thread-state transitions occur). This allows the calling native code to 
proceed without a pending exception potentially getting in the way.

Presumably this code is special in this regard. In general we would 
handle any pending exception by doing quick returns so that the 
exception will actually reach Java code asap.

David

> Thanks,
> Coleen
> 
> On 12/18/19 6:42 AM, Robbin Ehn wrote:
>> 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
>