8376400: C2: folding ifs may cause incorrect execution when trap is taken

[rwestrel]

For the following test method:

    private static void test1(int i) {
        int v;
        if (i + MIN_VALUE >= 16 + Integer.MIN_VALUE) {
            v = 0;
        } else {
            v = 1;
        }
        if (v == 0) {
            throw new RuntimeException("never taken");
        }
        if (i + MIN_VALUE < 8 + Integer.MIN_VALUE) {
            taken1++; // never taken by the time compilation happens
        }
    }

The exception path and the 3rd if branch are compiled as uncommon
traps so in pseudo code:

    private static void test1(int i) {
        int v;
        if (i + MIN_VALUE >= 16 + Integer.MIN_VALUE) {
            v = 0;
        } else {
            v = 1;
        } // v is Phi(0, 1)
        if (v == 0) {
            uncommon_trap(); // reexecutes the if (v == 0) { above, captures v as stack argument to ifeq bytecode
        }
        if (i + MIN_VALUE < 8 + Integer.MIN_VALUE) {
            uncommon_trap(); // reexecutes the if (i + MIN_VALUE < 8 + Integer.MIN_VALUE) {
        }
    }

next split thru phi runs:

    private static void test1(int i) {
        if (i + MIN_VALUE >= 16 + Integer.MIN_VALUE) {
            uncommon_trap(); // reexecutes the if (v == 0) that was removed { above, captures v = 0 as stack argument to ifeq bytecode
        }
        if (i + MIN_VALUE < 8 + Integer.MIN_VALUE) {
            uncommon_trap(); // reexecutes the if (i + MIN_VALUE < 8 + Integer.MIN_VALUE) {
        }
    }

next IfNode::fold_compares() runs to transform the 2 signed
comparison into a single unsigned comparison:

    private static void test1(int i) {
        if (i - 8 >=u 8) {
            uncommon_trap(); // reexecutes the if (v == 0) that was removed { above, captures v = 0 as stack argument to ifeq bytecode
        }
    }

test1 is passed 0. That causes the uncommon trap to execute and
execution after deoptimization to resume at the if (v == 0) { which
is reexecuted. The bug is that split if causes the stack captured in
the uncommon trap to be updated (the Phi v is replaced in the v = 0
branch by 0). When execution resumes interpreted, it restarts with v = 0
and the if (v == 0) { branch is taken (which is wrong).

Folding the 2 ifs created an unsigned condition that is stricter that:

if (i + MIN_VALUE >= 16 + Integer.MIN_VALUE) {

which would be fine if execution resumed at if (v == 0) { with the
state of the interprer unmodified but, here, it is not in this case.

The fix I propose is to mark the uncommon trap Call when split if
happens and to check if the uncommon trap is marked in
IfNode::fold_compares(). I, initially, tried to fix this by trying
to validate the jvms state in IfNode::fold_compares() before
proceeding with the transformation (such as checking that the 2
uncommon traps have the same jvms state). Running some experiments, it
seems that would exclude a lot of cases where
IfNode::fold_compares() happens and is likely safe so I was
concerned code quality would be impacted and by being overly
conservative.

I see that @merykitty mentions transformating the shape of the control
flow in a comment in the bug. My concern here is it could also impact
code quality (increase code size that gets in the way of inlining or
some hard to predict effect on register allocation) when it's usually
fine to proceed with IfNode::fold_compares().

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

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-8376400: C2: folding ifs may cause incorrect execution when trap is taken (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30677

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@rwestrel 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.

[rwestrel]

/template append

[openjdk]

@rwestrel The pull request template has been appended to the pull request body

[mlbridge]

Webrevs

• 00: Full (dcd32aa7)

[dean-long]

Is it only uncommon_trap that has this problem? Can we rule out similar problems with other safepoints?