RFR (S) 5103339: Strengthen NoSafepointVerifier

[Robbin Ehn]

Hi Coleen,

On 8/6/19 10:38 PM, coleen.phillimore at oracle.com wrote:
> 
> Hi Robbin,
> 
> On 8/6/19 3:45 AM, Robbin Ehn wrote:
>> Hi Coleen,
>>
>> On 2019-08-05 18:00, coleen.phillimore at oracle.com wrote:
>>> Summary: Add NSV check at possible safepoint transition or places that could 
>>> take out locks. Consolidate with clearing unhandled oops.
>>
>> I would like us to instead poison the poll check it's self.
>> So any polls between A and B will just assert directly.
>> It doesn't matter if there is actually a safepoint or not, the poll is not
>> allowed. check_for_valid_safepoint_state is also only checked for safepoint,
>> not handshakes. Handshakes should not be allowed either.
>>
>> E.g.
>> bool SafepointMechanism::local_poll_armed(JavaThread* thread) {
>>   assert(thread->poll_allowed(), "Hit poll site");
>>
>> What you think?
> 
> When I started working on this, this was my intention.   But I looked at the 
> callers of local_poll_armed, which are SafepointMechanism::should_block() and 
> SafepointMechanism::block_if_requested() and there was already a more extensive 
> safepoint check in all the higher level callers.  For example, in memory 
> allocation, the safepoint check is in check_for_valid_safepoint_state, which 
> checks that the caller has no NoSafepointVerifier even if the allocation doesn't 
> result in a safepoint poll.  Also some selected 'else' clauses should be checked 
> for safepoints allowed, and more could be added when we run into them.  The 
> missing 'else' clause checks were what we found were missed with the clear 
> unhandled oops code.
> 
> So the only place that was missing was a transition from or *back* to 
> thread_in_vm, which needs to clear the unhanded oops and check for 
> NoSafepointVerifier.  The other transitions don't make sense to check because 
> the NoSafepointVerifier is in vm code.  That's why it was added there.
> 
> For now, the handshake code goes through a VMOperation, which has a 
> check_for_valid_safepoint_state() so it does get checked for NSV.
> 
> I wrote up a complete list of places where should_block and block_if_requested 
> are called and why where NSV checking from my change is better.  I'll send it to 
> you.
> 
> I'm attracted to the simplicity of this idea but I don't think it's something we 
> want to check more than once and it will miss places we do want to check.
> 

Ok, thanks for looking into it!

/Robbin

> Thanks,
> Coleen
> 
> 
>>
>> /Robbin
>>
>>>
>>> This change checks for NoSafepointVerifier no_safepoint_counts at possible 
>>> safepoints.  The starting set is at transitions, and in the "else" clauses 
>>> where CHECK_UNHANDLED_OOPS were clearing unhandled oops. Some of these were 
>>> removed because they weren't places with possible safepoints, so were wrong.
>>>
>>> The unhandled oops clearing and no_safepoint counter check are now done in 
>>> the same function.  MemAllocator -> check_for_valid_allocation_state calls 
>>> check_for_valid_safepoint_state which calls check_possible_safepoint.
>>>
>>> Calls to check_possible_safepoint are in DEBUG_ONLY when Thread::current() is 
>>> called.
>>>
>>> I had to remove it from the else clause in JvmtiThreadState because it's 
>>> called from a place that cannot safepoint (see vtableStubs.cpp).
>>>
>>> os.cpp ResourceMark needed for debugging.
>>>
>>> Tested with tier1 on all Oracle platforms, and tier 1-3 on linux-x64-debug.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/2019/5103339.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-5103339
>>>
>>> Thanks,
>>> Coleen
>