RFR (S) 5103339: Strengthen NoSafepointVerifier

[coleen.phillimore at oracle.com]

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.

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