Skip method unwind callbacks if exception was thrown by method exit
When a method exit callback is called the method is still on the stack.
This is required because we should allow the method being exited to
be force popped and re-execute the caller frame. The way it is
implemented currently requires the method that is being exited on the
stack. However, method exit callbacks could also throw which triggers
a stack unwind. We should not call method unwind events for methods
that already called the method exit event. This CL makes sure
method unwind events are not called when an exception is thrown from
method exit callbacks.
Currently, method entry / exit callbacks force interpreter entry
points so this isn't a problem. This is in preperation for future CLs
that change this.
Change-Id: I6124898acf0c20b8c9944c0e1e5b9f23c5633b14
diff --git a/runtime/arch/arm/quick_entrypoints_arm.S b/runtime/arch/arm/quick_entrypoints_arm.S
index 4189e4e..3d39eb5 100644
--- a/runtime/arch/arm/quick_entrypoints_arm.S
+++ b/runtime/arch/arm/quick_entrypoints_arm.S
@@ -2570,14 +2570,8 @@
mov r0, rSELF @ pass Thread::Current
blx artMethodExitHook @ (Thread*, ArtMethod*, gpr_res*, fpr_res*)
- .cfi_remember_state
- cbnz r0, .Ldo_deliver_instrumentation_exception_exit @ Deliver exception
-
// Normal return.
RESTORE_SAVE_EVERYTHING_FRAME
REFRESH_MARKING_REGISTER
blx lr
-.Ldo_deliver_instrumentation_exception_exit:
- CFI_RESTORE_STATE_AND_DEF_CFA sp, FRAME_SIZE_SAVE_EVERYTHING
- DELIVER_PENDING_EXCEPTION_FRAME_READY
END art_quick_method_exit_hook
diff --git a/runtime/arch/arm64/quick_entrypoints_arm64.S b/runtime/arch/arm64/quick_entrypoints_arm64.S
index da7eff5..b12945b 100644
--- a/runtime/arch/arm64/quick_entrypoints_arm64.S
+++ b/runtime/arch/arm64/quick_entrypoints_arm64.S
@@ -2673,15 +2673,9 @@
mov x0, xSELF // Thread::Current
bl artMethodExitHook // (Thread*, ArtMethod*, gpr_res*, fpr_res*)
- .cfi_remember_state
- cbnz x0, .Ldo_deliver_instrumentation_exception_exit // Handle exception
-
// Normal return.
RESTORE_SAVE_EVERYTHING_FRAME
REFRESH_MARKING_REGISTER
ret
-.Ldo_deliver_instrumentation_exception_exit:
- CFI_RESTORE_STATE_AND_DEF_CFA sp, FRAME_SIZE_SAVE_EVERYTHING
- DELIVER_PENDING_EXCEPTION_FRAME_READY
END art_quick_method_exit_hook
diff --git a/runtime/arch/x86/quick_entrypoints_x86.S b/runtime/arch/x86/quick_entrypoints_x86.S
index 1a49fff..82aa6b2 100644
--- a/runtime/arch/x86/quick_entrypoints_x86.S
+++ b/runtime/arch/x86/quick_entrypoints_x86.S
@@ -2376,16 +2376,9 @@
addl LITERAL(32), %esp // Pop arguments and grp_result.
CFI_ADJUST_CFA_OFFSET(-32)
- cmpl LITERAL(1), %eax // Check if we returned error.
- CFI_REMEMBER_STATE
- je .Ldo_deliver_instrumentation_exception_exit
-
// Normal return.
RESTORE_SAVE_EVERYTHING_FRAME
ret
-.Ldo_deliver_instrumentation_exception_exit:
- CFI_RESTORE_STATE_AND_DEF_CFA esp, FRAME_SIZE_SAVE_EVERYTHING
- DELIVER_PENDING_EXCEPTION_FRAME_READY
END_FUNCTION art_quick_method_exit_hook
diff --git a/runtime/arch/x86_64/quick_entrypoints_x86_64.S b/runtime/arch/x86_64/quick_entrypoints_x86_64.S
index 3c3501f..fa1cd95 100644
--- a/runtime/arch/x86_64/quick_entrypoints_x86_64.S
+++ b/runtime/arch/x86_64/quick_entrypoints_x86_64.S
@@ -2164,7 +2164,6 @@
END_FUNCTION art_quick_method_entry_hook
// On entry, method is at the bottom of the stack.
-// and r8 has should_deopt_frame value.
DEFINE_FUNCTION art_quick_method_exit_hook
SETUP_SAVE_EVERYTHING_FRAME
@@ -2175,14 +2174,7 @@
movq %gs:THREAD_SELF_OFFSET, %rdi // Thread::Current
call SYMBOL(artMethodExitHook) // (Thread*, SP, gpr_res*, fpr_res*)
- cmpq LITERAL(1), %rax
- CFI_REMEMBER_STATE
- je .Ldo_deliver_instrumentation_exception_exit
-
// Normal return.
RESTORE_SAVE_EVERYTHING_FRAME
ret
-.Ldo_deliver_instrumentation_exception_exit:
- CFI_RESTORE_STATE_AND_DEF_CFA rsp, FRAME_SIZE_SAVE_EVERYTHING
- DELIVER_PENDING_EXCEPTION_FRAME_READY
END_FUNCTION art_quick_method_entry_hook
diff --git a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
index ea6501c..e8687ea 100644
--- a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
+++ b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
@@ -2662,7 +2662,7 @@
}
}
-extern "C" int artMethodExitHook(Thread* self,
+extern "C" void artMethodExitHook(Thread* self,
ArtMethod* method,
uint64_t* gpr_result,
uint64_t* fpr_result)
@@ -2711,7 +2711,10 @@
}
if (self->IsExceptionPending() || self->ObserveAsyncException()) {
- return 1;
+ // 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();
}
if (deoptimize) {
@@ -2720,8 +2723,6 @@
artDeoptimize(self);
UNREACHABLE();
}
-
- return 0;
}
} // namespace art
diff --git a/runtime/quick_exception_handler.cc b/runtime/quick_exception_handler.cc
index f8bfb5a..8adc3b3 100644
--- a/runtime/quick_exception_handler.cc
+++ b/runtime/quick_exception_handler.cc
@@ -68,12 +68,15 @@
Context* context,
Handle<mirror::Throwable>* exception,
QuickExceptionHandler* exception_handler,
- uint32_t skip_frames)
+ uint32_t skip_frames,
+ bool skip_top_unwind_callback)
REQUIRES_SHARED(Locks::mutator_lock_)
: StackVisitor(self, context, StackVisitor::StackWalkKind::kIncludeInlinedFrames),
exception_(exception),
exception_handler_(exception_handler),
- skip_frames_(skip_frames) {
+ skip_frames_(skip_frames),
+ skip_unwind_callback_(skip_top_unwind_callback) {
+ DCHECK_IMPLIES(skip_unwind_callback_, skip_frames_ == 0);
}
bool VisitFrame() override REQUIRES_SHARED(Locks::mutator_lock_) {
@@ -101,9 +104,14 @@
// can potentially throw, so we want to call these after we find the catch block.
// We stop the stack walk when we find the catch block. If we are ending the stack walk we don't
// have to unwind this method so don't record it.
- if (continue_stack_walk) {
+ if (continue_stack_walk && !skip_unwind_callback_) {
+ // Skip unwind callback is only used when method exit callback has thrown an exception. In
+ // that case, we should have runtime method (artMethodExitHook) on top of stack and the
+ // second should be the method for which method exit was called.
+ DCHECK_IMPLIES(skip_unwind_callback_, GetFrameDepth() == 2);
unwound_methods_.push(method);
}
+ skip_unwind_callback_ = false;
return continue_stack_walk;
}
@@ -155,13 +163,18 @@
// The list of methods we would skip to reach the catch block. We record these to call
// MethodUnwind callbacks.
std::queue<ArtMethod*> unwound_methods_;
+ // Specifies if the unwind callback should be ignored for method at the top of the stack.
+ bool skip_unwind_callback_;
DISALLOW_COPY_AND_ASSIGN(CatchBlockStackVisitor);
};
// Finds the appropriate exception catch after calling all method exit instrumentation functions.
-// Note that this might change the exception being thrown.
-void QuickExceptionHandler::FindCatch(ObjPtr<mirror::Throwable> exception) {
+// Note that this might change the exception being thrown. If is_method_exit_exception is true
+// skip the method unwind call for the method on top of the stack as the exception was thrown by
+// method exit callback.
+void QuickExceptionHandler::FindCatch(ObjPtr<mirror::Throwable> exception,
+ bool is_method_exit_exception) {
DCHECK(!is_deoptimization_);
instrumentation::Instrumentation* instr = Runtime::Current()->GetInstrumentation();
// The number of total frames we have so far popped.
@@ -169,6 +182,7 @@
bool popped_to_top = true;
StackHandleScope<1> hs(self_);
MutableHandle<mirror::Throwable> exception_ref(hs.NewHandle(exception));
+ bool skip_top_unwind_callback = is_method_exit_exception;
// Sending the instrumentation events (done by the InstrumentationStackPopper) can cause new
// exceptions to be thrown which will override the current exception. Therefore we need to perform
// the search for a catch in a loop until we have successfully popped all the way to a catch or
@@ -182,11 +196,15 @@
}
// Walk the stack to find catch handler.
- CatchBlockStackVisitor visitor(self_, context_,
+ CatchBlockStackVisitor visitor(self_,
+ context_,
&exception_ref,
this,
- /*skip_frames=*/already_popped);
+ /*skip_frames=*/already_popped,
+ skip_top_unwind_callback);
visitor.WalkStack(true);
+ skip_top_unwind_callback = false;
+
uint32_t new_pop_count = handler_frame_depth_;
DCHECK_GE(new_pop_count, already_popped);
already_popped = new_pop_count;
diff --git a/runtime/quick_exception_handler.h b/runtime/quick_exception_handler.h
index 4ff981d..9554f1d 100644
--- a/runtime/quick_exception_handler.h
+++ b/runtime/quick_exception_handler.h
@@ -49,7 +49,8 @@
// Find the catch handler for the given exception and call all required Instrumentation methods.
// Note this might result in the exception being caught being different from 'exception'.
- void FindCatch(ObjPtr<mirror::Throwable> exception) REQUIRES_SHARED(Locks::mutator_lock_);
+ void FindCatch(ObjPtr<mirror::Throwable> exception, bool is_method_exit_exception)
+ REQUIRES_SHARED(Locks::mutator_lock_);
// 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
diff --git a/runtime/thread.cc b/runtime/thread.cc
index bc39d7f..0c53609 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -3709,7 +3709,7 @@
os << offset;
}
-void Thread::QuickDeliverException() {
+void Thread::QuickDeliverException(bool is_method_exit_exception) {
// Get exception from thread.
ObjPtr<mirror::Throwable> exception = GetException();
CHECK(exception != nullptr);
@@ -3802,7 +3802,7 @@
// resolution.
ClearException();
QuickExceptionHandler exception_handler(this, false);
- exception_handler.FindCatch(exception);
+ exception_handler.FindCatch(exception, is_method_exit_exception);
if (exception_handler.GetClearException()) {
// Exception was cleared as part of delivery.
DCHECK(!IsExceptionPending());
diff --git a/runtime/thread.h b/runtime/thread.h
index 51d1fdc..c1c7036 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -552,8 +552,12 @@
// that needs to be dealt with, false otherwise.
bool ObserveAsyncException() REQUIRES_SHARED(Locks::mutator_lock_);
- // Find catch block and perform long jump to appropriate exception handle
- NO_RETURN void QuickDeliverException() REQUIRES_SHARED(Locks::mutator_lock_);
+ // Find catch block and perform long jump to appropriate exception handle. When
+ // is_method_exit_exception is true, the exception was thrown by the method exit callback and we
+ // should not send method unwind for the method on top of the stack since method exit callback was
+ // already called.
+ NO_RETURN void QuickDeliverException(bool is_method_exit_exception = false)
+ REQUIRES_SHARED(Locks::mutator_lock_);
Context* GetLongJumpContext();
void ReleaseLongJumpContext(Context* context) {