8331717: C2: Crash with SIGFPE Because Loop Predication Wrongly Hoists Division Requiring Zero Check

[theoweidmannoracle]

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:

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:

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 (293 RangeCheck), which can be hoisted:

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:

jdk/src/hotspot/share/opto/loopPredicate.cpp

Lines 456 to 475 in ceb4366

	// n is invariant if it's inputs are all invariant
	bool all_inputs_invariant = true;
	for (uint i = 0; i < n->req(); i++) {
	Node* in = n->in(i);
	if (in == nullptr) continue;
	assert(_visited.test(in->_idx), "must have visited input");
	if (!_invariant.test(in->_idx)) { // bad guy
	all_inputs_invariant = false;
	break;
	}
	}
	if (all_inputs_invariant) {
	// If n's control is a predicate that was moved out of the
	// loop, it was marked invariant but n is only invariant if
	// it depends only on that test. Otherwise, unless that test
	// is out of the loop, it's not invariant.
	if (n->is_CFG() || n->depends_only_on_test() || n->in(0) == nullptr || !_phase->is_member(_lpt, n->in(0))) {
	_invariant.set(n->_idx); // I am a invariant too
	}
	}

Therefore, when recursively traversing the inputs for 230 Div, the hoisted, unrelated check 293 RangeCheck 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 invariant as they are constant.)

To fix this, Invariance::compute_invariance must check that the node not only depends_only_on_test() on line 472 but also that it has no_dependent_zero_check(n).

Similar past bug, which introduced no_dependent_zero_check: openjdk/jdk16#9

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8331717: C2: Crash with SIGFPE Because Loop Predication Wrongly Hoists Division Requiring Zero Check (Bug - P3)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) Review applies to 50146ee8
• Christian Hagedorn (@chhagedorn - Reviewer)
• Quan Anh Mai (@merykitty - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22666/head:pull/22666
$ git checkout pull/22666

Update a local copy of the PR:
$ git checkout pull/22666
$ git pull https://git.openjdk.org/jdk.git pull/22666/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22666

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22666.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back theoweidmannoracle! 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]

@theoweidmannoracle 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:

8331717: C2: Crash with SIGFPE Because Loop Predication Wrongly Hoists Division Requiring Zero Check

Reviewed-by: chagedorn, qamai, 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 263 new commits pushed to the master branch:

• 1a0fe49: 8347256: Epsilon: Demote heap size and AlwaysPreTouch warnings to info level
• 8b07617: 8347345: Remove redundant test policy file from ModelMBeanInfoSupport directory
• f6492aa: 8347279: Problemlist TestEvilSyncBug.java#generational
• 0210a63: 8347379: Problem list failed tests after JDK-8321413
• a9351df: 8346787: Fix two C2 IR matching tests for RISC-V
• 89ee1a5: 8343978: Update the default value of CodeEntryAlignment for Ampere-1A and 1B
• 931914a: 8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region
• 665c39c: 8347375: Extra

  tag in robot specification

• b335ea9: 8347122: Add missing @serial tags to module java.desktop
• df28cec: 8345144: Robot does not specify all causes of IllegalThreadStateException
• ... and 253 more: https://git.openjdk.org/jdk/compare/4f855d1342d55aeee93b7d0c5796fbfd4994c856...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vnkozlov, @chhagedorn, @merykitty) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@theoweidmannoracle 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]

Webrevs

• 01: Full - Incremental (8da7cb58)
• 00: Full (50146ee8)

[vnkozlov]

Looks good for this issue.

[vnkozlov]

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

[chhagedorn]

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.

[merykitty]

It seems strange to me that a DivINode here has the property depends_only_on_test but its control input is an unrelated test. Maybe we should fix whatever is rewiring the DivI to the range check here?

[merykitty]

Or if the control here is used for something else you can add another input to Div and Mod to signify the test they are depending on.

[merykitty]

Another way to tackle this is to think of a Div similar to an array access, when we rewire an array access, we need to pin it to the current control flow. Similarly, when we rewire a Div we need to pin it, and the Div is no longer depends_only_on_test.

[theoweidmannoracle]

Maybe we should fix whatever is rewiring the DivI to the range check here?

