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

[Coleen Phillimore]

On Mon, 31 Jul 2023 13:23:15 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> As seen in [JDK-8313081](https://bugs.openjdk.org/browse/JDK-8313081), it is fairly easy to pass nullptr `Mutex` to `MutexLocker` by accident, which would just silently avoid the lock.
>> 
>> There are a few places in Hotspot where we pass `nullptr` to simulate re-entrancy and/or conditionally take the lock. Those places can be more explicit, and the default `MutexLocker` can disallow nullptrs for extra safety.
>> 
>> Open for some bikeshedding on the names of the new `MutexLockers`. Particularly `ReentrantMutexLocker` might lull readers into believing it does safepoint checks on re-entrant "lock", which it actually does not do.
>> 
>> More thorough testing with different GC/JIT combinations is running now, we might find more issues there. Meanwhile, please comment on the approach.
>> 
>> Additional testing:
>>  - [x] `grep -R "MutexLocker " src/hotspot | grep -i null`, no hits
>>  - [x] `grep -R "MutexLocker " src/hotspot | grep -i ?`, no hits
>>  - [x] Linux AArch64 fastdebug, `tier1 tier2 tier3` (re-run in progress)
>
> 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 six additional commits since the last revision:
> 
>  - 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

This still looks like an improvement to me.

src/hotspot/share/oops/instanceKlass.cpp line 3409:

> 3407: bool InstanceKlass::remove_osr_nmethod(nmethod* n) {
> 3408:   // This is a short non-blocking critical region, so the no safepoint check is ok.
> 3409:   ConditionalMutexLocker ml(CompiledMethod_lock, !CompiledMethod_lock->owned_by_self(), Mutex::_no_safepoint_check_flag);

This is sort of an unfortunate pattern.  It's too bad we don't have a better name for this MutexLocker, but on the other hand maybe this should be fixed to take the lock at a higher place in this call stack and these conditional mutexLockers should be asserts.

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15043#pullrequestreview-1557523791
PR Review Comment: https://git.openjdk.org/jdk/pull/15043#discussion_r1280962806