RFR (S) 5103339: Strengthen NoSafepointVerifier

[David Holmes]

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

src/hotspot/share/oops/objArrayKlass.cpp
src/hotspot/share/oops/typeArrayKlass.cpp

Unclear why the check is predicated on !or_null ??

---

src/hotspot/share/code/vtableStubs.cpp

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

Unclear what "code" has the NSV ??

---

src/hotspot/share/runtime/interfaceSupport.inline.hpp

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

---

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

---

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?

Thanks,
David
-----

> Thanks,
> Coleen