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",