RFR: 8323274: C2: array load may float above range check
Roland Westrelin
roland at openjdk.org
Tue Jan 30 17:13:38 UTC 2024
This PR includes 5 test cases in which an array load floats above its
range check and the resulting compiled code can be made to segfault by
passing an out of bound index to the test method. Each test case takes
advantage of a different transformation to make the array load happen
too early:
For instance, with TestArrayAccessAboveRCAfterSplitIf:
if (k == 2) {
v = array1[i];
array = array1;
if (l == m) {
}
} else {
v = array2[i];
array = array2;
}
v += array[i]; // range check + array load
The range check is split through phi:
if (k == 2) {
v = array1[i];
array = array1;
if (l == m) {
}
// range check here
} else {
v = array2[i];
array = array2;
// range check here
}
v += array[i]; // array load
Then an identical dominating range check is found:
if (k == 2) {
v = array1[i]; // range check here
array = array1;
if (l == m) {
}
} else {
v = array2[i]; // range check here
array = array2;
}
v += array[i]; // array load
Then a branch dies:
v = array1[i]; // range check here
array = array1;
if (l == m) {
}
v += array[i]; // array load
The array load is dependent on the `if (l == m) {` condition. An
identical dominating condition is then found which causes the control
dependent range check to float above the range check.
Something similar can be triggered with:
- TestArrayAccessAboveRCAfterPartialPeeling: sometimes, during partial
peeling a load is assigned the loop head as control so something
gets in between the range check and an array load and steps similar
to the above can cause the array load to float above its range check.
- TestArrayAccessAboveRCAfterUnswitching: cloning a loop body adds
regions on exits of the loop and nodes that only have uses out of
the loop can end up control dependent on one of the regions. In the
test case, unswitching is what causes the cloning to happen. Again
similar steps as above make the array load floats above its range
check. I suppose similar bugs could be triggered with other loop
transformations that rely on loop body cloning.
TestArrayAccessAboveRCAfterSinking is a bit different in that it can
change the control of an array load to be the projection of some
arbitrary test. That test can then be replaced by a dominating one
causing the array to float.
Finally, in TestArrayAccessAboveRCForArrayCopyLoad, an array copy is
converted to a series of loads/stores that's guarded by a test for
`srcPos < dstPos`. A dominating identical test exists so an array load
floats above the runtime checks that guarantee the arraycopy is legal.
In all cases, the fix I propose is similar to 8319793: mark the array
access nodes pinned when the transformation happens.
This might be over conservative in some cases. I intend to address
some of that with: 8324976 (C2: allow array loads known to be within
bounds to float) which would set a load's control to null in the cases
when it is known to be within bounds.
I've also been working on a verification pass to catch these issues. I
intend to propose it later.
-------------
Commit messages:
- whitespaces
- tests & fixes
Changes: https://git.openjdk.org/jdk/pull/17635/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17635&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8323274
Stats: 663 lines in 9 files changed: 659 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/17635.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17635/head:pull/17635
PR: https://git.openjdk.org/jdk/pull/17635
More information about the hotspot-compiler-dev
mailing list