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)

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

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

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.