RFR: 8316746: Top of lock-stack does not match the unlocked object

[Martin Doerr]

On Tue, 24 Oct 2023 12:19:04 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

> In case of OSR compilations, `map()->peek_monitor_obj()` may return the objects in wrong order. We should use the popped object. I'm not changing the code for `LM_LEGACY`. That may be done separately.
> Note that C1 uses the popped object, too: `case Bytecodes::_monitorexit    : monitorexit (apop(), s.cur_bci());`
> 
> This PR is not complete: A lot of C2 code uses the BoxLock. Ideas to solve that are welcome! I think we should get rid of these nodes in the long term because they are only really needed by `LM_LEGACY`.
> The vmTestbase/nsk/jdi/StepEvent tests are passing stable on x64 and ppc64.

This is my understanding of the problem:

The locked object can be found at two places in the OSR buffer: 
1. In the copy of the interpreter frame's monitor section:
https://github.com/openjdk/jdk/blob/3cea892bd464566eef5590d2930b2e0adf2c2874/src/hotspot/share/opto/parse1.cpp#L233
It can be retrieved by `map()->peek_monitor_obj()` in `Parse::do_monitor_exit()`.
2. In the copy of the interpreter frame's locals:
https://github.com/openjdk/jdk/blob/3cea892bd464566eef5590d2930b2e0adf2c2874/src/hotspot/share/opto/parse1.cpp#L310
It can be retrieved by `pop();` in `Parse::do_monitor_exit()`.

Normally, both ways should deliver the right object because the interpreter handles both structures like a kind of stack if the bytecode is well-formed regarding the monitors (which is checked by the JIT compilers). However, I believe that it's possible to mess things up by using a debugger. Note that the failing test is vmTestbase/nsk/jdi/StepEvent.

The interpreter uses the 2nd variant for unlocking (atos is the object to unlock) and doesn't care about the order of 1.  C1 does the same by `monitorexit (apop()...` which explains why the issue is not reproducible by -XX:TieredStopAtLevel=1. C2 uses the 1st variant and the test fails on PPC64. (I guess that the OSR compilation happens to run slightly different on other platforms. E.g. preferring uncommon trap.) This PR suggests to follow what the interpreter and C1 are doing. This makes the problem disappear.

The idea of "JDK-8316746-reproducer.patch" (in the JBS issue) is to show that the objects in 1. are not ordered the way as C2 needs them on x64.

The question is if we can somehow make the order of 1. correct, but I think it will be more robust to follow the interpreter and C1.

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

PR Comment: https://git.openjdk.org/jdk/pull/16345#issuecomment-1780917707