8358751: C2: Recursive inlining check for compiled lambda forms is broken

[iwanowww]

Recursive inlining checks are relaxed for compiled LambdaForms. Since LambdaForms are heavily reused, the check is performed on MethodHandle receivers instead.

Unfortunately, the current implementation is broken. JVMState doesn't guarantee presence of receivers for caller frames.
An attempt to fetch pruned receiver reports unrelated info, but, in the worst case, it ends up as an out-of-bounds access into node's input array and crashes the JVM.

Proposed fix captures receiver information as part of inlining and preserves it on JVMState for every compiled LambdaForm frame, so it can be reliably recovered during subsequent inlining attempts.

Testing: hs-tier1 - hs-tier8

(Special thanks to @mroth23 who prepared a reproducer of the bug.)

---

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-8358751: C2: Recursive inlining check for compiled lambda forms is broken (Bug - P2)

Reviewers

• Dean Long (@dean-long - Reviewer)
• Roland Westrelin (@rwestrel - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26891

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8358751: C2: Recursive inlining check for compiled lambda forms is broken

Reviewed-by: dlong, roland

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

• b0f5b23: 8366145: G1: Help diagnose ubsan division by zero in computing pause time ratios (g1Analytics.cpp)
• a5a2340: 8365053: Refresh hotspot precompiled.hpp with headers based on current frequency
• 7469a27: 8365939: [Redo] G1: Move collection set related full gc reset code into abandon_collection_set() method
• ... and 97 more: https://git.openjdk.org/jdk/compare/ecab52c09b078201ebeb8d45c0982b0481e15dc3...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]

@iwanowww
The hotspot-compiler label was successfully added.

[mlbridge]

Webrevs

• 00: Full (eb510bee)

[dean-long]

Can you explain why a MH invoker needs to be handled as a special case? Also, it seems like we should be saving the receiver info as a snapshot of arg0/local0 in the callee JVMState, rather than changing it in the caller JVMState for every call site. Don't we already save the receiver somewhere, so that late inlining works correctly?

[iwanowww]

Can you explain why a MH invoker needs to be handled as a special case?

MH invokers don't have a receiver. They are linked to indy/MH.invoke/MH.invokeExact call sites and there's no dispatching happening when they are invoked (compared to other cases when MH.invokeBasic is used).

... it seems like we should be saving the receiver info as a snapshot of arg0/local0 in the callee JVMState, ...

That's what the patch does. There's a number of places where receiver_info is simply copied when JVMState is cloned, but each compiled lambda form frame in JVMState should have its constant receiver (MethodHandle instance) recorded as receiver_info. (Constant receiver is a pre-requisite for inlining through MH.invokeBasic() to happen.)

Don't we already save the receiver somewhere, so that late inlining works correctly?

For late inlining the situation is different: corresponding call site keeps the receiver as an argument. It's not the case with ancestor frames in deep inlined call chains. There are no guarantees that receivers from ancestor frames are still alive when a call site is being considered for inlining during parsing. (Effectively dead locals are aggressively pruned from JVMState during parsing.)

[rwestrel]

Ignoring the bug you're fixing, is that logic expected to compute the same inline_level that the current logic computes? You changed it a bit (iterate from the current frame rather than the caller, the extra test for is_method_handle_invoker and the extra test for lform_caller_recv == nullptr in the loop that I'm not sure what the answer to that question is.

[iwanowww]

I don't see a compelling reason to treat immediate caller specially here. Since current logic is broken, I decided to make the unification along with the fix. I can separate it into an RFE, if you have any concerns.

[dean-long]

Would it be possible to add a test?

[rwestrel]

Looks good to me.

[TobiHartmann]

What about a regression test?