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

[coleen.phillimore at oracle.com]

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   }

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