RFR: 8331194: NPE in ArrayCreationTree.java with -XX:-UseCompressedOops [v4]

[Emanuel Peter]

On Thu, 11 Jul 2024 20:04:42 GMT, Cesar Soares Lucas <cslucas at openjdk.org> wrote:

>> Please, review this PR to fix issue in debug information serialization encountered when RAM reduces a Phi for which one of the inputs is an object already scalar replaced.
>> 
>> **Details:**
>> 
>> Consider class `Picture` that has two reference fields, `first` and `second` of type `Point`. In a random method in the application an object `obj` of this class is created, and the fields of this object are initialized such that `first` is assigned a new object whereas `second` receives the output of a `Phi` node merging the object assigned to `first` and some other allocation. Also, assumes `obj` is used as debug information in an `uncommon_trap` and none of these objects escapes. I.e., we have a scenario like this:
>> 
>> 
>> Picture obj       = new Picture(); // allocation A
>>         obj.first = new Point(); // allocation B
>> 
>> Point    p2    = obj.first;
>> if (<cond>) p2 = new Point(); // allocation C
>> 
>> obj.second = p2;
>> 
>> <trap>
>> 
>> 
>> After one iteration of EA+SR, Allocation `A` will be scalar replaced and debug information in `<Trap>` adjusted accordingly. The description of field `second` in the debug information on `<Trap>` will, however, still involve a `Phi` node between allocation `B` and `C`. In the next iteration of EA+SR the `Phi` node for field `second` will be reduced by RAM and debug information will be adjusted accordingly. So far nothing is wrong.
>> 
>> The issue happens because the existing code in `Process_OopMap_Node` to serialize debug information was missing a check. Simply, the check for setting `is_root` of an `ObjectValue` wasn't checking that the `ObjectValue` might be a description of a field of a scalar replaced object. It was only checking whether `ObjectValue` was a local, stack or monitor. Consequently, the allocation assigned to `obj.first` (yes, `first`) was **not** being marked as _root_.
>> 
>> But the issue only manifested if the `<trap>` was exercised **AND** the result of `<cond>` was _true_. If the result of `<cond>` was `false` when the trap was exercised, then no problem would happen. The reason is, when `<cond>` is _true_ the `select` method in [ObjectMergeValue](https://github.com/openjdk/jdk/blob/1472124489c841642996ae984e21c533ffec8091/src/hotspot/share/code/debugInfo.cpp#L237) would flag, correctly, that the allocation inside the `if` needs to be rematerialized and the other input of the ObjectMergeValue shouldn't be rematerialized because `_is_root == false`, meaning it's just a candidate for rematerialization....
>
> Cesar Soares Lucas has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Revert moving tests.
>  - Revert moving tests. Another rename.

test/hotspot/jtreg/compiler/escapeAnalysis/TestReduceAllocationAndNestedScalarized.java line 31:

> 29:  *          needs to be rematerialized because it's used regardless of the
> 30:  *          Phi output.
> 31:  * @requires vm.compiler2.enabled

Again, I think you do not neet do restrict test execution to C2. The `requires` does not make C2 appear magically, but rather it just means the test is not executed if C2 is not available. So why restrict the test? Maybe some other compiler also has a similar or totally different bug triggered with your test. Would that not be nice?

test/hotspot/jtreg/compiler/escapeAnalysis/TestReduceAllocationAndNestedScalarized.java line 46:

> 44:  */
> 45: 
> 46: package compiler.c2;

You moved your test file. The package name should either be removed or adjusted accordingly ;)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20087#discussion_r1675400480
PR Review Comment: https://git.openjdk.org/jdk/pull/20087#discussion_r1675401484