8380434: [lworld] C2: Wrong execution caused by treating a non-buffered InlineType as an oop in PredictedCallGenerator

[benoitmaillard]

This PR prevents a wrong execution / VM segfault resulting from a wrong use
of InlineTypeNode::make_from_oop when generating a slow and fast path with
PredictedCallGenerator.

This was initially spotted as a failure with compiler/valhalla/inlinetypes/TestUnloadedReturnTypes.java.

Bug Summary

We have an OSR compilation, which matters because a is initialized outside of the loop and thus its type won't be exact.

A a = new A();

// We want an OSR compilation here
for (int i = 0; i < 100_000; i++) {
    Asserts.assertEquals(a.virtual(true), new MyValueRetType());
}

We then try to predict the virtual call and generate both fast (inlined) and slow (dynamic call)
paths.

Another important detail here is that the result from the dynamic call is not scalarized, as it seems there are not enough registers available in this case (cf TypeFunc::returns_inline_type_as_fields).

This means that we end up with a Phi that has on one side the result of the inlined path,
which is a non-buffered InlineTypeNode, and on the other a Proj for the oop resulting from the call.

(the method and type names match the initial reproducer here)

This is wrong already, because the assumption here is that the both branches contain an oop.
In Parse::do_call, we end up looking at the return node produced by the call generator
and determine that we should build an InlineTypeNode from the oop:

valhalla/src/hotspot/share/opto/doCall.cpp

Lines 812 to 820 in 1257fbd

	if (!rtype->is_void()) {
	Node* retnode = peek();
	const Type* rettype = gvn().type(retnode);
	if (!cg->method()->return_value_is_larval() && !retnode->is_InlineType() && rettype->is_inlinetypeptr()) {
	retnode = InlineTypeNode::make_from_oop(this, retnode, rettype->inline_klass());
	dec_sp(1);
	push(retnode);
	}
	}

The graph is very clearly broken at this stage, and this leads to wrong executions or even a segfault
as soon as we take the fast path (which always happens here) because the oop input for the InlineTypeNode is effectively null as we are trying to get rid of the allocation.

Fix

In Parse::do_call, the assumption when dealing with an inline type as return value is that we either:

• get an InlineTypeNode directly (inlining case) and we don't have anything to do
• get an oop, and then we can safely call InlineTypeNode::make_from_oop

Here we have something in the middle, and clearly we have to be more careful. I think there are
several options:

The first option would be to slightly modify the existing behavior in Parse::do_call and
detect the broken phi and call InlineTypeNode::make_from_oop on the phi inputs that are oops.
This seems a bit hacky, as we allow the broken phi to exist for a while, and it's hard to know
if we could be missing some cases.

The second option would be to fill in the blanks and add an InlineTypeNode::make_from_oop call
in the call generator, either in VirtualCallGenerator or PredictedCallGenerator. I think this
is also hacky, we would need to call InlineTypeNode::make_from_oop at several places.

The third option is to have the make_from_oop call closer to the creation of the return node. This ensures that we do not miss any case
and that we only have to do it in one place. I think GraphKit::set_results_for_java_call fits
these requirements, and there is already some inline type related stuff going on there. Unfortunately, I noticed after some preliminary testing that we still have to keep the the make_from_oop call in do_call, as there are cases where we rely on it to create an InlineTypeNode for the inlined path.

I have also added a check to ensure we never merge a non-buffered InlineType with an oop in PredictedCallGenerator.

Testing

• GitHub Actions
• tier1-4, plus some internal testing

Thank you for reviewing!

---

Progress

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

Issue

• JDK-8380434: [lworld] C2: Wrong execution caused by treating a non-buffered InlineType as an oop in PredictedCallGenerator (Bug - P3)

Reviewers

• Quan Anh Mai (@merykitty - Committer) ⚠️ Review applies to 9f9fec44

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2256

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2256.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back bmaillard! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

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

8380434: [lworld] C2: Wrong execution caused by treating a non-buffered InlineType as an oop in PredictedCallGenerator

Reviewed-by: qamai

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 224 new commits pushed to the lworld branch:

• 7ed0bd9: 8379542: [lworld] compiler/valhalla/inlinetypes/TestArrayCopyWithOops.java times out
• 9681478: 8380870: [lworld] More unification of 32-bit code in markWord.hpp
• d3b6dca: 8380282: [lworld] Problem-list several SA/jhsdb tests when running with --enable-preview
• ... and 221 more: https://git.openjdk.org/valhalla/compare/a6a6ed0ae7b222ecd8996b34144734931d0935fa...lworld

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@merykitty, @TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[mlbridge]

Webrevs

• 01: Full - Incremental (d13f9ef6)
• 00: Full (9f9fec44)

[marc-chevalier]

Does that work? I think C preprocessor is case sensitive. I know about ASSERT but not assert. Little mistake or did I learn something? (But I'm not too hopeful, I can't find any lowercase in the codebase).

[merykitty]

It would be better to add a comment explaining why do we need to have InlineTypeNode::make_from_oop there while we have already had it at Parse::do_call. It is a little funny because I have just removed the exact thing recently only to realize it is necessary now :)

[marc-chevalier]

Is that to force loading B, so that A.virtual doesn't look like it has a unique implementer?

[TobiHartmann]

Nice analysis! It's scary that we didn't hit an assert. I would have expected that the "Should have been buffered" assert in Compile::process_inline_types would have caught this when replacing the non-buffered InlineTypeNode. But looking at the code, I see that there still is a TODO in the Phi case because we had false positives ...

I have a closer look at this tomorrow.