[jdk17] RFR: 8270461: ZGC: Invalid oop passed to ZBarrierSetRuntime::load_barrier_on_oop_array

[Tobias Hartmann]

On Fri, 16 Jul 2021 13:59:02 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> For object arrays, C2's clone intrinsic emits calls to the `oop_disjoint_arraycopy_uninit` stub. With ZGC, load barriers on the source array elements are applied via `BarrierSetAssembler::arraycopy_prologue` before copying to the destination array: https://github.com/openjdk/jdk17/blob/1e3b418a53a080a53827989393362338b43dd363/src/hotspot/cpu/x86/stubGenerator_x86_64.cpp#L2400-L2403
> 
> The problem is that `BarrierSetC2::arraycopy_payload_base_offset` may 8-byte align the array offset to ensure that a copy in 8-byte chunks of the 8-byte aligned object is possible: https://github.com/openjdk/jdk17/blob/1e3b418a53a080a53827989393362338b43dd363/src/hotspot/share/gc/shared/c2/barrierSetC2.cpp#L658-L662
> Now with ` -XX:-UseCompressedClassPointers`, the offset starts at the 4-byte length field of the array (8 bytes mark word + 8 byte klass = 16 byte). That is fine if we don't need load barriers and can copy the array as `T_LONG` but with ZGC we crash in `ZBarrier::mark` because the element is not a valid oop.
> 
> I propose to simply set the offset to the first array element when cloning Object arrays with ZGC. We can still copy in 8 byte chunks because the oop elements are 8 byte on 64-bit (and ZGC is only supported on 64-bit).
> 
> I found this when investigating intermittent ZGC crashes in project Valhalla. The bug was introduced by [JDK-8268125](https://bugs.openjdk.java.net/browse/JDK-8268125) in JDK 17. The code is quite messy and will hopefully be cleaned up by [JDK-8268020](https://bugs.openjdk.java.net/browse/JDK-8268020).
> 
> Thanks,
> Tobias

Thanks for the review, Vladimir!

The problem is that in `arraycopy_payload_base_offset` we don't know the basic type and therefore can't use `arrayOopDesc::base_offset_in_bytes(bt)`:
https://github.com/openjdk/jdk17/blob/1e3b418a53a080a53827989393362338b43dd363/src/hotspot/share/gc/shared/c2/barrierSetC2.cpp#L660

Also, modifying that code would affect the other GCs using `T_LONG` for cloning Object arrays. Since we are late for JDK 17, I would rather prefer going with this conservative fix and postpone cleaning things up to [JDK-8268020](https://bugs.openjdk.java.net/browse/JDK-8268020). What do you think?

And yes, for `ArrayCopyNodes` used for clone, `src_offset` is always a constant and equal to `dest_offset` (see code in `BarrierSetC2::clone`).

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

PR: https://git.openjdk.java.net/jdk17/pull/252