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

[Vladimir Kozlov]

On Wed, 10 Jul 2024 20:09:41 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:
> 
>  - Update src/hotspot/share/opto/output.cpp
>    
>    Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>
>  - Update src/hotspot/share/opto/machnode.hpp
>    
>    Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>

The fix looks fine. I have few comments.

src/hotspot/share/opto/machnode.hpp line 870:

> 868:     return in(_jvmadj + jvms->monitor_box_offset(idx));
> 869:   }
> 870:   Node* scalarized(const JVMState* jvms, uint idx) const {

May be rename to `scalarized_obj()`.

src/hotspot/share/opto/output.cpp line 983:

> 981:   for (int i = 0; i < jvms->scl_size(); i++) {
> 982:     Node* n = sfn->scalarized(jvms, i);
> 983:     if (!n->is_SafePointScalarObject()) {

Needs comment explaining what other Node type could be here. May be assert check for them in `else` case.

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

PR Review: https://git.openjdk.org/jdk/pull/20087#pullrequestreview-2172337616
PR Review Comment: https://git.openjdk.org/jdk/pull/20087#discussion_r1674257549
PR Review Comment: https://git.openjdk.org/jdk/pull/20087#discussion_r1674263492