Skip to content
This repository has been archived by the owner. It is now read-only.

8278413: C2 crash when allocating array of size too large #30

Closed
wants to merge 1 commit into from

Conversation

rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Dec 15, 2021

On the fallthrough path from an AllocateArray, the length of the
allocated array is casted (with a CastII) to [0, max_size] with
max_size some number that depends on the array type and can be less
than max_jint.

Allocating an array of a length that's not in [0, max_size] causes the
CastII to become top. The fallthrough path must be killed as well in
that case otherwise this can lead to a broken graph. Currently c2 has
logic to protect against an allocation of array of negative size in
AllocateArrayNode::Ideal(). That call replaces the fallthrough path
with an Halt node. But if the size is too big, then the fallthrough
path is left as is.

This patch fixes that issues. It also reworks the length negative
case. I added a Bool/CmpU input to the AllocateArray that tests for a
valid length. If that input becomes false, CatchNode::Value() kills
the fallthrough path. That logic is similar to that for a virtual call
with a null receiver. I also removed AllocateArrayNode::Ideal() now
that CatchNode::Value() takes care of the same corner case. The code
in AllocateArrayNode::Ideal() was added by Vladimir and he told me he
tried extending CatchNode::Value() at the time but that caused test
failures. I had no issues in my testing so I assume doing it that way
is ok now.

The new input to AllocateArray is moved to the CallStaticJava runtime
call for array allocation on macro expansion as a precedence edge. The
reason for that is that final graph reshape needs a way to tell
whether the missing path out of the allocation is legal or not. final
graph reshape then removes the then useless precedence edge.


Progress

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

Issue

  • JDK-8278413: C2 crash when allocating array of size too large

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 15, 2021

👋 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 openjdk bot added the rfr label Dec 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 15, 2021

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

@openjdk openjdk bot added the hotspot-compiler label Dec 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 15, 2021

Webrevs

@neliasso
Copy link

@neliasso neliasso commented Dec 16, 2021

Have you run any micros to verify that nothing regresses?

Copy link

@neliasso neliasso left a comment

I general I really like the change. I simplifies things nicely.

} else {
Node* valid_length_test = call->in(AllocateNode::ValidLengthTest);
const Type* valid_length_test_t = phase->type(valid_length_test);
if (valid_length_test_t->isa_int() && valid_length_test_t->is_int()->is_con(0)) {

Choose a reason for hiding this comment

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

Here you do:

Node* valid_length_test = call->in(AllocateNode::ValidLengthTest);
const Type* valid_length_test_t = phase->type(valid_length_test);
if (valid_length_test_t->isa_int() && valid_length_test_t->is_int()->is_con(0)) {

But in compile.cpp:3766 you do:

Node* valid_length_test = call->in(call->req());
call->rm_prec(call->req());
if (valid_length_test->find_int_con(1) == 0) {

Why "call->req()" and not "call->in(AllocateNode::ValidLengthTest)"?

And why not "if (valid_length_test->find_int_con(1) == 0) {" in both places?

Copy link
Contributor Author

@rwestrel rwestrel Dec 16, 2021

Choose a reason for hiding this comment

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

Here you do:

Node* valid_length_test = call->in(AllocateNode::ValidLengthTest);
const Type* valid_length_test_t = phase->type(valid_length_test);
if (valid_length_test_t->isa_int() && valid_length_test_t->is_int()->is_con(0)) {

But in compile.cpp:3766 you do:

Node* valid_length_test = call->in(call->req());
call->rm_prec(call->req());
if (valid_length_test->find_int_con(1) == 0) {

Why "call->req()" and not "call->in(AllocateNode::ValidLengthTest)"?

call->in(AllocateNode::ValidLengthTest) only works for allocateArrayNode. The code I modified in compile.cpp runs after macro expansion so there's no AllocateArrayNode anymore. Instead there's a call to the runtime. When the AllocateArrayNode is macro expanded I move in(AllocateNode::ValidLengthTest) to the new runtime call as a precedence edge that is at req(). I tried adding it as an extra parameter that would be removed in compile.cpp but that messed up debug infos and I hit some asserts.

And why not "if (valid_length_test->find_int_con(1) == 0) {" in both places?

So`that the Value() call does the right thing during CCP where a node can be registered as a constant but not transformed yet to a ConNode.

Choose a reason for hiding this comment

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

That's make sense. Thanks for explaining!

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Dec 16, 2021

Have you run any micros to verify that nothing regresses?

Thanks for looking at this. This fixes a crash for a corner case (array size close to max_jint). I don't think it can affect performance so I haven't run any performance testing.

@neliasso
Copy link

@neliasso neliasso commented Dec 17, 2021

Passes testing tier1-3

Copy link

@neliasso neliasso left a comment

Looks good!

@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2021

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

8278413: C2 crash when allocating array of size too large

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

  • 8fbe172: 8278604: SwingSet2 table demo does not have accessible description set for images
  • b46f0b0: 8277447: Hotspot C1 compiler crashes on Kotlin suspend fun with loop
  • 9cd7090: 8278587: StringTokenizer(String, String, boolean) documentation bug
  • fffa73c: 8269425: 2 jdk/jfr/api/consumer/streaming tests failed to attach
  • b9a477b: 8275638: GraphKit::combine_exception_states fails with "matching stack sizes" assert
  • bb7efb3: 8278790: Inner loop of long loop nest runs for too few iterations
  • 8494fec: 8278796: Incorrect behavior of FloatVector.withLane on X86
  • f5d7c77: 8276826: Clarify the ModuleDescriptor.Version specification’s treatment of repeated punctuation characters
  • be6b90d: 8278574: update --help-extra message to include default value of --finalization option
  • aec1b03: 8278389: SuspendibleThreadSet::_suspend_all should be volatile/atomic
  • ... and 7 more: https://git.openjdk.java.net/jdk18/compare/475ec8e6c5abc3431344d69bd46395e8c4b46e4c...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.

@openjdk openjdk bot added the ready label Dec 17, 2021
Copy link

@vnkozlov vnkozlov left a comment

Looks good. Thank you for fixing it.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Dec 20, 2021

@neliasso @vnkozlov thanks for the reviews.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Dec 20, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Dec 20, 2021

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

  • 36676db: 8278970: [macos] SigningPackageTest is failed with runtime exception
  • a5f5d60: 8273107: RunThese24H times out with "java.lang.management.ThreadInfo.getLockName()" is null
  • 8fbe172: 8278604: SwingSet2 table demo does not have accessible description set for images
  • b46f0b0: 8277447: Hotspot C1 compiler crashes on Kotlin suspend fun with loop
  • 9cd7090: 8278587: StringTokenizer(String, String, boolean) documentation bug
  • fffa73c: 8269425: 2 jdk/jfr/api/consumer/streaming tests failed to attach
  • b9a477b: 8275638: GraphKit::combine_exception_states fails with "matching stack sizes" assert
  • bb7efb3: 8278790: Inner loop of long loop nest runs for too few iterations
  • 8494fec: 8278796: Incorrect behavior of FloatVector.withLane on X86
  • f5d7c77: 8276826: Clarify the ModuleDescriptor.Version specification’s treatment of repeated punctuation characters
  • ... and 9 more: https://git.openjdk.java.net/jdk18/compare/475ec8e6c5abc3431344d69bd46395e8c4b46e4c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Dec 20, 2021
@openjdk openjdk bot closed this Dec 20, 2021
@openjdk openjdk bot removed ready rfr labels Dec 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 20, 2021

@rwestrel Pushed as commit deaf75a.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
hotspot-compiler integrated
3 participants