Navigation Menu

Skip to content
This repository has been archived by the owner on Sep 2, 2022. It is now read-only.

8269771: assert(tmp == _callprojs.fallthrough_catchproj) failed: allocation control projection #193

Closed
wants to merge 3 commits into from

Conversation

neliasso
Copy link

@neliasso neliasso commented Jul 1, 2021

When I investigated this bug I found out it was already fixed by JDK-8268405, or at least - it didn't happen any more. Since I had a good reproducer I took a look at why it happened and found two issues.

The noteable thing about the test case is that there are still allocations that isn't used. The initalizeNode lead to HaltNodes. This looks like a temporary state while the graph is simplified.

  1. In macro.cpp - process_users_of_allocation - there was an assert that was wrong. We must assert before the replace_node - otherwise tmp will always be NULL and fail. Why this hasn't happened before is a mystery.

  2. memnode.cpp - MemBarNode::remove - returns without doing anything if there are not exactly 2 outs. In this case there is a InitalizeNode that leads to a HaltNode. The memory uses has been removed - so the memory projection from the InitalizeNode is already removed.

If you want to check if this patch fixes the issue - you need to sync to jdk-17+26 - or before the fix for JDK-8268405.

Please review,
Nils Eliasson


Progress

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

Issue

  • JDK-8269771: assert(tmp == _callprojs.fallthrough_catchproj) failed: allocation control projection

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 193

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

Using diff file

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

@neliasso neliasso marked this pull request as ready for review July 1, 2021 11:05
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 1, 2021

👋 Welcome back neliasso! 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 Jul 1, 2021

@neliasso 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.java.net label Jul 1, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 1, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 1, 2021

Webrevs

@openjdk
Copy link

openjdk bot commented Jul 1, 2021

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

8269771: assert(tmp == _callprojs.fallthrough_catchproj) failed: allocation control projection

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

  • a4d2a9a: 8269745: [JVMCI] restore original qualified exports to Graal
  • e377397: 8268566: java/foreign/TestResourceScope.java timed out
  • 6c76e77: 8260684: vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java timed out
  • 4bbf11d: 8269580: assert(is_valid()) failed: invalid register (-1)
  • 54dd510: 8269704: Typo in j.t.Normalizer.normalize()
  • a8385fe: 8269354: javac crashes when processing parenthesized pattern in instanceof
  • c16d1fc: 8269285: Crash/miscompile in CallGenerator::for_method_handle_inline after JDK-8191998
  • ad27d9b: 8269088: C2 fails with assert(!n->is_Store() && !n->is_LoadStore()) failed: no node with a side effect
  • c67a7b0: 8269230: C2: main loop in micro benchmark never executed
  • 962f1c1: 8262886: javadoc generates broken links with {@inheritdoc}
  • ... and 6 more: https://git.openjdk.java.net/jdk17/compare/38260122815aed32627472e5d58b516e89ef7bd7...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 Pull request is ready to be integrated label Jul 1, 2021
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.

I suggest to create new open bug and close current as dup.

I also don't get why your change in macro.cpp fixed the issue. fallthrough_catchproj is local cached value - it should not be updated with call to replace_node().

Can you tell more what happened here?

@neliasso
Copy link
Author

neliasso commented Jul 1, 2021

I suggest to create new open bug and close current as dup.