The DivI is rewired and attached to the RangeCheck by IGVN in this case. Looking at the code

int i1 = 0;

for (int i4 : iArr) {
    i4 = i1;
    try {
        iArr[0] = 1 / i4;  // 183 DivI
        i4 = iArr[2 / i4]; // 230 DivI
   } catch (ArithmeticException a_e) {
   }
}

and the resulting relevant nodes:

After several rounds of IGVN, we arrive at this state, where everything is still completely expected. 230 DivI is connected to its zero-check

IGVN/Idealization will now see that the zero-check-if for 230 DivI is dominated by the zero-check-if for 183 DivI and therefore remove it from the graph and attach 230 DivI to the RangeCheck. This seems a benign and useful transformation in general.

[theoweidmannoracle]

It seems strange to me that a DivINode here has the property depends_only_on_test but its control input is an unrelated test.

I think that's fine. After all, depends_only_on_test indicates that

If this node is control-dependent on a test, can it be rerouted to a dominating equivalent test?

which is absolutely true for division and was also correctly used by the IGVN transformation.

The source of the problem here is really with the way Invariance works.

[merykitty]

IGVN/Idealization will now see that the zero-check-if for 230 DivI is dominated by the zero-check-if for 183 DivI and therefore remove it from the graph and attach 230 DivI to the RangeCheck. This seems a benign and useful transformation in general.

