Merge "Bailout from compilation if an invoke is malformed." into mnc-dev

6 files changed, 141 insertions, 10 deletions

diff --git a/compiler/optimizing/builder.cc b/compiler/optimizing/builder.cc
index c2a9b5ccf1..a53c488e32 100644
--- a/compiler/optimizing/builder.cc
+++ b/compiler/optimizing/builder.cc
@@ -605,7 +605,12 @@ bool HGraphBuilder::BuildInvoke(const Instruction& instruction,
   const char* descriptor = dex_file_->StringDataByIdx(proto_id.shorty_idx_);
   Primitive::Type return_type = Primitive::GetType(descriptor[0]);
   bool is_instance_call = invoke_type != kStatic;
-  size_t number_of_arguments = strlen(descriptor) - (is_instance_call ? 0 : 1);
+  // Remove the return type from the 'proto'.
+  size_t number_of_arguments = strlen(descriptor) - 1;
+  if (is_instance_call) {
+    // One extra argument for 'this'.
+    ++number_of_arguments;
+  }
 
   MethodReference target_method(dex_file_, method_idx);
   uintptr_t direct_code;
@@ -616,7 +621,8 @@ bool HGraphBuilder::BuildInvoke(const Instruction& instruction,
   if (!compiler_driver_->ComputeInvokeInfo(dex_compilation_unit_, dex_pc, true, true,
                                            &optimized_invoke_type, &target_method, &table_index,
                                            &direct_code, &direct_method)) {
-    VLOG(compiler) << "Did not compile " << PrettyMethod(method_idx, *dex_file_)
+    VLOG(compiler) << "Did not compile "
+                   << PrettyMethod(dex_compilation_unit_->GetDexMethodIndex(), *dex_file_)
                    << " because a method call could not be resolved";
     MaybeRecordStat(MethodCompilationStat::kNotCompiledUnresolvedMethod);
     return false;
@@ -738,19 +744,29 @@ bool HGraphBuilder::BuildInvoke(const Instruction& instruction,
     start_index = 1;
   }
 
-  uint32_t descriptor_index = 1;
+  uint32_t descriptor_index = 1;  // Skip the return type.
   uint32_t argument_index = start_index;
   if (is_string_init) {
     start_index = 1;
   }
-  for (size_t i = start_index; i < number_of_vreg_arguments; i++, argument_index++) {
+  for (size_t i = start_index;
+       // Make sure we don't go over the expected arguments or over the number of
+       // dex registers given. If the instruction was seen as dead by the verifier,
+       // it hasn't been properly checked.
+       (i < number_of_vreg_arguments) && (argument_index < number_of_arguments);
+       i++, argument_index++) {
     Primitive::Type type = Primitive::GetType(descriptor[descriptor_index++]);
     bool is_wide = (type == Primitive::kPrimLong) || (type == Primitive::kPrimDouble);
-    if (!is_range && is_wide && args[i] + 1 != args[i + 1]) {
-      LOG(WARNING) << "Non sequential register pair in " << dex_compilation_unit_->GetSymbol()
-                   << " at " << dex_pc;
-      // We do not implement non sequential register pair.
-      MaybeRecordStat(MethodCompilationStat::kNotCompiledNonSequentialRegPair);
+    if (!is_range
+        && is_wide
+        && ((i + 1 == number_of_vreg_arguments) || (args[i] + 1 != args[i + 1]))) {
+      // Longs and doubles should be in pairs, that is, sequential registers. The verifier should
+      // reject any class where this is violated. However, the verifier only does these checks
+      // on non trivially dead instructions, so we just bailout the compilation.
+      VLOG(compiler) << "Did not compile "
+                     << PrettyMethod(dex_compilation_unit_->GetDexMethodIndex(), *dex_file_)
+                     << " because of non-sequential dex register pair in wide argument";
+      MaybeRecordStat(MethodCompilationStat::kNotCompiledMalformedOpcode);
       return false;
     }
     HInstruction* arg = LoadLocal(is_range ? register_index + i : args[i], type);
@@ -759,7 +775,14 @@ bool HGraphBuilder::BuildInvoke(const Instruction& instruction,
       i++;
     }
   }
-  DCHECK_EQ(argument_index, number_of_arguments);
+
+  if (argument_index != number_of_arguments) {
+    VLOG(compiler) << "Did not compile "
+                   << PrettyMethod(dex_compilation_unit_->GetDexMethodIndex(), *dex_file_)
+                   << " because of wrong number of arguments in invoke instruction";
+    MaybeRecordStat(MethodCompilationStat::kNotCompiledMalformedOpcode);
+    return false;
+  }
 
   if (clinit_check_requirement == HInvokeStaticOrDirect::ClinitCheckRequirement::kExplicit) {
     // Add the class initialization check as last input of `invoke`.
diff --git a/compiler/optimizing/optimizing_compiler_stats.h b/compiler/optimizing/optimizing_compiler_stats.h
index b6b1bb1cad..6d340e2261 100644
--- a/compiler/optimizing/optimizing_compiler_stats.h
+++ b/compiler/optimizing/optimizing_compiler_stats.h
@@ -37,6 +37,7 @@ enum MethodCompilationStat {
   kNotCompiledClassNotVerified,
   kNotCompiledHugeMethod,
   kNotCompiledLargeMethodNoBranches,
+  kNotCompiledMalformedOpcode,
   kNotCompiledNoCodegen,
   kNotCompiledNonSequentialRegPair,
   kNotCompiledPathological,
@@ -105,6 +106,7 @@ class OptimizingCompilerStats {
       case kNotCompiledClassNotVerified : return "kNotCompiledClassNotVerified";
       case kNotCompiledHugeMethod : return "kNotCompiledHugeMethod";
       case kNotCompiledLargeMethodNoBranches : return "kNotCompiledLargeMethodNoBranches";
+      case kNotCompiledMalformedOpcode : return "kNotCompiledMalformedOpcode";
       case kNotCompiledNoCodegen : return "kNotCompiledNoCodegen";
       case kNotCompiledNonSequentialRegPair : return "kNotCompiledNonSequentialRegPair";
       case kNotCompiledPathological : return "kNotCompiledPathological";
diff --git a/test/503-dead-instructions/expected.txt b/test/503-dead-instructions/expected.txt
new file mode 100644
index 0000000000..ccaf6f8f0f
--- /dev/null
+++ b/test/503-dead-instructions/expected.txt
@@ -0,0 +1 @@
+Enter
diff --git a/test/503-dead-instructions/info.txt b/test/503-dead-instructions/info.txt
new file mode 100644
index 0000000000..7e3f1aba34
--- /dev/null
+++ b/test/503-dead-instructions/info.txt
@@ -0,0 +1,2 @@
+Regression test for the building phase of the optimizing
+compiler. See comment in smali file.
diff --git a/test/503-dead-instructions/smali/DeadInstructions.smali b/test/503-dead-instructions/smali/DeadInstructions.smali
new file mode 100644
index 0000000000..9f6c5653fa
--- /dev/null
+++ b/test/503-dead-instructions/smali/DeadInstructions.smali
@@ -0,0 +1,63 @@
+# Copyright (C) 2015 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.
+
+.class public LDeadInstructions;
+
+.super Ljava/lang/Object;
+
+.method public static method1()V
+   .registers 2
+   return-void
+   # Create a label and a branch to that label to trick the
+   # optimizing compiler into thinking the invoke is live.
+   :start
+   const/4 v0, 0
+   const/4 v1, 0
+   # Provide more arguments than we should. Because this is dead
+   # code, the verifier will not check the argument count. So
+   # the compilers must do the same.
+   invoke-static {v0, v1}, LDeadInstructions;->method1()V
+   goto :start
+.end method
+
+.method public static method2(J)V
+   .registers 3
+   return-void
+   :start
+   const/4 v0, 0
+   const/4 v1, 0
+   const/4 v2, 0
+   # Give a non-sequential pair for the long argument.
+   invoke-static {v0, v2}, LDeadInstructions;->method2(J)V
+   goto :start
+.end method
+
+.method public static method3()V
+   .registers 1
+   return-void
+   :start
+   const/4 v0, 0
+   # Give one half of a pair.
+   invoke-static {v0}, LDeadInstructions;->method2(J)V
+   goto :start
+.end method
+
+.method public static method4()V
+   .registers 2
+   return-void
+   :start
+   # Provide less arguments than we should.
+   invoke-static {}, LDeadInstructions;->method3(J)V
+   goto :start
+.end method
diff --git a/test/503-dead-instructions/src/Main.java b/test/503-dead-instructions/src/Main.java
new file mode 100644
index 0000000000..6249dc79fa
--- /dev/null
+++ b/test/503-dead-instructions/src/Main.java
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2015 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;
+
+public class Main {
+  public static void main(String[] args) throws Exception {
+    // Workaround for b/18051191.
+    System.out.println("Enter");
+    Class<?> c = Class.forName("DeadInstructions");
+    Method m = c.getMethod("method1");
+    Object[] arguments1 = { };
+    m.invoke(null, arguments1);
+
+    Object[] arguments2 = { (long)4 };
+    m = c.getMethod("method2", long.class);
+    m.invoke(null, arguments2);
+
+    Object[] arguments3 = { };
+    m = c.getMethod("method3");
+    m.invoke(null, arguments3);
+
+    Object[] arguments4 = { };
+    m = c.getMethod("method4");
+    m.invoke(null, arguments4);
+  }
+}