8299570: [JVMCI] Insufficient error handling when CodeBuffer is exhausted #11945

dougxc · 2023-01-11T14:29:43Z

This PR fixes the handling of a full CodeBuffer when emitting stubs as part of JVMCI code installation.

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-8299570: [JVMCI] Insufficient error handling when CodeBuffer is exhausted

Reviewers

Tom Rodriguez (@tkrodriguez - Reviewer) ⚠️ Review applies to 7f1d45aa
Andrew Dinn (@adinn - Reviewer) ⚠️ Review applies to 9b1d1fbe
Andrew Haley (@theRealAph - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11945

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

Using diff file

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

bridgekeeper · 2023-01-11T14:42:09Z

👋 Welcome back dnsimon! 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 · 2023-01-11T14:46:07Z

@dougxc 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.

mlbridge · 2023-01-11T14:50:00Z

Webrevs

tkrodriguez · 2023-01-11T16:58:53Z

src/hotspot/share/jvmci/jvmciCodeInstaller.cpp

@@ -1178,7 +1178,9 @@ void CodeInstaller::site_Call(CodeBuffer& buffer, u1 tag, jint pc_offset, HotSpo
    CodeInstaller::pd_relocate_JavaMethod(buffer, method, pc_offset, JVMCI_CHECK);
    if (_next_call_type == INVOKESTATIC || _next_call_type == INVOKESPECIAL) {
      // Need a static call stub for transitions from compiled to interpreted.
-      CompiledStaticCall::emit_to_interp_stub(buffer, _instructions->start() + pc_offset);
+      if (CompiledStaticCall::emit_to_interp_stub(buffer, _instructions->start() + pc_offset) == nullptr) {


We use estimate_stubs_size to presize the buffer to include space for these so it should never fail I think, but we should be checking anyway. Maybe estimate_stubs_size should be computing space for the aarch64 trampolines?

it looks like estimate_stubs_size is computing space for the trampolines via

size += trampoline_stubs * CompiledStaticCall::to_trampoline_stub_size();?

It seems like the estimate agrees with the code emitted for the trampoline, so we either have a general problem in this computation or we have some other unexpected use of code cache space. It isn't critical that we get this estimate right but we do try to avoid resizing during code installation as it can be expensive.

Yes, this is a very defensive (and cheap) check that should never fail. Still, better to have it.

tkrodriguez · 2023-01-11T17:01:16Z

src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp

  MacroAssembler a(&cbuf);
  address stub = NULL;

  if (a.far_branches()
      && ! is_NativeCallTrampolineStub_at(instruction_address() + displacement())) {
    stub = a.emit_trampoline_stub(instruction_address() - cbuf.insts()->start(), dest);
+    if (stub == nullptr) {
+      JVMCI_ERROR_0("could not emit trampoline stub - code cache is full");
+    }
  }


The if (stub == null) test below should be the else branch of this if which I think makes it clearer. Why do we even bother returning the stub?

The if (stub == null) test below should be the else branch of this if which I think makes it clearer

I'm suspecting this is indeed a dormant bug. I will investigate further.

@theRealAph any chance you recall the intended logic here?

I've pushed 0e4fb65 which attempts to clear up the logic in this method. It would be great if some aarch64 experts could help review it (cc @adinn @theRealAph).

@dougxc I'm not sure why the above code is written the way it is rather than the way you rewrote it. I cannot see any reason why there should already be a trampoline stub in place when trampoline_jump is called given how it is being called at present. I thought perhaps it might be something to do with the (newly introduced) shared trampoline code but that is not relevant here and, besides, this routine has been thew way it is since it was first introduced.

I know the trampoline (and related far jump) code has been subject to change over the years so it may be something to do with how this routine was called in an earlier incarnation of the code. Andrew Haley will have a better idea than me as he was the original author.

Anyway, if we may need far branches and the call to is_NativeCallTrampolineStub_at fails then it does not seem tome to make any sense to call set_destination (at least null is returned which is correct). So, I think your rewrite looks like it is doing the right thing.

I think you probably need an ok from Andrew Haley here though.

Ok, thanks for your input. I'll wait for @theRealAph to review it as well.

@theRealAph @adinn can I now merge this PR?

@dougxc Still ok with me. I just pinged Andrew Haley to see if he is ok with it.

openjdk · 2023-01-11T17:01:58Z

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

8299570: [JVMCI] Insufficient error handling when CodeBuffer is exhausted

Reviewed-by: never, adinn, aph

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 1 new commit pushed to the master branch:

ddcb369: 8303605: Memory leaks in Metaspace gtests

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 · 2023-01-20T12:39:39Z

@dougxc this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8299570
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

adinn

I'm happy to approve this.

adinn · 2023-03-08T10:52:40Z

@dougxc I'm assuming the test failures are unrelated? If so then ok to push.

theRealAph · 2023-03-08T10:54:24Z

src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp

+      JVMCI_ERROR("single-use stub should not exist");
+    }
+  } else {
+    // If not using far branches, patch this call directly to dest.


This is all very complicated. Can't we just add JVMCI_ERRORs where we need them?

I'm not following - isn't this exactly what the code is doing? Maybe you could demonstrate how you think it should look.

Maybe it would be nicer to get the ! far_branches code path out of the way first, and return immediately.

Something like this?

diff --git a/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp b/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp index 88dc59f80d0..83ec182d2c7 100644 --- a/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp +++ b/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp @@ -532,21 +532,22 @@ void NativeCallTrampolineStub::set_destination(address new_destination) { void NativeCall::trampoline_jump(CodeBuffer &cbuf, address dest, JVMCI_TRAPS) { MacroAssembler a(&cbuf); - if (a.far_branches()) { - if (!is_NativeCallTrampolineStub_at(instruction_address() + displacement())) { - address stub = a.emit_trampoline_stub(instruction_address() - cbuf.insts()->start(), dest); - if (stub == nullptr) { - JVMCI_ERROR("could not emit trampoline stub - code cache is full"); - } - // The relocation is created while emitting the stub will ensure this - // call instruction is subsequently patched to call the stub. - } else { - // Not sure how this can be happen but be defensive - JVMCI_ERROR("single-use stub should not exist"); - } - } else { + if (!a.far_branches()) { // If not using far branches, patch this call directly to dest. set_destination(dest); + return; + } + + if (!is_NativeCallTrampolineStub_at(instruction_address() + displacement())) { + address stub = a.emit_trampoline_stub(instruction_address() - cbuf.insts()->start(), dest); + if (stub == nullptr) { + JVMCI_ERROR("could not emit trampoline stub - code cache is full"); + } + // The relocation is created while emitting the stub will ensure this + // call instruction is subsequently patched to call the stub. + } else { + // Not sure how this can be happen but be defensive + JVMCI_ERROR("single-use stub should not exist"); } } #endif

Yes. Or maybe

// Generate a trampoline for a branch to dest. If there's no need for a // trampoline, simply patch the call directly to dest. void NativeCall::trampoline_jump(CodeBuffer &cbuf, address dest, JVMCI_TRAPS) { MacroAssembler a(&cbuf); if (! a.far_branches()) { // If not using far branches, patch this call directly to dest. set_destination(dest); } else if (!is_NativeCallTrampolineStub_at(instruction_address() + displacement())) { // If we want far branches and there isn't a trampoline stub, emit one. address stub = a.emit_trampoline_stub(instruction_address() - cbuf.insts()->start(), dest); if (stub == nullptr) { JVMCI_ERROR("could not emit trampoline stub - code cache is full"); } // The relocation is created while emitting the stub will ensure this // call instruction is subsequently patched to call the stub. } else { // Not sure how this can be happen but be defensive JVMCI_ERROR("single-use stub should not exist"); } }

dougxc · 2023-03-08T22:32:43Z

Thanks for the reviews @adinn @theRealAph and @tkrodriguez .

/integrate

openjdk · 2023-03-08T22:33:54Z

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

05ceb37: 8303833: java.util.LocaleISOData has wrong comments for 'Norwegian Bokmål' and 'Volapük'
25de222: 8303839: [BACKOUT] JDK-8302799 and JDK-8302189
5b43804: 8282201: Consider removal of expiry check in VerifyCACerts.java test
f813dc7: 8302508: Add timestamp to the output TraceCompilerThreads
404d5bd: 8302161: Upgrade jQuery UI to version 1.13.2
d287a5e: 8303617: update for deprecated sprintf for jdk.jdwp.agent
ddcb369: 8303605: Memory leaks in Metaspace gtests

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-08T22:34:14Z

@dougxc Pushed as commit ad326fc.

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

properly handle CodeBuffer exhaustion in JVMCI backend

7f1d45a

openjdk bot added the rfr Pull request is ready for review label Jan 11, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 11, 2023

tkrodriguez approved these changes Jan 11, 2023

View reviewed changes

tkrodriguez reviewed Jan 11, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 11, 2023

dougxc added 2 commits January 18, 2023 11:22

try rationalize code in NativeCall::trampoline_jump

0e4fb65

changed return type of NativeCall::trampoline_jump to void

8e85e3d

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jan 20, 2023

Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8299570

9b1d1fb

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jan 20, 2023

adinn approved these changes Mar 8, 2023

View reviewed changes

theRealAph reviewed Mar 8, 2023

View reviewed changes

dougxc added 2 commits March 8, 2023 14:49

reorganized code in NativeCall::trampoline_jump

d723563

Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8299570

92bcac5

theRealAph approved these changes Mar 8, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 8, 2023

openjdk bot closed this Mar 8, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 8, 2023

dougxc deleted the JDK-8299570 branch August 20, 2024 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8299570: [JVMCI] Insufficient error handling when CodeBuffer is exhausted #11945

8299570: [JVMCI] Insufficient error handling when CodeBuffer is exhausted #11945

dougxc commented Jan 11, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

mlbridge bot commented Jan 11, 2023 •

edited

Loading

tkrodriguez Jan 11, 2023

teshull Jan 17, 2023

tkrodriguez Jan 17, 2023

dougxc Jan 18, 2023

tkrodriguez Jan 11, 2023

dougxc Jan 11, 2023

dougxc Jan 18, 2023 •

edited

Loading

adinn Jan 20, 2023 •

edited

Loading

dougxc Jan 20, 2023

dougxc Mar 7, 2023

adinn Mar 7, 2023

openjdk bot commented Jan 11, 2023 •

edited

Loading

openjdk bot commented Jan 20, 2023

adinn left a comment

adinn commented Mar 8, 2023

theRealAph Mar 8, 2023

dougxc Mar 8, 2023

theRealAph Mar 8, 2023

dougxc Mar 8, 2023

theRealAph Mar 8, 2023

dougxc Mar 8, 2023

dougxc commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

8299570: [JVMCI] Insufficient error handling when CodeBuffer is exhausted #11945

8299570: [JVMCI] Insufficient error handling when CodeBuffer is exhausted #11945

Conversation

dougxc commented Jan 11, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

mlbridge bot commented Jan 11, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougxc Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

adinn Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jan 11, 2023 • edited Loading

openjdk bot commented Jan 20, 2023

adinn left a comment

Choose a reason for hiding this comment

adinn commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougxc commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

openjdk bot commented Mar 8, 2023

dougxc commented Jan 11, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 11, 2023 •

edited

Loading

dougxc Jan 18, 2023 •

edited

Loading

adinn Jan 20, 2023 •

edited

Loading

openjdk bot commented Jan 11, 2023 •

edited

Loading