RFR 8213150: Add verification for locking by VMThread

[coleen.phillimore at oracle.com]

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