Adjust instrumentation CHECK to be correct WRT obsolete methods

Some instrumentation sanity-check code didn't take into account
obsolete methods and so could sometimes spuriously fail if the right
sequence of redefines and deoptimizations occur.

This can occur if one tries debugging a test using inline-mockito, for
example.

Test: atest -wbit RecentsAnimationTest and attach debugger with
      breakpoint in test
Test: ./test.py --host

Bug: 120630577

Change-Id: Iaeb79aebb084990b397e59f56a186e0feaffd654
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 12f1522..21c4272 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -323,8 +323,9 @@
 
         const InstrumentationStackFrame& frame =
             (*instrumentation_stack_)[instrumentation_stack_depth_];
-        CHECK_EQ(m, frame.method_) << "Expected " << ArtMethod::PrettyMethod(m)
-                                   << ", Found " << ArtMethod::PrettyMethod(frame.method_);
+        CHECK_EQ(m->GetNonObsoleteMethod(), frame.method_->GetNonObsoleteMethod())
+            << "Expected " << ArtMethod::PrettyMethod(m)
+            << ", Found " << ArtMethod::PrettyMethod(frame.method_);
         return_pc = frame.return_pc_;
         if (kVerboseInstrumentation) {
           LOG(INFO) << "Ignoring already instrumented " << frame.Dump();
@@ -470,7 +471,9 @@
           if (instrumentation_frame.interpreter_entry_) {
             CHECK(m == Runtime::Current()->GetCalleeSaveMethod(CalleeSaveType::kSaveRefsAndArgs));
           } else {
-            CHECK(m == instrumentation_frame.method_) << ArtMethod::PrettyMethod(m);
+            CHECK_EQ(m->GetNonObsoleteMethod(),
+                     instrumentation_frame.method_->GetNonObsoleteMethod())
+                << ArtMethod::PrettyMethod(m);
           }
           SetReturnPc(instrumentation_frame.return_pc_);
           if (instrumentation_->ShouldNotifyMethodEnterExitEvents() &&
diff --git a/test/1959-redefine-object-instrument/expected.txt b/test/1959-redefine-object-instrument/expected.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/1959-redefine-object-instrument/expected.txt
diff --git a/test/1959-redefine-object-instrument/fake_redef_object.cc b/test/1959-redefine-object-instrument/fake_redef_object.cc
new file mode 100644
index 0000000..b1201ab
--- /dev/null
+++ b/test/1959-redefine-object-instrument/fake_redef_object.cc
@@ -0,0 +1,134 @@
+/*
+ * Copyright (C) 2018 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <limits>
+#include <memory>
+
+#include "jni.h"
+#include "jvmti.h"
+
+// Test infrastructure
+#include "jvmti_helper.h"
+#include "test_env.h"
+
+// Slicer's headers have code that triggers these warnings. b/65298177
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wsign-compare"
+#pragma clang diagnostic ignored "-Wunused-parameter"
+#include "slicer/instrumentation.h"
+#include "slicer/reader.h"
+#include "slicer/writer.h"
+#pragma clang diagnostic pop
+
+namespace art {
+namespace Test1959RedefineObjectInstrument {
+
+// Just pull it out of the dex file but don't bother changing anything.
+static void JNICALL RedefineObjectHook(jvmtiEnv *jvmti_env,
+                                       JNIEnv* env,
+                                       jclass class_being_redefined ATTRIBUTE_UNUSED,
+                                       jobject loader ATTRIBUTE_UNUSED,
+                                       const char* name,
+                                       jobject protection_domain ATTRIBUTE_UNUSED,
+                                       jint class_data_len,
+                                       const unsigned char* class_data,
+                                       jint* new_class_data_len,
+                                       unsigned char** new_class_data) {
+  if (strcmp(name, "java/lang/Object") != 0) {
+    return;
+  }
+
+  dex::Reader reader(class_data, class_data_len);
+  dex::u4 class_index = reader.FindClassIndex("Ljava/lang/Object;");
+  if (class_index == dex::kNoIndex) {
+    env->ThrowNew(env->FindClass("java/lang/RuntimeException"),
+                  "Failed to find object in dex file!");
+    return;
+  }
+
+  reader.CreateClassIr(class_index);
+  auto dex_ir = reader.GetIr();
+  dex::Writer writer(dex_ir);
+
+  class JvmtiAllocator : public dex::Writer::Allocator {
+   public:
+    explicit JvmtiAllocator(jvmtiEnv* jvmti) : jvmti_(jvmti) {}
+
+    void* Allocate(size_t size) override {
+      unsigned char* res = nullptr;
+      jvmti_->Allocate(size, &res);
+      return res;
+    }
+
+    void Free(void* ptr) override {
+      jvmti_->Deallocate(reinterpret_cast<unsigned char*>(ptr));
+    }
+
+   private:
+    jvmtiEnv* jvmti_;
+  };
+  JvmtiAllocator allocator(jvmti_env);
+  size_t new_size;
+  *new_class_data = writer.CreateImage(&allocator, &new_size);
+  if (new_size > std::numeric_limits<jint>::max()) {
+    *new_class_data = nullptr;
+    env->ThrowNew(env->FindClass("java/lang/RuntimeException"),
+                  "transform result is too large!");
+    return;
+  }
+  *new_class_data_len = static_cast<jint>(new_size);
+}
+
+extern "C" JNIEXPORT void JNICALL Java_Main_forceRedefine(JNIEnv* env,
+                                                          jclass klass ATTRIBUTE_UNUSED,
+                                                          jclass obj_class,
+                                                          jthread thr) {
+  if (IsJVM()) {
+    // RI so don't do anything.
+    return;
+  }
+  jvmtiCapabilities caps {.can_retransform_classes = 1};
+  if (JvmtiErrorToException(env, jvmti_env, jvmti_env->AddCapabilities(&caps))) {
+    return;
+  }
+  jvmtiEventCallbacks cb {.ClassFileLoadHook = RedefineObjectHook };
+  if (JvmtiErrorToException(env, jvmti_env, jvmti_env->SetEventCallbacks(&cb, sizeof(cb)))) {
+    return;
+  }
+  if (JvmtiErrorToException(env,
+                            jvmti_env,
+                            jvmti_env->SetEventNotificationMode(JVMTI_ENABLE,
+                                                                JVMTI_EVENT_CLASS_FILE_LOAD_HOOK,
+                                                                thr))) {
+    return;
+  }
+  if (JvmtiErrorToException(env,
+                            jvmti_env,
+                            jvmti_env->RetransformClasses(1, &obj_class))) {
+    return;
+  }
+  if (JvmtiErrorToException(env,
+                            jvmti_env,
+                            jvmti_env->SetEventNotificationMode(JVMTI_DISABLE,
+                                                                JVMTI_EVENT_CLASS_FILE_LOAD_HOOK,
+                                                                thr))) {
+    return;
+  }
+}
+
+}  // namespace Test1959RedefineObjectInstrument
+}  // namespace art
+
diff --git a/test/1959-redefine-object-instrument/info.txt b/test/1959-redefine-object-instrument/info.txt
new file mode 100644
index 0000000..d15c0e0
--- /dev/null
+++ b/test/1959-redefine-object-instrument/info.txt
@@ -0,0 +1,9 @@
+Regression test for bug related to interaction between instrumentation
+installation and class redefinition.
+
+Redefining a class does not update the instrumentation stack of a thread.
+This is generally fine because the method pointer in the instrumentation
+stack is only used for some sanity-checks, logging and method-exit events
+(where it being the non-obsolete version is advantageous). Unfortunately some
+of the checks fail to account for obsolete methods and can fail sanity
+checks.
\ No newline at end of file
diff --git a/test/1959-redefine-object-instrument/run b/test/1959-redefine-object-instrument/run
new file mode 100755
index 0000000..c6e62ae
--- /dev/null
+++ b/test/1959-redefine-object-instrument/run
@@ -0,0 +1,17 @@
+#!/bin/bash
+#
+# Copyright 2016 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+./default-run "$@" --jvmti
diff --git a/test/1959-redefine-object-instrument/src/Main.java b/test/1959-redefine-object-instrument/src/Main.java
new file mode 100644
index 0000000..b3201f6
--- /dev/null
+++ b/test/1959-redefine-object-instrument/src/Main.java
@@ -0,0 +1,76 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.LinkedList;
+import java.lang.reflect.Executable;
+
+import art.*;
+
+public class Main {
+  /**
+   * NB This test cannot be run on the RI.
+   * TODO We should make this run on the RI.
+   */
+
+  public static void main(String[] args) throws Exception {
+    doTest();
+  }
+
+  public static volatile boolean started = false;
+
+  public static void doNothing() {}
+  public static void notifyBreakpointReached(Thread thr, Executable e, long l) {}
+
+  public static void doTest() throws Exception {
+    final Object lock = new Object();
+    Breakpoint.Manager man = new Breakpoint.Manager();
+    Breakpoint.startBreakpointWatch(
+        Main.class,
+        Main.class.getDeclaredMethod("notifyBreakpointReached", Thread.class, Executable.class, Long.TYPE),
+        null);
+    Thread thr = new Thread(() -> {
+      synchronized (lock) {
+        started = true;
+        // Wait basically forever.
+        try {
+          lock.wait(Integer.MAX_VALUE - 1);
+        } catch (Exception e) {
+          throw new Error("WAIT EXCEPTION", e);
+        }
+      }
+    });
+    // set the breakpoint.
+    man.setBreakpoint(Main.class.getDeclaredMethod("doNothing"), 0l);
+    thr.start();
+    while (!started || thr.getState() != Thread.State.TIMED_WAITING);
+    // Redefine while thread is paused.
+    forceRedefine(Object.class, Thread.currentThread());
+    // Clear breakpoints.
+    man.clearAllBreakpoints();
+    // set the breakpoint again.
+    man.setBreakpoint(Main.class.getDeclaredMethod("doNothing"), 0l);
+    // Wakeup
+    synchronized(lock) {
+      lock.notifyAll();
+    }
+    thr.join();
+  }
+
+  private static native void forceRedefine(Class c, Thread thr);
+}
diff --git a/test/1959-redefine-object-instrument/src/art/Breakpoint.java b/test/1959-redefine-object-instrument/src/art/Breakpoint.java
new file mode 100644
index 0000000..bbb89f7
--- /dev/null
+++ b/test/1959-redefine-object-instrument/src/art/Breakpoint.java
@@ -0,0 +1,202 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package art;
+
+import java.lang.reflect.Executable;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.Objects;
+
+public class Breakpoint {
+  public static class Manager {
+    public static class BP {
+      public final Executable method;
+      public final long location;
+
+      public BP(Executable method) {
+        this(method, getStartLocation(method));
+      }
+
+      public BP(Executable method, long location) {
+        this.method = method;
+        this.location = location;
+      }
+
+      @Override
+      public boolean equals(Object other) {
+        return (other instanceof BP) &&
+            method.equals(((BP)other).method) &&
+            location == ((BP)other).location;
+      }
+
+      @Override
+      public String toString() {
+        return method.toString() + " @ " + getLine();
+      }
+
+      @Override
+      public int hashCode() {
+        return Objects.hash(method, location);
+      }
+
+      public int getLine() {
+        try {
+          LineNumber[] lines = getLineNumberTable(method);
+          int best = -1;
+          for (LineNumber l : lines) {
+            if (l.location > location) {
+              break;
+            } else {
+              best = l.line;
+            }
+          }
+          return best;
+        } catch (Exception e) {
+          return -1;
+        }
+      }
+    }
+
+    private Set<BP> breaks = new HashSet<>();
+
+    public void setBreakpoints(BP... bs) {
+      for (BP b : bs) {
+        if (breaks.add(b)) {
+          Breakpoint.setBreakpoint(b.method, b.location);
+        }
+      }
+    }
+    public void setBreakpoint(Executable method, long location) {
+      setBreakpoints(new BP(method, location));
+    }
+
+    public void clearBreakpoints(BP... bs) {
+      for (BP b : bs) {
+        if (breaks.remove(b)) {
+          Breakpoint.clearBreakpoint(b.method, b.location);
+        }
+      }
+    }
+    public void clearBreakpoint(Executable method, long location) {
+      clearBreakpoints(new BP(method, location));
+    }
+
+    public void clearAllBreakpoints() {
+      clearBreakpoints(breaks.toArray(new BP[0]));
+    }
+  }
+
+  public static void startBreakpointWatch(Class<?> methodClass,
+                                          Executable breakpointReached,
+                                          Thread thr) {
+    startBreakpointWatch(methodClass, breakpointReached, false, thr);
+  }
+
+  /**
+   * Enables the trapping of breakpoint events.
+   *
+   * If allowRecursive == true then breakpoints will be sent even if one is currently being handled.
+   */
+  public static native void startBreakpointWatch(Class<?> methodClass,
+                                                 Executable breakpointReached,
+                                                 boolean allowRecursive,
+                                                 Thread thr);
+  public static native void stopBreakpointWatch(Thread thr);
+
+  public static final class LineNumber implements Comparable<LineNumber> {
+    public final long location;
+    public final int line;
+
+    private LineNumber(long loc, int line) {
+      this.location = loc;
+      this.line = line;
+    }
+
+    public boolean equals(Object other) {
+      return other instanceof LineNumber && ((LineNumber)other).line == line &&
+          ((LineNumber)other).location == location;
+    }
+
+    public int compareTo(LineNumber other) {
+      int v = Integer.valueOf(line).compareTo(Integer.valueOf(other.line));
+      if (v != 0) {
+        return v;
+      } else {
+        return Long.valueOf(location).compareTo(Long.valueOf(other.location));
+      }
+    }
+  }
+
+  public static native void setBreakpoint(Executable m, long loc);
+  public static void setBreakpoint(Executable m, LineNumber l) {
+    setBreakpoint(m, l.location);
+  }
+
+  public static native void clearBreakpoint(Executable m, long loc);
+  public static void clearBreakpoint(Executable m, LineNumber l) {
+    clearBreakpoint(m, l.location);
+  }
+
+  private static native Object[] getLineNumberTableNative(Executable m);
+  public static LineNumber[] getLineNumberTable(Executable m) {
+    Object[] nativeTable = getLineNumberTableNative(m);
+    long[] location = (long[])(nativeTable[0]);
+    int[] lines = (int[])(nativeTable[1]);
+    if (lines.length != location.length) {
+      throw new Error("Lines and locations have different lengths!");
+    }
+    LineNumber[] out = new LineNumber[lines.length];
+    for (int i = 0; i < lines.length; i++) {
+      out[i] = new LineNumber(location[i], lines[i]);
+    }
+    return out;
+  }
+
+  public static native long getStartLocation(Executable m);
+
+  public static int locationToLine(Executable m, long location) {
+    try {
+      Breakpoint.LineNumber[] lines = Breakpoint.getLineNumberTable(m);
+      int best = -1;
+      for (Breakpoint.LineNumber l : lines) {
+        if (l.location > location) {
+          break;
+        } else {
+          best = l.line;
+        }
+      }
+      return best;
+    } catch (Exception e) {
+      return -1;
+    }
+  }
+
+  public static long lineToLocation(Executable m, int line) throws Exception {
+    try {
+      Breakpoint.LineNumber[] lines = Breakpoint.getLineNumberTable(m);
+      for (Breakpoint.LineNumber l : lines) {
+        if (l.line == line) {
+          return l.location;
+        }
+      }
+      throw new Exception("Unable to find line " + line + " in " + m);
+    } catch (Exception e) {
+      throw new Exception("Unable to get line number info for " + m, e);
+    }
+  }
+}
+
diff --git a/test/Android.bp b/test/Android.bp
index d85e2a6..57fcd86 100644
--- a/test/Android.bp
+++ b/test/Android.bp
@@ -322,6 +322,7 @@
         "1940-ddms-ext/ddm_ext.cc",
         "1944-sudden-exit/sudden_exit.cc",
         // "1952-pop-frame-jit/pop_frame.cc",
+        "1959-redefine-object-instrument/fake_redef_object.cc",
     ],
     static_libs: [
         "libz",