RFR: 8323274: C2: array load may float above range check [v6]

[Emanuel Peter]

On Fri, 9 Feb 2024 14:01:17 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> 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 t...
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix broken try_sink_out_of_loop

Thanks for the updates @rwestrel , looks good to me now 😊

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17635#pullrequestreview-1872549333