Revert^6 "Ensure that OSR still is possible with jvmti"

The instrumentation uninstall could set methods to non-debuggable
boot.oat code. This could cause events to be missed due to methods
being inlined. We needed to change the path so that we would only have
the JIT/interpreter replace methods. We do this by adding a new
callback that can be used to determine if a method needs to be
debuggable and being more careful about replacing code when this is
true.

This reverts commit 5f3005c8844d851d7d218b88b5f90d6c9083ce24.
This unreverts commit b9ad26d1ed9146b89555d4333021f44eeb831f05.

Reason for revert: Fixed issue causing CTS version of test 993 failure.

Test: cts-tradefed run cts-dev CtsJvmtiRunTest993HostTestCases
Test: ./test.py --host -j50 --all -t 993
Test: ./test.py --host
Test: while ./test/run-test --host --jit 1935; do; done
Test: while ./test/run-test --host --jit --jvmti-redefine-stress 1935; do; done
Test: am start --attach-agent -n com.example.android.displayingbitmaps/.ui.ImageGridActivity
      Run blur filter.
Bug: 76226464
Bug: 77306669

Change-Id: I5068201a03f7613787c66981405499b6499c24e1

11 files changed, 127 insertions, 50 deletions

diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index 6d84ffa53f..a6f1207f34 100644
--- a/openjdkjvmti/deopt_manager.cc
+++ b/openjdkjvmti/deopt_manager.cc
@@ -53,20 +53,29 @@
 namespace openjdkjvmti {
 
 // TODO We should make this much more selective in the future so we only return true when we
-// actually care about the method (i.e. had locals changed, have breakpoints, etc.). For now though
-// we can just assume that we care we are loaded at all.
-//
-// Even if we don't keep track of this at the method level we might want to keep track of it at the
-// level of enabled capabilities.
-bool JvmtiMethodInspectionCallback::IsMethodBeingInspected(
-    art::ArtMethod* method ATTRIBUTE_UNUSED) {
-  return true;
+// actually care about the method at this time (ie active frames had locals changed). For now we
+// just assume that if anything has changed any frame's locals we care about all methods. If nothing
+// has we only care about methods with active breakpoints on them. In the future we should probably
+// rewrite all of this to instead do this at the ShadowFrame or thread granularity.
+bool JvmtiMethodInspectionCallback::IsMethodBeingInspected(art::ArtMethod* method) {
+  // Non-java-debuggable runtimes we need to assume that any method might not be debuggable and
+  // therefore potentially being inspected (due to inlines). If we are debuggable we rely hard on
+  // inlining not being done since we don't keep track of which methods get inlined where and simply
+  // look to see if the method is breakpointed.
+  return !art::Runtime::Current()->IsJavaDebuggable() ||
+      manager_->HaveLocalsChanged() ||
+      manager_->MethodHasBreakpoints(method);
 }
 
 bool JvmtiMethodInspectionCallback::IsMethodSafeToJit(art::ArtMethod* method) {
   return !manager_->MethodHasBreakpoints(method);
 }
 
+bool JvmtiMethodInspectionCallback::MethodNeedsDebugVersion(
+    art::ArtMethod* method ATTRIBUTE_UNUSED) {
+  return true;
+}
+
 DeoptManager::DeoptManager()
   : deoptimization_status_lock_("JVMTI_DeoptimizationStatusLock",
                                 static_cast<art::LockLevel>(
@@ -75,7 +84,10 @@ DeoptManager::DeoptManager()
     performing_deoptimization_(false),
     global_deopt_count_(0),
     deopter_count_(0),
-    inspection_callback_(this) { }
+    breakpoint_status_lock_("JVMTI_BreakpointStatusLock",
+                            static_cast<art::LockLevel>(art::LockLevel::kAbortLock + 1)),
+    inspection_callback_(this),
+    set_local_variable_called_(false) { }
 
 void DeoptManager::Setup() {
   art::ScopedThreadStateChange stsc(art::Thread::Current(),
@@ -121,14 +133,11 @@ void DeoptManager::FinishSetup() {
 }
 
 bool DeoptManager::MethodHasBreakpoints(art::ArtMethod* method) {
-  art::MutexLock lk(art::Thread::Current(), deoptimization_status_lock_);
+  art::MutexLock lk(art::Thread::Current(), breakpoint_status_lock_);
   return MethodHasBreakpointsLocked(method);
 }
 
 bool DeoptManager::MethodHasBreakpointsLocked(art::ArtMethod* method) {
-  if (deopter_count_ == 0) {
-    return false;
-  }
   auto elem = breakpoint_status_.find(method);
   return elem != breakpoint_status_.end() && elem->second != 0;
 }
@@ -158,18 +167,23 @@ void DeoptManager::AddMethodBreakpoint(art::ArtMethod* method) {
 
   art::ScopedThreadSuspension sts(self, art::kSuspended);
   deoptimization_status_lock_.ExclusiveLock(self);
-
-  DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request";
-
-  if (MethodHasBreakpointsLocked(method)) {
-    // Don't need to do anything extra.
-    breakpoint_status_[method]++;
-    // Another thread might be deoptimizing the very method we just added new breakpoints for. Wait
-    // for any deopts to finish before moving on.
-    WaitForDeoptimizationToFinish(self);
-    return;
+  {
+    breakpoint_status_lock_.ExclusiveLock(self);
+
+    DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request";
+
+    if (MethodHasBreakpointsLocked(method)) {
+      // Don't need to do anything extra.
+      breakpoint_status_[method]++;
+      // Another thread might be deoptimizing the very method we just added new breakpoints for.
+      // Wait for any deopts to finish before moving on.
+      breakpoint_status_lock_.ExclusiveUnlock(self);
+      WaitForDeoptimizationToFinish(self);
+      return;
+    }
+    breakpoint_status_[method] = 1;
+    breakpoint_status_lock_.ExclusiveUnlock(self);
   }
-  breakpoint_status_[method] = 1;
   auto instrumentation = art::Runtime::Current()->GetInstrumentation();
   if (instrumentation->IsForcedInterpretOnly()) {
     // We are already interpreting everything so no need to do anything.
@@ -196,17 +210,22 @@ void DeoptManager::RemoveMethodBreakpoint(art::ArtMethod* method) {
   // need but since that is very heavy we will instead just use a condition variable to make sure we
   // don't race with ourselves.
   deoptimization_status_lock_.ExclusiveLock(self);
-
-  DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request";
-  DCHECK(MethodHasBreakpointsLocked(method)) << "Breakpoint on a method was removed without "
-                                             << "breakpoints present!";
+  bool is_last_breakpoint;
+  {
+    art::MutexLock mu(self, breakpoint_status_lock_);
+
+    DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request";
+    DCHECK(MethodHasBreakpointsLocked(method)) << "Breakpoint on a method was removed without "
+                                              << "breakpoints present!";
+    breakpoint_status_[method] -= 1;
+    is_last_breakpoint = (breakpoint_status_[method] == 0);
+  }
   auto instrumentation = art::Runtime::Current()->GetInstrumentation();
-  breakpoint_status_[method] -= 1;
   if (UNLIKELY(instrumentation->IsForcedInterpretOnly())) {
     // We don't need to do anything since we are interpreting everything anyway.
     deoptimization_status_lock_.ExclusiveUnlock(self);
     return;
-  } else if (breakpoint_status_[method] == 0) {
+  } else if (is_last_breakpoint) {
     if (UNLIKELY(is_default)) {
       RemoveDeoptimizeAllMethodsLocked(self);
     } else {
diff --git a/openjdkjvmti/deopt_manager.h b/openjdkjvmti/deopt_manager.h
index a495b6835c..6e991dee3d 100644
--- a/openjdkjvmti/deopt_manager.h
+++ b/openjdkjvmti/deopt_manager.h
@@ -32,6 +32,7 @@
 #ifndef ART_OPENJDKJVMTI_DEOPT_MANAGER_H_
 #define ART_OPENJDKJVMTI_DEOPT_MANAGER_H_
 
+#include <atomic>
 #include <unordered_map>
 
 #include "jni.h"
@@ -62,6 +63,9 @@ struct JvmtiMethodInspectionCallback : public art::MethodInspectionCallback {
   bool IsMethodSafeToJit(art::ArtMethod* method)
       OVERRIDE REQUIRES_SHARED(art::Locks::mutator_lock_);
 
+  bool MethodNeedsDebugVersion(art::ArtMethod* method)
+      OVERRIDE REQUIRES_SHARED(art::Locks::mutator_lock_);
+
  private:
   DeoptManager* manager_;
 };
@@ -107,9 +111,17 @@ class DeoptManager {
 
   static DeoptManager* Get();
 
+  bool HaveLocalsChanged() const {
+    return set_local_variable_called_.load();
+  }
+
+  void SetLocalsUpdated() {
+    set_local_variable_called_.store(true);
+  }
+
  private:
   bool MethodHasBreakpointsLocked(art::ArtMethod* method)
-      REQUIRES(deoptimization_status_lock_);
+      REQUIRES(breakpoint_status_lock_);
 
   // Wait until nothing is currently in the middle of deoptimizing/undeoptimizing something. This is
   // needed to ensure that everything is synchronized since threads need to drop the
@@ -156,13 +168,20 @@ class DeoptManager {
   // Number of users of deoptimization there currently are.
   uint32_t deopter_count_ GUARDED_BY(deoptimization_status_lock_);
 
+  // A mutex that just protects the breakpoint-status map. This mutex should always be at the
+  // bottom of the lock hierarchy. Nothing more should be locked if we hold this.
+  art::Mutex breakpoint_status_lock_ ACQUIRED_BEFORE(art::Locks::abort_lock_);
   // A map from methods to the number of breakpoints in them from all envs.
   std::unordered_map<art::ArtMethod*, uint32_t> breakpoint_status_
-      GUARDED_BY(deoptimization_status_lock_);
+      GUARDED_BY(breakpoint_status_lock_);
 
   // The MethodInspectionCallback we use to tell the runtime if we care about particular methods.
   JvmtiMethodInspectionCallback inspection_callback_;
 
+  // Set to true if anything calls SetLocalVariables on any thread since we need to be careful about
+  // OSR after this.
+  std::atomic<bool> set_local_variable_called_;
+
   // Helper for setting up/tearing-down for deoptimization.
   friend class ScopedDeoptimizationContext;
 };
diff --git a/openjdkjvmti/ti_method.cc b/openjdkjvmti/ti_method.cc
index bf2e6cd104..b83310dc85 100644
--- a/openjdkjvmti/ti_method.cc
+++ b/openjdkjvmti/ti_method.cc
@@ -915,6 +915,9 @@ jvmtiError MethodUtil::SetLocalVariableGeneric(jvmtiEnv* env ATTRIBUTE_UNUSED,
   if (depth < 0) {
     return ERR(ILLEGAL_ARGUMENT);
   }
+  // Make sure that we know not to do any OSR anymore.
+  // TODO We should really keep track of this at the Frame granularity.
+  DeoptManager::Get()->SetLocalsUpdated();
   art::Thread* self = art::Thread::Current();
   // Suspend JIT since it can get confused if we deoptimize methods getting jitted.
   art::jit::ScopedJitSuspend suspend_jit;
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 4a9449640b..28659cb11d 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -364,6 +364,11 @@ bool DebuggerActiveMethodInspectionCallback::IsMethodSafeToJit(ArtMethod* m) {
   return !Dbg::MethodHasAnyBreakpoints(m);
 }
 
+bool DebuggerActiveMethodInspectionCallback::MethodNeedsDebugVersion(
+    ArtMethod* m ATTRIBUTE_UNUSED) {
+  return Dbg::IsDebuggerActive();
+}
+
 void InternalDebuggerControlCallback::StartDebugger() {
   // Release the mutator lock.
   ScopedThreadStateChange stsc(art::Thread::Current(), kNative);
diff --git a/runtime/debugger.h b/runtime/debugger.h
index 74018137a0..e1de991812 100644
--- a/runtime/debugger.h
+++ b/runtime/debugger.h
@@ -56,6 +56,7 @@ class Thread;
 struct DebuggerActiveMethodInspectionCallback : public MethodInspectionCallback {
   bool IsMethodBeingInspected(ArtMethod* method) OVERRIDE REQUIRES_SHARED(Locks::mutator_lock_);
   bool IsMethodSafeToJit(ArtMethod* method) OVERRIDE REQUIRES_SHARED(Locks::mutator_lock_);
+  bool MethodNeedsDebugVersion(ArtMethod* method) OVERRIDE REQUIRES_SHARED(Locks::mutator_lock_);
 };
 
 struct DebuggerDdmCallback : public DdmCallback {
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 84a148f21c..d7f33d5e43 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -139,10 +139,13 @@ static void UpdateEntrypoints(ArtMethod* method, const void* quick_code)
 
 bool Instrumentation::NeedDebugVersionFor(ArtMethod* method) const
     REQUIRES_SHARED(Locks::mutator_lock_) {
-  return Runtime::Current()->IsJavaDebuggable() &&
+  art::Runtime* runtime = Runtime::Current();
+  // If anything says we need the debug version or we are debuggable we will need the debug version
+  // of the method.
+  return (runtime->GetRuntimeCallbacks()->MethodNeedsDebugVersion(method) ||
+          runtime->IsJavaDebuggable()) &&
          !method->IsNative() &&
-         !method->IsProxyMethod() &&
-         Runtime::Current()->GetRuntimeCallbacks()->IsMethodBeingInspected(method);
+         !method->IsProxyMethod();
 }
 
 void Instrumentation::InstallStubsForMethod(ArtMethod* method) {
diff --git a/runtime/runtime_callbacks.cc b/runtime/runtime_callbacks.cc
index cd3c0b7c88..758917cf7e 100644
--- a/runtime/runtime_callbacks.cc
+++ b/runtime/runtime_callbacks.cc
@@ -106,6 +106,15 @@ bool RuntimeCallbacks::IsMethodBeingInspected(ArtMethod* m) {
   return false;
 }
 
+bool RuntimeCallbacks::MethodNeedsDebugVersion(ArtMethod* m) {
+  for (MethodInspectionCallback* cb : method_inspection_callbacks_) {
+    if (cb->MethodNeedsDebugVersion(m)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 void RuntimeCallbacks::AddThreadLifecycleCallback(ThreadLifecycleCallback* cb) {
   thread_callbacks_.push_back(cb);
 }
diff --git a/runtime/runtime_callbacks.h b/runtime/runtime_callbacks.h
index 24386ba14a..9f0410d102 100644
--- a/runtime/runtime_callbacks.h
+++ b/runtime/runtime_callbacks.h
@@ -130,6 +130,10 @@ class MethodInspectionCallback {
   // Note that '!IsMethodSafeToJit(m) implies IsMethodBeingInspected(m)'. That is that if this
   // method returns false IsMethodBeingInspected must return true.
   virtual bool IsMethodSafeToJit(ArtMethod* method) REQUIRES_SHARED(Locks::mutator_lock_) = 0;
+
+  // Returns true if we expect the method to be debuggable but are not doing anything unusual with
+  // it currently.
+  virtual bool MethodNeedsDebugVersion(ArtMethod* method) REQUIRES_SHARED(Locks::mutator_lock_) = 0;
 };
 
 class RuntimeCallbacks {
@@ -198,6 +202,11 @@ class RuntimeCallbacks {
   // entrypoint should not be changed to JITed code.
   bool IsMethodSafeToJit(ArtMethod* method) REQUIRES_SHARED(Locks::mutator_lock_);
 
+  // Returns true if some MethodInspectionCallback indicates the method needs to use a debug
+  // version. This allows later code to set breakpoints or perform other actions that could be
+  // broken by some optimizations.
+  bool MethodNeedsDebugVersion(ArtMethod* method) REQUIRES_SHARED(Locks::mutator_lock_);
+
   void AddMethodInspectionCallback(MethodInspectionCallback* cb)
       REQUIRES_SHARED(Locks::mutator_lock_);
   void RemoveMethodInspectionCallback(MethodInspectionCallback* cb)
diff --git a/test/1935-get-set-current-frame-jit/expected.txt b/test/1935-get-set-current-frame-jit/expected.txt
index cdb8f6a825..a685891775 100644
--- a/test/1935-get-set-current-frame-jit/expected.txt
+++ b/test/1935-get-set-current-frame-jit/expected.txt
@@ -1,7 +1,5 @@
 JNI_OnLoad called
 From GetLocalInt(), value is 42
-isInOsrCode? false
 	Value is '42'
 Setting TARGET to 1337
-isInOsrCode? false
 	Value is '1337'
diff --git a/test/1935-get-set-current-frame-jit/run b/test/1935-get-set-current-frame-jit/run
index 51875a7e86..e569d08ffd 100755
--- a/test/1935-get-set-current-frame-jit/run
+++ b/test/1935-get-set-current-frame-jit/run
@@ -14,5 +14,5 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-# Ask for stack traces to be dumped to a file rather than to stdout.
-./default-run "$@" --jvmti
+# Ensure the test is not subject to code collection
+./default-run "$@" --jvmti --runtime-option -Xjitinitialsize:32M
diff --git a/test/1935-get-set-current-frame-jit/src/Main.java b/test/1935-get-set-current-frame-jit/src/Main.java
index 714a98aaf3..378aaf7a94 100644
--- a/test/1935-get-set-current-frame-jit/src/Main.java
+++ b/test/1935-get-set-current-frame-jit/src/Main.java
@@ -21,6 +21,7 @@ import java.lang.reflect.Constructor;
 import java.lang.reflect.Executable;
 import java.lang.reflect.Method;
 import java.nio.ByteBuffer;
+import java.time.Instant;
 import java.util.concurrent.Semaphore;
 import java.util.Arrays;
 import java.util.Collection;
@@ -49,9 +50,11 @@ public class Main {
   public static class IntRunner implements Runnable {
     private volatile boolean continueBusyLoop;
     private volatile boolean inBusyLoop;
-    public IntRunner() {
+    private final boolean expectOsr;
+    public IntRunner(boolean expectOsr) {
       this.continueBusyLoop = true;
       this.inBusyLoop = false;
+      this.expectOsr = expectOsr;
     }
     public void run() {
       int TARGET = 42;
@@ -59,14 +62,23 @@ public class Main {
       while (continueBusyLoop) {
         inBusyLoop = true;
       }
-      int i = 0;
-      while (Main.isInterpreted() && i < 10000) {
-        Main.ensureJitCompiled(IntRunner.class, "run");
-        i++;
-      }
-      // We shouldn't be doing OSR since we are using JVMTI and the get/set prevents OSR.
+      // Wait up to 300 seconds for OSR to kick in if we expect it. If we don't give up after only
+      // 3 seconds.
+      Instant osrDeadline = Instant.now().plusSeconds(expectOsr ? 600 : 3);
+      do {
+        // Don't actually do anything here.
+        inBusyLoop = true;
+      } while (hasJit() && !Main.isInOsrCode("run") && osrDeadline.compareTo(Instant.now()) > 0);
+      // We shouldn't be doing OSR since we are using JVMTI and the set prevents OSR.
       // Set local will also push us to interpreter but the get local may remain in compiled code.
-      System.out.println("isInOsrCode? " + (hasJit() && Main.isInOsrCode("run")));
+      if (hasJit()) {
+        boolean inOsr = Main.isInOsrCode("run");
+        if (expectOsr && !inOsr) {
+          throw new Error("Expected to be in OSR but was not.");
+        } else if (!expectOsr && inOsr) {
+          throw new Error("Expected not to be in OSR but was.");
+        }
+      }
       reportValue(TARGET);
     }
     public void waitForBusyLoopStart() { while (!inBusyLoop) {} }
@@ -78,7 +90,7 @@ public class Main {
   public static void runGet() throws Exception {
     Method target = IntRunner.class.getDeclaredMethod("run");
     // Get Int
-    IntRunner int_runner = new IntRunner();
+    IntRunner int_runner = new IntRunner(true);
     Thread target_get = new Thread(int_runner, "GetLocalInt - Target");
     target_get.start();
     int_runner.waitForBusyLoopStart();
@@ -108,7 +120,7 @@ public class Main {
   public static void runSet() throws Exception {
     Method target = IntRunner.class.getDeclaredMethod("run");
     // Set Int
-    IntRunner int_runner = new IntRunner();
+    IntRunner int_runner = new IntRunner(false);
     Thread target_set = new Thread(int_runner, "SetLocalInt - Target");
     target_set.start();
     int_runner.waitForBusyLoopStart();
@@ -157,7 +169,6 @@ public class Main {
     throw new Error("Unable to find stack frame in method " + target + " on thread " + thr);
   }
 
-  public static native void ensureJitCompiled(Class k, String f);
   public static native boolean isInterpreted();
   public static native boolean isInOsrCode(String methodName);
   public static native boolean hasJit();