8358889: C2 hits assert in backend due to malformed uncommon trap

[TobiHartmann]

On AArch64, when Compile::current()->max_vector_size() > 0, C2's backend calls uncommon_trap_request() to distinguish an uncommon trap from an ordinary call:

jdk/src/hotspot/cpu/aarch64/aarch64.ad

Lines 3662 to 3664 in f587450

	// Only non uncommon_trap calls need to reinitialize ptrue.
	if (Compile::current()->max_vector_size() > 0 && uncommon_trap_request() == 0) {
	__ reinitialize_ptrue();

->

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

Lines 837 to 839 in f587450

	int MachCallStaticJavaNode::uncommon_trap_request() const {
	if (_name != nullptr && !strcmp(_name, "uncommon_trap")) {
	return CallStaticJavaNode::extract_uncommon_trap_request(this);

This calls CallStaticJavaNode::extract_uncommon_trap_request which asserts that the trap_request input is always a ConI:

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

Lines 1264 to 1271 in f587450

	int CallStaticJavaNode::extract_uncommon_trap_request(const Node* call) {
	#ifndef PRODUCT
	if (!(call->req() > TypeFunc::Parms &&
	call->in(TypeFunc::Parms) != nullptr &&
	call->in(TypeFunc::Parms)->is_Con() &&
	call->in(TypeFunc::Parms)->bottom_type()->isa_int())) {
	assert(in_dump() != 0, "OK if dumping");
	tty->print("[bad uncommon trap]");

This is not guaranteed because the register allocator might spill the value on the stack and thus replace the input with a MachSpillCopyNode. Since in_dump() is not set, we assert. The fix is to check the type of the node instead which is in line with the code just below:

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

Line 1275 in f587450

return call->in(TypeFunc::Parms)->bottom_type()->is_int()->get_con();

Thanks,
Tobias

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8358889: C2 hits assert in backend due to malformed uncommon trap (Bug - P3)

Reviewers

• Quan Anh Mai (@merykitty - Reviewer)
• Roberto Castañeda Lozano (@robcasloz - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 32507

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back thartmann! 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]

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

8358889: C2 hits assert in backend due to malformed uncommon trap

Reviewed-by: qamai, rcastanedalo

You can use pull request commands such as /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 19 new commits pushed to the master branch:

• 51bb52c: 8391051: UnsafeAccessErrorHandshakeClosure should not be passed nullptr
• f1e6f6d: 8390469: LogTest_large_message_vm_Test::TestBody crashes in case of low disc space
• 05074de: 8389666: C2: assert(false) failed: unexpected scalar opcode for integer DivV*
• ... and 16 more: https://git.openjdk.org/jdk/compare/62a166a0647bdbb7ef41bcc4c042caed3bc7c638...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]

@TobiHartmann 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]

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot-compiler. This can be overridden with the /reviewers command.

[mlbridge]

Webrevs

• 00: Full (5b7b1471)

[robcasloz]

Thanks for working on this, Tobias. Why does C2 prefer to spill the trap request input instead of rematerializing it?

[TobiHartmann]

Thanks for looking at this, Roberto!

Why does C2 prefer to spill the trap request input instead of rematerializing it?

Short answer: It is rematerialized initially, but post_allocate_copy_removal eliminates that rematerialization and reuses an existing reload.

Gory details:

There are three -10 int the test:

• A: argument in max(dontInline(), -10)
• B: null-check trap request
• C: int product = -10

  mov  w16, #-10  // Java constant A and C
  ...
  str  w16, [sp]  // spilling across dontInline()
  bl   dontInline
  ldr  w1, [sp]  // reload A and C into w1
  ...
  cmp  w0, w1
  csel w10, w0, w1, gt  // max(dontInline(), -10) re-uses w1
  ...
  ldr  w12, [x29, #8]  // implicit null check of inner array doubles[0] -> trap_handler
  ...
  mov  w16, #79
  mul  w1, w1, w16  // loop computing product *= 79
  ...

  trap_handler:
    // no rematerialization of -10 here because w1 still contains it after loading from [sp] above
    bl UncommonTrapBlob

So the RA spills the value shared by A and C. The trap request B is then rematerialized initially, but post-allocation copy removal eliminates that rematerialization and reuses the existing reload.

[merykitty]

Thanks for the explanation. That makes sense.

[TobiHartmann]

Thanks for the review Quan-Anh!

[robcasloz]

Thanks for the details, Tobias. Spilling -10 across dontInline() seems suboptimal, no? A better option would have been to not spill in the first place and instead rematerialize it after the dontInline() call (and then reuse it among the different users). Would it make sense to file a RFE to investigate why this is not done?

In any case, I agree with the fix, it is good to handle the general case for robustness.

[TobiHartmann]

Thanks for the review, Roberto!

Spilling -10 across dontInline() seems suboptimal, no? A better option would have been to not spill in the first place and instead rematerialize it after the dontInline() call (and then reuse it among the different users). Would it make sense to file a RFE to investigate why this is not done?

Yes, agreed. I'm not familiar enough with the internals of the register allocator to tell if there's any particular reason for this though or if it's just a missing optimization. Given that, I'd lean towards not filing it for now.

[dean-long]

I don't think we ever return from UncommonTrapBlob. Instead, the next instruction is an abort/stop/ShouldNotReachHere. Instead of materializing constant arguments for UncommonTrapBlob, we could instead store the values as data, and have the blob or C++ code retrieve it:

  trap_handler:
    // no args passed
    bl UncommonTrapBlob
    # ShouldNotReachHere
    <uncommon trap info>

[TobiHartmann]

Thanks Dean, that's an interesting idea but I'm not sure there's enough benefit to justify the additional complexity (it would require quite some significant changes to the trap blobs, call-site layout and post-call metadata). Currently, rematerialization for the uncommon trap should only occur on the trap slow path and in this case it is eliminated altogether by post-allocation copy removal.

Also, it would not fix the issue that Roberto pointed out about spilling -10 across dontInline() because that spill still serves users A and C, even if trap argument B is removed.

Definitely out of scope of this PR, so I'm integrating the fix for now 🙂

[TobiHartmann]

/integrate

[openjdk]

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

• 58c3b26: 8390337: [Valhalla] Missing stopped() check in Parse::acmp_type_check
• 4690ea7: 8379453: Fix problems with ErrorLogTimeout
• 393c3a0: 8390826: RISC-V: Use dedicated vector mask logical instructions
• ... and 28 more: https://git.openjdk.org/jdk/compare/62a166a0647bdbb7ef41bcc4c042caed3bc7c638...master

Your commit was automatically rebased without conflicts.

[openjdk]

@TobiHartmann Pushed as commit 22933b7.

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