8287432: C2: assert(tn->in(0) != __null) failed: must have live top node

[chhagedorn]

When intrisifying java.lang.Thread::currentThread(), we are creating an AddP node that has the top node as base to indicate that we do not have an oop (using NULL instead leads to crashes as it does not seem to be expected to have a NULL base):

jdk/src/hotspot/share/opto/library_call.cpp

Line 904 in 6ff2d89

Node* p = basic_plus_adr(top()/*!oop*/, thread, in_bytes(handle_offset));

This node is used on a chain of data nodes into two MemBarAcquire nodes as precedence edge in the test case:

Later, in final_graph_reshaping_impl(), we are removing the precedence edge of both MemBarAcquire nodes and clean up all now dead nodes as a result of the removal:

jdk/src/hotspot/share/opto/compile.cpp

Lines 3655 to 3679 in 6ff2d89

	if (n->as_MemBar()->trailing_load() && n->req() > MemBarNode::Precedent) {
	// At parse time, the trailing MemBarAcquire for a volatile load
	// is created with an edge to the load. After optimizations,
	// that input may be a chain of Phis. If those phis have no
	// other use, then the MemBarAcquire keeps them alive and
	// register allocation can be confused.
	ResourceMark rm;
	Unique_Node_List wq;
	wq.push(n->in(MemBarNode::Precedent));
	n->set_req(MemBarNode::Precedent, top());
	while (wq.size() > 0) {
	Node* m = wq.pop();
	if (m->outcnt() == 0) {
	for (uint j = 0; j < m->req(); j++) {
	Node* in = m->in(j);
	if (in != NULL) {
	wq.push(in);
	}
	}
	m->disconnect_inputs(this);
	}
	}
	}
	break;
	}

We iteratively call disconnect_inputs() for all nodes that have no output anymore (i.e. dead nodes). This code, however, also treats the top node as dead since outcnt() of top is always zero:

jdk/src/hotspot/share/opto/node.hpp

Lines 495 to 500 in 6ff2d89

	// Add an output edge to the end of the list
	void add_out( Node *n ) {
	if (is_top()) return;
	if( _outcnt == _outmax ) out_grow(_outcnt);
	_out[_outcnt++] = n;
	}

And we end up disconnecting top which results in the assertion failure.

The code misses a check for top(). I suggest to add this check before processing a node for which outcnt() is zero. This is a pattern which can also be found in other places in the code. I've checked all other usages of oucnt() == 0 and could not find a case where this additional top() check is missing. Maybe we should refactor these two checks into a single method at some point to not need to worry about top anymore in the future when checking if a node is dead based on the outputs.

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-8287432: C2: assert(tn->in(0) != __null) failed: must have live top node

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 3f351d40
• Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to 3f351d40

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9060

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

Using diff file

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

[bridgekeeper]

👋 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]

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

[mlbridge]

Webrevs

• 01: Full - Incremental (46a1ef11)
• 00: Full (3f351d40)

[vnkozlov]

Your analysis and fix is correct.

[openjdk]

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

8287432: C2: assert(tn->in(0) != __null) failed: must have live top node

Reviewed-by: kvn, thartmann

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

• 41d5809: 8287811: JFR: jfr configure error message should not print stack trace
• 5d4ea9b: 8273346: Expand library mappings to IEEE 754 operations
• 96641c0: 8287745: jni/nullCaller/NullCallerTest.java fails with "exitValue = 1"
• 67f1bd7: 8286620: Create regression test for verifying setMargin() of JRadioButton
• 062db59: 8286206: Missing cases for RECORD
• ee4a6c2: 8287799: JFR: Less noisy platform threads with jfr print
• 1499e5e: 8273573: [macos12] ActionListenerCalledTwiceTest.java fails on macOS 12
• 2f62f15: 8287808: javac generates illegal class file for pattern matching switch with records
• 905bcbe: 8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option
• 8d28734: 8287741: Fix of JDK-8287107 (unused cgv1 freezer controller) was incomplete
• ... and 113 more: https://git.openjdk.java.net/jdk/compare/6e55a72f25f7273e3a8a19e0b9a97669b84808e9...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.

[TobiHartmann]

Nice analysis and test. Looks good.

[chhagedorn]

Thanks Vladimir and Tobias for your reviews!

[devinrsmith]

I'm not qualified to review this, but I can confirm the test is triggering the PhaseAggressiveCoalesce::coalesce SIGSEGV against the 11/17 LTS versions I'm running. Thanks for your investigation and fix!

[chhagedorn]

Thanks @devinrsmith for confirming this!

/integrate

[openjdk]

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

• f7791ad: 8287895: Some langtools tests fail on msys2
• 5ad6286: 8287970: riscv: jdk/incubator/vector/*VectorTests failing
• a9d46f3: 8287894: Use fixed timestamp as an alternative of DATE macro in jdk.jdi to make Windows build reproducible
• 6e3e470: 8285965: TestScenarios.java does not check for "" correctly
• d959c22: 8288000: compiler/loopopts/TestOverUnrolling2.java fails with release VMs
• 230726e: 8287735: Provide separate event category for dll operations
• ecf0078: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles
• 5c39a36: 8287522: StringConcatFactory: Add in prependers and mixers in batches
• 47d3c2a: 8287980: Build is broken due to SuperWordMaxVectorSize when C2 is disabled after JDK-8287697
• bf0e625: 8286451: C2: assert(nb == 1) failed: only when the head is not shared
• ... and 142 more: https://git.openjdk.java.net/jdk/compare/6e55a72f25f7273e3a8a19e0b9a97669b84808e9...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn Pushed as commit 78d3712.

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

[devinrsmith]

@chhagedorn, thanks again! I'm hoping this can be backported to 11/17 - let me know if there is anything I can do to make that happen.