RFR: 8313202: MutexLocker should disallow null Mutexes [v5]

[Aleksey Shipilev]

On Sun, 3 Sep 2023 13:46:13 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8313202-mutexlocker-nulls
>>  - Merge branch 'master' into JDK-8313202-mutexlocker-nulls
>>  - Accept one more potentially nullptr mutex
>>  - Merge branch 'master' into JDK-8313202-mutexlocker-nulls
>>  - Replace ReentrantMutexLocker with ConditionalMutexLocker
>>  - Workaround for JDK-8313210
>>  - Fixing CodeCache analytics
>>  - Initial work
>
> src/hotspot/share/code/stubs.cpp line 241:
> 
>> 239: 
>> 240: void StubQueue::print() {
>> 241:   ConditionalMutexLocker lock(_mutex, _mutex != nullptr, Mutex::_no_safepoint_check_flag);
> 
> Why is this one now a `ConditionalMutexLocker`?

I think there was a test failure that indicated we enter here from some error path? Let me reproduce it again.

> src/hotspot/share/runtime/mutexLocker.hpp line 277:
> 
>> 275: 
>> 276:   MonitorLocker(Thread* thread, Monitor* monitor, Mutex::SafepointCheckFlag flag = Mutex::_safepoint_check_flag) :
>> 277:     MutexLocker(thread, monitor, flag), _flag(flag) {}
> 
> The assert will now be: `"null mutex is not allowed"`
> instead of `"null monitor not allowed"`. Not really a
> problem, just trying to make it clear.

Right. I think there is no reason to sub-class `MutexLocker` and trying to save the line of code there. Instead, let's subclass `MutexLockerImpl`, and do the proper asserts. New commit reinstates the asserts and some adjacent comments in `MonitorLocking`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15043#discussion_r1314693244
PR Review Comment: https://git.openjdk.org/jdk/pull/15043#discussion_r1314692724