RFR: 8324517: C2: crash in compiled code because of dependency on removed range check CastIIs
Roland Westrelin
roland at openjdk.org
Tue Mar 19 13:27:31 UTC 2024
Range check `CastII` nodes are removed once loop opts are over. The
test case for this change includes 3 cases where elimination of a
range check `CastII` causes a crash in compiled code because either a
out of bounds array load or a division by zero happen.
In `test1`:
- the range checks for the `array[otherArray.length]` loads constant
fold: `otherArray.length` is a `CastII` of i at the `otherArray`
allocation. `i` is less than 9. The `CastII` at the allocation
narrows the type down further to `[0-9]`.
- the `array[otherArray.length]` loads are control dependent on the
unrelated:
if (flag == 0) {
test. There's an identical dominating test which replaces that one. As
a consequence, the `array[otherArray.length]` loads become control
dependent on the dominating test.
- The `CastII` nodes at the `otherArray` allocations are replaced by a
dominating range check `CastII` nodes for:
newArray[i] = 42;
- After loop opts, the range check `CastII` nodes are removed and the
2 `array[otherArray.length]` loads common at the first:
if (flag == 0) {
test before the:
float[] otherArray = new float[i];
and
newArray[i] = 42;
that guarantee `i` is positive.
- `test1` is called with `i = -1`, the array load proceeds with an out
of bounds index and the crash occurs.
`test2` and `test3` are mostly identical except for the check that's
eliminated (a null divisor check) and the instruction that causes a
fault (an integer division).
The fix I propose is to not eliminate range check `CastII` nodes after
loop opts. When range check`CastII` nodes were introduced, performance
was observed to regress. Removing them after loop opts was found to
preserve both correctness and performance. Today, the performance
regression still exists when `CastII` nodes are left in. So I propose
we keep them until the end of optimizations (so the 2 array loads
above don't lose a dependency and wrongly common) but remove them at
the end of all optimizations.
In the case of the array loads, they are dependent on a range check
for another array through a range check `CastII` and we must not lose
that dependency otherwise the array loads could float above the range
check at gcm time. I propose we deal with that problem the way it's
handled for `CastPP` nodes: add the dependency to the load (or
division)nodes as a precedence edge when the cast is removed.
@TobiHartmann ran performance testing for that patch (Thanks!) and reported
no regression.
-------------
Commit messages:
- test and fix
Changes: https://git.openjdk.org/jdk/pull/18377/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18377&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8324517
Stats: 212 lines in 4 files changed: 186 ins; 23 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/18377.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18377/head:pull/18377
PR: https://git.openjdk.org/jdk/pull/18377
More information about the hotspot-compiler-dev
mailing list