RFR 8213150: Add verification for locking by VMThread

[David Holmes]

Hi Coleen,

Looking at version 2. Overall this seems a lot clearer regarding how the 
checks work. A few minor comments

src/hotspot/share/gc/shared/memAllocator.cpp

     // Allocation of an oop can always invoke a safepoint,
     // hence, the true argument.
!   _thread->check_for_valid_safepoint_state();

The comment needs fixing as no argument any more.

---

  src/hotspot/share/oops/constantPool.cpp

What is the impact of this change? When would we typically call this 
code at a safepoint? Can we miss an error by not doing the verification?

---

src/hotspot/share/utilities/concurrentHashTable.inline.hpp
src/hotspot/share/utilities/events.hpp

So these changes are saying that the VMThread can take these locks. Was 
it simply wrong before? (Ditto for the change to ThreadsSMRDelete_lock 
in mutexLocker.cpp).

---

src/hotspot/share/runtime/mutex.cpp

The changes between product/non-product/debug caused me quite a bit of 
confusion. IIUC previously we potentially had three versions of 
set_owner_implementation:
- trivial product version
- more complex appearing non-product version that actually degenerated 
to trivial product version
- full debug/assert version

now we only have two:
- trivial product
- full debug/assert version

in which case this doesn't need to be DEBUG_ONLY:

  466     DEBUG_ONLY(_last_owner = old_owner;)
---

Thanks,
David
-----

On 24/09/2019 4:49 am, coleen.phillimore at oracle.com wrote:
> 
> 
> On 9/23/19 9:37 AM, Robbin Ehn wrote:
>> Hi Coleen,
>>
>> src/hotspot/share/oops/constantPool.cpp
>>  561     if (access_check && k != NULL) {
>>
>> access_check must be same as !SafepointSynchronize::is_at_safepoint(), 
>> correct?
>> So either change access_check to 
>> !SafepointSynchronize::is_at_safepoint() or assert it.
> 
> I can change it to check that it's at a safepoint in the constant pool 
> function, which is better than a bool parameter.
>>
>> src/hotspot/share/runtime/mutex.cpp
>>   49   if (do_safepoint_check && thread->is_active_Java_thread()) {
>>   50     assert(((JavaThread *)thread)->thread_state() == 
>> _thread_in_vm || rank() == Mutex::special,
>>   51            "wrong thread state for using safepoint checking locks");
>>   52
>>   53     if (rank() != Mutex::special) {
>>   54       thread->check_for_valid_safepoint_state();
>>   55     }
>>   56   }
>>
>> This is not correct, you should be using _mutex->rank() <= 
>> Mutex::special.
>> But there is no point in checking it here, just check this in the 
>> constructor that special and below are never locks. (since it is a 
>> never lock we should not do_safepoint_check thus state check is not 
>> needed)
> 
> I'm going to work on the issues with 'special' locks as part of : 
> https://bugs.openjdk.java.net/browse/JDK-8184732 next, but the rewriting 
> (below) removed these two uses of this ranking check.
> 
>>
>> do_safepoint_check is checked four times (once via not_allowed), can 
>> you please re-write this method, or better yet remove the bool input.
>> The bool is hardcoded so can you just create two methods, with one 
>> helper method for the common checks. (yes there will be more code, but 
>> readable :) )
> 
> This is a good suggestion.  I've rewritten it to be a lot clearer, and 
> am retesting it now with tier1 (all Oracle platforms) and tier2,3 
> linux-x64-debug.
> 
> incremental webrev at 
> http://cr.openjdk.java.net/~coleenp/2019/8213150.02.incr/webrev
> full webrev at http://cr.openjdk.java.net/~coleenp/2019/8213150.02/webrev
> 
> Thanks,
> Coleen
> 
>>
>> Thanks, Robbin
>>
>> On 9/20/19 11:36 PM, coleen.phillimore at oracle.com wrote:
>>> Summary: extend verification for all locking not just VMOperations, 
>>> and fix CLDG lock to not be taken by VM thread.
>>>
>>> See bug comments for more details about this change.
>>>
>>> Tested with tier1 all Oracle platforms, tier2-8 linux-x64-debug.
>>>
>>> open webrev at 
>>> http://cr.openjdk.java.net/~coleenp/2019/8213150.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8213150
>>>
>>> Thanks,
>>> Coleen
>