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

[Emanuel Peter]

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>

@JohnTortugo 

> This test is intended to "test" a transformation in C2, it won't fail if run in C1, but it won't actually test what's intended for. The flagless option is to make sure that C2 produce an IR graph that will trigger the optimization, same reasoning for all the "CompileCommand" options. I may remove the "vm.bits" part, I included it because of the "-UseCompressedOops".

The `flagless` requires will just mean that your test is not run if there are flags. So we still rely on the test configuration having some run without additional flags. So your requires seems pointless, especially because your test does not crash without it. Unless your test takes a lot of time and you are trying to conserve runtime.

About flags: I would just have a second run without any flags.

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

PR Comment: https://git.openjdk.org/jdk/pull/20087#issuecomment-2222588730