RFR: 8276112: Inconsistent scalar replacement debug info at safepoints [v2]

[Tobias Hartmann]

On Thu, 11 Nov 2021 11:02:20 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> [JDK-8261137](https://bugs.openjdk.java.net/browse/JDK-8261137) introduced aggressive scalar replacement of primitive boxes during incremental inlining if the box is only referenced by safepoint debug info:
>> https://github.com/openjdk/jdk/blob/e01d6d00bc4ab5ca0d38f8894a78e6d911e0fe93/src/hotspot/share/opto/callGenerator.cpp#L678-L683
>> 
>> It works by replacing safepoint usages by `SafePointScalarObject` nodes and adjusting the JVMState accordingly. For example, in `TestSafepointDebugInfo::test1` the `helper` method in line 56 is inlined and the box result of `Integer.valueOf` is scalar replaced in the safepoint debug info:
>> 
>> 
>>  315  SafePointScalarObject  ===  0  [[ 40  316  319  337 ]]  # fields@[0..0]  Oop:java/lang/Integer:NotNull:exact * !jvms: 
>>  316  SafePoint  ===  5  6  317  8  1  1  1  315  10  1  10  [[]]  SafePoint  !jvms: TestSafepointDebugInfo::test1 @ bci:-1 (line 56)
>> JVMS depth=1 loc=6 stk=8 arg=8 mon=10 scalar=10 end=11 mondepth=0 sp=0 bci=6 reexecute=false method=static jint compiler.eliminateAutobox.TestSafepointDebugInfo.test1(jint)
>> 
>> 
>> The `scalar` offset in the JVMState points to the integer field in the debug info. The problem is now that additional inlining can happen afterwards "on top of" this JVMState. In this case, the call to `Integer.valueOf` in line 57 is inlined, leading to the following JVMState for the inlined callee:
>> 
>> 
>>  315  SafePointScalarObject  ===  0  [[ 40  316  319  337 ]]  # fields@[0..0]  Oop:java/lang/Integer:NotNull:exact * !jvms: 
>>  316  SafePoint  ===  5  6  317  8  1  1  1  315  10  1  10  [[]]  SafePoint  !jvms: TestSafepointDebugInfo::test1 @ bci:-1 (line 56)
>> JVMS depth=1 loc=6 stk=8 arg=8 mon=10 scalar=10 end=11 mondepth=0 sp=0 bci=6 reexecute=false method=static jint compiler.eliminateAutobox.TestSafepointDebugInfo.test1(jint)
>> JVMS depth=2 loc=5 stk=6 arg=8 mon=10 scalar=10 end=10 mondepth=0 sp=2 bci=3 reexecute=false method=static jobject java.lang.Integer.valueOf(jint)
>> ``` 
>> 
>> In this simple case, both caller and (inlined) callee state share the same `SafePointNode`. However, the `scalar` offset in the JVMState of the callee (depth 2) is not correct anymore and out of bounds.
>> 
>> Parsing then emits an `unstable_if` trap and `GraphKit::add_safepoint_edges` merges above states to:
>> 
>>  315  SafePointScalarObject  ===  0  [[ 40  316  319  337 ]]  # fields@[0..0]  Oop:java/lang/Integer:NotNull:exact * !jvms: 
>>  337  CallStaticJava  ===  332  1  7  8  1 ( 336  1  315  10  10  10  328 ) [[]] # Static uncommon_trap(reason='unstable_if' action='reinterpret' debug_id='0')  void ( int ) Integer::valueOf @ bci:3 (line 1075) reexecute TestSafepointDebugInfo::test1 @ bci:6 (line 57) !jvms: Integer::valueOf @ bci:3 (line 1075) TestSafepointDebugInfo::test1 @ bci:6 (line 57)
>> JVMS depth=1 loc=6 stk=8 arg=8 mon=8 scalar=8 end=9 mondepth=0 sp=0 bci=6 reexecute=false method=static jint compiler.eliminateAutobox.TestSafepointDebugInfo.test1(jint)
>> JVMS depth=2 loc=9 stk=10 arg=12 mon=12 scalar=12 end=12 mondepth=0 sp=2 bci=3 reexecute=true method=static jobject java.lang.Integer.valueOf(jint)
>> 
>> 
>> We then crash in `PhaseOutput::FillLocArray` when processing the `SafePointScalarObject` and trying to access the corresponding field in the debug info at (out of bounds) offset 12:
>> https://github.com/openjdk/jdk/blob/e01d6d00bc4ab5ca0d38f8894a78e6d911e0fe93/src/hotspot/share/opto/output.cpp#L833-L836
>> 
>> Even worse, in some scenarios we don't crash/assert but emit incorrect debug info leading to wrong results after deoptimization. For example, `TestSafepointDebugInfo::test2` fails because the `SafePointScalarObject` for `box1` and `box2` point to the same field in the debug info. This can happen if scalar replacement happens again "on top of" an already inconsistent JVMState. Afterwards, the out of bounds offset accidentally points to the field of the newly scalarized object.
>> 
>> Originally, this issue only reproduced intermittently with a long running internal stress test but I was able to extract a set of simple regression tests that trigger different failure modes in the compiler or wrong execution.
>> 
>> I think fixing this is complicated and I therefore propose to disable [JDK-8261137](https://bugs.openjdk.java.net/browse/JDK-8261137) for now and file an enhancement to fix and re-enable it later. We should then also add proper verification code and more complete tests. 
>> 
>> Thanks,
>> Tobias
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Removed unnecessary inline compile command

I filed [JDK-8276998](https://bugs.openjdk.java.net/browse/JDK-8276998) to re-implement this.

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

PR: https://git.openjdk.java.net/jdk/pull/6333