[lworld] RFR: 8345250: [lworld] C2: Array loads and stores on inexact flat arrays cause crashes
Christian Hagedorn
chagedorn at openjdk.org
Fri Dec 6 17:25:12 UTC 2024
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 `Object` or an abstract value class) and thus `is_inlinetypeptr()` is false. If we don't do that, we will hit this assert with `-XX:-UncommonNullCast` that ensures that flat arrays are null-free:
https://github.com/openjdk/valhalla/blob/265189e2b311daaa8cac3d387c3a70051b1e419a/src/hotspot/share/opto/parse2.cpp#L235-L237
The right/complete way to fix this would be to change `TypeAry::is_null_free()` to also handle non-exact inline types. However, this is invasive since we use this check in many places. Changing this method could have a bigger impact due to possibly having more places where we assume that only exact inline types are null-free. I filed [JDK-8345681](https://bugs.openjdk.org/browse/JDK-8345681) to revisit this. For this PR, I just implemented a point fix in `array_store_check()` for now.
### Applied some Additional Renamings and Simple Refactorings
While trying to better understand the `array_load/store()`, I've applied some renamings and simple refactorings. I'm planning to do some more refactorings to reduce the size of `array_load/store()`. But I will do that separately with [JDK-8345696](https://bugs.openjdk.org/browse/JDK-8345696).
Thanks,
Christian
-------------
Commit messages:
- Fix null free
- 8345250: [lworld] C2: Array loads and stores on inexact flat arrays cause crashes
Changes: https://git.openjdk.org/valhalla/pull/1314/files
Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=1314&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8345250
Stats: 448 lines in 7 files changed: 292 ins; 62 del; 94 mod
Patch: https://git.openjdk.org/valhalla/pull/1314.diff
Fetch: git fetch https://git.openjdk.org/valhalla.git pull/1314/head:pull/1314
PR: https://git.openjdk.org/valhalla/pull/1314
More information about the valhalla-dev
mailing list