Update entrypoints after switching the runtime to non-debuggable

When detaching a debugger or stopping tracing in non-debuggable
runtimes, we switch back to non-debuggable mode. We used to switch to
non-debuggable mode after updating the entrypoints of methods. This
meant we don't use AOT code for these methods. For this to work
correctly we should
1. Update instrumentation level
2. Switch runtime if possible
3. Update entry points.

Since this order is important, we no longer expose MaybeSwitchRuntime as
a public member. Instead we pass a parameter that says if we should try
and switch the runtime to non-debuggable.

Bug: 303686344
Test: art/test.py
Change-Id: Id655ca50964ed10072ff5bfc3e565c08ecb9987b

5 files changed, 73 insertions, 39 deletions

diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index 025bed53b2..42ab5d30dc 100644
--- a/openjdkjvmti/deopt_manager.cc
+++ b/openjdkjvmti/deopt_manager.cc
@@ -388,9 +388,14 @@ void DeoptManager::Shutdown() {
     return;
   }
 
-  runtime->GetInstrumentation()->DisableDeoptimization(kInstrumentationKey);
-  runtime->GetInstrumentation()->DisableDeoptimization(kDeoptManagerInstrumentationKey);
-  runtime->GetInstrumentation()->MaybeSwitchRuntimeDebugState(self);
+  // If we attach a debugger to a non-debuggable runtime, we switch the runtime to debuggable to
+  // provide a consistent (though still best effort) support. Since we are detaching the debugger
+  // now, switch it back to non-debuggable if there are no other debugger / profiling tools are
+  // active.
+  runtime->GetInstrumentation()->DisableDeoptimization(kInstrumentationKey,
+                                                       /*try_switch_to_non_debuggable=*/true);
+  runtime->GetInstrumentation()->DisableDeoptimization(kDeoptManagerInstrumentationKey,
+                                                       /*try_switch_to_non_debuggable=*/true);
 }
 
 void DeoptManager::RemoveDeoptimizeAllMethodsLocked(art::Thread* self) {
@@ -475,7 +480,8 @@ void DeoptManager::RemoveDeoptimizationRequester() {
   deopter_count_--;
   if (deopter_count_ == 0) {
     ScopedDeoptimizationContext sdc(self, this);
-    art::Runtime::Current()->GetInstrumentation()->DisableDeoptimization(kInstrumentationKey);
+    art::Runtime::Current()->GetInstrumentation()->DisableDeoptimization(
+        kInstrumentationKey, /*try_switch_to_non_debuggable=*/false);
     return;
   } else {
     deoptimization_status_lock_.ExclusiveUnlock(self);
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index fa1634e78b..8bd83d9214 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -987,12 +987,9 @@ Instrumentation::InstrumentationLevel Instrumentation::GetCurrentInstrumentation
   return instrumentation_level_;
 }
 
-bool Instrumentation::RequiresInstrumentationInstallation(InstrumentationLevel new_level) const {
-  // We need to reinstall instrumentation if we go to a different level.
-  return GetCurrentInstrumentationLevel() != new_level;
-}
-
-void Instrumentation::ConfigureStubs(const char* key, InstrumentationLevel desired_level) {
+void Instrumentation::ConfigureStubs(const char* key,
+                                     InstrumentationLevel desired_level,
+                                     bool try_switch_to_non_debuggable) {
   // Store the instrumentation level for this key or remove it.
   if (desired_level == InstrumentationLevel::kInstrumentNothing) {
     // The client no longer needs instrumentation.
@@ -1002,7 +999,7 @@ void Instrumentation::ConfigureStubs(const char* key, InstrumentationLevel desir
     requested_instrumentation_levels_.Overwrite(key, desired_level);
   }
 
-  UpdateStubs();
+  UpdateStubs(try_switch_to_non_debuggable);
 }
 
 void Instrumentation::UpdateInstrumentationLevel(InstrumentationLevel requested_level) {
@@ -1011,7 +1008,9 @@ void Instrumentation::UpdateInstrumentationLevel(InstrumentationLevel requested_
 
 void Instrumentation::EnableEntryExitHooks(const char* key) {
   DCHECK(Runtime::Current()->IsJavaDebuggable());
-  ConfigureStubs(key, InstrumentationLevel::kInstrumentWithEntryExitHooks);
+  ConfigureStubs(key,
+                 InstrumentationLevel::kInstrumentWithEntryExitHooks,
+                 /*try_switch_to_non_debuggable=*/false);
 }
 
 void Instrumentation::MaybeRestoreInstrumentationStack() {
@@ -1048,22 +1047,33 @@ void Instrumentation::MaybeRestoreInstrumentationStack() {
   }
 }
 
-void Instrumentation::UpdateStubs() {
+void Instrumentation::UpdateStubs(bool try_switch_to_non_debuggable) {
   // Look for the highest required instrumentation level.
   InstrumentationLevel requested_level = InstrumentationLevel::kInstrumentNothing;
   for (const auto& v : requested_instrumentation_levels_) {
     requested_level = std::max(requested_level, v.second);
   }
 
-  if (!RequiresInstrumentationInstallation(requested_level)) {
+  if (GetCurrentInstrumentationLevel() == requested_level) {
     // We're already set.
     return;
   }
+
   Thread* const self = Thread::Current();
   Runtime* runtime = Runtime::Current();
   Locks::mutator_lock_->AssertExclusiveHeld(self);
   Locks::thread_list_lock_->AssertNotHeld(self);
+  // The following needs to happen in the same order.
+  // 1. Update the instrumentation level
+  // 2. Switch the runtime to non-debuggable if requested. We switch to non-debuggable only when
+  // the instrumentation level is set to kInstrumentNothing. So this needs to happen only after
+  // updating the instrumentation level.
+  // 3. Update the entry points. We use AOT code only if we aren't debuggable runtime. So update
+  // entrypoints after switching the instrumentation level.
   UpdateInstrumentationLevel(requested_level);
+  if (try_switch_to_non_debuggable) {
+    MaybeSwitchRuntimeDebugState(self);
+  }
   InstallStubsClassVisitor visitor(this);
   runtime->GetClassLinker()->VisitClasses(&visitor);
   if (requested_level > InstrumentationLevel::kInstrumentNothing) {
@@ -1309,9 +1319,9 @@ bool Instrumentation::IsDeoptimized(ArtMethod* method) {
   return IsDeoptimizedMethod(method);
 }
 
-void Instrumentation::DisableDeoptimization(const char* key) {
+void Instrumentation::DisableDeoptimization(const char* key, bool try_switch_to_non_debuggable) {
   // Remove any instrumentation support added for deoptimization.
-  ConfigureStubs(key, InstrumentationLevel::kInstrumentNothing);
+  ConfigureStubs(key, InstrumentationLevel::kInstrumentNothing, try_switch_to_non_debuggable);
   Locks::mutator_lock_->AssertExclusiveHeld(Thread::Current());
   // Undeoptimized selected methods.
   while (true) {
@@ -1349,12 +1359,22 @@ void Instrumentation::MaybeSwitchRuntimeDebugState(Thread* self) {
 }
 
 void Instrumentation::DeoptimizeEverything(const char* key) {
-  ConfigureStubs(key, InstrumentationLevel::kInstrumentWithInterpreter);
+  // We want to switch to non-debuggable only when the debugger / profile tools are detaching.
+  // This call is used for supporting debug related features (ex: single stepping across all
+  // threads) while the debugger is still connected.
+  ConfigureStubs(key,
+                 InstrumentationLevel::kInstrumentWithInterpreter,
+                 /*try_switch_to_non_debuggable=*/false);
 }
 
 void Instrumentation::UndeoptimizeEverything(const char* key) {
   CHECK(InterpreterStubsInstalled());
-  ConfigureStubs(key, InstrumentationLevel::kInstrumentNothing);
+  // We want to switch to non-debuggable only when the debugger / profile tools are detaching.
+  // This is used when we no longer need to run in interpreter. The debugger is still connected
+  // so don't switch the runtime. We use "DisableDeoptimization" when detaching the debugger.
+  ConfigureStubs(key,
+                 InstrumentationLevel::kInstrumentNothing,
+                 /*try_switch_to_non_debuggable=*/false);
 }
 
 void Instrumentation::EnableMethodTracing(const char* key,
@@ -1366,7 +1386,8 @@ void Instrumentation::EnableMethodTracing(const char* key,
   } else {
     level = InstrumentationLevel::kInstrumentWithEntryExitHooks;
   }
-  ConfigureStubs(key, level);
+  // We are enabling method tracing here and need to stay in debuggable.
+  ConfigureStubs(key, level, /*try_switch_to_non_debuggable=*/false);
 
   MutexLock mu(Thread::Current(), *Locks::thread_list_lock_);
   for (Thread* thread : Runtime::Current()->GetThreadList()->GetList()) {
@@ -1375,7 +1396,11 @@ void Instrumentation::EnableMethodTracing(const char* key,
 }
 
 void Instrumentation::DisableMethodTracing(const char* key) {
-  ConfigureStubs(key, InstrumentationLevel::kInstrumentNothing);
+  // We no longer need to be in debuggable runtime since we are stopping method tracing. If no
+  // other debugger / profiling tools are active switch back to non-debuggable.
+  ConfigureStubs(key,
+                 InstrumentationLevel::kInstrumentNothing,
+                 /*try_switch_to_non_debuggable=*/true);
 }
 
 const void* Instrumentation::GetCodeForInvoke(ArtMethod* method) {
diff --git a/runtime/instrumentation.h b/runtime/instrumentation.h
index f1692cfa68..29107b149c 100644
--- a/runtime/instrumentation.h
+++ b/runtime/instrumentation.h
@@ -246,7 +246,13 @@ class Instrumentation {
       REQUIRES(Locks::mutator_lock_, !Locks::thread_list_lock_, !Locks::classlinker_classes_lock_);
 
   // Calls UndeoptimizeEverything which may visit class linker classes through ConfigureStubs.
-  void DisableDeoptimization(const char* key)
+  // try_switch_to_non_debuggable specifies if we can switch the runtime back to non-debuggable.
+  // When a debugger is attached to a non-debuggable app, we switch the runtime to debuggable and
+  // when we are detaching the debugger we move back to non-debuggable. If we are disabling
+  // deoptimization for other reasons (ex: removing the last breakpoint) while the debugger is still
+  // connected, we pass false to stay in debuggable. Switching runtimes is expensive so we only want
+  // to switch when we know debug features aren't needed anymore.
+  void DisableDeoptimization(const char* key, bool try_switch_to_non_debuggable)
       REQUIRES(Locks::mutator_lock_, Roles::uninterruptible_);
 
   // Enables entry exit hooks support. This is called in preparation for debug requests that require
@@ -254,10 +260,6 @@ class Instrumentation {
   void EnableEntryExitHooks(const char* key)
       REQUIRES(Locks::mutator_lock_, Roles::uninterruptible_);
 
-  // Switches the runtime state to non-java debuggable if entry / exit hooks are no longer required
-  // and the runtime did not start off as java debuggable.
-  void MaybeSwitchRuntimeDebugState(Thread* self)
-      REQUIRES(Locks::mutator_lock_, Roles::uninterruptible_);
 
   bool AreAllMethodsDeoptimized() const {
     return InterpreterStubsInstalled();
@@ -592,10 +594,6 @@ class Instrumentation {
       REQUIRES_SHARED(Locks::mutator_lock_);
 
  private:
-  // Returns true if moving to the given instrumentation level requires the installation of stubs.
-  // False otherwise.
-  bool RequiresInstrumentationInstallation(InstrumentationLevel new_level) const;
-
   // Update the current instrumentation_level_.
   void UpdateInstrumentationLevel(InstrumentationLevel level);
 
@@ -604,19 +602,25 @@ class Instrumentation {
   // the caller must pass a unique key (a string) identifying it so we remind which
   // instrumentation level it needs. Therefore the current instrumentation level
   // becomes the highest instrumentation level required by a client.
-  void ConfigureStubs(const char* key, InstrumentationLevel desired_instrumentation_level)
+  void ConfigureStubs(const char* key,
+                      InstrumentationLevel desired_instrumentation_level,
+                      bool try_switch_to_non_debuggable)
       REQUIRES(Locks::mutator_lock_, Roles::uninterruptible_)
-      REQUIRES(!Locks::thread_list_lock_,
-               !Locks::classlinker_classes_lock_);
-  void UpdateStubs() REQUIRES(Locks::mutator_lock_, Roles::uninterruptible_)
-      REQUIRES(!Locks::thread_list_lock_,
-               !Locks::classlinker_classes_lock_);
+      REQUIRES(!Locks::thread_list_lock_, !Locks::classlinker_classes_lock_);
+  void UpdateStubs(bool try_switch_to_non_debuggable)
+      REQUIRES(Locks::mutator_lock_, Roles::uninterruptible_)
+      REQUIRES(!Locks::thread_list_lock_, !Locks::classlinker_classes_lock_);
 
   // If there are no pending deoptimizations restores the stack to the normal state by updating the
   // return pcs to actual return addresses from the instrumentation stack and clears the
   // instrumentation stack.
   void MaybeRestoreInstrumentationStack() REQUIRES(Locks::mutator_lock_);
 
+  // Switches the runtime state to non-java debuggable if entry / exit hooks are no longer required
+  // and the runtime did not start off as java debuggable.
+  void MaybeSwitchRuntimeDebugState(Thread* self)
+      REQUIRES(Locks::mutator_lock_, Roles::uninterruptible_);
+
   // No thread safety analysis to get around SetQuickAllocEntryPointsInstrumented requiring
   // exclusive access to mutator lock which you can't get if the runtime isn't started.
   void SetEntrypointsInstrumented(bool instrumented) NO_THREAD_SAFETY_ANALYSIS;
diff --git a/runtime/instrumentation_test.cc b/runtime/instrumentation_test.cc
index 1e98e57c2a..f6d8a8b78a 100644
--- a/runtime/instrumentation_test.cc
+++ b/runtime/instrumentation_test.cc
@@ -195,7 +195,7 @@ class InstrumentationTest : public CommonRuntimeTest {
                                     gc::kGcCauseInstrumentation,
                                     gc::kCollectorTypeInstrumentation);
     ScopedSuspendAll ssa("Instrumentation::ConfigureStubs");
-    instr->ConfigureStubs(key, level);
+    instr->ConfigureStubs(key, level, /*try_switch_to_non_debuggable=*/false);
   }
 
   Instrumentation::InstrumentationLevel GetCurrentInstrumentationLevel() {
@@ -286,7 +286,7 @@ class InstrumentationTest : public CommonRuntimeTest {
     ScopedSuspendAll ssa("Single method undeoptimization");
     instrumentation->Undeoptimize(method);
     if (disable_deoptimization) {
-      instrumentation->DisableDeoptimization(key);
+      instrumentation->DisableDeoptimization(key, /*try_switch_to_non_debuggable=*/false);
     }
   }
 
@@ -313,7 +313,7 @@ class InstrumentationTest : public CommonRuntimeTest {
     ScopedSuspendAll ssa("Full undeoptimization");
     instrumentation->UndeoptimizeEverything(key);
     if (disable_deoptimization) {
-      instrumentation->DisableDeoptimization(key);
+      instrumentation->DisableDeoptimization(key, /*try_switch_to_non_debuggable=*/false);
     }
   }
 
diff --git a/runtime/trace.cc b/runtime/trace.cc
index e0cf418def..c70d7d8e54 100644
--- a/runtime/trace.cc
+++ b/runtime/trace.cc
@@ -657,7 +657,6 @@ void Trace::StopTracing(bool flush_entries) {
               instrumentation::Instrumentation::kMethodUnwind,
           is_fast_trace);
       runtime->GetInstrumentation()->DisableMethodTracing(kTracerInstrumentationKey);
-      runtime->GetInstrumentation()->MaybeSwitchRuntimeDebugState(self);
     }
 
     // Flush thread specific buffer from all threads before resetting the_trace_ to nullptr.