8390650: Unsafe access with constant zero offset triggers assert in EA

[TobiHartmann]

The original failure happened with CTW of some internal code. The issue happens when C2 compiles an (unreachable) unsafe access with a constant zero offset and IGVN folds the AddP address to the base oop. We then hit an assert in escape analysis because it expects memory addresses to always be represented by an AddP.

By coincidence, we hit a similar issue in Valhalla when adding support for Unsafe.makePrivateBuffer/finishPrivateBuffer (see JDK-8206144), which had to read/write a mark word bit and therefore emitted a load/store on the mark word address in the header without an AddP. We fixed it like this:

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

Lines 172 to 181 in c92288e

	if ((n->Opcode() == Op_LoadX || n->Opcode() == Op_StoreX) &&
	!n->in(MemNode::Address)->is_AddP() &&
	_igvn->type(n->in(MemNode::Address))->isa_oopptr()) {
	// Load/Store at mark work address is at offset 0 so has no AddP which confuses EA
	Node* addp = AddPNode::make_with_base(n->in(MemNode::Address), n->in(MemNode::Address), _igvn->MakeConX(0));
	_igvn->register_new_node_with_optimizer(addp);
	_igvn->replace_input_of(n, MemNode::Address, addp);
	ideal_nodes.push(addp);
	_nodes.at_put_grow(addp->_idx, nullptr, nullptr);
	}

Now this code is dead since Unsafe.makePrivateBuffer/finishPrivateBuffer and their C2 mark-word loads/stores were removed by JDK-8373375 and JDK-8380802.

I think we should still revive the code to handle this case which is a much older issue and independent of Valhalla.

The alternative would be to emit a runtime check and guard it with an OpaqueConstantBoolNode such that it's folded later. I quickly prototyped this but it feels way too complicated to handle this edge case. Also, it would not help if we add mark word loads/stores back in the future.

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-8390650: Unsafe access with constant zero offset triggers assert in EA (Bug - P4)

Reviewers

• Quan Anh Mai (@merykitty - Reviewer)
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 32466

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/32466.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:

8390650: Unsafe access with constant zero offset triggers assert in EA

Reviewed-by: qamai, kvn

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

• eef3c8a: 8390257: New test runtime/valhalla/inlinetypes/NPEInPreviewTest.java fails with -Xcomp
• 46f0e2b: 8388476: [Valhalla] C2 incorrectly eliminates store to flat array
• 8545f91: 8324189: Test javax/sound/sampled/Clip/SetPositionHang.java timed out
• ... and 44 more: https://git.openjdk.org/jdk/compare/90f142d65372d2d0f0460b790404dda297336972...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 (cfaab134)

[merykitty]

Looks reasonable to me.

[TobiHartmann]

Thanks Quan-Anh!

[vnkozlov]

May be use oopDesc::mark_offset_in_bytes() instead of 0.

[TobiHartmann]

But that would be incorrect if oopDesc::mark_offset_in_bytes() ever became non-zero, right? So we would at least need to add an assert here which feels misplaced.

[vnkozlov]

Yes, you are right. I forgot it just unsafe access to some offset. :(

[vnkozlov]

Good.