[lworld] RFR: 8345250: [lworld] C2: Array loads and stores on inexact flat arrays cause crashes

Fri Dec 6 17:25:13 UTC 2024

On Fri, 6 Dec 2024 17:10:20 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This PR fixes crashes that occur when trying to load/store from/to a flat array from which we statically know it is flat but we do not know the exact layout because the klass is inexact.
> 
> ### The Layout of a Flat Array is only Statically Known If the Klass is Exact
> Currently, the code for getting the array element address in `GraphKit::array_element_address()` assumes that whenever we have an array from which we know that it is flat, then we also know the exact array layout (i.e. assume the value klass is exact):
> https://github.com/openjdk/valhalla/blob/5e876d607aba68e921cb8fa6a1a012acd1735825/src/hotspot/share/opto/graphKit.cpp#L1835-L1838
> 
> This does not need to be true as shown in the following test case where we have a flat array with an inexact klass/type:
> https://github.com/openjdk/valhalla/blob/95fc9c71bffcf5b92a4cdbfb34ad9e3149266f79/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorld.java#L4568-L4578
> 
> ### Emit Runtime Call to Load/Store from/to the Correct Array Element Address
> What we actually would need for a flat array with an inexact klass is to emit a runtime call to load/store from/to the correct array element address. We cannot do better statically because we do not know the exact layout.
> 
> ### Challenges
> However, the problem is that in `GraphKit::array_element_address()` we somehow need to bail out back to `Parse::array_load/store()` such that we can emit the runtime call there. We could use a sentinel node because we do not actually need the element address node. But this could mess with some GVN transformations in between. What we do instead is to just fall to the `else` case in `GraphKit::array_element_address()` for non-flat arrays and still construct the element address node, even though it's not needed (will be folded away later):
> https://github.com/openjdk/valhalla/blob/0858ce110da58ed7f1b634ae381e7c4eb0f37c3a/src/hotspot/share/opto/graphKit.cpp#L1840-L1849
> 
> ### Additionally Required Fixes
> 1. `TypeAryKlassPtr::cast_to_exactness()` asserts that all null-free or flat arrays are exact which is not the case as shown above. Fix: Adjust assert.
> 2. `ciArrayKlass::make()` assumes that all null-free arrays are concrete value classes which does not need to be the case as shown above. Fix: Add check whether the klass is an inline type (which is only true for concrete inline type klasses).
> 3. `Parse::array_store_check()` needs to pass `null_free` to the `gen_checkcast()` in case we have a null-free flat array that is inexact (i.e. either ...

src/hotspot/share/ci/ciArrayKlass.cpp line 113:

> 111:   assert(!null_free || !klass->is_loaded() || klass->is_inlinetype() || klass->is_abstract() ||
> 112:          klass->is_java_lang_Object(), "only value classes are null free");
> 113:   if (null_free && klass->is_loaded() && klass->is_inlinetype()) {

Added `klass->is_inlinetype()` to be able to get the `InlineKlass`.

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

> 3505:   const TypeOopPtr* toop = improved_klass_ptr_type->cast_to_exactness(false)->as_instance_type();
> 3506:   bool safe_for_replace = (failure_control == nullptr);
> 3507:   assert(!null_free || toop->can_be_inline_type(), "must be an inline type pointer");

When we pass now `null_free == true` for inexact inline types, `is_inlinetypeptr()` is false (only true for concrete value classes). `can_be_inline_type()` is the next better thing we could check.

src/hotspot/share/opto/parse2.cpp line 93:

> 91:     // Load from non-flat inline type array (elements can never be null)
> 92:     bt = T_OBJECT;
> 93:   } else if (!ary_t->is_not_flat()) {

Swapped cases and inserted null free case further down after this case.

src/hotspot/share/opto/parse2.cpp line 94:

> 92:       // Emit a runtime call to load the element from the flat array.
> 93:       inline_type = load_from_unknown_flat_array(array, array_index, element_ptr);
> 94:       inline_type = record_profile_for_speculation_at_array_load(inline_type);

Cover missing case, other changes in `array_load()` are just renaming/refactoring

src/hotspot/share/opto/parse2.cpp line 147:

> 145:         // Element type is unknown, and thus we cannot statically determine the exact flat array layout. Emit a
> 146:         // runtime call to correctly load the inline type element from the flat array.
> 147:         Node* inline_type = load_from_unknown_flat_array(array, array_index, element_ptr);

Extracted to new method such that we can reuse this above.

src/hotspot/share/opto/parse2.cpp line 267:

> 265:         // Emit a runtime call to store the element to the flat array.
> 266:         store_to_unknown_flat_array(array, array_index, stored_value_casted);
> 267:       }

New case, other code is just refactoring in `array_store()`.

src/hotspot/share/opto/parse2.cpp line 270:

> 268:       return;
> 269:     }
> 270:     if (array_type->is_null_free()) {

`else if` can be turned into simple `if` because the other case returns.

src/hotspot/share/opto/parse2.cpp line 310:

> 308:           inline_Klass = elemtype->inline_klass();
> 309:         }
> 310:         if (!stopped()) {

Simplified cases since both cases checked `!stopped()` before.

src/hotspot/share/opto/parse2.cpp line 331:

> 329:           } else {
> 330:             // Element type is unknown, emit a runtime call since the flat array layout is not statically known.
> 331:             store_to_unknown_flat_array(array, array_index, null_checked_stored_value_casted);

Can now call extracted method which is shared with new code inserted above.

src/hotspot/share/opto/parseHelper.cpp line 272:

> 270:     a_e_klass = makecon(TypeKlassPtr::make(elemtype->inline_klass()));
> 271:   } else {
> 272:     // TODO: Should move to TypeAry::is_null_free() with JDK-8345681

See PR description under additional fixes point 3.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873722653
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873723659
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873726538
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873724796
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873725380
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873727458
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873728036
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873728648
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873729253
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1873729905