I believe the issue is right there. If a check is removed because it is equivalent to a dominating check, then the DivI (as well as all nodes that depends_only_on_test needs to be rewired to that dominating check, not to the Region that immediately dominates the removed one. In this case, the 230 DivI needs to be rewired to 175 IfTrue (the zero check of 183 DivI), not to 293 RangeCheck.

If you want to rewire the DivI to 293 RangeCheck, then you need to pin the DivI, making it not depends_only_on_test. A similar logic can be found in here for array accesses:

jdk/src/hotspot/share/opto/ifnode.cpp

Line 1583 in 8efc558

Node* clone = s->pin_array_access_node();

[chhagedorn]

If you want to rewire the DivI to 293 RangeCheck, then you need to pin the DivI, making it not depends_only_on_test. A similar logic can be found in here for array accesses

It's probably the most safe way to eventually handle this similarly to array accesses as you suggest. Back there when the checks for no_dependent_zero_check() were introduced, we did not have the pinning for array accesses, in place.

However, for this bug, I think it's better to go with this rather simple point fix and file a follow-up RFE to revisit this and think about aligning it with array accesses with a pinning that's checkable with depends_only_on_test() as well. What do you think?

[merykitty]

@chhagedorn I think that the wrong control input of the DivINode is a potential severe issue that may have more adverse impacts beyond this case. This fix also effectively disables loop predication to run on divisions and their outputs, which may introduce regressions. I believe the case here is peculiar because the zero check is not predicated out of the loop due to failed speculation. As a result, if this issue is not very likely to be hit, I strongly prefer the proper fix of correctly wiring the 230 Div to 175 IfTrue. Otherwise, if you want to fix this issue ASAP, then I agree with this patch. Thanks a lot.

[chhagedorn]

I think that the wrong control input of the DivINode is a potential severe issue that may have more adverse impacts beyond this case.

This is a general problem which I've been thinking about many times: Should we make sure that a division, whose divisor could be zero, is always pinned at a zero check? I'm not entirely sure about all places where a division node could become control dependent on other control nodes.

Back there, when I worked on JDK-8257822, the problem was that we split a zero check through a region and the div node ended up at a region. Eventually, the division was wrongly hoisted before the actual zero check.

We were not really sure about all the places where a zero check could lose the connection to its zero check. As soon as this happens, I think we always have a problem when rewiring a division, pinned at some control node that's unrelated, to an earlier dominating check: We could skip over the original check.

Therefore, for JDK-8257822, we've decided to block the rewiring of division nodes instead of relying on spotting all the places where the disconnect could happen to fix it there. There were follow-up fixes to block other places and it seems that we have now found yet another place where we rewire a division that's not control dependent on the original check. Maybe it should have been handled with depends_only_on_test() as now done with array accesses. However, there was some indication that this has a negative performance impact (see RFR for JDK-8242108 where we tried this with a Cast node, maybe directly do it on Div nodes is better and could be investigated).

I'm not sure how big the impact of this patch will be on performance. But my guess is that it's probably on the lower side. If we look again at the graph posted by Theo above:

If Loop Predication did not hoist 293 RangeCheck, then we cannot hoist 240 RangeCheck, either. If we hoist 293 RangeCheck and decided to only try to hoist 240 RangeCheck in the next round of loop opts, we would not be able to hoist it because 230 Div would then be pinned at 190 IfTrue inside the loop.

So, from my understanding, the only cases we now forbid and that could have a performance impact are those where we hoist multiple zero checks in one round of loop opts. But as shown above, it's unsafe to perform this hoisting because the current code expects that division nodes are not rewired and stay pinned inside the loop.

Long story short, I think there are several ways to handle this general problem of divisions floating above the actual zero check:

• Allow divisions to lose the direct pin to its zero check and disable the rewiring of such divisions:

  1. Done today with no_dependent_zero_check() but incomplete and requires more locations to check this.
  2. Implement a pinning with depends_only_on_test() as done with array accesses. There were regressions when done with additional cast nodes but it might be more efficient to directly implement it on the division nodes. Would require some performance testing. It's safer than 1) but we would possibly block more cases.

• Disallow that zero checks and divisions are separated:

  3. Need to find all the places where this happens and disallow it or find another way to make this strong connection. Then we can safely rewire divisions to dominating checks, which are always zero excluding checks.

Option 1 (i) seems to be the simplest one for now. Option 2 would be more safe/correct but possibly has a bigger performance impact and might not be justified without a failing case. Option 3 would be interesting but seems to have a big impact and might not be so easy to do - could be worth to file an RFE.

Even when going with Option 1 or 2, we could still allow rewiring of divisions to a dominating check if we can prove that this is a zero check that covers the division to be rewired (e.g. same divisor and comparison excludes zero etc.) - and maybe that's all we need (discussed this idea with @eme64). Could be investigated with/against Option 3.

This became longer than I've intended but I guess it helped to reflect on the situation and the ways to go from here. Let me know what you think about it and the suggestion to go with option 1 for now. And also if there are some more or other things to consider.

[merykitty]

@chhagedorn Thanks for your really detailed explanation! I really appreciate it.

I am worried that this change may prohibit hoisting division out of loops in general. For example,

loop {
    int x = invar1 / invar2;
}

Then the division is anchored in the loop because the zero check of invar2 is in the loop. Currently, this will also move the division out of the loop, but with this change, the division will stay in the loop and be pinned to the loop head. Please correct me if I am misunderstanding the situation here.

One find suggestion, though, what do you think about overriding DivINode::depends_only_on_test and checking if the control node is a pattern that does exclude 0 from the value range of the divisor. This will do the job as the control of 230 DivI is an unrelated RangeCheck, making depends_only_on_test return false and the division stays in the loop while for the aforementioned case the control is the zero check and the division can be hoisted out of the loop. I think this can be applied to CastNode, too. As I believe we may have the same issue with them when a CastNode that depends_only_on_test is disconnected from its true test.

[chhagedorn]

You're welcome :-)

Then the division is anchored in the loop because the zero check of invar2 is in the loop

Unfortunately, this is already the case before this fix. We can only hoist the zero check out of the loop but not the division itself - it ends up at the next control node which is the LoopNode if it was the first check. Thus, we propose to have a closer look at that separately - there is definitely room for improvement:

Even when going with Option 1 or 2, we could still allow rewiring of divisions to a dominating check if we can prove that this is a zero check that covers the division to be rewired (e.g. same divisor and comparison excludes zero etc.) - and maybe that's all we need (discussed this idea with @eme64). Could be investigated with/against Option 3.

I think that aligns with your suggestion here:

One find suggestion, though, what do you think about overriding DivINode::depends_only_on_test and checking if the control node is a pattern that does exclude 0 from the value range of the divisor. This will do the job as the control of 230 DivI is an unrelated RangeCheck, making depends_only_on_test return false and the division stays in the loop while for the aforementioned case the control is the zero check and the division can be hoisted out of the loop.

Not exactly sure what you mean with the CastNode. Are you thinking in a more general sense to handle CastNodes, unrelated to divisions? It indeed generally is a problem for any kind of pinned data node that loses connection to its check. For CastNodes, we also pin them for array accesses with pin_array_access_node() - not only load nodes. There might be more problems with CastNodes where we need to do this more careful pinning as for array accesses. One would need to further investigate.

Nevertheless, with this fix, I don't think we make the current situation worse: We cannot hoist divisions out of the loop and Theo's patch just fixes an edge case, where we wrongly hoisted a division out of a loop. My guess is that disallowing that edge case does not affect performance much.

[merykitty]

Got it, thanks for the clarification.

[chhagedorn]

Sure, you're welcome :-) Just to let you know, Theo is currently away and will come back to this after the Christmas break.

[merykitty]

I have looked more deeply into the issue with depends_only_on_test in general and I have a really deep concern regarding the current state of how it is handled.

To start with, what is depends_only_on_test? Given a node n with the control input c, if c can be deduced from c' and n->depends_only_on_test() == true, then we can rewire the control input of n to c'. This means that depends_only_on_test does not mean that the node depends on a test, it means that the node depends on the test that is its control input.

For example:

if (y != 0) {
    if (x > 0) {
        if (y != 0) {
            x / y;
        }
    }
}

Then x/y depends_only_on_test because its control input is the test y != 0. Then, we can rewire the control input of the division to the outer y != 0, resulting in:

if (y != 0) {
    x / y;
    if (x > 0) {
    }
}

On the other hand, consider this case:

if (x > 0) {
    if (y != 0) {
        if (x > 0) {
            x / y;
        }
    }
}

Then x/y does not depends_only_on_test because its control input is the test x > 0 which is unrelated, we can see that if we rewire the division to the outer x > 0 test, the division floats above the actual test y != 0. This means that depends_only_on_test is a dynamic property of a node, and not a static property of the division operation. It can change when we transform the graph and it can be different for different nodes of the same kind.

So, what is the issue in JDK-8257822? When the zero check of the division is split through Phi but the division is not, it is wired to the merge point, not the tests themselves. This means that the division no longer depends_only_on_test and should return false. However, it still reports that it depends_only_on_test, which makes PhaseIdealLoop::dominated_by move it out of the loop when its control input is moved.

The fix was to make PhaseIdealLoop::dominated_by treat a division as if it does not depends_only_on_test when we cannot prove that the divisor is non-zero. This fixed the issue in that particular instance, as it achieved the same result as an actual correct fix of making depends_only_on_test return false. However, the node still reports itself as depends_only_on_test, and that opens more opportunities of miscompilation.

One important consideration regarding nodes that depends_only_on_test is that we have to move them from a test to an equivalent test if we want to keep its property of depends_only_on_test. Look at the previous example:

if (y != 0) {
    if (x > 0) {
        if (y != 0) {
            x / y;
        }
    }
}

If we want to keep x/y being depends_only_on_test, we have to move it to the outer y != 0, and not to the x > 0, because in that case:

if (y != 0) {
    if (x > 0) {
        x / y;
    }
}

It can easily be seen that the division now does not depends_only_on_test. The fix for JDK-8257822 violated this, which resulted in more cases when the division mistakenly report depends_only_on_test.

For example:

for () {
    if (y != 0) {
        x / y;
    }
}

Without the fix, x/y will be moved out of the loop together with the test y != 0, and as it still has y != 0 as its control input, it is correct when reporting itself as depends_only_on_test. However, with the fix:

if (y != 0) {
}
for () {
    x / y;
}

Now x/y has its control input being the loop head, but it still depends_only_on_test. This is wrong and opens more chances of miscompilation.

For the case in this issue, it is precisely caused by this wrong approach:

for () {
    if (y != 0) {
        1 / y;
        if rangecheck {
            if (y != 0) {
                2 / y;
            }
        }
    }
}

Because IfNode::dominated_by checks for s->depends_only_on_test() && igvn->no_dependent_zero_check(s), we rewire 2 / y to the rangecheck. This makes it wrongly report itself as depends_only_on_test when its control input is a rangecheck, which is unrelated, which leads to this issue. Without this wrong approach, 2 / y will be correctly rewired to the outer y != 0, and there would be no mistaken hoist of 2 / y here.

As a result, this will inevitably converge to us doing s->depends_only_on_test() && igvn->no_dependent_zero_check(s) at every place depends_only_on_test being used, which is really equivalent to divisions being not depends_only_on_test. And this would be a safe conservative fix: to make division not depends_only_on_test, which effectively pins them to where they are created.

However, it is still not enough. The core issue here is that a node being disconnected from its test but still reporting itself as depends_only_on_test, this applies for Div, Mod, ConstraintCast, .... For example:

int x;
if (b) {} else {}
if (x > 3) {
    cast(x, [4, max_int];
}

Then if the test x > 3 is split through the region, but the CastIINode is not, then its control input would be the merge point, which is not the test it depends on, but the node still reports itself as depends_only_on_test. The difference is that a division will immediately complain when its divisor is 0, while a CastIINode will not complain anything when the value of the input does not actually lie inside the range of the CastIINode. I have tried adding a flag that verifies this and it seems there are multiple failures #22880 .

I have skimmed through the code and it seems we do actually pin the node when the new control input is not equivalent to the old control input, but only for array accesses? I believe this should be applied to all depends_only_on_test nodes, we need to pin all of them when they are no longer directly connected to their tests.

In conclusion, I would strongly prefer a proper fix in which we pin all depends_only_on_test nodes when the new control input is not equivalent to the old control input, I believe looking at the places where we currently pin array accesses in Node::pin_array_access_node should be adequate. Another approach is to make divisions not depends_only_on_test. This should fix most issues we have with SIGFPE but it may introduce regressions and it does not solve similar issues for other depends_only_on_test nodes, which often silently miscompile.

[theoweidmannoracle]

@merykitty Thank you for your detailed write-up! @chhagedorn and I talked about it and we also agree with you that there seems to be some fundamental flaw here that needs to be addressed. We should definitely address it soon. It looks like a bigger endeavor though and we should probably file your write-up as RFE so it does not get buried here.

Do you think we should give up on this point fix then? Or do you think it's fine if we merge it and address the underlying cause separately? We still believe that there's no harm in applying this band-aid patch in the way I proposed, while, of course, we have to address the underlying issue here too.

@rwestrel added pin_array_access_node. Maybe you also want to weigh in on this?

[merykitty]

@theoweidmannoracle I think this fix is fine, please go ahead

It looks like a bigger endeavor though and we should probably file your write-up as RFE so it does not get buried here.

I created https://bugs.openjdk.org/browse/JDK-8347365

[theoweidmannoracle]

@merykitty Thanks for opening the RFE!

[theoweidmannoracle]

/integrate

[openjdk]

@theoweidmannoracle
Your change (at version 8da7cb5) is now ready to be sponsored by a Committer.

[chhagedorn]

/sponsor

[openjdk]

Going to push as commit 55c6904.
Since your change was applied there have been 263 commits pushed to the master branch:

• 1a0fe49: 8347256: Epsilon: Demote heap size and AlwaysPreTouch warnings to info level
• 8b07617: 8347345: Remove redundant test policy file from ModelMBeanInfoSupport directory
• f6492aa: 8347279: Problemlist TestEvilSyncBug.java#generational
• 0210a63: 8347379: Problem list failed tests after JDK-8321413
• a9351df: 8346787: Fix two C2 IR matching tests for RISC-V
• 89ee1a5: 8343978: Update the default value of CodeEntryAlignment for Ampere-1A and 1B
• 931914a: 8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region
• 665c39c: 8347375: Extra

  tag in robot specification

• b335ea9: 8347122: Add missing @serial tags to module java.desktop
• df28cec: 8345144: Robot does not specify all causes of IllegalThreadStateException
• ... and 253 more: https://git.openjdk.org/jdk/compare/4f855d1342d55aeee93b7d0c5796fbfd4994c856...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn @theoweidmannoracle Pushed as commit 55c6904.

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