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

8328702: C2: Crash during parsing because sub type check is not folded #18512

Closed
wants to merge 1 commit into from

Conversation

chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Mar 27, 2024

The test case shows a problem where data is folded during parsing while control is not. This leaves the graph in a broken state and we fail with an assertion.

We have the following (pseudo) code for some class X:

o = flag ? new Object[] : new byte[]; 
if (o instanceof X) {
   X x = (X)o; // checkcast
}

For the checkcast, C2 knows that the type of o is some kind of array, i.e. type [bottom. But this cannot be a sub type of X. Therefore, the CheckCastPP node created for the checkcast result is replaced by top by the type system. However, the SubTypeCheckNode for the checkcast is not folded and the graph is broken.

The problem of not folding the SubTypeCheckNode can be traced back to SubTypeCheckNode::sub calling static_subtype_check() when transforming the node after it's creation. static_subtype_check() should detect that the sub type check is always wrong here:

if (subk->is_java_subtype_of(superk)) {
return SSC_always_true; // (0) and (1) this test cannot fail
}
if (!subk->maybe_java_subtype_of(superk)) {
return SSC_always_false; // (2) true path dead; no dynamic test needed
}

But it does not because these two checks return the following:

  1. Check: is o a sub type of X? -> returns no, so far so good.
  2. Check: could o be a sub type of X? -> returns no which is wrong! [bottom is only a sub type of Object and can never be a subtype of X

In maybe_java_subtype_of_helper_for_arr(), we wrongly conclude that any array with a base element type bottom could be a sub type of anything:

bool this_top_or_bottom = (this_one->base_element_type(dummy) == Type::TOP || this_one->base_element_type(dummy) == Type::BOTTOM);
if (!this_one->is_loaded() || !other->is_loaded() || this_top_or_bottom) {
return true;
}

But this is only true if the super class is also an array class - but not if other (super klass) is an instance klass as in this case.

The fix for this is to first check the immediately following check which handles the case of comparing an array klass to an instance klass: An array klass can only ever be a sub class of an instance klass if it's the Object class. But in our case, we have X and this would return false:

if (this_one->is_instance_type(other)) {
return other->klass()->equals(ciEnv::current()->Object_klass()) && other->_interfaces->intersection_with(this_one->_interfaces)->eq(other->_interfaces);
}

The very same problem can also be triggered with X being an interface instead. There are tests for both these cases.

Additionally Required Fix

When running with -XX:+ExpandSubTypeCheckAtParseTime, we eagerly expand the sub type check during parsing and therefore do not emit a SubTypeCheckNode. When additionally running with -XX:+StressReflectiveCode, the static sub type performed in static_subtype_check() is skipped when emitting the expanded sub type check (skip is default-initialized with the flag value of StressReflectiveCode):

if (skip) {
return SSC_full_test; // Let caller generate the general case.
}

And again, the graph is left in a broken state because the sub type check cannot be folded.

I therefore propose to always perform the static sub type check when a sub type check is expanded during parsing (i.e. if ExpandSubTypeCheckAtParseTime is set). I've added a run with these flags as well.

Thanks,
Christian


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-8328702: C2: Crash during parsing because sub type check is not folded (Bug - P2)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18512

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 27, 2024

👋 Welcome back chagedorn! 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
Copy link

openjdk bot commented Mar 27, 2024

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

8328702: C2: Crash during parsing because sub type check is not folded

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

  • db15914: 8328753: Open source few Undecorated Frame tests
  • 925d829: 8329013: StackOverflowError when starting Apache Tomcat with signed jar
  • dd5d7d0: 8327002: (fs) java/nio/file/Files/CopyMoveVariations.java should be able to test across file systems
  • 6ae1cf1: 8329368: Generational ZGC: Remove the unnecessary friend classes in ZAllocator
  • 7eb78e3: 8320676: Manual printer tests have no Pass/Fail buttons, instructions close set 1
  • 5ac067f: 8329289: Unify SetupJdkExecutable and SetupJdkLibrary
  • 5ae849d: 8329292: Fix missing cleanups in java.management and jdk.management
  • ed821cb: 8324807: Manual printer tests have no Pass/Fail buttons, instructions close set 2
  • 5cf457b: 8329320: Simplify awt/print/PageFormat/NullPaper.java test
  • 8b934aa: 8329358: Generational ZGC: Remove the unused method ZPointer::set_remset_bits
  • ... and 79 more: https://git.openjdk.org/jdk/compare/af15c68f3ccb72537b0a60d942f12d600f13ebb6...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 rfr Pull request is ready for review label Mar 27, 2024
@openjdk
Copy link

openjdk bot commented Mar 27, 2024

@chhagedorn 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 Mar 27, 2024
@mlbridge
Copy link

mlbridge bot commented Mar 27, 2024

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.

Looks good to me.

int dummy;
bool this_top_or_bottom = (this_one->base_element_type(dummy) == Type::TOP || this_one->base_element_type(dummy) == Type::BOTTOM);
if (!this_one->is_loaded() || !other->is_loaded() || this_top_or_bottom) {
if (!this_one->is_loaded() || !other->is_loaded()) {
return true;
}
if (this_one->is_instance_type(other)) {
return other->klass()->equals(ciEnv::current()->Object_klass()) && other->_interfaces->intersection_with(this_one->_interfaces)->eq(other->_interfaces);
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeInterfaces has a contains method that does intersection_with + eq. Could we use it here? i.e. this_one->_interfaces->contains(other->_interfaces)

Copy link
Member Author

Choose a reason for hiding this comment

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

That would definitely be better but I've seen that there are three other uses of intersection_with + eq. We should probably update them all together but not sure if I should squeeze this in here. Should I follow up with an RFE?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed JDK-8329201 - will do a follow up PR.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 27, 2024
@rwestrel
Copy link
Contributor

When running with -XX:+ExpandSubTypeCheckAtParseTime

Do we want to retire ExpandSubTypeCheckAtParseTime? Is there any reason to keep it?

Copy link
Member Author

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

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

Thanks Roland for your review!

When running with -XX:+ExpandSubTypeCheckAtParseTime

Do we want to retire ExpandSubTypeCheckAtParseTime? Is there any reason to keep it?

I'm not sure about how much benefit it gives us. A quick JBS search for "ExpandSubTypeCheckAtParseTime" revealed a few issues - but would need to double check how many of them really only triggered with that flag and were real bugs. So, apart from having it as a stress option, I don't see a real benefit for it - but that might be a good enough reason to keep it for now.

What do you think?

int dummy;
bool this_top_or_bottom = (this_one->base_element_type(dummy) == Type::TOP || this_one->base_element_type(dummy) == Type::BOTTOM);
if (!this_one->is_loaded() || !other->is_loaded() || this_top_or_bottom) {
if (!this_one->is_loaded() || !other->is_loaded()) {
return true;
}
if (this_one->is_instance_type(other)) {
return other->klass()->equals(ciEnv::current()->Object_klass()) && other->_interfaces->intersection_with(this_one->_interfaces)->eq(other->_interfaces);
Copy link
Member Author

Choose a reason for hiding this comment

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

That would definitely be better but I've seen that there are three other uses of intersection_with + eq. We should probably update them all together but not sure if I should squeeze this in here. Should I follow up with an RFE?

@rwestrel
Copy link
Contributor

I'm not sure about how much benefit it gives us. A quick JBS search for "ExpandSubTypeCheckAtParseTime" revealed a few issues - but would need to double check how many of them really only triggered with that flag and were real bugs. So, apart from having it as a stress option, I don't see a real benefit for it - but that might be a good enough reason to keep it for now.

What do you think?

It also has a maintenance cost (you had to make a code change for it in this PR and I also remember having to take ExpandSubTypeCheckAtParseTime into consideration at some point). I would vote for removing it unless it's known to have some value.

@chhagedorn
Copy link
Member Author

chhagedorn commented Mar 27, 2024

I'm not sure about how much benefit it gives us. A quick JBS search for "ExpandSubTypeCheckAtParseTime" revealed a few issues - but would need to double check how many of them really only triggered with that flag and were real bugs. So, apart from having it as a stress option, I don't see a real benefit for it - but that might be a good enough reason to keep it for now.
What do you think?

It also has a maintenance cost (you had to make a code change for it in this PR and I also remember having to take ExpandSubTypeCheckAtParseTime into consideration at some point). I would vote for removing it unless it's known to have some value.

You're right, it indeed has a maintenance cost (as proved again here) which I'm also not so sure of whether it's worth. Removing the flag sounds reasonable from that standpoint.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

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

Looks good.

@chhagedorn
Copy link
Member Author

Thanks @rwestrel and @vnkozlov for your reviews!

@chhagedorn
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Apr 4, 2024

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

  • f762637: 8326962: C2 SuperWord: cache VPointer
  • 2931458: 8328938: C2 SuperWord: disable vectorization for large stride and scale
  • 4196688: 8329494: Serial: Merge GenMarkSweep into MarkSweep
  • 8020183: 8329470: Remove obsolete CDS SharedStrings tests
  • 8267d65: 8329564: [JVMCI] TranslatedException::debugPrintStackTrace does not work in the libjvmci compiler.
  • 16576b8: 8328957: Update PKCS11Test.java to not use hardcoded path
  • 375bfac: 8327474: Review use of java.io.tmpdir in jdk tests
  • 233619b: 8329557: Fix statement around MathContext.DECIMAL128 rounding
  • 023f7f1: 8320799: Bump minimum boot jdk to JDK 22
  • 8dc43aa: 8325217: MethodSymbol.getModifiers() returns SEALED for restricted methods
  • ... and 102 more: https://git.openjdk.org/jdk/compare/af15c68f3ccb72537b0a60d942f12d600f13ebb6...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 4, 2024
@openjdk openjdk bot closed this Apr 4, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 4, 2024
@openjdk
Copy link

openjdk bot commented Apr 4, 2024

@chhagedorn Pushed as commit e5e21a8.

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

@chhagedorn
Copy link
Member Author

/backport jdk22u

@openjdk
Copy link

openjdk bot commented Apr 4, 2024

@chhagedorn the backport was successfully created on the branch backport-chhagedorn-e5e21a8a in my personal fork of openjdk/jdk22u. To create a pull request with this backport targeting openjdk/jdk22u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit e5e21a8a from the openjdk/jdk repository.

The commit being backported was authored by Christian Hagedorn on 4 Apr 2024 and was reviewed by Roland Westrelin and Vladimir Kozlov.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22u:

$ git fetch https://github.com/openjdk-bots/jdk22u.git backport-chhagedorn-e5e21a8a:backport-chhagedorn-e5e21a8a
$ git checkout backport-chhagedorn-e5e21a8a
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22u.git backport-chhagedorn-e5e21a8a

⚠️ @chhagedorn You are not yet a collaborator in my fork openjdk-bots/jdk22u. An invite will be sent out and you need to accept it before you can proceed.

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
3 participants