Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8267904: C2 crash when compile negative Arrays.copyOf length after loop #4238

Closed
wants to merge 5 commits into from

Conversation

huishi-hs
Copy link

@huishi-hs huishi-hs commented May 28, 2021

C2 crash when Arrays.copyOf has a negative length after a loop. This happens in release and debug build. Test and hs_err are in JBS.

Crash reason is:

  • CastIINode is created in GraphKit::new_array (in AllocateArrayNode::make_ideal_length), Cast array lenght to range [0, maxint-2]. This is safe when allocation is success and CastIINode 's input control is InitializeNode's proj control.
  • In LibraryCallKit::inline_arraycopy, InitializeNode's proj control's use nodes' control is replaced with AllocateArrayNode's input control (in LibraryCallKit::arraycopy_move_allocation_here). This is necessary to move allocation after array copy checks. But this also includes CastIINode.
   C->gvn_replace_by(init->proj_out(TypeFunc::Control), alloc->in(0));
  • CastIINode's control is also adjust to AllocateArrayNode's input control, which is illegal state in laster IGVN phase, casting a negative to [0, maxint-2].
  • This cause control and nodes after loop become top and removed. The previous loop has no fall-through edge and crash.

Fix is:

  • In LibraryCallKit::arraycopy_move_allocation_here
    • Before replacing init->proj_out(TypeFunc::Control) in, find and replace CastIINode nodes with original array length.
    • After move allocation node, create CastIINode again if necessary.

Before fix: node 250 is CastII which should be after InitializeNode.
image

After fix: all arry copy check is performed on original array length node 203
image

New test test/hotspot/jtreg/compiler/c2/TestNegativeArrayCopyAfterLoop.java is added and pass.
Tests performs on Linux X64 and no regression

  • Tier1/2/3/hotspot_all_no_apps on release and fastdebug build.
  • Tier1/2/3 with option "-XX:-TieredCompilation -Xbatch" on fastdebug build

Progress

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

Issue

  • JDK-8267904: C2 crash when compile negative Arrays.copyOf length after loop

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4238

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented May 28, 2021

👋 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 openjdk bot added the rfr Pull request is ready for review label May 28, 2021
@openjdk
Copy link

openjdk bot commented May 28, 2021

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

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label May 28, 2021
@mlbridge
Copy link

mlbridge bot commented May 28, 2021

Webrevs

Copy link
Contributor

@rwestrel rwestrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall fix looks ok to me.

src/hotspot/share/opto/library_call.cpp Show resolved Hide resolved
Copy link
Contributor

@rwestrel rwestrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise looks good to me.

}
#endif
C->gvn_replace_by(init_out, alloc_length);
record_for_igvn(init_out);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a call to record_for_igvn() is needed here.

Copy link
Author

@huishi-hs huishi-hs Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review and commit is updated.

Yes, "record_for_igvn(init_out)" is redundant as it will be "record_for_igvn" again in "C->gvn_replace_by(init->proj_out(TypeFunc::Control), alloc->in(0))", as it is user of "init->proj_out(TypeFunc::Control)".

After "C->gvn_replace_by(init_out, alloc_length)", CastIINode has no use and should be deleted. This is why record_for_igvn is added here in my thought.

@openjdk
Copy link

openjdk bot commented Jun 3, 2021

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

8267904: C2 crash when compile negative Arrays.copyOf length after loop

Reviewed-by: roland, 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 128 new commits pushed to the master branch:

  • 042f0bd: 8256465: [macos] Java frame and dialog presented full screen freeze application
  • 8abf36c: 8268289: build failure due to missing signed flag in x86 evcmpb instruction
  • b05c40c: 8266951: Partial in-lining for vectorized mismatch operation using AVX512 masked instructions
  • f768fbf: 8268286: ProblemList serviceability/sa/TestJmapCore.java on linux-aarch64 with ZGC
  • b2e9eb9: 8268087: Update documentation of the JPasswordField
  • 91f9adc: 8268139: CDS ArchiveBuilder may reference unloaded classes
  • 36bff6f: 8066694: Strange code in JavacParser.java
  • 6c838c5: 8266846: Add java.time.InstantSource
  • 7f55dc1: 8179880: Refactor javax/security shell tests to plain java tests
  • 7e41ca3: 8266957: SA has not followed JDK-8220587 and JDK-8224965
  • ... and 118 more: https://git.openjdk.java.net/jdk/compare/0c9daa7ed579cd82343f37a68964876ebc48122e...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 (@rwestrel, @vnkozlov) 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 openjdk bot added the ready Pull request is ready to be integrated label Jun 3, 2021
@huishi-hs
Copy link
Author

May I have a second review for this PR?

@huishi-hs
Copy link
Author

Thanks for helping review @vnkozlov @rwestrel !

@huishi-hs
Copy link
Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 6, 2021
@openjdk
Copy link

openjdk bot commented Jun 6, 2021

@huishi-hs
Your change (at version cdb3b15) is now ready to be sponsored by a Committer.

@DamonFool
Copy link
Member

/sponsor

@openjdk openjdk bot closed this Jun 7, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 7, 2021
@openjdk
Copy link

openjdk bot commented Jun 7, 2021

@DamonFool @huishi-hs Since your change was applied there have been 130 commits pushed to the master branch:

  • 95ddf7d: 8267839: trivial mem leak in numa
  • 52d88ee: 8268292: compiler/intrinsics/VectorizedMismatchTest.java fails with release VMs
  • 042f0bd: 8256465: [macos] Java frame and dialog presented full screen freeze application
  • 8abf36c: 8268289: build failure due to missing signed flag in x86 evcmpb instruction
  • b05c40c: 8266951: Partial in-lining for vectorized mismatch operation using AVX512 masked instructions
  • f768fbf: 8268286: ProblemList serviceability/sa/TestJmapCore.java on linux-aarch64 with ZGC
  • b2e9eb9: 8268087: Update documentation of the JPasswordField
  • 91f9adc: 8268139: CDS ArchiveBuilder may reference unloaded classes
  • 36bff6f: 8066694: Strange code in JavacParser.java
  • 6c838c5: 8266846: Add java.time.InstantSource
  • ... and 120 more: https://git.openjdk.java.net/jdk/compare/0c9daa7ed579cd82343f37a68964876ebc48122e...master

Your commit was automatically rebased without conflicts.

Pushed as commit b05fa02.

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

@dean-long
Copy link
Member

This PR seems to have caused a failure in one of Oracle's internal tests. It may need to be backed out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
5 participants