RFR: 8329757: Crash with fatal error: DEBUG MESSAGE: Fast Unlock lock on stack [v4]

Fri Apr 12 20:18:50 UTC 2024

On Thu, 11 Apr 2024 18:22:08 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> `Deoptimization::relock_objects` may reorder locks within in the `LockStack` which are added inside the same vframe. This can be handled by the interpreter but if OSR has occurred C2 may observe this invalid order in the `LockStack`, which breaks its assumption leading to incorrect behaviour.
>> 
>> This patch functionally makes sure that the LockStack is always consistent by always inflating eliminated locks when `Deoptimization::relock_objects`  is called.
>> 
>> It also adds verification code which checks that the LockStack is consistent with the lock order observed inside the deoptimized vframes. 
>> 
>> Note: for leaf deoptimizations we have enough information to recreate a correct top of the LockStack with minimal inflations, however that should be a separate RFE. This only inflates eliminated locks so the worth of solving that may be minimal or even detrimental. 
>> 
>> Tests still running. Tier 1-7 done.
>
> Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Change to ASSERT

I'm sorry that I wasn't able to finish this review before your integration.

I started, paused and resumed a couple of times trying to get my head
wrapped around the deopt code. If the review comments seem disjointed,
I apologize.

I only have nits here. I've read the test program a couple of more times
and I still don't grok how the submitter got to that test program's logic
flow.

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

> 1656:           }
> 1657:         }
> 1658:         if (LockingMode == LM_LIGHTWEIGHT) {

I like that the core of the fix is just this line.

src/hotspot/share/runtime/lockStack.cpp line 107:

> 105: 
> 106: #ifdef ASSERT
> 107: void LockStack::verify_consistent_lock_order(GrowableArray<oop>& lock_order, bool leaf_frame) const {

Thanks for adding the verification code.

I spun my head around a bit with `exec_mode != Deoptimization::Unpack_none`
being passed as `leaf_frame`, but I think I got myself reoriented on the correct
meaning of `if (!leaf_frame) {`.

src/hotspot/share/runtime/lockStack.cpp line 127:

> 125: 
> 126:         if (VM_Version::supports_recursive_lightweight_locking()) {
> 127:           // With recursive looks there may be more of the same object

nit typo: s/looks/locks/

test/hotspot/jtreg/compiler/escapeAnalysis/Test8329757.java line 28:

> 26:  * @bug 8329757
> 27:  * @summary Deoptimization with nested eliminated and not eliminated locks
> 28:  *          caused reordered lock stacks. This can be handled by the interpreter

nit typo: s/caused/causing/

test/hotspot/jtreg/compiler/escapeAnalysis/Test8329757.java line 42:

> 40: 
> 41:     int a = 400;
> 42:     Double ddd;

This variable isn't used, but would the bug repro if it isn't there?

test/hotspot/jtreg/compiler/escapeAnalysis/Test8329757.java line 60:

> 58:             } while (--e > 0);
> 59:             }
> 60:         }

Indents are bit off here:
- L47 -> L60 should be indented by an additional four spaces to get an indent for the first `synchronized`
- L50 -> L58 should be indented by an additional four spaces to get an indent for  the for-loop

I've never seen a "do switch" formatted like this before and the switch statement's
case code needs some indenting and a `// fall-thru` comment after L55. The do-while
is missing '{' and '}' and there should be some formatting of that switch statement.

test/hotspot/jtreg/compiler/escapeAnalysis/Test8329757.java line 65:

> 63: 
> 64:     void n() {
> 65:         for (int j = 6; j < 274; ++j) q();

The `q();` should be on a line by itself and there should be '{' and '}'
for the for-loop.

test/hotspot/jtreg/compiler/escapeAnalysis/Test8329757.java line 70:

> 68:     public static void main(String[] args) {
> 69:         Test8329757 r = new Test8329757();
> 70:         for (int i = 0; i < 1000; i++) r.n();

The `r.n();` should be on a line by itself and there should be '{' and '}'
for the for-loop.

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

PR Review: https://git.openjdk.org/jdk/pull/18715#pullrequestreview-1998306513
PR Review Comment: https://git.openjdk.org/jdk/pull/18715#discussion_r1563123416
PR Review Comment: https://git.openjdk.org/jdk/pull/18715#discussion_r1563136180
PR Review Comment: https://git.openjdk.org/jdk/pull/18715#discussion_r1563098171
PR Review Comment: https://git.openjdk.org/jdk/pull/18715#discussion_r1563137396
PR Review Comment: https://git.openjdk.org/jdk/pull/18715#discussion_r1563121460
PR Review Comment: https://git.openjdk.org/jdk/pull/18715#discussion_r1563117744
PR Review Comment: https://git.openjdk.org/jdk/pull/18715#discussion_r1563119677
PR Review Comment: https://git.openjdk.org/jdk/pull/18715#discussion_r1563119187