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

[Tobias Hartmann]

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

Great work Christian! The fix looks good and it's nice to have better test coverage now.

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

> 201:   insert_mem_bar_volatile(Op_MemBarCPUOrder, C->get_alias_index(TypeAryPtr::INLINES));
> 202: 
> 203:   // Keep track of the information that the inline type is in flat arrays

Suggestion:

  // Keep track of the information that the inline type is flat in arrays

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

> 263:   // If we statically know that this is an inline type array, use precise element klass for checkcast
> 264:   const TypeAryPtr* arytype = _gvn.type(ary)->is_aryptr();
> 265:   const TypePtr* elem_ptr = elemtype->make_ptr();

Should this be moved to the else branch?

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

> 272:     // TODO: Should move to TypeAry::is_null_free() with JDK-8345681
> 273:     TypePtr::PTR ptr = elem_ptr->ptr();
> 274:     null_free = ptr == TypePtr::NotNull || ptr == TypePtr::AnyNull;

Suggestion:

    null_free = ((ptr == TypePtr::NotNull) || (ptr == TypePtr::AnyNull));

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

Marked as reviewed by thartmann (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1314#pullrequestreview-2492199793
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1878032097
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1878019806
PR Review Comment: https://git.openjdk.org/valhalla/pull/1314#discussion_r1878021493