8323274: C2: array load may float above range check #17635

rwestrel · 2024-01-30T16:50:08Z

This PR includes 5 test cases in which an array load floats above its
range check and the resulting compiled code can be made to segfault by
passing an out of bound index to the test method. Each test case takes
advantage of a different transformation to make the array load happen
too early:

For instance, with TestArrayAccessAboveRCAfterSplitIf:

if (k == 2) {
    v = array1[i];
    array = array1;
    if (l == m) {
    }
} else {
    v = array2[i];
    array = array2;
}
v += array[i]; // range check + array load

The range check is split through phi:

if (k == 2) {
    v = array1[i];
    array = array1;
    if (l == m) {
    }
    // range check here
} else {
    v = array2[i];
    array = array2;
    // range check here
}
v += array[i]; // array load

Then an identical dominating range check is found:

if (k == 2) {
    v = array1[i]; // range check here
    array = array1;
    if (l == m) {
    }
} else {
    v = array2[i];  // range check here
    array = array2;
}
v += array[i]; // array load

Then a branch dies:

v = array1[i]; // range check here
array = array1;
if (l == m) {
}
v += array[i]; // array load

The array load is dependent on the if (l == m) { condition. An
identical dominating condition is then found which causes the control
dependent range check to float above the range check.

Something similar can be triggered with:

TestArrayAccessAboveRCAfterPartialPeeling: sometimes, during partial
peeling a load is assigned the loop head as control so something
gets in between the range check and an array load and steps similar
to the above can cause the array load to float above its range check.
TestArrayAccessAboveRCAfterUnswitching: cloning a loop body adds
regions on exits of the loop and nodes that only have uses out of
the loop can end up control dependent on one of the regions. In the
test case, unswitching is what causes the cloning to happen. Again
similar steps as above make the array load floats above its range
check. I suppose similar bugs could be triggered with other loop
transformations that rely on loop body cloning.

TestArrayAccessAboveRCAfterSinking is a bit different in that it can
change the control of an array load to be the projection of some
arbitrary test. That test can then be replaced by a dominating one
causing the array to float.

Finally, in TestArrayAccessAboveRCForArrayCopyLoad, an array copy is
converted to a series of loads/stores that's guarded by a test for
srcPos < dstPos. A dominating identical test exists so an array load
floats above the runtime checks that guarantee the arraycopy is legal.

In all cases, the fix I propose is similar to 8319793: mark the array
access nodes pinned when the transformation happens.

This might be over conservative in some cases. I intend to address
some of that with: 8324976 (C2: allow array loads known to be within
bounds to float) which would set a load's control to null in the cases
when it is known to be within bounds.

I've also been working on a verification pass to catch these issues. I
intend to propose it later.

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-8323274: C2: array load may float above range check (Bug - P2)

Reviewers

Emanuel Peter (@eme64 - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17635

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-30T16:54:33Z

👋 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 · 2024-01-30T16:58:59Z

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

mlbridge · 2024-01-30T17:13:36Z

Webrevs

eme64

Generally looks reasonable, I'm mostly suggesting cosmetic changes.

src/hotspot/share/opto/split_if.cpp

src/hotspot/share/opto/loopopts.cpp

src/hotspot/share/opto/arraycopynode.cpp

eme64 · 2024-02-07T09:03:14Z

test/hotspot/jtreg/compiler/rangechecks/TestArrayAccessAboveRCAfterSinking.java

+        try {
+            test1(allTrue, array, -1, true, 0);
+            test2(allTrue, array, -1, true, 0);
+        } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) {


Is there a reason both are together in the same try-catch block? It seems like if the first throws, then the second is never executed (and may as well be removed), or we only expect the second to throw, and so the first one should be outside the bock. What do you think?

You're right. I will change that.

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

eme64 · 2024-02-07T13:19:52Z

src/hotspot/share/opto/loopnode.hpp

@@ -1739,6 +1739,8 @@ class PhaseIdealLoop : public PhaseTransform {
  void update_addp_chain_base(Node* x, Node* old_base, Node* new_base);

  bool can_move_to_inner_loop(Node* n, LoopNode* n_loop, Node* x);
+
+  void pin_array_access_nodes(Node* ctrl);


Suggested change

void pin_array_access_nodes(Node* ctrl);

void pin_array_access_nodes_dependent_on(Node* ctrl);

Does this even build without?

src/hotspot/share/opto/loopopts.cpp

eme64 · 2024-02-07T13:21:39Z

@rwestrel just ping me again when I should re-review ;)

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

rwestrel · 2024-02-09T13:58:13Z

I updated the change and it's ready for another review.
I noticed that one change in PhaseIdealLoop::try_sink_out_of_loop() is incorrect as it clones a node but doesn't connect it to any uses. This is fixed in the new commit.

eme64

Thanks for the updates @rwestrel , looks good to me now 😊

openjdk · 2024-02-09T14:39:52Z

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

8323274: C2: array load may float above range check

Reviewed-by: epeter, thartmann

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 287 new commits pushed to the master branch:

3742bc6: 8323795: jcmd Compiler.codecache should print total size of code cache
099b744: 8326117: ProblemList serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java#default in Xcomp mode
39627bc: 6510914: JScrollBar.getMinimumSize() breaks the contract of JComponent.setMinimumSize()
7004c27: 8303972: (zipfs) Make test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java independent of the zip command line
c2d9fa2: 8326000: Remove obsolete comments for class sun.security.ssl.SunJSSE
f50df10: 8299023: TestPLABResize.java and TestPLABPromotion.java are failing intermittently
cf13086: 8317697: refactor-encapsulate x86 VM_Version::CpuidInfo
3b76372: 8325687: SimpleJavaFileObject specification would benefit from implSpec
b5df2f4: 8323170: j2dbench is using outdated javac source/target to be able to build by itself
267780b: 8324680: Replace NULL with nullptr in JVMTI generated code
... and 277 more: https://git.openjdk.org/jdk/compare/7a798d3cebea0915f8a73af57333b3488c2091af...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.

TobiHartmann

Great analysis, Roland. The fix looks good to me. I'll run correctness and performance testing and report back.

TobiHartmann · 2024-02-21T05:59:55Z

All tests passed. No performance regression detected.

rwestrel · 2024-02-22T11:06:01Z

@eme64 @TobiHartmann thanks for the reviews and testing.

rwestrel · 2024-02-22T11:06:11Z

/integrate

openjdk · 2024-02-22T11:07:16Z

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

cc1e216: 8326461: tools/jlink/CheckExecutable.java fails as .debuginfo files are not executable
10eafdc: 8325870: Zap end padding bits for ArrayOops in non-release builds
0f4cd8f: 8326414: Serial: Inline SerialHeap::create_rem_set
8b30503: 8323695: RenderPerf (2D) enhancements (23.12)
8e5f6dd: 8324243: Compilation failures in java.desktop module with gcc 14
0bcece9: 8325342: Remove unneeded exceptions in compare.sh
64f7972: 8326158: Javadoc for java.time.DayOfWeek#minus(long)
f0f4d63: 8326351: Update the Zlib version in open/src/java.base/share/legal/zlib.md to 1.3.1
51e2dde: 8326235: RISC-V: Size CodeCache for short calls encoding
c022431: 8326412: debuginfo files should not have executable bit set
... and 331 more: https://git.openjdk.org/jdk/compare/7a798d3cebea0915f8a73af57333b3488c2091af...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-22T11:07:25Z

@rwestrel Pushed as commit 4406915.

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

tests & fixes

7dc67a7

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 30, 2024

whitespaces

c5dc8db

openjdk bot added the rfr Pull request is ready for review label Jan 30, 2024

eme64 suggested changes Feb 7, 2024

View reviewed changes

rwestrel and others added 6 commits February 7, 2024 13:48

Update src/hotspot/share/opto/split_if.cpp

8567ab2

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/split_if.cpp

ad1f271

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/loopopts.cpp

1ba9f0c

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/loopopts.cpp

cff9810

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/arraycopynode.cpp

61cc66d

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/loopopts.cpp

5fe8019

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

eme64 reviewed Feb 7, 2024

View reviewed changes

review

b6ba4b6

eme64 reviewed Feb 7, 2024

View reviewed changes

src/hotspot/share/opto/loopopts.cpp Outdated Show resolved Hide resolved

rwestrel and others added 2 commits February 7, 2024 14:28

Update src/hotspot/share/opto/loopopts.cpp

dc4895f

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

fix broken try_sink_out_of_loop

e84ac6b

eme64 approved these changes Feb 9, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 9, 2024

TobiHartmann approved these changes Feb 19, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 22, 2024

openjdk bot closed this Feb 22, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8323274: C2: array load may float above range check #17635

8323274: C2: array load may float above range check #17635

rwestrel commented Jan 30, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 30, 2024

openjdk bot commented Jan 30, 2024

mlbridge bot commented Jan 30, 2024 •

edited

Loading

eme64 left a comment

eme64 Feb 7, 2024

rwestrel Feb 7, 2024

eme64 Feb 7, 2024

eme64 commented Feb 7, 2024

rwestrel commented Feb 9, 2024 •

edited

Loading

eme64 left a comment

openjdk bot commented Feb 9, 2024 •

edited

Loading

TobiHartmann left a comment

TobiHartmann commented Feb 21, 2024

rwestrel commented Feb 22, 2024

rwestrel commented Feb 22, 2024

openjdk bot commented Feb 22, 2024

openjdk bot commented Feb 22, 2024

	void pin_array_access_nodes(Node* ctrl);
	void pin_array_access_nodes_dependent_on(Node* ctrl);

8323274: C2: array load may float above range check #17635

8323274: C2: array load may float above range check #17635

Conversation

rwestrel commented Jan 30, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 30, 2024

openjdk bot commented Jan 30, 2024

mlbridge bot commented Jan 30, 2024 • edited Loading

Webrevs

eme64 left a comment

Choose a reason for hiding this comment

eme64 Feb 7, 2024

Choose a reason for hiding this comment

rwestrel Feb 7, 2024

Choose a reason for hiding this comment

eme64 Feb 7, 2024

Choose a reason for hiding this comment

eme64 commented Feb 7, 2024

rwestrel commented Feb 9, 2024 • edited Loading

eme64 left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 9, 2024 • edited Loading

TobiHartmann left a comment

Choose a reason for hiding this comment

TobiHartmann commented Feb 21, 2024

rwestrel commented Feb 22, 2024

rwestrel commented Feb 22, 2024

openjdk bot commented Feb 22, 2024

openjdk bot commented Feb 22, 2024

rwestrel commented Jan 30, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 30, 2024 •

edited

Loading

rwestrel commented Feb 9, 2024 •

edited

Loading

openjdk bot commented Feb 9, 2024 •

edited

Loading