diff options
author | 2023-01-09 17:29:12 +0000 | |
---|---|---|
committer | 2023-01-11 15:34:31 +0000 | |
commit | b76f55152345ad1d2813d7fe3f3e03e988ca7d5c (patch) | |
tree | 6fab051dc6d37d511517f791a49b6238b9518049 | |
parent | 5f510ac3e95fd716f911bd884bc9f175297a383a (diff) |
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
-rw-r--r-- | runtime/arch/arm/quick_entrypoints_arm.S | 3 | ||||
-rw-r--r-- | runtime/arch/arm64/quick_entrypoints_arm64.S | 3 | ||||
-rw-r--r-- | runtime/arch/x86/quick_entrypoints_x86.S | 9 | ||||
-rw-r--r-- | runtime/arch/x86_64/quick_entrypoints_x86_64.S | 3 | ||||
-rw-r--r-- | runtime/entrypoints/quick/quick_deoptimization_entrypoints.cc | 16 | ||||
-rw-r--r-- | runtime/entrypoints/quick/quick_trampoline_entrypoints.cc | 31 | ||||
-rw-r--r-- | runtime/instrumentation.cc | 6 | ||||
-rw-r--r-- | runtime/quick_exception_handler.cc | 34 | ||||
-rw-r--r-- | runtime/quick_exception_handler.h | 5 | ||||
-rw-r--r-- | runtime/thread.cc | 10 |
10 files changed, 75 insertions, 45 deletions
diff --git a/runtime/arch/arm/quick_entrypoints_arm.S b/runtime/arch/arm/quick_entrypoints_arm.S index 83a60ecb94..26561461d7 100644 --- a/runtime/arch/arm/quick_entrypoints_arm.S +++ b/runtime/arch/arm/quick_entrypoints_arm.S @@ -2566,7 +2566,8 @@ ENTRY art_quick_method_entry_hook 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 354a3bd533..1df24c1223 100644 --- a/runtime/arch/arm64/quick_entrypoints_arm64.S +++ b/runtime/arch/arm64/quick_entrypoints_arm64.S @@ -2667,7 +2667,8 @@ ENTRY art_quick_method_entry_hook 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 eb8582d4bd..c768229b32 100644 --- a/runtime/arch/x86/quick_entrypoints_x86.S +++ b/runtime/arch/x86/quick_entrypoints_x86.S @@ -2346,15 +2346,18 @@ END_FUNCTION art_quick_compile_optimized 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 b612c353b6..babd1bc6b2 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 @@ DEFINE_FUNCTION art_quick_method_entry_hook 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 d06dbcb12a..89d5c18551 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 @@ NO_RETURN static void artDeoptimizeImpl(Thread* self, DeoptimizationKind kind, b 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 @@ NO_RETURN static void artDeoptimizeImpl(Thread* self, DeoptimizationKind kind, b } } -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 @@ extern "C" NO_RETURN void artDeoptimizeFromCompiledCode(DeoptimizationKind kind, 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 0178444bd4..4baf60c4ea 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 @@ extern "C" void artJniMethodEntryHook(Thread* self) 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 @@ extern "C" void artMethodExitHook(Thread* self, 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 @@ extern "C" void artMethodExitHook(Thread* self, // 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 104c46532b..0863ddf68a 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 @@ void Instrumentation::DeoptimizeIfNeeded(Thread* self, 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 1dc90097e0..8aac7020bd 100644 --- a/runtime/quick_exception_handler.cc +++ b/runtime/quick_exception_handler.cc @@ -384,8 +384,8 @@ class DeoptimizeStackVisitor final : public StackVisitor { 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 @@ class DeoptimizeStackVisitor final : public StackVisitor { 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 @@ class DeoptimizeStackVisitor final : public StackVisitor { 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 @@ class DeoptimizeStackVisitor final : public StackVisitor { 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::PrepareForLongJumpToInvokeStubOrInterpreterBridge() } } -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::DeoptimizeStack() { 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 559c3165ad..81c907ee0d 100644 --- a/runtime/quick_exception_handler.h +++ b/runtime/quick_exception_handler.h @@ -59,7 +59,10 @@ class QuickExceptionHandler { // 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 24608bc5f7..dc5cd3c513 100644 --- a/runtime/thread.cc +++ b/runtime/thread.cc @@ -134,7 +134,7 @@ namespace art { 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 @@ void Thread::DumpThreadOffset(std::ostream& os, uint32_t offset) { 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 @@ void Thread::QuickDeliverException(bool is_method_exit_exception) { // 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 @@ void Thread::QuickDeliverException(bool is_method_exit_exception) { 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 @@ void Thread::QuickDeliverException(bool is_method_exit_exception) { // 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()); |