RFR: 8297730: C2: Arraycopy intrinsic throws incorrect exception

[Tobias Hartmann]

On Fri, 20 Jan 2023 17:10:58 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> In `LibraryCallKit::inline_arraycopy()`, we emit some guards to safely perform `System.arraycopy()`. These guards are added to the IR right before the `ArrayCopy` node. If one of the emitted guards fails at runtime, we need to deoptimize and jump back to the interpreter.
> 
> If there is an additional tightly coupled allocation (array copy right after an array allocation), then the array copy can perform the initialization of the array instead of the standard array initialization. To make this work, the traps for the emitted guards for the array copy need to re-execute in the interpreter at the bci for the array allocation (fixed by [JDK-8064703](https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2014-December/016404.html)). Otherwise, the interpreter calls array copy on an uninitialized array and would crash.
> 
> This fix was not sufficient, though, as we now allocate the array twice. This leaves an uninitialized array in the heap which could cause problems for GCs. This was than fixed by [JDK-8073866](https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2015-March/017334.html) by moving the entire array allocation from before the guards to after the guards. This is possible due to the pattern that tightly coupled allocations have to obey (only other unrelated tests with UCTs, no unexpected observers of the allocation etc.).
> 
> As it turns out, this fix is still not complete. In this bug here, we've observed that the current implementation leads to a situation where we throw a wrong exception for a tightly coupled allocation that was initialized with a negative size. The reason for that is the following:
> 
> In `tightly_coupled_allocation()`, we allow tests with uncommon traps between the allocation and the array copy which we consider harmless:
> https://github.com/openjdk/jdk/blob/c6d560039682ec52efa6fa7755d2aa86f20e1148/src/hotspot/share/opto/library_call.cpp#L5347-L5362
> 
> We could therefore still identify a tightly allocated allocation in the following code where we have an additional unrelated null check trap between the array allocation and the array copy:
> 
> byte[] b = new byte[-1];
> int x = byArr.length; // with null_check trap
> // <guards for arraycopy>
> System.arraycopy(byArr, 0, b, 0, x);
> 
> We perform the optimization to use the array copy to initialize the array and move the allocation down. This gives us:
> 
> int x = byArr.length; // with null_check trap
> // <guards for arraycopy>
> byte[] b = new byte[-1];
> System.arraycopy(arg1, 0, b, 0, x);
> 
> If `byArr` is now `null` at runtime, we take the `null_check` trap when executing `byArr.length` and jump to the interpreter. We re-execute this access and throw a `NullPointerException`. But this is wrong: We actually should have thrown a `NegativeArraySizeException` instead because the array allocation with the negative size should have been executed first. But we do not since we've moved the allocation down to the array copy.
> 
> To solve this, I propose to replace all "unrelated uncommon traps" between the array allocation and the array copy (or more precisely the emitted guards) with uncommon traps that have the same state as the dedicated uncommon trap of the emitted array copy guards (i.e. the state before the array allocation). However, we need to be more careful and cannot simply reuse the same trap as we could end up with dominance problems: The dedicated emitted guard trap could use improved nodes dependent on the true projection of some guards. But the unrelated uncommon traps come before these guards traps. Additionally, we need to make sure to use the original deoptimization reason and action of the unrelated uncommon traps.
> 
> I've refactored some code to make the implementation easier:
> - reused the safepoint creation code for the state before the array allocation for the uncommon trap of the emitted guards
> - refactored and reused code to skip over harmless tests with uncommon traps in `tightly_allocated_allocation()`
> - some renamings to make the code easier to read
> 
> Thanks,
> Christian

Nice summary of the history. The fix looks good to me.

src/hotspot/share/opto/library_call.cpp line 5079:

> 5077: // Replace the unrelated uncommon traps with new uncommon trap nodes by reusing the action and reason. The new uncommon
> 5078: // traps will have the state of the array allocation. Let the old uncommon trap nodes die.
> 5079: void LibraryCallKit::create_new_uncommon_traps_with_alloc_state(JVMState* saved_jvms_before_guards) {

Should we rename this to replace_... ?

src/hotspot/share/opto/library_call.cpp line 5102:

> 5100:                   Deoptimization::trap_request_action(trap_request));
> 5101:     assert(stopped(), "Should be stopped");
> 5102:   }

I think the scope around `PreserveJVMState` can be removed.

test/hotspot/jtreg/compiler/arraycopy/TestArrayCopyIntrinsicWithUCT.java line 122:

> 120:     }
> 121: 
> 122: 

Suggestion:

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12120