8269795: C2: Out of bounds array load floats above its range check in loop peeling resulting in SEGV #235

chhagedorn · 2021-07-08T20:24:00Z

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 dominated 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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8269795: C2: Out of bounds array load floats above its range check in loop peeling resulting in SEGV

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Roland Westrelin (@rwestrel - Reviewer) ⚠️ Review applies to c29fd24
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/235/head:pull/235
$ git checkout pull/235

Update a local copy of the PR:
$ git checkout pull/235
$ git pull https://git.openjdk.java.net/jdk17 pull/235/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 235

View PR using the GUI difftool:
$ git pr show -t 235

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/235.diff

… loop peeling resulting in SEGV

bridgekeeper · 2021-07-08T20:24:31Z

👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-07-08T20:26:43Z

@chhagedorn The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-07-08T20:34:27Z

Webrevs

01: Full - Incremental (bd921036)
00: Full (c29fd247)

TobiHartmann

Nice analysis, the fix looks good to me! Just some minor comments below.

[...] which is only the initial Bool node of the dominating test before the first invocation [...]

Should be "of the dominated test", right?

TobiHartmann · 2021-07-09T06:07:03Z

test/hotspot/jtreg/compiler/loopopts/TestPeelingRemoveDominatedTest.java

+
+/*
+ * @test
+ * @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. But some newer tests miss these keys. Should we update them in an RFE to be consistent?

Yes, I think that would be good.

Okay, I filed JDK-8270156 to clean it up later.

TobiHartmann · 2021-07-09T06:09:43Z

test/hotspot/jtreg/compiler/loopopts/TestPeelingRemoveDominatedTest.java

+        }
+    }
+
+    public void mainTest() {


Is that method needed?

It is needed, otherwise, it does not reproduce.

TobiHartmann · 2021-07-09T06:19:12Z

src/hotspot/share/opto/loopTransform.cpp

          // Condition is not a member of this loop?
          !loop->is_member(get_loop(get_ctrl(test->in(1))))){
        // Walk loop body looking for instances of this test
+        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.

Right, too bad.

Add comment why you cache the value - dominated_by() call may modify it.

openjdk · 2021-07-09T06:23:09Z

@chhagedorn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: thartmann, roland, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 38 new commits pushed to the master branch:

4fc3180: 8266345: (fs) Custom DefaultFileSystemProvider security related loops
999ced0: 8269873: serviceability/sa/Clhsdb tests are using a C2 specific VMStruct field
e1d3e73: 8268965: TCP Connection Reset when connecting simple socket to SSL server
3d82b0e: 8269558: fix of JDK-8252657 missed to update history at the end of JVM TI spec
2546006: 8270216: [macOS] Update named used for Java run loop mode
6889a39: 8268826: Cleanup Override in Context-Specific Deserialization Filters
f791fdf: 8261147: C2: Node is wrongly marked as reduction resulting in a wrong execution due to wrong vector instructions
1196b35: 8270151: IncompatibleClassChangeError on empty pattern switch statement case
885f7b1: 8269146: Missing unreported constraints on pattern and other case label combination
62ff55d: 8269952: compiler/vectorapi/VectorCastShape*Test.java tests failed on avx2 machines
... and 28 more: https://git.openjdk.java.net/jdk17/compare/e14801cdd9b108aa4ca47d0bc1dc67fca575764c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

chhagedorn · 2021-07-09T07:59:48Z

Nice analysis, the fix looks good to me! Just some minor comments below.

Thanks for your review Tobias!

[...] which is only the initial Bool node of the dominating test before the first invocation [...]

Should be "of the dominated test", right?

Yes, you're right as we are updating test to nodes in the loop. I updated the text.

rwestrel

Looks good to me.

chhagedorn · 2021-07-09T09:47:51Z

Thanks Roland for your review!

vnkozlov

Looks good. I have few comments.

vnkozlov · 2021-07-09T15:28:02Z

src/hotspot/share/opto/loopTransform.cpp

          // Condition is not a member of this loop?
          !loop->is_member(get_loop(get_ctrl(test->in(1))))){
        // Walk loop body looking for instances of this test
+        Node* test_cond = test->in(1);


Add comment why you cache the value - dominated_by() call may modify it.

vnkozlov · 2021-07-09T15:34:06Z

src/hotspot/share/opto/loopTransform.cpp

-    Node *test = prev->in(0);
+    progress = false; // Reset for next iteration
+    Node* prev = loop->_head->in(LoopNode::LoopBackControl); // loop->tail();
+    Node* test = prev->in(0);


May be add assert(test != NULL). idom() call at the end of loop has such asserts.
In other places we have check test != NULL but we not doing that here (at line 519).

vnkozlov · 2021-07-09T15:37:24Z

src/hotspot/share/opto/loopTransform.cpp

      int p_op = prev->Opcode();
      if ((p_op == Op_IfFalse || p_op == Op_IfTrue) &&
-          test->is_If() &&      // Test?
+          test->is_If() && // Test?


We can do what Tobias is asking by pre-check:

int p_op = prev->Opcode(); Node* test_cond = NULL; if ((p_op == Op_IfFalse || p_op == Op_IfTrue) && test->is_If()) { test_cond = test->in(1); } if (test_cond != NULL &&

vnkozlov

Good.

TobiHartmann

Looks good!

chhagedorn · 2021-07-13T08:48:47Z

Thanks Vladimir, Tobias and Roland for your reviews!

chhagedorn · 2021-07-13T08:48:56Z

/integrate

openjdk · 2021-07-13T08:50:21Z

Going to push as commit 040c02b.
Since your change was applied there have been 39 commits pushed to the master branch:

0f32982: 8270203: Missing build dependency between jdk.jfr-gendata and buildtools-hotspot
4fc3180: 8266345: (fs) Custom DefaultFileSystemProvider security related loops
999ced0: 8269873: serviceability/sa/Clhsdb tests are using a C2 specific VMStruct field
e1d3e73: 8268965: TCP Connection Reset when connecting simple socket to SSL server
3d82b0e: 8269558: fix of JDK-8252657 missed to update history at the end of JVM TI spec
2546006: 8270216: [macOS] Update named used for Java run loop mode
6889a39: 8268826: Cleanup Override in Context-Specific Deserialization Filters
f791fdf: 8261147: C2: Node is wrongly marked as reduction resulting in a wrong execution due to wrong vector instructions
1196b35: 8270151: IncompatibleClassChangeError on empty pattern switch statement case
885f7b1: 8269146: Missing unreported constraints on pattern and other case label combination
... and 29 more: https://git.openjdk.java.net/jdk17/compare/e14801cdd9b108aa4ca47d0bc1dc67fca575764c...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-07-13T08:50:34Z

@chhagedorn Pushed as commit 040c02b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8269795: C2: Out of bounds array load floats above its range check in…

c29fd24

… loop peeling resulting in SEGV

openjdk bot added the rfr Pull request is ready for review label Jul 8, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.java.net label Jul 8, 2021

TobiHartmann approved these changes Jul 9, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 9, 2021

rwestrel approved these changes Jul 9, 2021

View reviewed changes

vnkozlov reviewed Jul 9, 2021

View reviewed changes

review comments by Vladimir K

bd92103

vnkozlov approved these changes Jul 12, 2021

View reviewed changes

TobiHartmann approved these changes Jul 13, 2021

View reviewed changes

openjdk bot closed this Jul 13, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 13, 2021

8269795: C2: Out of bounds array load floats above its range check in loop peeling resulting in SEGV #235

8269795: C2: Out of bounds array load floats above its range check in loop peeling resulting in SEGV #235

Uh oh!

Conversation

chhagedorn commented Jul 8, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 8, 2021

Uh oh!

openjdk bot commented Jul 8, 2021

Uh oh!

mlbridge bot commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chhagedorn commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwestrel left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented Jul 9, 2021

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented Jul 13, 2021

Uh oh!

chhagedorn commented Jul 13, 2021

Uh oh!

openjdk bot commented Jul 13, 2021

Uh oh!

openjdk bot commented Jul 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

chhagedorn commented Jul 8, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 8, 2021 •

edited

Loading

openjdk bot commented Jul 9, 2021 •

edited

Loading

chhagedorn commented Jul 9, 2021 •

edited

Loading