Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8277602: Deopt code does not extend the stack enough if the caller is an optimize entry blob #6522

Closed
wants to merge 4 commits into from

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Nov 23, 2021

Deoptimization code does not recreate c2i adapter 'frames'. For compiled callers this means that the stack needs to be adjusted manually to make room for the parameters when the callee is converted to an interpreter frame (essentially emulating what a c2i adapter would do).

To check if the caller does a compiled call, the current code uses frame::is_compiled_frame(), which is true if the codeblob of the caller frame is an instance of CompiledMethod.

However, optimized entry blobs also do compiled calls, are not detected by this test, and therefore don't get their stack adjusted correctly.

To address this, I've added a new frame::is_compiled_caller function to determine if the caller is doing a compiled call, and I use that in the deopt code instead of is_compiled_frame.

This patch also removes an old workaround that tried to fix the issue by allocating some spill space in the optimized entry blob frame, but this only accounts for the first argument. If there are more arguments we still have a problem. The suggested patch fixes this the right way I think.

Thanks,
Jorn

Testing: run-test-jdk_foreign on Windows x64 and Linux x64 (afaik these are the only tests that use optimized entry blobs)


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8277602: Deopt code does not extend the stack enough if the caller is an optimize entry blob

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6522/head:pull/6522
$ git checkout pull/6522

Update a local copy of the PR:
$ git checkout pull/6522
$ git pull https://git.openjdk.java.net/jdk pull/6522/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6522

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6522.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 23, 2021

👋 Welcome back jvernee! 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.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Nov 23, 2021

/label hotspot-compiler

@openjdk openjdk bot added the hotspot-compiler label Nov 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 2021

@JornVernee
The hotspot-compiler label was successfully added.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Nov 23, 2021

I've CC'd hotspot-compiler on this since compiler folks are probably the most familiar with deoptimization code.

@JornVernee JornVernee marked this pull request as ready for review Nov 23, 2021
@openjdk openjdk bot added the rfr label Nov 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 23, 2021

Webrevs

@dean-long
Copy link
Member

@dean-long dean-long commented Nov 23, 2021

This seems safe enough, since it should only affect optimized entry blob frames.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 2021

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

8277602: Deopt code does not extend the stack enough if the caller is an optimize entry blob

Reviewed-by: dlong, thartmann

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

  • c3a7f2f: 8277382: make c1 BlockMerger use IR::verify only when necessary
  • 8f9eb62: 8274784: jshell: Garbled character was displayed by System.out.println(...) on Japanese Windows
  • e9b36a8: 8276670: G1: Rename G1CardSetFreePool and related classes
  • b9eb532: 8276685: Malformed Javadoc inline tags in JDK source in /jdk/management/jfr/RecordingInfo.java
  • 40fef23: 8275908: Record null_check traps for calls and array_check traps in the interpreter
  • 3d810ad: 8277411: C2 fast_unlock intrinsic on AArch64 has unnecessary ownership check
  • ce0234b: 8277860: PPC: Remove duplicate info != NULL check
  • 040b2c5: 8277139: Improve code readability in PredecessorValidator (c1_IR.cpp)
  • 3e798dd: 8275330: C2: assert(n->is_Root() || n->is_Region() || n->is_Phi() || n->is_MachMerge() || def_block->dominates(block)) failed: uses must be dominated by definitions
  • 99e4bda: 8277417: C1 LIR instruction for load-klass
  • ... and 4 more: https://git.openjdk.java.net/jdk/compare/987992042454f92936d3efbd01e7beb921e3b70e...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 openjdk bot added the ready label Nov 23, 2021
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Nov 23, 2021

Thanks for the review.

FWIW, we have a test case which is a tomcat benchmark that uses a custom panama-foreign based SSL library. A memory corruption crash occurs in that benchmark, gets more frequent with -XX:+DeoptimizeALot, and goes away with this fix.

There's a test that used to also test this deopt code path in the jdk_foreign test suite as well, but the conditions for the failure are quite specific (optimized entry blob needs to be the deoptee's caller), and due to some code changing elsewhere the test seems to no longer hit the offending code path.

This doesn't need to be integrated right away (though I would like to get it into 18). In the mean time I'll also see if I can improve the existing test to catch this case again.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good but a targeted regression test (or a noreg-* label) is required.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Nov 25, 2021

I've added a test and a couple of asserts that catch the case I'm trying to fix (mostly so that the test fails in a more obvious way). If I revert the fix in deoptimization.cpp the latter assert fires (that's the case I found during debugging as well), and when I re-add the fix tests pass again.

I ran this through tier1-3 as well.

The asserts I've added only check for 'overflow' in the case of compiled callers. I played around with adding a similar check for interpreted callers as well, but I wasn't able to provoke an assertion failure with that, and I'm not 100% what the right check should be. I suspect interpreted callers are rare when we deopt due to an uncommon trap. For these reasons, I've left the asserts to check for overflow in the case of compiled callers only, for now.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me but please run this through tier4-5 as well.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Nov 30, 2021

Tier4-5 came back clean, except for some failures due to known issues.

Will go ahead an integrate this.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Nov 30, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Nov 30, 2021

Going to push as commit 98a9f03.
Since your change was applied there have been 41 commits pushed to the master branch:

  • 9150840: 8277899: Parallel: Simplify PSVirtualSpace::initialize logic
  • 01cefc9: 8277977: Incorrect references to --enable-reproducible-builds in docs
  • 69f56a0: 8264485: build.tools.depend.Depend.toString(byte[]) creates malformed hex strings
  • fecf906: 8267928: Loop predicate gets inexact loop limit before PhaseIdealLoop::rc_predicate
  • a5f2a58: 8277846: Implement fast-path for ASCII-compatible CharsetEncoders on ppc64
  • ceae380: 8277843: [Vector API] scalar2vector generates incorrect type info for mask operations if Op_MaskAll is unavailable
  • 3ee26c6: 8267767: Redundant condition check in SafepointSynchronize::thread_not_running
  • d230fee: 8277931: Parallel: Remove unused PSVirtualSpace::expand_into
  • fde6fe7: 8277824: Remove empty RefProcSubPhasesWorkerTimeTracker destructor
  • 27299ea: 8277803: vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001 fails with "Synthetic fields not found"
  • ... and 31 more: https://git.openjdk.java.net/jdk/compare/987992042454f92936d3efbd01e7beb921e3b70e...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 30, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 30, 2021

@JornVernee Pushed as commit 98a9f03.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler integrated
3 participants