RFR (S) 5103339: Strengthen NoSafepointVerifier

David Holmes david.holmes at oracle.com
Tue Aug 6 01:47:43 UTC 2019

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.

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?

> "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.

> 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?

> 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


Unclear why the check is predicated on !or_null ??



// cause a safepoint in this code that has NSV.

Unclear what "code" has the NSV ??



As mentioned above why not put the check inside 
SafepointMechanism::block_if_requested instead?



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.)



// NSV checking
void check_for_valid_safepoint_state(bool potential_vm_operation) 
void check_possible_safepoint() NOT_DEBUG_RETURN;

Please expand NSV. Does the comment apply to both methods or only the first?


> Thanks,
> Coleen

More information about the hotspot-runtime-dev mailing list