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.

Sign up for GitHub

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

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

Closed
wants to merge 6 commits into from
+35 −18
Closed

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7f1d45a
properly handle CodeBuffer exhaustion in JVMCI backend
dougxc Jan 11, 2023
0e4fb65
try rationalize code in NativeCall::trampoline_jump
dougxc Jan 18, 2023
8e85e3d
changed return type of NativeCall::trampoline_jump to void
dougxc Jan 20, 2023
9b1d1fb
Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8299570
dougxc Jan 20, 2023
d723563
reorganized code in NativeCall::trampoline_jump
dougxc Mar 8, 2023
92bcac5
Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8299570
dougxc Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp
View file
Original file line number Diff line number Diff line change
@@ -532,21 +532,20 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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 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");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@dougxc dougxc Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@adinn adinn Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theRealAph @adinn can I now merge this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
#endif