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