Cleanup method inspection callbacks from jvmti

We used to use three different inspection callbacks for kind of similar
checks.
- IsMethodBeingInspected - returns true if method has breakpoints or if
  we need to observe any locals (ideally for only this method but
  current implementation is not ideal). This is used to check if we can
  JIT code.
- IsMethodSafeToJit - returns true if method has breakpoints. Though we
  also check if a method is being inspected making this check redundant.
- IsMethodNeedsDebugVersion - this is used to check if we need to use
  switch interpreter. This was always returning true forcing us to use
  switch interpreter when debugger is attached. We should use
  IsMethodBeingInspected instead.

This CL merges all these into IsMethodBeingInspected callback.

Bug: 206029744
Test: test.py --gtest --run-test
Change-Id: Idcde354f39775092be5ddb79f09a92f81e07b1ee

7 files changed, 7 insertions, 64 deletions

diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index 396459619f..a15e6678f1 100644
--- a/openjdkjvmti/deopt_manager.cc
+++ b/openjdkjvmti/deopt_manager.cc
@@ -67,22 +67,10 @@ namespace openjdkjvmti {
 // 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;
+  // In non-java-debuggable runtimes the breakpoint check would miss if we have breakpoints on
+  // methods that are inlined. Since these features are best effort in non-java-debuggable
+  // runtimes it is OK to be less precise. For debuggable runtimes, inlining is disabled.
+  return manager_->HaveLocalsChanged() || manager_->MethodHasBreakpoints(method);
 }
 
 DeoptManager::DeoptManager()
diff --git a/openjdkjvmti/deopt_manager.h b/openjdkjvmti/deopt_manager.h
index 9789c5ed91..c0a788b1b5 100644
--- a/openjdkjvmti/deopt_manager.h
+++ b/openjdkjvmti/deopt_manager.h
@@ -60,12 +60,6 @@ struct JvmtiMethodInspectionCallback : public art::MethodInspectionCallback {
   bool IsMethodBeingInspected(art::ArtMethod* method)
       override REQUIRES_SHARED(art::Locks::mutator_lock_);
 
-  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_;
 };
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index a48577ebe0..091c98d0e7 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -288,7 +288,7 @@ bool Instrumentation::InterpretOnly(ArtMethod* method) REQUIRES_SHARED(Locks::mu
   }
   return InterpretOnly() ||
          IsDeoptimized(method) ||
-         Runtime::Current()->GetRuntimeCallbacks()->MethodNeedsDebugVersion(method);
+         Runtime::Current()->GetRuntimeCallbacks()->IsMethodBeingInspected(method);
 }
 
 static bool CanUseAotCode(ArtMethod* method, const void* quick_code)
diff --git a/runtime/jit/jit.cc b/runtime/jit/jit.cc
index f6b45774cc..9c275dab88 100644
--- a/runtime/jit/jit.cc
+++ b/runtime/jit/jit.cc
@@ -281,7 +281,7 @@ bool Jit::CompileMethod(ArtMethod* method,
 
   RuntimeCallbacks* cb = Runtime::Current()->GetRuntimeCallbacks();
   // Don't compile the method if it has breakpoints.
-  if (cb->IsMethodBeingInspected(method) && !cb->IsMethodSafeToJit(method)) {
+  if (cb->IsMethodBeingInspected(method)) {
     VLOG(jit) << "JIT not compiling " << method->PrettyMethod()
               << " due to not being safe to jit according to runtime-callbacks. For example, there"
               << " could be breakpoints in this method.";
diff --git a/runtime/runtime_callbacks.cc b/runtime/runtime_callbacks.cc
index e0f57c013d..753ac280e3 100644
--- a/runtime/runtime_callbacks.cc
+++ b/runtime/runtime_callbacks.cc
@@ -105,17 +105,6 @@ void RuntimeCallbacks::RemoveMethodInspectionCallback(MethodInspectionCallback*
   Remove(cb, &method_inspection_callbacks_);
 }
 
-bool RuntimeCallbacks::IsMethodSafeToJit(ArtMethod* m) {
-  for (MethodInspectionCallback* cb : COPY(method_inspection_callbacks_)) {
-    if (!cb->IsMethodSafeToJit(m)) {
-      DCHECK(cb->IsMethodBeingInspected(m))
-          << "Contract requires that !IsMethodSafeToJit(m) -> IsMethodBeingInspected(m)";
-      return false;
-    }
-  }
-  return true;
-}
-
 bool RuntimeCallbacks::IsMethodBeingInspected(ArtMethod* m) {
   for (MethodInspectionCallback* cb : COPY(method_inspection_callbacks_)) {
     if (cb->IsMethodBeingInspected(m)) {
@@ -125,15 +114,6 @@ bool RuntimeCallbacks::IsMethodBeingInspected(ArtMethod* m) {
   return false;
 }
 
-bool RuntimeCallbacks::MethodNeedsDebugVersion(ArtMethod* m) {
-  for (MethodInspectionCallback* cb : COPY(method_inspection_callbacks_)) {
-    if (cb->MethodNeedsDebugVersion(m)) {
-      return true;
-    }
-  }
-  return false;
-}
-
 void RuntimeCallbacks::AddThreadLifecycleCallback(ThreadLifecycleCallback* cb) {
   WriterMutexLock mu(Thread::Current(), *callback_lock_);
   thread_callbacks_.push_back(cb);
diff --git a/runtime/runtime_callbacks.h b/runtime/runtime_callbacks.h
index 3cadd974e1..b1a7e55ae4 100644
--- a/runtime/runtime_callbacks.h
+++ b/runtime/runtime_callbacks.h
@@ -146,15 +146,6 @@ class MethodInspectionCallback {
   // Returns true if the method is being inspected currently and the runtime should not modify it in
   // potentially dangerous ways (i.e. replace with compiled version, JIT it, etc).
   virtual bool IsMethodBeingInspected(ArtMethod* method) REQUIRES_SHARED(Locks::mutator_lock_) = 0;
-
-  // Returns true if the method is safe to Jit, false otherwise.
-  // 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;
 };
 
 // Callback to let something request to be notified when reflective objects are being visited and
@@ -238,16 +229,6 @@ class RuntimeCallbacks {
   // on by some code.
   bool IsMethodBeingInspected(ArtMethod* method) REQUIRES_SHARED(Locks::mutator_lock_);
 
-  // Returns false if some MethodInspectionCallback indicates the method cannot be safetly jitted
-  // (which implies that it is being Inspected). Returns true otherwise. If it returns false the
-  // 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/common/runtime_state.cc b/test/common/runtime_state.cc
index df7244b0b0..0b72612a88 100644
--- a/test/common/runtime_state.cc
+++ b/test/common/runtime_state.cc
@@ -234,7 +234,7 @@ static void ForceJitCompiled(Thread* self,
 
   {
     ScopedObjectAccess soa(self);
-    if (!Runtime::Current()->GetRuntimeCallbacks()->IsMethodSafeToJit(method)) {
+    if (Runtime::Current()->GetRuntimeCallbacks()->IsMethodBeingInspected(method)) {
       std::string msg(method->PrettyMethod());
       msg += ": is not safe to jit!";
       ThrowIllegalStateException(msg.c_str());