[lworld] RFR: 8348962: [lworld] compiler/arraycopy/TestObjectArrayClone.java fails with SIGSEGV in PhaseMacroExpand::expand_arraycopy_node [v2]

[Tobias Hartmann]

On Mon, 10 Feb 2025 09:35:56 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> When we call `clone()` on an array, we are calling the `Object::clone()` instrinsic with an array object receiver. For that, we call `inline_native_clone()`. When additionally running with `-XX:-ReduceInitialCardMarks`, `CardTableBarrierSetC2::array_copy_requires_gc_barriers()` will return true. As a result, we mark the newly created `ArrayCopyNode` for the cloning as `CloneOopArray`:
>> 
>> https://github.com/openjdk/valhalla/blob/d42ac5c615e34a1d6ba20cfec5aa34316eafc374/src/hotspot/share/opto/library_call.cpp#L5631-L5632
>> 
>> The destination type of `ArrayCopyNode` is known to be some kind of array since we called `Object::clone()` on an array. Therefore, when expanding the node during macro expansion, the destination type will always be an array type and consequently, `top_dest` will be nun-null in `PhaseMacroExpand::expand_arraycopy_node()`: 
>> 
>> https://github.com/openjdk/valhalla/blob/d42ac5c615e34a1d6ba20cfec5aa34316eafc374/src/hotspot/share/opto/macroArrayCopy.cpp#L1360
>> 
>> However, with reflection, we can still directly call `Object::clone()` on an array without C2 knowing that it's an array. This is done in the failing test when calling `TestObjectArrayClone::testCloneObjectWithMethodHandle()`:
>> 
>> https://github.com/openjdk/valhalla/blob/d42ac5c615e34a1d6ba20cfec5aa34316eafc374/test/hotspot/jtreg/compiler/arraycopy/TestObjectArrayClone.java#L192-L194
>> 
>> `clone` is the `Object::clone()` method and `obj` is a `String[]` but passed in as an `Object` to the method. As a result, C2 just knows that the destination type is `Object`. Therefore, `top_dest` will be null here (i.e. not an array type):
>> 
>> https://github.com/openjdk/valhalla/blob/d42ac5c615e34a1d6ba20cfec5aa34316eafc374/src/hotspot/share/opto/macroArrayCopy.cpp#L1360
>> 
>> Later, we check whether the destination is a flat array:
>> https://github.com/openjdk/valhalla/blob/d42ac5c615e34a1d6ba20cfec5aa34316eafc374/src/hotspot/share/opto/macroArrayCopy.cpp#L1382
>> 
>> Since `top_dest` is null, we crash with a segfault.
>> 
>> The solution is to just check whether `top_dest` is non-null. Note that in this case, we do not need a membar. When it turns out that the array behind `Object` is flat during runtime, we will go to the slow path and make a call to `clone()`:
>> 
>> https://github.com/openjdk/valhalla/blob/d42ac5c615e34a1d6ba20cfec5aa34316eafc374/src/hotspot/share/opto/library_call.cpp#L5611-L5613
>> 
>> I added the failing flag combination to the existing test which covers this bug.
>> 
>> Tha...
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update

Embarrassing that I didn't spot that :) Looks good!

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

Marked as reviewed by thartmann (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1356#pullrequestreview-2605512134