RFR (S) 5103339: Strengthen NoSafepointVerifier

[coleen.phillimore at oracle.com]

Thanks David for reading this.

On 8/5/19 9:47 PM, David Holmes wrote:
> Hi Coleen,
>
> Could please update the bug explaining why it was re-opened and what 
> the intended enhancement is. It was closed in 2005 as not necessary 
> because of the unhandled-oop checking.

Yes, I added a comment.
>
> On 6/08/2019 2:00 am, 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.
>>
>> This change checks for NoSafepointVerifier no_safepoint_counts at 
>> possible safepoints.  The starting set is at transitions, and in the 
>
> Wouldn't it be better placed in the actual Safepoint checking methods 
> rather than callers of those methods?

See my reply to Robbin.  We want to make the checks in places that 
conditionally may not safepoint poll, so that timing doesn't prevent 
finding when we've violated the NoSafepointVerifier check.

>
>> "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).
>
> Can you elaborate on that further please. It's not obvious, without 
> following call chains, when any code may or may not hit a safepoint 
> check.

No, it isn't.   There's quite a large call stack here, but the 
CompiledICLocker has an embedded NoSafepointVerifier, so here's the call 
stack:

V  [libjvm.so+0x16c5d05]  Thread::check_possible_safepoint()+0x55
V  [libjvm.so+0x10fb15c] 
JvmtiExport::post_dynamic_code_generated_while_holding_locks(char 
const*, unsigned char*, unsigned char*)+0x4c
V  [libjvm.so+0x17c8fac]  VtableStubs::find_stub(bool, int)+0x2ac
V  [libjvm.so+0x9dd18e]  CompiledIC::set_to_megamorphic(CallInfo*, 
Bytecodes::Code, bool&, Thread*)+0x9e
V  [libjvm.so+0x1562bb4] 
SharedRuntime::handle_ic_miss_helper_internal(Handle, CompiledMethod*, 
frame const&, methodHandle, Bytecodes::Code, CallInfo&, bool&, 
Thread*)+0x434
V  [libjvm.so+0x156d202] 
SharedRuntime::handle_ic_miss_helper(JavaThread*, Thread*)+0x372
V  [libjvm.so+0x156d7a4] 
SharedRuntime::handle_wrong_method_ic_miss(JavaThread*)+0x184

The function handle_ic_miss_helper_internal has a CompiledICLocker, 
which has an embedded NoSafepointVerifier.

This seems like a lot of code to be protected with a 
NoSafepointVerifier, but I assume that at least the find_stub code 
shouldn't get cleaned up by a safepoint, so would require it.

I also ran tests with an assert that the jvmti_thread_state() != NULL, 
just to see if it's ever NULL and it wasn't.  I would have liked to keep 
the else.
>
>> os.cpp ResourceMark needed for debugging.
>
> Interesting catch - I would have expected the RM to be higher up the 
> call if needed. How did you detect this?

Debugging, but I don't remember which thing I was debugging though.
>
>> 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
>
> src/hotspot/share/oops/objArrayKlass.cpp
> src/hotspot/share/oops/typeArrayKlass.cpp
>
> Unclear why the check is predicated on !or_null ??

If the parameter passed or_null is true, then the function never 
safepoints. I added this comment.

     // If passed or_null, this function will not try to safepoint.
     if (!or_null) THREAD->check_possible_safepoint();

This is also called with NSV with or_null true.   I can't remember 
where, I think it was the heap walker.

>
> ---
>
> src/hotspot/share/code/vtableStubs.cpp
>
> // cause a safepoint in this code that has NSV.
>
> Unclear what "code" has the NSV ??

See above.  I can change the comment to:

237 // all locks. Only post this event if a new state is not required. 
Creating a new state would
238 // cause a safepoint and the caller of this code has a 
NoSafepointVerifier.

>
> ---
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>
> As mentioned above why not put the check inside 
> SafepointMechanism::block_if_requested instead?

The other callers of block_if_requested already have a 
check_possible_safepoint call. The only caller that made sense to add 
was transition from or to vm.

>
> ---
>
> src/hotspot/share/runtime/mutex.cpp
>
> The Mutex constructor cleanup is incidental but okay. (Does make me 
> wonder about the whole Monitor construction process though ... 
> ClearMonitor doesn't need to be protected any more.)

This leaked into this change.  I'll take it out and post a different RFR 
for it.  The no-arg constructor Monitor isn't needed either so 
ClearMonitor can be deleted.
>
> ---
>
> src/hotspot/share/runtime/thread.hpp
>
> // NSV checking
> void check_for_valid_safepoint_state(bool potential_vm_operation) 
> NOT_DEBUG_RETURN;
> void check_possible_safepoint() NOT_DEBUG_RETURN;
>
> Please expand NSV. Does the comment apply to both methods or only the 
> first?

Both methods.  I'll change it to:

   // These functions check conditions on a JavaThread before possibly 
going to a safepoint,
   // including NoSafepointVerifier.
   void check_for_valid_safepoint_state(bool potential_vm_operation) 
NOT_DEBUG_RETURN;
   void check_possible_safepoint() NOT_DEBUG_RETURN;


Thanks!
Coleen
>
> Thanks,
> David
> -----
>
>> Thanks,
>> Coleen