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());