RFR: 8261912: Code IfNode::fold_compares_helper more defensively

[Aleksey Shipilev-5]

As [JDK-8261914](https://bugs.openjdk.java.net/browse/JDK-8261914) indicates, there are cases that break the internal asserts in `IfNode::fold_compares_helper`, code added by JDK-8073480 in JDK 9. Unfortunately, release builds would happily miscompile when that happens. It would be better to code `IfNode::fold_compares_helper` more defensively, so it bails when asserts are violated. This implicitly works around the bug in JDK-8261914. The goal for this limited workaround is to be trivially backportable in order to quickly unbreak 11u, 16u and 17.

The alternative is, instead of the early returns is to do:

 lo = NULL;
 hi = NULL;

...and then wait for for the method epilog to handle it. I have no preference to either style, as the blocks this patch affects already has some early returns, and `lo/hi = NULL` are also used.

Additional testing:
 - [x] Linux x86_64 fastdebug `tier1`
 - [x]  Linux x86_64 fastdebug `tier2`
 - [x] Failing JRuby reproducer from JDK-8261914, now passing in release mode with hundreds of iterations

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

Commit messages:
 - 8261912: Code IfNode::fold_compares_helper more defensively

Changes: https://git.openjdk.java.net/jdk/pull/2610/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2610&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261912
  Stats: 23 lines in 1 file changed: 15 ins; 4 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2610.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2610/head:pull/2610

PR: https://git.openjdk.java.net/jdk/pull/2610

[Vladimir Kozlov-2]

On Wed, 17 Feb 2021 16:11:39 GMT, Aleksey Shipilev <[hidden email]> wrote:

> As [JDK-8261914](https://bugs.openjdk.java.net/browse/JDK-8261914) indicates, there are cases that break the internal asserts in `IfNode::fold_compares_helper`, code added by JDK-8073480 in JDK 9. Unfortunately, release builds would happily miscompile when that happens. It would be better to code `IfNode::fold_compares_helper` more defensively, so it bails when asserts are violated. This implicitly works around the bug in JDK-8261914. The goal for this limited workaround is to be trivially backportable in order to quickly unbreak 11u, 16u and 17.
>
> The alternative is, instead of the early returns is to do:
>
>  lo = NULL;
>  hi = NULL;
>
> ...and then wait for for the method epilog to handle it. I have no preference to either style, as the blocks this patch affects already has some early returns, and `lo/hi = NULL` are also used.
>
> Additional testing:
>  - [x] Linux x86_64 fastdebug `tier1`
>  - [x]  Linux x86_64 fastdebug `tier2`
>  - [x] Failing JRuby reproducer from JDK-8261914, now passing in release mode with hundreds of iterations

I am fine with this defensive change.

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

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2610

[Tobias Hartmann-3]

On Wed, 17 Feb 2021 16:11:39 GMT, Aleksey Shipilev <[hidden email]> wrote:

> As [JDK-8261914](https://bugs.openjdk.java.net/browse/JDK-8261914) indicates, there are cases that break the internal asserts in `IfNode::fold_compares_helper`, code added by JDK-8073480 in JDK 9. Unfortunately, release builds would happily miscompile when that happens. It would be better to code `IfNode::fold_compares_helper` more defensively, so it bails when asserts are violated. This implicitly works around the bug in JDK-8261914. The goal for this limited workaround is to be trivially backportable in order to quickly unbreak 11u, 16u and 17.
>
> The alternative is, instead of the early returns is to do:
>
>  lo = NULL;
>  hi = NULL;
>
> ...and then wait for for the method epilog to handle it. I have no preference to either style, as the blocks this patch affects already has some early returns, and `lo/hi = NULL` are also used.
>
> Additional testing:
>  - [x] Linux x86_64 fastdebug `tier1`
>  - [x]  Linux x86_64 fastdebug `tier2`
>  - [x] Failing JRuby reproducer from JDK-8261914, now passing in release mode with hundreds of iterations

Looks good.

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2610

[Aleksey Shipilev-5]

On Thu, 18 Feb 2021 07:16:56 GMT, Tobias Hartmann <[hidden email]> wrote:

>> As [JDK-8261914](https://bugs.openjdk.java.net/browse/JDK-8261914) indicates, there are cases that break the internal asserts in `IfNode::fold_compares_helper`, code added by JDK-8073480 in JDK 9. Unfortunately, release builds would happily miscompile when that happens. It would be better to code `IfNode::fold_compares_helper` more defensively, so it bails when asserts are violated. This implicitly works around the bug in JDK-8261914. The goal for this limited workaround is to be trivially backportable in order to quickly unbreak 11u, 16u and 17.
>>
>> The alternative is, instead of the early returns is to do:
>>
>>  lo = NULL;
>>  hi = NULL;
>>
>> ...and then wait for for the method epilog to handle it. I have no preference to either style, as the blocks this patch affects already has some early returns, and `lo/hi = NULL` are also used.
>>
>> Additional testing:
>>  - [x] Linux x86_64 fastdebug `tier1`
>>  - [x]  Linux x86_64 fastdebug `tier2`
>>  - [x] Failing JRuby reproducer from JDK-8261914, now passing in release mode with hundreds of iterations
>
> Looks good.

The remaining test failure should be resolved with #2614. I'll integrate once this PR is 24 hours old.

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

PR: https://git.openjdk.java.net/jdk/pull/2610

[Aleksey Shipilev-5]

On Wed, 17 Feb 2021 16:11:39 GMT, Aleksey Shipilev <[hidden email]> wrote:

> As [JDK-8261914](https://bugs.openjdk.java.net/browse/JDK-8261914) indicates, there are cases that break the internal asserts in `IfNode::fold_compares_helper`, code added by JDK-8073480 in JDK 9. Unfortunately, release builds would happily miscompile when that happens. It would be better to code `IfNode::fold_compares_helper` more defensively, so it bails when asserts are violated. This implicitly works around the bug in JDK-8261914. The goal for this limited workaround is to be trivially backportable in order to quickly unbreak 11u, 16u and 17.
>
> The alternative is, instead of the early returns is to do:
>
>  lo = NULL;
>  hi = NULL;
>
> ...and then wait for for the method epilog to handle it. I have no preference to either style, as the blocks this patch affects already has some early returns, and `lo/hi = NULL` are also used.
>
> Additional testing:
>  - [x] Linux x86_64 fastdebug `tier1`
>  - [x]  Linux x86_64 fastdebug `tier2`
>  - [x] Failing JRuby reproducer from JDK-8261914, now passing in release mode with hundreds of iterations

This pull request has now been integrated.

Changeset: e9f3aab7
Author:    Aleksey Shipilev <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/e9f3aab7
Stats:     23 lines in 1 file changed: 15 ins; 4 del; 4 mod

8261912: Code IfNode::fold_compares_helper more defensively

Reviewed-by: kvn, thartmann

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

PR: https://git.openjdk.java.net/jdk/pull/2610