RFR: 8331717: C2: Crash with SIGFPE Because Loop Predication Wrongly Hoists Division Requiring Zero Check [v2]

[Christian Hagedorn]

On Thu, 12 Dec 2024 08:48:14 GMT, theoweidmannoracle <duke at openjdk.org> wrote:

>> Fixes a bug in loop predication where not strictly invariant tests involving divisions or modulo are pulled out of the loop.
>> 
>> The bug can be seen in this code:
>> 
>> 
>> public class Reduced {
>>     static int iArr[] = new int[100];
>> 
>>     public static void main(String[] strArr) {
>>         for (int i = 0; i < 10000; i++) {
>>             test();
>>         }
>>     }
>> 
>>     static void test() {
>>         int i1 = 0;
>> 
>>         for (int i4 : iArr) {
>>             i4 = i1;
>>             try {
>>                 iArr[0] = 1 / i4;
>>                 i4 = iArr[2 / i4]; // Source of the crash
>>             } catch (ArithmeticException a_e) {
>>             }
>>         }
>>     }
>> }
>> 
>> 
>> The crucial element is the division `2 / i4`. Since it is used to access an array, it is the input to a range check. See node 230:
>> <img width="699" alt="Screenshot 2024-12-11 at 15 14 47" src="https://github.com/user-attachments/assets/0b2ed978-7135-4a7e-bd10-25c0ffe7a9bb" />
>> 
>> Loop predication will try to pull this range check together with its input, the division, before the `for` loop. Due to a bug in Invariance::compute_invariance loop predication is allowed to do so, which results in the division being pulled out without its non-zero check. 322 is a clone of 230 placed before the loop head without any zero check for the divisor:
>> 
>> <img width="798" alt="Screenshot 2024-12-11 at 15 11 48" src="https://github.com/user-attachments/assets/9d4911cc-9967-4b7a-9969-98e01a55cd0d" />
>> 
>> 
>> More specifically, this bug occurs because 230's zero check (174 If) is not its direct control. Between the zero check and the division is another unrelated check (189 If), which can be hoisted:
>> 
>> <img width="825" alt="Screenshot 2024-12-12 at 09 14 37" src="https://github.com/user-attachments/assets/bd572556-1d30-41d5-b74c-22b673963dc2" />
>> 
>> Due to the way the Invariance class works, a check that can be hoisted will be marked as invariant. Then, to determine if any given node is invariant, Invariance::compute_invariance checks if all its inputs are invariant:
>> 
>> https://github.com/openjdk/jdk/blob/ceb4366ebf02f64165acc4a23195e9e3a7398a5c/src/hotspot/share/opto/loopPredicate.cpp#L456-L475
>> 
>> Therefore, when recursively traversing the inputs for 230 Div, the hoisted, unrelated check 189 If is hit before the zero check. As that check has been hoisted before already, it is marked invariant and `all_inputs_invariant` will be set to true. (All other inputs are also trivially ...
>
> theoweidmannoracle has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Combine test files

Nice summary! Looks good to me, too.

> Would be interesting if we can collapse such graph by propagating `I1 == 0` through `i4` into zero check. As separate RFE.

I think in this case, it's not possible because we have an OSR compilation where `i1` is just a `LoadI`.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22666#pullrequestreview-2508031613