Check if we need a deopt after method entry / exit callbacks
We need to check if we need a deopt after method entry / exit callbacks.
We were using the proxy of IsDeoptimized(method) to check if we need a
deopt but that isn't sufficient always. For example, when there is a
frame pop request or other requests that don't explicitly deopt the
method.
Checking for a deopt after method exit callback requires special care
to avoid calling method exit callbacks again from the interpreter after
a deopt. We already have a mechanism to skip method exit listeners so it
just involves updating the shadow frame to skip method exit listeners.
Bug: 206029744
Test: art/test.py
Change-Id: Ib95ab4348e40de345583e0718617879300828cb7
diff --git a/runtime/arch/arm/quick_entrypoints_arm.S b/runtime/arch/arm/quick_entrypoints_arm.S
index 83a60ec..2656146 100644
--- a/runtime/arch/arm/quick_entrypoints_arm.S
+++ b/runtime/arch/arm/quick_entrypoints_arm.S
@@ -2566,7 +2566,8 @@
SETUP_SAVE_EVERYTHING_FRAME r0
ldr r0, [sp, FRAME_SIZE_SAVE_EVERYTHING] @ pass ArtMethod
mov r1, rSELF @ pass Thread::Current
- bl artMethodEntryHook @ (ArtMethod*, Thread*)
+ mov r2, sp @ pass SP
+ bl artMethodEntryHook @ (ArtMethod*, Thread*, SP)
RESTORE_SAVE_EVERYTHING_FRAME
REFRESH_MARKING_REGISTER
blx lr
diff --git a/runtime/arch/arm64/quick_entrypoints_arm64.S b/runtime/arch/arm64/quick_entrypoints_arm64.S
index 354a3bd..1df24c1 100644
--- a/runtime/arch/arm64/quick_entrypoints_arm64.S
+++ b/runtime/arch/arm64/quick_entrypoints_arm64.S
@@ -2667,7 +2667,8 @@
ldr x0, [sp, #FRAME_SIZE_SAVE_EVERYTHING] // pass ArtMethod*
mov x1, xSELF // pass Thread::Current
- bl artMethodEntryHook // (ArtMethod*, Thread*)
+ mov x2, sp // pass SP
+ bl artMethodEntryHook // (ArtMethod*, Thread*, SP)
RESTORE_SAVE_EVERYTHING_FRAME // Note: will restore xSELF
REFRESH_MARKING_REGISTER
diff --git a/runtime/arch/x86/quick_entrypoints_x86.S b/runtime/arch/x86/quick_entrypoints_x86.S
index eb8582d..c768229 100644
--- a/runtime/arch/x86/quick_entrypoints_x86.S
+++ b/runtime/arch/x86/quick_entrypoints_x86.S
@@ -2346,15 +2346,18 @@
DEFINE_FUNCTION art_quick_method_entry_hook
SETUP_SAVE_EVERYTHING_FRAME edx
mov FRAME_SIZE_SAVE_EVERYTHING(%esp), %eax // Fetch ArtMethod
- subl LITERAL(8), %esp
- CFI_ADJUST_CFA_OFFSET(8)
+ mov %esp, %edx // Store esp before pushing anything on stack.
+ subl LITERAL(4), %esp
+ CFI_ADJUST_CFA_OFFSET(4)
+ push %edx // Pass SP
+ CFI_ADJUST_CFA_OFFSET(4)
pushl %fs:THREAD_SELF_OFFSET // Pass Thread::Current().
CFI_ADJUST_CFA_OFFSET(4)
pushl %eax // Pass Method*.
CFI_ADJUST_CFA_OFFSET(4)
- call SYMBOL(artMethodEntryHook) // (Method*, Thread*)
+ call SYMBOL(artMethodEntryHook) // (Method*, Thread*, SP)
addl LITERAL(16), %esp // Pop arguments.
CFI_ADJUST_CFA_OFFSET(-16)
diff --git a/runtime/arch/x86_64/quick_entrypoints_x86_64.S b/runtime/arch/x86_64/quick_entrypoints_x86_64.S
index b612c35..babd1bc 100644
--- a/runtime/arch/x86_64/quick_entrypoints_x86_64.S
+++ b/runtime/arch/x86_64/quick_entrypoints_x86_64.S
@@ -2167,8 +2167,9 @@
movq FRAME_SIZE_SAVE_EVERYTHING(%rsp), %rdi // pass ArtMethod
movq %gs:THREAD_SELF_OFFSET, %rsi // pass Thread::Current()
+ movq %rsp, %rdx // SP
- call SYMBOL(artMethodEntryHook) // (ArtMethod*, Thread*)
+ call SYMBOL(artMethodEntryHook) // (ArtMethod*, Thread*, sp)
RESTORE_SAVE_EVERYTHING_FRAME
ret
diff --git a/runtime/entrypoints/quick/quick_deoptimization_entrypoints.cc b/runtime/entrypoints/quick/quick_deoptimization_entrypoints.cc
index d06dbcb..89d5c18 100644
--- a/runtime/entrypoints/quick/quick_deoptimization_entrypoints.cc
+++ b/runtime/entrypoints/quick/quick_deoptimization_entrypoints.cc
@@ -25,7 +25,10 @@
namespace art {
-NO_RETURN static void artDeoptimizeImpl(Thread* self, DeoptimizationKind kind, bool single_frame)
+NO_RETURN static void artDeoptimizeImpl(Thread* self,
+ DeoptimizationKind kind,
+ bool single_frame,
+ bool skip_method_exit_callbacks)
REQUIRES_SHARED(Locks::mutator_lock_) {
Runtime::Current()->IncrementDeoptimizationCount(kind);
if (VLOG_IS_ON(deopt)) {
@@ -43,7 +46,7 @@
if (single_frame) {
exception_handler.DeoptimizeSingleFrame(kind);
} else {
- exception_handler.DeoptimizeStack();
+ exception_handler.DeoptimizeStack(skip_method_exit_callbacks);
}
uintptr_t return_pc = exception_handler.UpdateInstrumentationStack();
if (exception_handler.IsFullFragmentDone()) {
@@ -57,9 +60,10 @@
}
}
-extern "C" NO_RETURN void artDeoptimize(Thread* self) REQUIRES_SHARED(Locks::mutator_lock_) {
+extern "C" NO_RETURN void artDeoptimize(Thread* self, bool skip_method_exit_callbacks)
+ REQUIRES_SHARED(Locks::mutator_lock_) {
ScopedQuickEntrypointChecks sqec(self);
- artDeoptimizeImpl(self, DeoptimizationKind::kFullFrame, false);
+ artDeoptimizeImpl(self, DeoptimizationKind::kFullFrame, false, skip_method_exit_callbacks);
}
// This is called directly from compiled code by an HDeoptimize.
@@ -74,7 +78,9 @@
self->GetException(),
/* from_code= */ true,
DeoptimizationMethodType::kDefault);
- artDeoptimizeImpl(self, kind, true);
+ // Deopting from compiled code, so method exit haven't run yet. Don't skip method exit callbacks
+ // if required.
+ artDeoptimizeImpl(self, kind, true, /* skip_method_exit_callbacks= */ false);
}
} // namespace art
diff --git a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
index 0178444..4baf60c 100644
--- a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
+++ b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
@@ -61,7 +61,7 @@
namespace art {
extern "C" NO_RETURN void artDeoptimizeFromCompiledCode(DeoptimizationKind kind, Thread* self);
-extern "C" NO_RETURN void artDeoptimize(Thread* self);
+extern "C" NO_RETURN void artDeoptimize(Thread* self, bool skip_method_exit_callbacks);
// Visits the arguments as saved to the stack by a CalleeSaveType::kRefAndArgs callee save frame.
class QuickArgumentVisitor {
@@ -2652,14 +2652,14 @@
instr->MethodEnterEvent(self, method);
}
-extern "C" void artMethodEntryHook(ArtMethod* method, Thread* self, ArtMethod** sp ATTRIBUTE_UNUSED)
+extern "C" void artMethodEntryHook(ArtMethod* method, Thread* self, ArtMethod** sp)
REQUIRES_SHARED(Locks::mutator_lock_) {
instrumentation::Instrumentation* instr = Runtime::Current()->GetInstrumentation();
if (instr->HasMethodEntryListeners()) {
instr->MethodEnterEvent(self, method);
// MethodEnter callback could have requested a deopt for ex: by setting a breakpoint, so
// check if we need a deopt here.
- if (instr->IsDeoptimized(method)) {
+ if (instr->ShouldDeoptimizeCaller(self, sp) || instr->IsDeoptimized(method)) {
// Instrumentation can request deoptimizing only a particular method (for ex: when
// there are break points on the method). In such cases deoptimize only this method.
// FullFrame deoptimizations are handled on method exits.
@@ -2693,7 +2693,6 @@
JValue return_value;
bool is_ref = false;
ArtMethod* method = *sp;
- bool deoptimize = instr->ShouldDeoptimizeCaller(self, sp, frame_size);
if (instr->HasMethodExitListeners()) {
StackHandleScope<1> hs(self);
@@ -2712,37 +2711,31 @@
// If we need a deoptimization MethodExitEvent will be called by the interpreter when it
// re-executes the return instruction. For native methods we have to process method exit
// events here since deoptimization just removes the native frame.
- if (!deoptimize || method->IsNative()) {
- instr->MethodExitEvent(self,
- method,
- /* frame= */ {},
- return_value);
- }
+ instr->MethodExitEvent(self, method, /* frame= */ {}, return_value);
if (is_ref) {
// Restore the return value if it's a reference since it might have moved.
*reinterpret_cast<mirror::Object**>(gpr_result) = res.Get();
return_value.SetL(res.Get());
}
- } else if (deoptimize) {
- return_value = instr->GetReturnValue(method, &is_ref, gpr_result, fpr_result);
}
if (self->IsExceptionPending() || self->ObserveAsyncException()) {
- // The exception was thrown from the method exit callback. We should not call method unwind
+ // The exception was thrown from the method exit callback. We should not call method unwind
// callbacks for this case.
self->QuickDeliverException(/* is_method_exit_exception= */ true);
UNREACHABLE();
}
+ bool deoptimize = instr->ShouldDeoptimizeCaller(self, sp, frame_size);
if (deoptimize) {
+ JValue ret_val = instr->GetReturnValue(method, &is_ref, gpr_result, fpr_result);
DeoptimizationMethodType deopt_method_type = instr->GetDeoptimizationMethodType(method);
- self->PushDeoptimizationContext(return_value,
- is_ref,
- self->GetException(),
- false,
- deopt_method_type);
- artDeoptimize(self);
+ self->PushDeoptimizationContext(
+ ret_val, is_ref, self->GetException(), false, deopt_method_type);
+ // Method exit callback has already been run for this method. So tell the deoptimizer to skip
+ // callbacks for this frame.
+ artDeoptimize(self, /*skip_method_exit_callbacks = */ true);
UNREACHABLE();
}
}
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 104c465..0863ddf 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -55,7 +55,7 @@
#include "thread_list.h"
namespace art {
-extern "C" NO_RETURN void artDeoptimize(Thread* self);
+extern "C" NO_RETURN void artDeoptimize(Thread* self, bool skip_method_exit_callbacks);
extern "C" NO_RETURN void artDeliverPendingExceptionFromCode(Thread* self);
namespace instrumentation {
@@ -1554,7 +1554,9 @@
nullptr,
/* from_code= */ false,
type);
- artDeoptimize(self);
+ // This is requested from suspend points or when returning from runtime methods so exit
+ // callbacks wouldn't be run yet. So don't skip method callbacks.
+ artDeoptimize(self, /* skip_method_exit_callbacks= */ false);
}
}
diff --git a/runtime/quick_exception_handler.cc b/runtime/quick_exception_handler.cc
index 1dc9009..8aac702 100644
--- a/runtime/quick_exception_handler.cc
+++ b/runtime/quick_exception_handler.cc
@@ -384,8 +384,8 @@
DeoptimizeStackVisitor(Thread* self,
Context* context,
QuickExceptionHandler* exception_handler,
- bool single_frame)
- REQUIRES_SHARED(Locks::mutator_lock_)
+ bool single_frame,
+ bool skip_method_exit_callbacks) REQUIRES_SHARED(Locks::mutator_lock_)
: StackVisitor(self, context, StackVisitor::StackWalkKind::kIncludeInlinedFrames),
exception_handler_(exception_handler),
prev_shadow_frame_(nullptr),
@@ -394,8 +394,8 @@
single_frame_done_(false),
single_frame_deopt_method_(nullptr),
single_frame_deopt_quick_method_header_(nullptr),
- callee_method_(nullptr) {
- }
+ callee_method_(nullptr),
+ skip_method_exit_callbacks_(skip_method_exit_callbacks) {}
ArtMethod* GetSingleFrameDeoptMethod() const {
return single_frame_deopt_method_;
@@ -482,6 +482,19 @@
Runtime::Current()->GetInstrumentation()->MethodSupportsExitEvents(
method, GetCurrentOatQuickMethodHeader());
new_frame->SetSkipMethodExitEvents(!supports_exit_events);
+ // If we are deoptimizing after method exit callback we shouldn't call the method exit
+ // callbacks again for the top frame. We may have to deopt after the callback if the callback
+ // either throws or performs other actions that require a deopt.
+ // We only need to skip for the top frame and the rest of the frames should still run the
+ // callbacks. So only do this check for the top frame.
+ if (GetFrameDepth() == 0U && skip_method_exit_callbacks_) {
+ new_frame->SetSkipMethodExitEvents(true);
+ // This exception was raised by method exit callbacks and we shouldn't report it to
+ // listeners for these exceptions.
+ if (GetThread()->IsExceptionPending()) {
+ new_frame->SetSkipNextExceptionEvent(true);
+ }
+ }
if (updated_vregs != nullptr) {
// Calling Thread::RemoveDebuggerShadowFrameMapping will also delete the updated_vregs
// array so this must come after we processed the frame.
@@ -638,6 +651,10 @@
ArtMethod* single_frame_deopt_method_;
const OatQuickMethodHeader* single_frame_deopt_quick_method_header_;
ArtMethod* callee_method_;
+ // This specifies if method exit callbacks should be skipped for the top frame. We may request
+ // a deopt after running method exit callbacks if the callback throws or requests events that
+ // need a deopt.
+ bool skip_method_exit_callbacks_;
DISALLOW_COPY_AND_ASSIGN(DeoptimizeStackVisitor);
};
@@ -657,13 +674,13 @@
}
}
-void QuickExceptionHandler::DeoptimizeStack() {
+void QuickExceptionHandler::DeoptimizeStack(bool skip_method_exit_callbacks) {
DCHECK(is_deoptimization_);
if (kDebugExceptionDelivery) {
self_->DumpStack(LOG_STREAM(INFO) << "Deoptimizing: ");
}
- DeoptimizeStackVisitor visitor(self_, context_, this, false);
+ DeoptimizeStackVisitor visitor(self_, context_, this, false, skip_method_exit_callbacks);
visitor.WalkStack(true);
PrepareForLongJumpToInvokeStubOrInterpreterBridge();
}
@@ -671,7 +688,10 @@
void QuickExceptionHandler::DeoptimizeSingleFrame(DeoptimizationKind kind) {
DCHECK(is_deoptimization_);
- DeoptimizeStackVisitor visitor(self_, context_, this, true);
+ // This deopt is requested while still executing the method. We haven't run method exit callbacks
+ // yet, so don't skip them.
+ DeoptimizeStackVisitor visitor(
+ self_, context_, this, true, /* skip_method_exit_callbacks= */ false);
visitor.WalkStack(true);
// Compiled code made an explicit deoptimization.
diff --git a/runtime/quick_exception_handler.h b/runtime/quick_exception_handler.h
index 559c316..81c907e 100644
--- a/runtime/quick_exception_handler.h
+++ b/runtime/quick_exception_handler.h
@@ -59,7 +59,10 @@
// Deoptimize the stack to the upcall/some code that's not deoptimizeable. For
// every compiled frame, we create a "copy" shadow frame that will be executed
// with the interpreter.
- void DeoptimizeStack() REQUIRES_SHARED(Locks::mutator_lock_);
+ // skip_method_exit_callbacks specifies if we should skip method exit callbacks for the top frame.
+ // It is set if a deopt is needed after calling method exit callback for ex: if the callback
+ // throws or performs other actions that require a deopt.
+ void DeoptimizeStack(bool skip_method_exit_callbacks) REQUIRES_SHARED(Locks::mutator_lock_);
// Deoptimize a single frame. It's directly triggered from compiled code. It
// has the following properties:
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 24608bc..dc5cd3c 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -134,7 +134,7 @@
using android::base::StringAppendV;
using android::base::StringPrintf;
-extern "C" NO_RETURN void artDeoptimize(Thread* self);
+extern "C" NO_RETURN void artDeoptimize(Thread* self, bool skip_method_exit_callbacks);
bool Thread::is_started_ = false;
pthread_key_t Thread::pthread_key_self_;
@@ -3872,7 +3872,7 @@
os << offset;
}
-void Thread::QuickDeliverException(bool is_method_exit_exception) {
+void Thread::QuickDeliverException(bool skip_method_exit_callbacks) {
// Get exception from thread.
ObjPtr<mirror::Throwable> exception = GetException();
CHECK(exception != nullptr);
@@ -3880,7 +3880,7 @@
// This wasn't a real exception, so just clear it here. If there was an actual exception it
// will be recorded in the DeoptimizationContext and it will be restored later.
ClearException();
- artDeoptimize(this);
+ artDeoptimize(this, skip_method_exit_callbacks);
UNREACHABLE();
}
@@ -3917,7 +3917,7 @@
exception,
/* from_code= */ false,
method_type);
- artDeoptimize(this);
+ artDeoptimize(this, skip_method_exit_callbacks);
UNREACHABLE();
} else {
LOG(WARNING) << "Got a deoptimization request on un-deoptimizable method "
@@ -3933,7 +3933,7 @@
// resolution.
ClearException();
QuickExceptionHandler exception_handler(this, false);
- exception_handler.FindCatch(exception, is_method_exit_exception);
+ exception_handler.FindCatch(exception, skip_method_exit_callbacks);
if (exception_handler.GetClearException()) {
// Exception was cleared as part of delivery.
DCHECK(!IsExceptionPending());