8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop #49

huishi-hs · 2021-06-15T02:08:30Z

Some closed tests fail due to original PR, the main reasons are:

8268325/8268345
CastIINode created in AllocateArrayNode::make_ideal_length apply on a negative array length. GVN transform converts these CastIINode to top. This breaks assumptions, eg., array index node type is TypeInt.
8268345
Some CastIINode is created in LoadRange::Ideal, which doesn't apply GVN transform. CastIINode can not be found in GVN hashtable and cause assertion failure.
Based on previous PR #4238, this PR unifies CastIINode processing post AllocateArrayNode::make_ideal_length. Setup type and add into igvn worklist. Avoid apply GVN transform to break different assumptions in parser.

Based on closed test failrue message, two new tests are added

TestNegArrayLengthAsIndex1 : fail in debug build even without previous PR #4238, it loads a negative array length and apply GVN transform (In GraphKit::load_array_length) which covert length to top node. It also assert in Parse::array_addressing.

TestNegArrayLengthAsIndex2 : similar with failrue in https://bugs.openjdk.java.net/browse/JDK-8268301

Progress

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

Issue

JDK-8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Roland Westrelin (@rwestrel - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 49

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

Using diff file

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

…fter loop

bridgekeeper · 2021-06-15T02:09:13Z

👋 Welcome back hshi! 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-06-15T02:11:12Z

@huishi-hs 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-06-15T02:16:08Z

Webrevs

dean-long · 2021-06-16T20:11:05Z

I tested your changes and there were no related failures.

huishi-hs · 2021-06-17T01:10:35Z

I tested your changes and there were no related failures.

@dean-long Thanks!

vnkozlov

Few comments.

vnkozlov · 2021-06-17T04:07:32Z

src/hotspot/share/opto/graphKit.cpp

    Node* ccast = alloc->make_ideal_length(_gvn.type(array)->is_oopptr(), &_gvn);
    if (ccast != alen) {
-      alen = _gvn.transform(ccast);
+      _gvn.set_type_bottom(ccast);
+      record_for_igvn(ccast);
+      alen = ccast;


I think we need GraphKit wrapper method which execute this code to avoid duplication here, in arraycopy_move_allocation_here() and in new_array().
Add comment why we can't do transform().

@vnkozlov Thanks for your review!

GraphKit::load_array_length code is different with new_array() and arraycopy_move_allocation_here(), it doesn't replace_in_map. Currently GraphKit::cast_replace_array_length_post_allocation warpper is used in new_array() and arraycopy_move_allocation_here().

Would you prefer a wrapper for all these three places? It would be like following code.

Node* GraphKit::cast_array_length_post_allocation(AllocateArrayNode* alloc, const TypeOopPtr* oop_type, bool replace_map /false in load_array_length/) { Node* length = alloc->in(AllocateNode::ALength); if (replace_map == false || map()->find_edge(length) >= 0) { Node* ccast = alloc->make_ideal_length(oop_type, &_gvn); if (ccast != length) { _gvn.set_type_bottom(ccast); record_for_igvn(ccast); if (replace_map) { replace_in_map(length, ccast); } return cast; } } return length; }

All places, please. I prefer different names for this method and its parameters GraphKit::array_ideal_length(alloc , ary_type, replace_length_in_map). No need for parameter's comment in declaration - it is clear from name.

done， new wrapper is added, all 3 places: 1) new_array 2) arraycopy_move_allocation_here 3) load_array_length use GraphKit::array_ideal_length(alloc , ary_type, replace_length_in_map)

vnkozlov · 2021-06-17T04:39:45Z

src/hotspot/share/opto/library_call.cpp

