RFR: 8315082: [REDO] Generational ZGC: Tests crash with assert(index == 0 || is_power_of_2(index))

[Vladimir Kozlov]

On Wed, 6 Sep 2023 11:54:04 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

> This changeset (REDO of [JDK-8312749](https://bugs.openjdk.org/browse/JDK-8312749)) ensures that the array copy stub underlying the intrinsic implementation of `Object.clone` only copies its (double-word aligned) payload, excluding the remaining object alignment padding words, when a non-default `ObjectAlignmentInBytes` value is used. This prevents the specialized ZGC stubs for `Object[]` array copy from processing undefined object alignment padding words as valid object pointers. For further details about the specific failure, see initial analysis of [JDK-8312749](https://bugs.openjdk.org/browse/JDK-8312749) by Erik Österlund and Stefan Karlsson and comments in `test/hotspot/jtreg/compiler/gcbarriers/TestArrayCopyWithLargeObjectAlignment.java`.
> 
> As a side-benefit, the changeset simplifies the array size computation logic in `GraphKit::new_array()` by decoupling computation of header size and alignment padding size.
> 
> #### Additional changes compared to [JDK-8312749](https://bugs.openjdk.org/browse/JDK-8312749)
> 
> This changeset proposes the exact same solution as [JDK-8312749](https://bugs.openjdk.org/browse/JDK-8312749), that is, identical changes to `barrierSetC2.cpp`, `graphKit.cpp`, `library_call.cpp`, and `TestArrayCopyWithLargeObjectAlignment.java`. On top of that, it relaxes an assertion in the idealization of `ArrayCopy` nodes violated by [JDK-8312749](https://bugs.openjdk.org/browse/JDK-8312749) and reported in [JDK-8315029](https://bugs.openjdk.org/browse/JDK-8315029) (new changes in `arraycopynode.cpp`, new regression test `TestCloneArrayWithDifferentLengthConstness.java`). The original, stricter assertion checks that, while idealizing an ArrayCopy node, the "constness" of the array copy's word-length (whether it is known by C2 to be constant or not) is equivalent to that of the array copy's element-length. For cases in which the element-length is within a small, fixed range (e.g. for an `int` array of length `3..4`) so that all element-length values lead to the same number of wo
 rds (`2`), the assertion used to hold before this changeset only because of weak type propagation in `AndL` (preventing the constant word-length to be discovered), see the left graph below:
> 
> ![from-element-to-word-length](https://github.com/openjdk/jdk/assets/8792647/3d5535cf-4afa-46dd-bc48-30430eead12f)
> 
> With the proposed changes, the array copy word-length is computed in a more straightforward way that enables C2 to infer the precise number of words in the same scenario ...

Few comments.

src/hotspot/share/gc/shared/c2/barrierSetC2.cpp line 684:

> 682:     // multiple of BytesPerLong for sub-long element types.
> 683:     payload_size = kit->gvn().transform(new AddXNode(payload_size, kit->MakeConX(BytesPerLong - 1)));
> 684:   }

Back to this change. Why rounding is only arrays? Do we have a check that object's alignment >= 8 bytes? If it less you may access beyond array.
Can `Add` result overflow in 32-bit VM?

src/hotspot/share/opto/graphKit.cpp line 3859:

> 3857:     abody = _gvn.transform(new LShiftXNode(lengthx, elem_shift));
> 3858:   }
> 3859:   Node* non_rounded_size = _gvn.transform(new AddXNode(headerx, abody));

Here

src/hotspot/share/opto/graphKit.cpp line 3869:

> 3867:   if (round_mask != 0) {
> 3868:     Node* mask1 = MakeConX(round_mask);
> 3869:     size = _gvn.transform(new AddXNode(size, mask1));

and here again question about overflow in 32-bit VM.
Do we generate compare with FastAllocateSizeLimit before this code is executed?

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

PR Review: https://git.openjdk.org/jdk/pull/15589#pullrequestreview-1627447238
PR Review Comment: https://git.openjdk.org/jdk/pull/15589#discussion_r1326319766
PR Review Comment: https://git.openjdk.org/jdk/pull/15589#discussion_r1326324968
PR Review Comment: https://git.openjdk.org/jdk/pull/15589#discussion_r1326326514