Handle quickened opcodes in instrumentation deopt.

Also update 602-deoptimizeable by removing frame assertions. These
are covered  by 685-deoptimizeable.

Test: 602-deoptimizeable, 685-deoptimizeable, 687-deopt

bug: 115849764
Change-Id: Ibca3b49b22fa77541be5b972149618ce19842af9

7 files changed, 96 insertions, 61 deletions

diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 5c7b0aeeaf..ee6f479784 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -1360,38 +1360,56 @@ struct RuntimeMethodShortyVisitor : public StackVisitor {
       : StackVisitor(thread, nullptr, StackVisitor::StackWalkKind::kIncludeInlinedFrames),
         shorty('V') {}
 
-  bool VisitFrame() override REQUIRES_SHARED(Locks::mutator_lock_) {
-    ArtMethod* m = GetMethod();
-    if (m != nullptr && !m->IsRuntimeMethod()) {
-      // The first Java method.
-      if (m->IsNative()) {
-        // Use JNI method's shorty for the jni stub.
-        shorty = m->GetShorty()[0];
-        return false;
+  static uint16_t GetMethodIndexOfInvoke(ArtMethod* caller,
+                                         const Instruction& inst,
+                                         uint32_t dex_pc)
+        REQUIRES_SHARED(Locks::mutator_lock_) {
+    switch (inst.Opcode()) {
+      case Instruction::INVOKE_VIRTUAL_RANGE_QUICK:
+      case Instruction::INVOKE_VIRTUAL_QUICK: {
+        uint16_t method_idx = caller->GetIndexFromQuickening(dex_pc);
+        CHECK_NE(method_idx, DexFile::kDexNoIndex16);
+        return method_idx;
       }
-      if (m->IsProxyMethod()) {
-        // Proxy method just invokes its proxied method via
-        // art_quick_proxy_invoke_handler.
-        shorty = m->GetInterfaceMethodIfProxy(kRuntimePointerSize)->GetShorty()[0];
-        return false;
+      default: {
+        return inst.VRegB();
       }
+    }
+  }
+
+  bool VisitFrame() override REQUIRES_SHARED(Locks::mutator_lock_) {
+    ArtMethod* m = GetMethod();
+    if (m == nullptr || m->IsRuntimeMethod()) {
+      return true;
+    }
+    // The first Java method.
+    if (m->IsNative()) {
+      // Use JNI method's shorty for the jni stub.
+      shorty = m->GetShorty()[0];
+    } else if (m->IsProxyMethod()) {
+      // Proxy method just invokes its proxied method via
+      // art_quick_proxy_invoke_handler.
+      shorty = m->GetInterfaceMethodIfProxy(kRuntimePointerSize)->GetShorty()[0];
+    } else {
       const Instruction& instr = m->DexInstructions().InstructionAt(GetDexPc());
       if (instr.IsInvoke()) {
+        uint16_t method_index = GetMethodIndexOfInvoke(m, instr, GetDexPc());
         const DexFile* dex_file = m->GetDexFile();
-        if (interpreter::IsStringInit(dex_file, instr.VRegB())) {
+        if (interpreter::IsStringInit(dex_file, method_index)) {
           // Invoking string init constructor is turned into invoking
           // StringFactory.newStringFromChars() which returns a string.
           shorty = 'L';
-          return false;
+        } else {
+          shorty = dex_file->GetMethodShorty(method_index)[0];
         }
-        // A regular invoke, use callee's shorty.
-        uint32_t method_idx = instr.VRegB();
-        shorty = dex_file->GetMethodShorty(method_idx)[0];
+      } else {
+        // It could be that a non-invoke opcode invokes a stub, which in turn
+        // invokes Java code. In such cases, we should never expect a return
+        // value from the stub.
       }
-      // Stop stack walking since we've seen a Java frame.
-      return false;
     }
-    return true;
+    // Stop stack walking since we've seen a Java frame.
+    return false;
   }
 
   char shorty;
diff --git a/test/602-deoptimizeable/info.txt b/test/602-deoptimizeable/info.txt
index 4b6147f7f1..d0952f903b 100644
--- a/test/602-deoptimizeable/info.txt
+++ b/test/602-deoptimizeable/info.txt
@@ -1,8 +1 @@
 Test various cases for full/partial-fragment deoptimization.
-
-TODO: we should remove this test as its expectations at point of
-writing was that debuggable apps could run un-deoptimizeable frames
-from the boot image. Today, we deoptimize the boot image as soon as
-we see the app being debuggable. Test 685-deoptimizeable is the proper
-version of this test, but we currently keep the 602 version around to
-try diagnosing a gcstress issue.
diff --git a/test/602-deoptimizeable/src/Main.java b/test/602-deoptimizeable/src/Main.java
index 7a3285d793..46584b0847 100644
--- a/test/602-deoptimizeable/src/Main.java
+++ b/test/602-deoptimizeable/src/Main.java
@@ -33,10 +33,7 @@ class DummyObject {
 
     public int hashCode() {
         sHashCodeInvoked = true;
-        Main.assertIsManaged();
         Main.deoptimizeAll();
-        Main.assertIsInterpreted();
-        Main.assertCallerIsManaged();  // Caller is from framework code HashMap.
         return i % 64;
     }
 }
@@ -46,13 +43,6 @@ public class Main {
 
     public static native void deoptimizeAll();
     public static native void undeoptimizeAll();
-    public static native void assertIsInterpreted();
-    public static native void assertIsManaged();
-    public static native void assertCallerIsInterpreted();
-    public static native void assertCallerIsManaged();
-    public static native void disableStackFrameAsserts();
-    public static native boolean hasOatFile();
-    public static native boolean isInterpreted();
 
     public static void execute(Runnable runnable) throws Exception {
       Thread t = new Thread(runnable);
@@ -62,19 +52,13 @@ public class Main {
 
     public static void main(String[] args) throws Exception {
         System.loadLibrary(args[0]);
-        // TODO: Stack frame assertions are irrelevant in this test as we now
-        // always run JIT with debuggable. 685-deoptimizeable is the proper version
-        // of this test, but we keep this version around to diagnose a gcstress issue.
-        disableStackFrameAsserts();
         final HashMap<DummyObject, Long> map = new HashMap<DummyObject, Long>();
 
         // Single-frame deoptimization that covers partial fragment.
         execute(new Runnable() {
             public void run() {
                 int[] arr = new int[3];
-                assertIsManaged();
                 int res = $noinline$run1(arr);
-                assertIsManaged();  // Only single frame is deoptimized.
                 if (res != 79) {
                     System.out.println("Failure 1!");
                     System.exit(0);
@@ -87,13 +71,11 @@ public class Main {
             public void run() {
                 try {
                     int[] arr = new int[3];
-                    assertIsManaged();
                     // Use reflection to call $noinline$run2 so that it does
                     // full-fragment deoptimization since that is an upcall.
                     Class<?> cls = Class.forName("Main");
                     Method method = cls.getDeclaredMethod("$noinline$run2", int[].class);
                     double res = (double)method.invoke(Main.class, arr);
-                    assertIsManaged();  // Only single frame is deoptimized.
                     if (res != 79.3d) {
                         System.out.println("Failure 2!");
                         System.exit(0);
@@ -107,9 +89,7 @@ public class Main {
         // Full-fragment deoptimization.
         execute(new Runnable() {
             public void run() {
-                assertIsManaged();
                 float res = $noinline$run3B();
-                assertIsInterpreted();  // Every deoptimizeable method is deoptimized.
                 if (res != 0.034f) {
                     System.out.println("Failure 3!");
                     System.exit(0);
@@ -123,9 +103,7 @@ public class Main {
         execute(new Runnable() {
             public void run() {
                 try {
-                    assertIsManaged();
                     map.put(new DummyObject(10), Long.valueOf(100));
-                    assertIsInterpreted();  // Every deoptimizeable method is deoptimized.
                     if (map.get(new DummyObject(10)) == null) {
                         System.out.println("Expected map to contain DummyObject(10)");
                     }
@@ -147,7 +125,6 @@ public class Main {
     }
 
     public static int $noinline$run1(int[] arr) {
-        assertIsManaged();
         // Prevent inlining.
         if (sFlag) {
             throw new Error();
@@ -161,18 +138,15 @@ public class Main {
             // This causes AIOOBE and triggers deoptimization from compiled code.
             arr[3] = 1;
         } catch (ArrayIndexOutOfBoundsException e) {
-            assertIsInterpreted(); // Single-frame deoptimization triggered.
             caught = true;
         }
         if (!caught) {
             System.out.println("Expected exception");
         }
-        assertIsInterpreted();
         return 79;
     }
 
     public static double $noinline$run2(int[] arr) {
-        assertIsManaged();
         // Prevent inlining.
         if (sFlag) {
             throw new Error();
@@ -186,37 +160,30 @@ public class Main {
             // This causes AIOOBE and triggers deoptimization from compiled code.
             arr[3] = 1;
         } catch (ArrayIndexOutOfBoundsException e) {
-            assertIsInterpreted();  // Single-frame deoptimization triggered.
             caught = true;
         }
         if (!caught) {
             System.out.println("Expected exception");
         }
-        assertIsInterpreted();
         return 79.3d;
     }
 
     public static float $noinline$run3A() {
-        assertIsManaged();
         // Prevent inlining.
         if (sFlag) {
             throw new Error();
         }
         // Deoptimize callers.
         deoptimizeAll();
-        assertIsInterpreted();
-        assertCallerIsInterpreted();  // $noinline$run3B is deoptimizeable.
         return 0.034f;
     }
 
     public static float $noinline$run3B() {
-        assertIsManaged();
         // Prevent inlining.
         if (sFlag) {
             throw new Error();
         }
         float res = $noinline$run3A();
-        assertIsInterpreted();
         return res;
     }
 }
diff --git a/test/687-deopt/expected.txt b/test/687-deopt/expected.txt
new file mode 100644
index 0000000000..6a5618ebc6
--- /dev/null
+++ b/test/687-deopt/expected.txt
@@ -0,0 +1 @@
+JNI_OnLoad called
diff --git a/test/687-deopt/info.txt b/test/687-deopt/info.txt
new file mode 100644
index 0000000000..ef56f51504
--- /dev/null
+++ b/test/687-deopt/info.txt
@@ -0,0 +1,2 @@
+Regression test for instrumentation deopt, which previously did not expect a
+quickened instruction when returning from instrumentation stub.
diff --git a/test/687-deopt/src/Main.java b/test/687-deopt/src/Main.java
new file mode 100644
index 0000000000..afe90d67f7
--- /dev/null
+++ b/test/687-deopt/src/Main.java
@@ -0,0 +1,53 @@
+/*
+ * 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.
+ */
+
+import java.util.HashMap;
+
+public class Main {
+  public static void main(String[] args) {
+    System.loadLibrary(args[0]);
+
+    // Jit compile HashMap.hash method, so that instrumentation stubs
+    // will deoptimize it.
+    ensureJitCompiled(HashMap.class, "hash");
+
+    Main key = new Main();
+    Integer value = new Integer(10);
+    HashMap<Main, Integer> map = new HashMap<>();
+    map.put(key, value);
+    Integer res = map.get(key);
+    if (!value.equals(res)) {
+      throw new Error("Expected 10, got " + res);
+    }
+  }
+
+  public int hashCode() {
+    // The call stack at this point is:
+    // Main.main
+    //  HashMap.put
+    //    HashMap.hash
+    //      Main.hashCode
+    //
+    // The opcode at HashMap.hash is invoke-virtual-quick which the
+    // instrumentation code did not expect and used to fetch the wrong
+    // method index for it.
+    deoptimizeAll();
+    return 42;
+  }
+
+  public static native void deoptimizeAll();
+  public static native void ensureJitCompiled(Class<?> cls, String methodName);
+}
diff --git a/test/knownfailures.json b/test/knownfailures.json
index d831993707..7b401609c3 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -948,6 +948,7 @@
           "685-deoptimizeable",
           "685-shifts",
           "686-get-this",
+          "687-deopt",
           "706-checker-scheduler",
           "707-checker-invalid-profile",
           "714-invoke-custom-lambda-metafactory",