[jdk17] RFR: 8269795: C2: Out of bounds array load floats above its range check in loop peeling resulting in SEGV

[Christian Hagedorn]

On Fri, 9 Jul 2021 06:19:12 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> In the testcase, an out of bounds array load (`LoadI`) floats above its range check resulting in a segfault. The problem can be traced back to incorrectly treating an `If` node, on which the `LoadI` is control dependent, as being dominated in `PhaseIdealLoop::peeled_dom_test_elim`. As a result, `PhaseIdealLoop::dominated_by()` moves the `LoadI` out and before the loop while the range check stays inside the loop.
>> 
>> `peeled_dom_test_elim()` walks through the idom chain of the loop body and tries to find loop-invariant tests. Once it finds one, it not only moves this test out of the loop but also looks for other tests with the same condition node which can be removed. The condition of the dominated tests in the loop are replaced with a `ConI` in `dominated_by()`. The data dependencies on the removed dominated tests are rewired to a peeled node before the loop.
>> 
>> However, the current code directly checks `test->in(1)` at L527 which is only the initial `Bool` node of the dominating test before the first invocation of `dominated_by()` in the loop on L525. Afterwards, `test->in(1)` is a `ConI` node. We now continue to look for tests with an identical `ConI` condition to move their data nodes out of this loop. This is wrong and exactly happens in the testcase: The loop body still contains a not yet cleaned up (by IGVN) `If` node (with a `ConI` condition) of an eliminated skeleton predicate on which an out of bounds  `LoadI` node is control dependent. The `LoadI` is then wrongly rewired out of the loop and before its range check.
>> 
>> The fix is to only check against the initial `test->in(1)` `Bool` node which satisfies the conditions checked on the lines before (L518-520).
>> 
>> Thanks,
>> Christian
>
> src/hotspot/share/opto/loopTransform.cpp line 524:
> 
>> 522:           !loop->is_member(get_loop(get_ctrl(test->in(1))))){
>> 523:         // Walk loop body looking for instances of this test
>> 524:         Node* test_cond = test->in(1);
> 
> Maybe move this up to line 517 and replace the `test->in(1)->is_Con()` and `get_ctrl(test->in(1))` usages in line 520/522 for better readability.

Unfortunately, that's not possible as `test` could be an `IfProj` with only a control input.

> test/hotspot/jtreg/compiler/loopopts/TestPeelingRemoveDominatedTest.java line 26:
> 
>> 24: /*
>> 25:  * @test
>> 26:  * @key stress randomness
> 
> There is no randomness in the test, right? Or is that referring to the use of `StressGCM`? If so, these keys are missing from other other tests that use this flag.

Yes, it's referring to `StressGCM`. @robcasloz once updated all tests with `StressGCM` to use these keys in [JDK-8253765](https://github.com/openjdk/jdk/commit/05459df0). But some newer tests miss these keys. Should we update them in an RFE to be consistent?

> test/hotspot/jtreg/compiler/loopopts/TestPeelingRemoveDominatedTest.java line 49:
> 
>> 47:     }
>> 48: 
>> 49:     public void mainTest() {
> 
> Is that method needed?

It is needed, otherwise, it does not reproduce.

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

PR: https://git.openjdk.java.net/jdk17/pull/235