RFR: 8339526: C2: store incorrectly removed for clone() transformed to series of loads/stores [v2]

[Emanuel Peter]

On Wed, 8 Oct 2025 09:00:34 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> In the `test1()` method of the test case:
>> 
>> `inlined2()` calls `clone()` for an object loaded from field `field`
>> that has inexact type `A` at parse time. The intrinsic for `clone()`
>> inserts an `Allocate` and an `ArrayCopy` nodes. When igvn runs, the
>> load of `field` is optimized out because it reads back a newly
>> allocated `B` written to `field` in the same method. `ArrayCopy` can
>> now be optimized because the type of its `src` input is known. The
>> type of its `dest` input is the `CheckCastPP` from the allocation of
>> the cloned object created at parse time. That one has type `A`. A
>> series of `Load`s/`Store`s are created to copy the fields of class `B`
>> from `src` (of type `B`) to `dest` of (type `A`).
>> 
>> Writting to `dest` with offsets for fields that don't exist in `A`,
>> causes this code in `Compile::flatten_alias_type()`:
>> 
>> 
>>     } else if (offset < 0 || offset >= ik->layout_helper_size_in_bytes()) {
>>       // Static fields are in the space above the normal instance
>>       // fields in the java.lang.Class instance.
>>       if (ik != ciEnv::current()->Class_klass()) {
>>         to = nullptr;
>>         tj = TypeOopPtr::BOTTOM;
>>         offset = tj->offset();
>>       }
>> 
>> 
>> to assign it some slice that doesn't match the one that's used at the
>> same offset in `B`.
>> 
>> That causes an assert in `ArrayCopyNode::try_clone_instance()` to
>> fire. With a release build, execution proceeds. `test1()` also has a
>> non escaping allocation. That one causes EA to run and
>> `ConnectionGraph::split_unique_types()` to move the store to the non
>> escaping allocation to a new slice. In the process, when it iterates
>> over `MergeMem` nodes, it notices the stores added by
>> `ArrayCopyNode::try_clone_instance()`, finds that some are not on the
>> right slice, tries to move them to the correct slice (expecting they
>> are from a non escaping EA). That causes some of the `Store`s to be
>> disconnected. When the resulting code runs, execution fails as some
>> fields are not copied.
>> 
>> The fix I propose is to skip `ArrayCopyNode::try_clone_instance()`
>> when `src` and `dest` classes don't match as this seems like a rare
>> enough corner case.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into JDK-8339526
>  - Update src/hotspot/share/opto/arraycopynode.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - test & fix

Hi Roland, thanks for looking into this!

Can you explain why the `clone` in `inlined2` creates an `ArrayCopy` node? I think I'm missing some context here. Because we are cloning an `A` and not an array, right?

test/hotspot/jtreg/compiler/arraycopy/TestCloneUnknownClassAtParseTime.java line 28:

> 26:  * @bug 8339526
> 27:  * @summary C2: store incorrectly removed for clone() transformed to series of loads/stores
> 28:  * @run main/othervm -XX:-BackgroundCompilation TestCloneUnknownClassAtParseTime

Would it make sense to also have a run without the flag?

test/hotspot/jtreg/compiler/arraycopy/TestCloneUnknownClassAtParseTime.java line 63:

> 61:     private static A inlined2() throws CloneNotSupportedException {
> 62:         A a = field;
> 63:         return (A)a.clone();

Out of curiosity: why do we even add a `ArrayCopy` here?

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

PR Review: https://git.openjdk.org/jdk/pull/27604#pullrequestreview-3330535225
PR Review Comment: https://git.openjdk.org/jdk/pull/27604#discussion_r2425606421
PR Review Comment: https://git.openjdk.org/jdk/pull/27604#discussion_r2425625118