This fixes the original reported bug - so that we run fine even without JDK-8268405 (which probably doesn't fix it - just hides it.)

I also don't get why your change in macro.cpp fixed the issue. fallthrough_catchproj is local cached value - it should not be updated with call to replace_node().

Can you tell more what happened here?

fallthrough_catchproj is chached. It's the tmp that will be NULL if the assert is after the replace. The Init-node is being removed. Maybe I should just remove the entire assert - it's a bit unnecessary.

@vnkozlov
Copy link
Contributor

vnkozlov commented Jul 1, 2021

I suggest to create new open bug and close current as dup.

This fixes the original reported bug - so that we run fine even without JDK-8268405 (which probably doesn't fix it - just hides it.)

I mean this bug 8269641 is marked as Confidential. Nobody can see it, even GitHub - see Issue. You need to create new bug which does not have our confidential information for review in open GitHub.

I also don't get why your change in macro.cpp fixed the issue. fallthrough_catchproj is local cached value - it should not be updated with call to replace_node().
Can you tell more what happened here?

fallthrough_catchproj is chached. It's the tmp that will be NULL if the assert is after the replace. The Init-node is being removed. Maybe I should just remove the entire assert - it's a bit unnecessary.

It looks like InitalizeNode has only control projection output and when it is replaced it also removes InitalizeNode as dead node too. In such case you just need to relax assert by checking tmp == NULL || and adding comment that InitalizeNode can become dead when it has only one projection.

@neliasso neliasso changed the title 8269641: assert(tmp == _callprojs.fallthrough_catchproj) failed: allocation control projection 8269771: assert(tmp == _callprojs.fallthrough_catchproj) failed: allocation control projection Jul 1, 2021
@neliasso
Copy link
Author

neliasso commented Jul 1, 2021

I suggest to create new open bug and close current as dup.

This fixes the original reported bug - so that we run fine even without JDK-8268405 (which probably doesn't fix it - just hides it.)

I mean this bug 8269641 is marked as Confidential. Nobody can see it, even GitHub - see Issue. You need to create new bug which does not have our confidential information for review in open GitHub.

I also don't get why your change in macro.cpp fixed the issue. fallthrough_catchproj is local cached value - it should not be updated with call to replace_node().
Can you tell more what happened here?

fallthrough_catchproj is chached. It's the tmp that will be NULL if the assert is after the replace. The Init-node is being removed. Maybe I should just remove the entire assert - it's a bit unnecessary.

It looks like InitalizeNode has only control projection output and when it is replaced it also removes InitalizeNode as dead node too. In such case you just need to relax assert by checking tmp == NULL || and adding comment that InitalizeNode can become dead when it has only one projection.

Changed bug, and added check with comment.

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.

Good.

@navyxliu
Copy link
Member

navyxliu commented Jul 2, 2021

hi, @vnkozlov ,

Are we handling code like this?

 \
-o----------
| Initializer |
-----------
  \
   o------
   | Halt   |  
   ---------

It looks like InitalizeNode has only control projection output and when it is replaced it also removes InitalizeNode as dead node too.

Yes, replace_node(ctrl_proj, init->in(TypeFunc::Control)); calls subsume_node(halt, init->in(TypeFunc::Control)), and it will empty halt's outputs, but can we say that init->in(TypeFunc::Control) is NULL after replace_node()?
Initializer is a dead node after then, but I don't see its in(0) become NULL.

@vnkozlov
Copy link
Contributor

vnkozlov commented Jul 2, 2021

hi @navyxliu
ctrl_proj and init->in(TypeFunc::Control) (which is Allocation's control out Proj node) are separate Proj nodes. After calling replace_node() Halt node will be attached to Allocation's control out Proj node.
No users of ctrl_proj left after control edge of Halt node is replaced. remove_dead_node(old) is called for it: phaseX.cpp#L1488
It calls remove_globally_dead_node(dead) which replaces input edges of ctrl_proj with NULL:
phaseX.cpp#L1379
After that it checks if its input (which is Initalize node) has any other outputs (users). If it does not have any - it is also dead and placed on stack for removal. As result Initalize node have its all inputs edges replaced with NULL too.

@navyxliu
Copy link
Member

navyxliu commented Jul 2, 2021

Thank you for the explanation! I missed remove_dead_node( old ) in subsume_node. now I see why initilizer->in(0) will become NULL after ctrl_proj is replaced.

This PR looks good to me.

@neliasso
Copy link
Author

neliasso commented Jul 2, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Jul 2, 2021

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

  • 5644c4f: 8265132: C2 compilation fails with assert "missing precedence edge"
  • a4d2a9a: 8269745: [JVMCI] restore original qualified exports to Graal
  • e377397: 8268566: java/foreign/TestResourceScope.java timed out
  • 6c76e77: 8260684: vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java timed out
  • 4bbf11d: 8269580: assert(is_valid()) failed: invalid register (-1)
  • 54dd510: 8269704: Typo in j.t.Normalizer.normalize()
  • a8385fe: 8269354: javac crashes when processing parenthesized pattern in instanceof
  • c16d1fc: 8269285: Crash/miscompile in CallGenerator::for_method_handle_inline after JDK-8191998
  • ad27d9b: 8269088: C2 fails with assert(!n->is_Store() && !n->is_LoadStore()) failed: no node with a side effect
  • c67a7b0: 8269230: C2: main loop in micro benchmark never executed
  • ... and 7 more: https://git.openjdk.java.net/jdk17/compare/38260122815aed32627472e5d58b516e89ef7bd7...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 2, 2021
@openjdk 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 Jul 2, 2021
@openjdk
Copy link

openjdk bot commented Jul 2, 2021

@neliasso Pushed as commit 7bc96db.

💡 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 hotspot-compiler-dev@openjdk.java.net integrated Pull request has been integrated
4 participants