RFR: 8330253: Skip verify_consistent_lock_order when deoptimizing from monitorenter bytecode. [v2]

[Daniel D. Daugherty]

On Wed, 17 Apr 2024 05:41:26 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> The verification added in [JDK-8329757](https://bugs.openjdk.org/browse/JDK-8329757) will not work then deoptimization occurs on a monitorenter bytecode. The locking may be in a transitional state. This patch will skip the verification when this occurs.
>> 
>> Currently have only seen this reproduce with JVMTI when deoptimization occurs while a java thread is waiting on a contended monitor. However this could potentially be triggered from a VM entry slow path, so simply checking `current_pending_monitor` could be flaky as well. So instead simply avoid verification.
>> 
>> Running JVMTI reproducer. Starting full testing soon.
>
> Axel Boldt-Christmas has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Handle previous bc being monitorenter
>  - Remove implicit conditions

Changes requested by dcubed (Reviewer).

src/hotspot/share/runtime/deoptimization.cpp line 443:

> 441:   }
> 442: #ifdef ASSERT
> 443:   if (LockingMode == LM_LIGHTWEIGHT && !realloc_failures) {

In the new code, you are no longer account for `realloc_failures` being true.
I'm not convinced that is okay here.

src/hotspot/share/runtime/deoptimization.cpp line 449:

> 447:                                                        : chunk->first()->method()->code_at(bci - 1) != Bytecodes::_monitorenter;
> 448:     const bool is_syncronized_entry = chunk->first()->method()->is_synchronized() &&
> 449:                                       chunk->first()->raw_bci() == SynchronizationEntryBCI;

nit typo: s/is_syncronized_entry/is_synchronized_entry/

src/hotspot/share/runtime/deoptimization.cpp line 450:

> 448:     const bool is_syncronized_entry = chunk->first()->method()->is_synchronized() &&
> 449:                                       chunk->first()->raw_bci() == SynchronizationEntryBCI;
> 450:     // If deoptimizing from monitorenter bytecode we maybe in transitional state. Skip verification.

nit typo: s/we maybe/we may be/
nit typo: s/in transitional state/is a transitional state/

src/hotspot/share/runtime/deoptimization.cpp line 451:

> 449:                                       chunk->first()->raw_bci() == SynchronizationEntryBCI;
> 450:     // If deoptimizing from monitorenter bytecode we maybe in transitional state. Skip verification.
> 451:     // When reexecuting the current bc, the previous bc may not have finished yet.

Should this:
`... the previous bc may not have finished yet.`
be:
`... the previous monitorenter bc may not have finished yet.`

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

PR Review: https://git.openjdk.org/jdk/pull/18782#pullrequestreview-2006612177
PR Review Comment: https://git.openjdk.org/jdk/pull/18782#discussion_r1569166375
PR Review Comment: https://git.openjdk.org/jdk/pull/18782#discussion_r1569159605
PR Review Comment: https://git.openjdk.org/jdk/pull/18782#discussion_r1569160377
PR Review Comment: https://git.openjdk.org/jdk/pull/18782#discussion_r1569168751