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

[Roberto Castañeda Lozano]

On Thu, 14 Sep 2023 17:48:27 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

> Few comments.

Thanks for reviewing, Vladimir! See my replies inlined for each comment/question.

> Why rounding is only arrays?

Class instance (bytewise) size is already rounded to `MinObjAlignmentInBytes` (i.e. it is a multiple of `BytesPerLong`, assuming minimum object alignment of 8 bytes), so rounding it again would be a no-op.

> Do we have a check that object's alignment >= 8 bytes? If it less you may access beyond array.

This code simply assumes that the minimum object alignment in HotSpot is 8 bytes, as per `src/hotspot/share/runtime/globals.hpp`:


#ifdef _LP64
  ...
  product(int, ObjectAlignmentInBytes, 8,                                   \
          "Default object alignment in bytes, 8 is minimum")                \
          range(8, 256)                                                     \
          constraint(ObjectAlignmentInBytesConstraintFunc, AtParse)
  ...
#else
  ...
  const int ObjectAlignmentInBytes = 8;
  ...
#endif


Is there any VM configuration where this might not hold? Or do I misunderstand your question?

> Can `Add` result overflow in 32-bit VM?

`payload_size + (BytesPerLong - 1)` cannot overflow because it is performed after a subtraction of a larger quantity (`base_off`). Would it make sense to add an assertion like this?:

`assert(base_off >= BytesPerLong - 1, "array payload size computation should not overflow");`

> 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

`headerx + abody` cannot overflow because of to the array length limit imposed by `TypeAryPtr::max_array_length()`, which is designed to fit the total bytes of an array header and payload (including external alignment) in 32 bits (on 32-bits platforms). I checked this using the length limits for all basic types. Exceeding this limit causes the array allocation to throw a `java.lang.OutOfMemoryError: Requested array size exceeds VM limit` exception).

> 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?

`size + mask1` cannot overflow because, as you suggest, it is only computed in the fast allocation path (i.e. for small values of `size` restricted by FastAllocateSizeLimit). The only way to bypass this check and indeed trigger the overflow is to set `-XX:FastAllocateSizeLimit=2147483647` (debug-only flag), this causes indeed a segmentation fault, reported in [JDK-8316512](https://bugs.openjdk.org/browse/JDK-8316512).

In general, the behavior of size computation within `GraphKit::new_array` w.r.t. overflow is not altered by this changeset, as far as I can see, because it essentially only changes whether `round_mask` is added to the header size (as done before) or to the total, non-rounded size (as proposed here).

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

PR Comment: https://git.openjdk.org/jdk/pull/15589#issuecomment-1725139074
PR Review Comment: https://git.openjdk.org/jdk/pull/15589#discussion_r1329825119
PR Review Comment: https://git.openjdk.org/jdk/pull/15589#discussion_r1329828675
PR Review Comment: https://git.openjdk.org/jdk/pull/15589#discussion_r1329829522