Merge "Fix index checks for error strings in DexFileVerifier." into nyc-dev
diff --git a/compiler/optimizing/instruction_builder.cc b/compiler/optimizing/instruction_builder.cc
index a9ceebb..93081b9 100644
--- a/compiler/optimizing/instruction_builder.cc
+++ b/compiler/optimizing/instruction_builder.cc
@@ -721,6 +721,11 @@
DCHECK(Runtime::Current()->IsAotCompiler());
return nullptr;
}
+ if (!methods_class->IsAssignableFrom(compiling_class.Get())) {
+ // We cannot statically determine the target method. The runtime will throw a
+ // NoSuchMethodError on this one.
+ return nullptr;
+ }
ArtMethod* actual_method;
if (methods_class->IsInterface()) {
actual_method = methods_class->FindVirtualMethodForInterfaceSuper(
diff --git a/compiler/optimizing/load_store_elimination.cc b/compiler/optimizing/load_store_elimination.cc
index 1b23622..b4d93ad 100644
--- a/compiler/optimizing/load_store_elimination.cc
+++ b/compiler/optimizing/load_store_elimination.cc
@@ -732,19 +732,14 @@
if (Primitive::PrimitiveKind(heap_value->GetType())
!= Primitive::PrimitiveKind(instruction->GetType())) {
// The only situation where the same heap location has different type is when
- // we do an array get from a null constant. In order to stay properly typed
- // we do not merge the array gets.
+ // we do an array get on an instruction that originates from the null constant
+ // (the null could be behind a field access, an array access, a null check or
+ // a bound type).
+ // In order to stay properly typed on primitive types, we do not eliminate
+ // the array gets.
if (kIsDebugBuild) {
DCHECK(heap_value->IsArrayGet()) << heap_value->DebugName();
DCHECK(instruction->IsArrayGet()) << instruction->DebugName();
- HInstruction* array = instruction->AsArrayGet()->GetArray();
- DCHECK(array->IsNullCheck()) << array->DebugName();
- HInstruction* input = HuntForOriginalReference(array->InputAt(0));
- DCHECK(input->IsNullConstant()) << input->DebugName();
- array = heap_value->AsArrayGet()->GetArray();
- DCHECK(array->IsNullCheck()) << array->DebugName();
- input = HuntForOriginalReference(array->InputAt(0));
- DCHECK(input->IsNullConstant()) << input->DebugName();
}
return;
}
diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc
index 679c274..5f3cdcf 100644
--- a/compiler/optimizing/nodes.cc
+++ b/compiler/optimizing/nodes.cc
@@ -2399,6 +2399,7 @@
}
if (!NeedsEnvironment()) {
RemoveEnvironment();
+ SetSideEffects(SideEffects::None());
}
}
diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index 10a51d6..56d6a98 100644
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -5549,6 +5549,7 @@
SetPackedFlag<kFlagIsInDexCache>(true);
DCHECK(!NeedsEnvironment());
RemoveEnvironment();
+ SetSideEffects(SideEffects::None());
}
size_t InputCount() const OVERRIDE {
diff --git a/runtime/entrypoints/entrypoint_utils-inl.h b/runtime/entrypoints/entrypoint_utils-inl.h
index 16fbfaa..fc62573 100644
--- a/runtime/entrypoints/entrypoint_utils-inl.h
+++ b/runtime/entrypoints/entrypoint_utils-inl.h
@@ -514,12 +514,18 @@
CHECK(self->IsExceptionPending());
return nullptr;
} else if (!method_reference_class->IsInterface()) {
- // It is not an interface.
- mirror::Class* super_class = referring_class->GetSuperClass();
+ // It is not an interface. If the referring class is in the class hierarchy of the
+ // referenced class in the bytecode, we use its super class. Otherwise, we throw
+ // a NoSuchMethodError.
+ mirror::Class* super_class = nullptr;
+ if (method_reference_class->IsAssignableFrom(referring_class)) {
+ super_class = referring_class->GetSuperClass();
+ }
uint16_t vtable_index = resolved_method->GetMethodIndex();
if (access_check) {
// Check existence of super class.
- if (super_class == nullptr || !super_class->HasVTable() ||
+ if (super_class == nullptr ||
+ !super_class->HasVTable() ||
vtable_index >= static_cast<uint32_t>(super_class->GetVTableLength())) {
// Behavior to agree with that of the verifier.
ThrowNoSuchMethodError(type, resolved_method->GetDeclaringClass(),
@@ -693,8 +699,13 @@
// Need to do full type resolution...
return nullptr;
} else if (!method_reference_class->IsInterface()) {
- // It is not an interface.
- mirror::Class* super_class = referrer->GetDeclaringClass()->GetSuperClass();
+ // It is not an interface. If the referring class is in the class hierarchy of the
+ // referenced class in the bytecode, we use its super class. Otherwise, we cannot
+ // resolve the method.
+ if (!method_reference_class->IsAssignableFrom(referring_class)) {
+ return nullptr;
+ }
+ mirror::Class* super_class = referring_class->GetSuperClass();
if (resolved_method->GetMethodIndex() >= super_class->GetVTableLength()) {
// The super class does not have the method.
return nullptr;
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index d05ae42..2b96328 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -4101,8 +4101,8 @@
<< " to super " << PrettyMethod(res_method);
return nullptr;
}
- mirror::Class* super_klass = super.GetClass();
- if (res_method->GetMethodIndex() >= super_klass->GetVTableLength()) {
+ if (!reference_class->IsAssignableFrom(GetDeclaringClass().GetClass()) ||
+ (res_method->GetMethodIndex() >= super.GetClass()->GetVTableLength())) {
Fail(VERIFY_ERROR_NO_METHOD) << "invalid invoke-super from "
<< PrettyMethod(dex_method_idx_, *dex_file_)
<< " to super " << super
diff --git a/test/586-checker-null-array-get/src/Main.java b/test/586-checker-null-array-get/src/Main.java
index 332cfb0..e0782bc 100644
--- a/test/586-checker-null-array-get/src/Main.java
+++ b/test/586-checker-null-array-get/src/Main.java
@@ -14,10 +14,20 @@
* limitations under the License.
*/
+class Test1 {
+ int[] iarr;
+}
+
+class Test2 {
+ float[] farr;
+}
+
public class Main {
public static Object[] getObjectArray() { return null; }
public static long[] getLongArray() { return null; }
public static Object getNull() { return null; }
+ public static Test1 getNullTest1() { return null; }
+ public static Test2 getNullTest2() { return null; }
public static void main(String[] args) {
try {
@@ -26,13 +36,25 @@
} catch (NullPointerException e) {
// Expected.
}
+ try {
+ bar();
+ throw new Error("Expected NullPointerException");
+ } catch (NullPointerException e) {
+ // Expected.
+ }
+ try {
+ test1();
+ throw new Error("Expected NullPointerException");
+ } catch (NullPointerException e) {
+ // Expected.
+ }
}
/// CHECK-START: void Main.foo() load_store_elimination (after)
- /// CHECK-DAG: <<Null:l\d+>> NullConstant
- /// CHECK-DAG: <<Check:l\d+>> NullCheck [<<Null>>]
- /// CHECK-DAG: <<Get1:j\d+>> ArrayGet [<<Check>>,{{i\d+}}]
- /// CHECK-DAG: <<Get2:l\d+>> ArrayGet [<<Check>>,{{i\d+}}]
+ /// CHECK-DAG: <<Null:l\d+>> NullConstant
+ /// CHECK-DAG: <<Check:l\d+>> NullCheck [<<Null>>]
+ /// CHECK-DAG: <<Get1:j\d+>> ArrayGet [<<Check>>,{{i\d+}}]
+ /// CHECK-DAG: <<Get2:l\d+>> ArrayGet [<<Check>>,{{i\d+}}]
public static void foo() {
longField = getLongArray()[0];
objectField = getObjectArray()[0];
@@ -56,7 +78,7 @@
// elimination pass to add a HDeoptimize. Not having the bounds check helped
// the load store elimination think it could merge two ArrayGet with different
// types.
- String[] array = ((String[])getNull());
+ String[] array = (String[])getNull();
objectField = array[0];
objectField = array[1];
objectField = array[2];
@@ -68,6 +90,23 @@
longField = longArray[3];
}
+ /// CHECK-START: float Main.test1() load_store_elimination (after)
+ /// CHECK-DAG: <<Null:l\d+>> NullConstant
+ /// CHECK-DAG: <<Check1:l\d+>> NullCheck [<<Null>>]
+ /// CHECK-DAG: <<FieldGet1:l\d+>> InstanceFieldGet [<<Check1>>] field_name:Test1.iarr
+ /// CHECK-DAG: <<Check2:l\d+>> NullCheck [<<FieldGet1>>]
+ /// CHECK-DAG: <<ArrayGet1:i\d+>> ArrayGet [<<Check2>>,{{i\d+}}]
+ /// CHECK-DAG: <<ArrayGet2:f\d+>> ArrayGet [<<Check2>>,{{i\d+}}]
+ /// CHECK-DAG: Return [<<ArrayGet2>>]
+ public static float test1() {
+ Test1 test1 = getNullTest1();
+ Test2 test2 = getNullTest2();;
+ int[] iarr = test1.iarr;
+ float[] farr = test2.farr;
+ iarr[0] = iarr[1];
+ return farr[0];
+ }
+
public static long longField;
public static Object objectField;
}
diff --git a/test/594-invoke-super/expected.txt b/test/594-invoke-super/expected.txt
new file mode 100644
index 0000000..de26026
--- /dev/null
+++ b/test/594-invoke-super/expected.txt
@@ -0,0 +1,7 @@
+new A
+I am A's foo
+new B
+I am B's foo
+new A
+new B
+passed
diff --git a/test/594-invoke-super/info.txt b/test/594-invoke-super/info.txt
new file mode 100644
index 0000000..440d8b8
--- /dev/null
+++ b/test/594-invoke-super/info.txt
@@ -0,0 +1 @@
+Invoke-super on various references.
diff --git a/test/594-invoke-super/smali/invoke-super.smali b/test/594-invoke-super/smali/invoke-super.smali
new file mode 100644
index 0000000..6f787dd
--- /dev/null
+++ b/test/594-invoke-super/smali/invoke-super.smali
@@ -0,0 +1,31 @@
+#
+# Copyright (C) 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.
+
+.class public LZ;
+.super LA;
+
+.method public constructor <init>()V
+.registers 1
+ invoke-direct {v0}, LA;-><init>()V
+ return-void
+.end method
+
+.method public foo()V
+.registers 3
+ new-instance v0, LY;
+ invoke-direct {v0}, LY;-><init>()V
+ invoke-super {v0}, LY;->foo()V
+ return-void
+.end method
diff --git a/test/594-invoke-super/src/Main.java b/test/594-invoke-super/src/Main.java
new file mode 100644
index 0000000..53f2bbf
--- /dev/null
+++ b/test/594-invoke-super/src/Main.java
@@ -0,0 +1,80 @@
+/*
+ * Copyright (C) 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.
+ */
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+//
+// Two classes A and B with method foo().
+//
+
+class A {
+ A() { System.out.println("new A"); }
+
+ public void foo() { System.out.println("I am A's foo"); }
+
+ // We previously used to invoke this method with a Y instance, due
+ // to invoke-super underspecified behavior.
+ public void bar() { System.out.println("I am A's bar"); }
+}
+
+class B {
+ B() { System.out.println("new B"); }
+
+ public void foo() { System.out.println("I am B's foo"); }
+}
+
+//
+// Two subclasses X and Y that call foo() on super.
+//
+
+class X extends A {
+ public void foo() { super.foo(); }
+}
+
+class Y extends B {
+ public void foo() { super.foo(); }
+}
+
+//
+// Driver class.
+//
+
+public class Main {
+
+ public static void main(String[] args) throws Exception {
+ // The normal stuff, X's super goes to A, Y's super goes to B.
+ new X().foo();
+ new Y().foo();
+
+ // And now it gets interesting.
+
+ // In bytecode, we define a class Z that is a subclass of A, and we call
+ // invoke-super on an instance of Y.
+ Class<?> z = Class.forName("Z");
+ Method m = z.getMethod("foo");
+ try {
+ m.invoke(z.newInstance());
+ throw new Error("Expected InvocationTargetException");
+ } catch (InvocationTargetException e) {
+ if (!(e.getCause() instanceof NoSuchMethodError)) {
+ throw new Error("Expected NoSuchMethodError");
+ }
+ }
+
+ System.out.println("passed");
+ }
+}
diff --git a/test/594-load-string-regression/expected.txt b/test/594-load-string-regression/expected.txt
new file mode 100644
index 0000000..365b0e1
--- /dev/null
+++ b/test/594-load-string-regression/expected.txt
@@ -0,0 +1 @@
+String: ""
diff --git a/test/594-load-string-regression/info.txt b/test/594-load-string-regression/info.txt
new file mode 100644
index 0000000..6a07ace
--- /dev/null
+++ b/test/594-load-string-regression/info.txt
@@ -0,0 +1,2 @@
+Regression test for LoadString listing side effects when it doesn't have any
+and triggering a DCHECK() failure when merging ClinitCheck into NewInstance.
diff --git a/test/594-load-string-regression/src/Main.java b/test/594-load-string-regression/src/Main.java
new file mode 100644
index 0000000..0b9f7b5
--- /dev/null
+++ b/test/594-load-string-regression/src/Main.java
@@ -0,0 +1,77 @@
+/*
+ * Copyright (C) 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.
+ */
+
+public class Main {
+ static boolean doThrow = false;
+
+ // Note: We're not doing checker tests as we cannot do them specifically for a non-PIC
+ // configuration. The check here would be "prepare_for_register_allocation (before)"
+ // CHECK: LoadClass
+ // CHECK-NEXT: ClinitCheck
+ // CHECK-NEXT: LoadString load_kind:BootImageAddress
+ // CHECK-NEXT: NewInstance
+ // and "prepare_for_register_allocation (after)"
+ // CHECK: LoadString
+ // CHECK-NEXT: NewInstance
+ // but the order of instructions for non-PIC mode is different.
+ public static int $noinline$test() {
+ if (doThrow) { throw new Error(); }
+
+ int r = 0x12345678;
+ do {
+ // LICM pulls the LoadClass and ClinitCheck out of the loop, leaves NewInstance in the loop.
+ Helper h = new Helper();
+ // For non-PIC mode, LICM pulls the boot image LoadString out of the loop.
+ // (For PIC mode, the LoadString can throw and will not be moved out of the loop.)
+ String s = ""; // Empty string is known to be in the boot image.
+ r = r ^ (r >> 5);
+ h.$noinline$printString(s);
+ // During DCE after inlining, the loop back-edge disappears and the pre-header is
+ // merged with the body, leaving consecutive LoadClass, ClinitCheck, LoadString
+ // and NewInstance in non-PIC mode. The prepare_for_register_allocation pass
+ // merges the LoadClass and ClinitCheck with the NewInstance and checks that
+ // there are no instructions with side effects in between. This check used to
+ // fail because LoadString was always listing SideEffects::CanTriggerGC() even
+ // when it doesn't really have any side effects, i.e. for direct references to
+ // boot image Strings or for Strings known to be in the dex cache.
+ } while ($inline$shouldContinue());
+ return r;
+ }
+
+ static boolean $inline$shouldContinue() {
+ return false;
+ }
+
+ public static void main(String[] args) {
+ assertIntEquals(0x12345678 ^ (0x12345678 >> 5), $noinline$test());
+ }
+
+ public static void assertIntEquals(int expected, int result) {
+ if (expected != result) {
+ throw new Error("Expected: " + expected + ", found: " + result);
+ }
+ }
+}
+
+class Helper {
+ static boolean doThrow = false;
+
+ public void $noinline$printString(String s) {
+ if (doThrow) { throw new Error(); }
+
+ System.out.println("String: \"" + s + "\"");
+ }
+}