+    Node* prev_cast = NULL;
+#endif
+    for (uint i = 0; i < init_control->outcnt(); i++) {
+      Node *init_out = init_control->raw_out(i);


Style: Node* init_out

vnkozlov · 2021-06-17T05:11:37Z

src/hotspot/share/opto/library_call.cpp

+      Node *init_out = init_control->raw_out(i);
+      if (init_out->is_CastII() && init_out->in(0) == init_control && init_out->in(1) == alloc_length) {
+#ifdef ASSERT
+        if (prev_cast == NULL) {


Add comment why multiply CastII could be here (because each load_array_length() call in guard checks will generate now separate CastII nodes). All these CastII nodes are replaced here with one: original alloc_length.

I am wondering if will affect result of guard checks in runtime.

done.

Would you share more detail about your concern for "if will affect result of guard checks in runtime."?

vnkozlov · 2021-06-17T05:12:50Z

src/hotspot/share/opto/library_call.cpp

+      Node* ccast = alloc->make_ideal_length(ary_type, &_gvn);
+      if (ccast != length) {
+        _gvn.set_type_bottom(ccast);
+        record_for_igvn(ccast);


Use wrapper method to avoid duplication.

…ocation & add more comments

vnkozlov

I forgot to say that tests should be in compiler/arraycopy/ directory.

vnkozlov · 2021-06-17T16:14:52Z

src/hotspot/share/opto/graphKit.cpp

    Node* ccast = alloc->make_ideal_length(_gvn.type(array)->is_oopptr(), &_gvn);
    if (ccast != alen) {
-      alen = _gvn.transform(ccast);
+      _gvn.set_type_bottom(ccast);
+      record_for_igvn(ccast);
+      alen = ccast;


All places, please. I prefer different names for this method and its parameters GraphKit::array_ideal_length(alloc , ary_type, replace_length_in_map). No need for parameter's comment in declaration - it is clear from name.

…n after array allocation. Update comments.

vnkozlov

Looks good. Let me test it.

openjdk · 2021-06-18T13:57:49Z

@huishi-hs 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:

8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop

Reviewed-by: kvn, roland

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

c294ae4: 8267042: bug in monitor locking/unlocking on ARM32 C1 due to uninitialized BasicObjectLock::_displaced_header
b358b54: 8269063: Build failure due to VerifyReceiverTypes was not declared after JDK-8268405
b8f073b: 8268316: Typo in JFR jdk.Deserialization event
b9d7337: 8268638: semaphores of AsyncLogWriter may be broken when JVM is exiting.
8caeca0: 8264775: ClhsdbFindPC still fails with java.lang.RuntimeException: 'In java stack' missing from stdout/stderr
7e03cf2: 8265073: XML transformation and indentation when using xml:space
60389ee: 8269025: jsig/Testjsig.java doesn't check exit code
dab00ee: 8266518: Refactor and expand scatter/gather tests
f9c8c1c: 8268903: JFR: RecordingStream::dump is missing @SInCE
d8a0582: 8265369: [macos-aarch64] java/net/MulticastSocket/Promiscuous.java failed with "SocketException: Cannot allocate memory"
... and 46 more: https://git.openjdk.java.net/jdk17/compare/4d8b5c70dff51470210a0ca93b932af1b27c9f27...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.

vnkozlov · 2021-06-18T23:02:44Z

Testing looks good.

huishi-hs · 2021-06-19T02:15:21Z

Testing looks good.

@vnkozlov thanks!

huishi-hs · 2021-06-21T01:36:10Z

May I have a second review for this PR?

rwestrel

Looks good to me.

huishi-hs · 2021-06-21T11:56:56Z

Looks good to me.

Thanks again! @rwestrel

huishi-hs · 2021-06-21T11:57:05Z

/integrate

openjdk · 2021-06-21T11:59:16Z

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

f8df953: 8268702: JFR diagnostic commands lack argument descriptors when viewed using Platform MBean Server
c294ae4: 8267042: bug in monitor locking/unlocking on ARM32 C1 due to uninitialized BasicObjectLock::_displaced_header
b358b54: 8269063: Build failure due to VerifyReceiverTypes was not declared after JDK-8268405
b8f073b: 8268316: Typo in JFR jdk.Deserialization event
b9d7337: 8268638: semaphores of AsyncLogWriter may be broken when JVM is exiting.
8caeca0: 8264775: ClhsdbFindPC still fails with java.lang.RuntimeException: 'In java stack' missing from stdout/stderr
7e03cf2: 8265073: XML transformation and indentation when using xml:space
60389ee: 8269025: jsig/Testjsig.java doesn't check exit code
dab00ee: 8266518: Refactor and expand scatter/gather tests
f9c8c1c: 8268903: JFR: RecordingStream::dump is missing @SInCE
... and 47 more: https://git.openjdk.java.net/jdk17/compare/4d8b5c70dff51470210a0ca93b932af1b27c9f27...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-06-21T11:59:31Z

@huishi-hs Pushed as commit 22ebd19.

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

8268362: [REDO] C2 crash when compile negative Arrays.copyOf length a…

fc6c095

…fter loop

huishi-hs mentioned this pull request Jun 15, 2021

8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop openjdk/jdk#4480

Closed

3 tasks

openjdk bot added the rfr Pull request is ready for review label Jun 15, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.java.net label Jun 15, 2021

vnkozlov reviewed Jun 17, 2021

View reviewed changes

move duplicate code into GraphKit::cast_replace_array_length_post_all…

a2989cd

…ocation & add more comments

vnkozlov reviewed Jun 17, 2021

View reviewed changes

Add GraphKit::array_ideal_length wrapper for all CastIINode generatio…

853bb36

…n after array allocation. Update comments.

vnkozlov approved these changes Jun 18, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 18, 2021

rwestrel approved these changes Jun 21, 2021

View reviewed changes

openjdk bot closed this Jun 21, 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 Jun 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop #49

8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop #49

huishi-hs commented Jun 15, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 15, 2021

openjdk bot commented Jun 15, 2021

mlbridge bot commented Jun 15, 2021 •

edited

Loading

dean-long commented Jun 16, 2021

huishi-hs commented Jun 17, 2021

vnkozlov left a comment

vnkozlov Jun 17, 2021

huishi-hs Jun 17, 2021

vnkozlov Jun 17, 2021

huishi-hs Jun 18, 2021

vnkozlov Jun 17, 2021

huishi-hs Jun 18, 2021

vnkozlov Jun 17, 2021

huishi-hs Jun 17, 2021

vnkozlov Jun 17, 2021

vnkozlov left a comment

vnkozlov Jun 17, 2021

vnkozlov left a comment

openjdk bot commented Jun 18, 2021 •

edited

Loading

vnkozlov commented Jun 18, 2021

huishi-hs commented Jun 19, 2021

huishi-hs commented Jun 21, 2021

rwestrel left a comment

huishi-hs commented Jun 21, 2021

huishi-hs commented Jun 21, 2021

openjdk bot commented Jun 21, 2021

openjdk bot commented Jun 21, 2021

8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop #49

8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop #49

Conversation

huishi-hs commented Jun 15, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 15, 2021

openjdk bot commented Jun 15, 2021

mlbridge bot commented Jun 15, 2021 • edited Loading

Webrevs

dean-long commented Jun 16, 2021

huishi-hs commented Jun 17, 2021

vnkozlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 18, 2021 • edited Loading

vnkozlov commented Jun 18, 2021

huishi-hs commented Jun 19, 2021

huishi-hs commented Jun 21, 2021

rwestrel left a comment

Choose a reason for hiding this comment

huishi-hs commented Jun 21, 2021

huishi-hs commented Jun 21, 2021

openjdk bot commented Jun 21, 2021

openjdk bot commented Jun 21, 2021

huishi-hs commented Jun 15, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 15, 2021 •

edited

Loading

openjdk bot commented Jun 18, 2021 •

edited

Loading