Revert "ART: Simple structural class check"
This reverts commit fd9eb3923dcf417afcf5ed4ebb13867fd10f2de3.
Remove the simple structural check. Its naive expectations for
resolution have been wrong since we introduced compiling APKs with
a classpath for shared libraries and have never been updated.
Bug: 62857870
Test: m test-art-host
Change-Id: I4a91e9b80b035b493294d0cac029dbac2d9d3bbe
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 71558e1..141df1e 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -5440,184 +5440,6 @@
return true;
}
-static void CountMethodsAndFields(ClassDataItemIterator& dex_data,
- size_t* virtual_methods,
- size_t* direct_methods,
- size_t* static_fields,
- size_t* instance_fields) {
- *static_fields = dex_data.NumStaticFields();
- *instance_fields = dex_data.NumInstanceFields();
- *direct_methods = dex_data.NumDirectMethods();
- *virtual_methods = dex_data.NumVirtualMethods();
-}
-
-static void DumpClass(std::ostream& os,
- const DexFile& dex_file, const DexFile::ClassDef& dex_class_def,
- const char* suffix) {
- ClassDataItemIterator dex_data(dex_file, dex_file.GetClassData(dex_class_def));
- os << dex_file.GetClassDescriptor(dex_class_def) << suffix << ":\n";
- os << " Static fields:\n";
- while (dex_data.HasNextStaticField()) {
- const DexFile::FieldId& id = dex_file.GetFieldId(dex_data.GetMemberIndex());
- os << " " << dex_file.GetFieldTypeDescriptor(id) << " " << dex_file.GetFieldName(id) << "\n";
- dex_data.Next();
- }
- os << " Instance fields:\n";
- while (dex_data.HasNextInstanceField()) {
- const DexFile::FieldId& id = dex_file.GetFieldId(dex_data.GetMemberIndex());
- os << " " << dex_file.GetFieldTypeDescriptor(id) << " " << dex_file.GetFieldName(id) << "\n";
- dex_data.Next();
- }
- os << " Direct methods:\n";
- while (dex_data.HasNextDirectMethod()) {
- const DexFile::MethodId& id = dex_file.GetMethodId(dex_data.GetMemberIndex());
- os << " " << dex_file.GetMethodName(id) << dex_file.GetMethodSignature(id).ToString() << "\n";
- dex_data.Next();
- }
- os << " Virtual methods:\n";
- while (dex_data.HasNextVirtualMethod()) {
- const DexFile::MethodId& id = dex_file.GetMethodId(dex_data.GetMemberIndex());
- os << " " << dex_file.GetMethodName(id) << dex_file.GetMethodSignature(id).ToString() << "\n";
- dex_data.Next();
- }
-}
-
-static std::string DumpClasses(const DexFile& dex_file1,
- const DexFile::ClassDef& dex_class_def1,
- const DexFile& dex_file2,
- const DexFile::ClassDef& dex_class_def2) {
- std::ostringstream os;
- DumpClass(os, dex_file1, dex_class_def1, " (Compile time)");
- DumpClass(os, dex_file2, dex_class_def2, " (Runtime)");
- return os.str();
-}
-
-
-// Very simple structural check on whether the classes match. Only compares the number of
-// methods and fields.
-static bool SimpleStructuralCheck(const DexFile& dex_file1,
- const DexFile::ClassDef& dex_class_def1,
- const DexFile& dex_file2,
- const DexFile::ClassDef& dex_class_def2,
- std::string* error_msg) {
- ClassDataItemIterator dex_data1(dex_file1, dex_file1.GetClassData(dex_class_def1));
- ClassDataItemIterator dex_data2(dex_file2, dex_file2.GetClassData(dex_class_def2));
-
- // Counters for current dex file.
- size_t dex_virtual_methods1, dex_direct_methods1, dex_static_fields1, dex_instance_fields1;
- CountMethodsAndFields(dex_data1,
- &dex_virtual_methods1,
- &dex_direct_methods1,
- &dex_static_fields1,
- &dex_instance_fields1);
- // Counters for compile-time dex file.
- size_t dex_virtual_methods2, dex_direct_methods2, dex_static_fields2, dex_instance_fields2;
- CountMethodsAndFields(dex_data2,
- &dex_virtual_methods2,
- &dex_direct_methods2,
- &dex_static_fields2,
- &dex_instance_fields2);
-
- if (dex_virtual_methods1 != dex_virtual_methods2) {
- std::string class_dump = DumpClasses(dex_file1, dex_class_def1, dex_file2, dex_class_def2);
- *error_msg = StringPrintf("Virtual method count off: %zu vs %zu\n%s",
- dex_virtual_methods1,
- dex_virtual_methods2,
- class_dump.c_str());
- return false;
- }
- if (dex_direct_methods1 != dex_direct_methods2) {
- std::string class_dump = DumpClasses(dex_file1, dex_class_def1, dex_file2, dex_class_def2);
- *error_msg = StringPrintf("Direct method count off: %zu vs %zu\n%s",
- dex_direct_methods1,
- dex_direct_methods2,
- class_dump.c_str());
- return false;
- }
- if (dex_static_fields1 != dex_static_fields2) {
- std::string class_dump = DumpClasses(dex_file1, dex_class_def1, dex_file2, dex_class_def2);
- *error_msg = StringPrintf("Static field count off: %zu vs %zu\n%s",
- dex_static_fields1,
- dex_static_fields2,
- class_dump.c_str());
- return false;
- }
- if (dex_instance_fields1 != dex_instance_fields2) {
- std::string class_dump = DumpClasses(dex_file1, dex_class_def1, dex_file2, dex_class_def2);
- *error_msg = StringPrintf("Instance field count off: %zu vs %zu\n%s",
- dex_instance_fields1,
- dex_instance_fields2,
- class_dump.c_str());
- return false;
- }
-
- return true;
-}
-
-// Checks whether a the super-class changed from what we had at compile-time. This would
-// invalidate quickening.
-static bool CheckSuperClassChange(Handle<mirror::Class> klass,
- const DexFile& dex_file,
- const DexFile::ClassDef& class_def,
- ObjPtr<mirror::Class> super_class)
- REQUIRES_SHARED(Locks::mutator_lock_) {
- // Check for unexpected changes in the superclass.
- // Quick check 1) is the super_class class-loader the boot class loader? This always has
- // precedence.
- if (super_class->GetClassLoader() != nullptr &&
- // Quick check 2) different dex cache? Breaks can only occur for different dex files,
- // which is implied by different dex cache.
- klass->GetDexCache() != super_class->GetDexCache()) {
- // Now comes the expensive part: things can be broken if (a) the klass' dex file has a
- // definition for the super-class, and (b) the files are in separate oat files. The oat files
- // are referenced from the dex file, so do (b) first. Only relevant if we have oat files.
- const OatDexFile* class_oat_dex_file = dex_file.GetOatDexFile();
- const OatFile* class_oat_file = nullptr;
- if (class_oat_dex_file != nullptr) {
- class_oat_file = class_oat_dex_file->GetOatFile();
- }
-
- if (class_oat_file != nullptr) {
- const OatDexFile* loaded_super_oat_dex_file = super_class->GetDexFile().GetOatDexFile();
- const OatFile* loaded_super_oat_file = nullptr;
- if (loaded_super_oat_dex_file != nullptr) {
- loaded_super_oat_file = loaded_super_oat_dex_file->GetOatFile();
- }
-
- if (loaded_super_oat_file != nullptr && class_oat_file != loaded_super_oat_file) {
- // Now check (a).
- const DexFile::ClassDef* super_class_def = dex_file.FindClassDef(class_def.superclass_idx_);
- if (super_class_def != nullptr) {
- // Uh-oh, we found something. Do our check.
- std::string error_msg;
- if (!SimpleStructuralCheck(dex_file, *super_class_def,
- super_class->GetDexFile(), *super_class->GetClassDef(),
- &error_msg)) {
- // Print a warning to the log. This exception might be caught, e.g., as common in test
- // drivers. When the class is later tried to be used, we re-throw a new instance, as we
- // only save the type of the exception.
- LOG(WARNING) << "Incompatible structural change detected: " <<
- StringPrintf(
- "Structural change of %s is hazardous (%s at compile time, %s at runtime): %s",
- dex_file.PrettyType(super_class_def->class_idx_).c_str(),
- class_oat_file->GetLocation().c_str(),
- loaded_super_oat_file->GetLocation().c_str(),
- error_msg.c_str());
- ThrowIncompatibleClassChangeError(klass.Get(),
- "Structural change of %s is hazardous (%s at compile time, %s at runtime): %s",
- dex_file.PrettyType(super_class_def->class_idx_).c_str(),
- class_oat_file->GetLocation().c_str(),
- loaded_super_oat_file->GetLocation().c_str(),
- error_msg.c_str());
- return false;
- }
- }
- }
- }
- }
- return true;
-}
-
bool ClassLinker::LoadSuperAndInterfaces(Handle<mirror::Class> klass, const DexFile& dex_file) {
CHECK_EQ(mirror::Class::kStatusIdx, klass->GetStatus());
const DexFile::ClassDef& class_def = dex_file.GetClassDef(klass->GetDexClassDefIndex());
@@ -5650,11 +5472,6 @@
}
CHECK(super_class->IsResolved());
klass->SetSuperClass(super_class);
-
- if (!CheckSuperClassChange(klass, dex_file, class_def, super_class)) {
- DCHECK(Thread::Current()->IsExceptionPending());
- return false;
- }
}
const DexFile::TypeList* interfaces = dex_file.GetInterfacesList(class_def);
if (interfaces != nullptr) {
diff --git a/test/131-structural-change/build b/test/131-structural-change/build
deleted file mode 100755
index ff0da20..0000000
--- a/test/131-structural-change/build
+++ /dev/null
@@ -1,39 +0,0 @@
-#!/bin/bash
-#
-# 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.
-
-# Stop if something fails.
-set -e
-
-if [ ${USE_JACK} = "true" ]; then
- ${JACK} --output-dex . src
- zip $TEST_NAME.jar classes.dex
-
- ${JACK} --output-dex . src-ex
- zip ${TEST_NAME}-ex.jar classes.dex
-else
- mkdir classes
- ${JAVAC} -d classes `find src -name '*.java'`
-
- mkdir classes-ex
- ${JAVAC} -d classes-ex `find src-ex -name '*.java'`
-
- if [ ${NEED_DEX} = "true" ]; then
- ${DX} -JXmx256m --debug --dex --dump-to=classes.lst --output=classes.dex --dump-width=1000 classes
- zip $TEST_NAME.jar classes.dex
- ${DX} -JXmx256m --debug --dex --dump-to=classes-ex.lst --output=classes.dex --dump-width=1000 classes-ex
- zip ${TEST_NAME}-ex.jar classes.dex
- fi
-fi
diff --git a/test/131-structural-change/expected.txt b/test/131-structural-change/expected.txt
deleted file mode 100644
index 1d19278..0000000
--- a/test/131-structural-change/expected.txt
+++ /dev/null
@@ -1,3 +0,0 @@
-JNI_OnLoad called
-Should really reach here.
-Done.
diff --git a/test/131-structural-change/info.txt b/test/131-structural-change/info.txt
deleted file mode 100644
index 6d5817b..0000000
--- a/test/131-structural-change/info.txt
+++ /dev/null
@@ -1 +0,0 @@
-Check whether a structural change in a (non-native) multi-dex scenario is detected.
diff --git a/test/131-structural-change/run b/test/131-structural-change/run
deleted file mode 100755
index 63fdb8c..0000000
--- a/test/131-structural-change/run
+++ /dev/null
@@ -1,18 +0,0 @@
-#!/bin/bash
-#
-# 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.
-
-# Use secondary switch to add secondary dex file to class path.
-exec ${RUN} "${@}" --secondary
diff --git a/test/131-structural-change/src-ex/A.java b/test/131-structural-change/src-ex/A.java
deleted file mode 100644
index 800347b..0000000
--- a/test/131-structural-change/src-ex/A.java
+++ /dev/null
@@ -1,20 +0,0 @@
-/*
- * 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.
- */
-
-public class A {
- public void bar() {
- }
-}
diff --git a/test/131-structural-change/src-ex/B.java b/test/131-structural-change/src-ex/B.java
deleted file mode 100644
index 61369db..0000000
--- a/test/131-structural-change/src-ex/B.java
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * 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.
- */
-
-public class B extends A {
- public void test() {
- System.out.println("Should not reach this!");
- }
-}
diff --git a/test/131-structural-change/src/A.java b/test/131-structural-change/src/A.java
deleted file mode 100644
index b07de58..0000000
--- a/test/131-structural-change/src/A.java
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- * 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.
- */
-
-public class A {
- public void foo() {
- }
-
- public void bar() {
- }
-
- public void baz() {
- }
-}
diff --git a/test/131-structural-change/src/Main.java b/test/131-structural-change/src/Main.java
deleted file mode 100644
index c748899..0000000
--- a/test/131-structural-change/src/Main.java
+++ /dev/null
@@ -1,54 +0,0 @@
-/*
- * 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.io.File;
-import java.lang.reflect.Constructor;
-import java.lang.reflect.Method;
-
-/**
- * Structural hazard test.
- */
-public class Main {
- public static void main(String[] args) {
- System.loadLibrary(args[0]);
- new Main().run();
- }
-
- private void run() {
- try {
- Class<?> bClass = getClass().getClassLoader().loadClass("A");
- System.out.println("Should really reach here.");
- } catch (Exception e) {
- e.printStackTrace(System.out);
- }
-
- boolean haveOatFile = hasOatFile();
- boolean gotError = false;
- try {
- Class<?> bClass = getClass().getClassLoader().loadClass("B");
- } catch (IncompatibleClassChangeError icce) {
- gotError = true;
- } catch (Exception e) {
- e.printStackTrace(System.out);
- }
- if (haveOatFile ^ gotError) {
- System.out.println("Did not get expected error. " + haveOatFile + " " + gotError);
- }
- System.out.println("Done.");
- }
-
- private native static boolean hasOatFile();
-}
diff --git a/test/knownfailures.json b/test/knownfailures.json
index 8bdaeaa..7bfa417 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -143,15 +143,6 @@
"complete test) JDWP must be set up."]
},
{
- "tests": "131-structural-change",
- "variant": "debug",
- "description": ["131 is an old test. The functionality has been",
- "implemented at an earlier stage and is checked",
- "in tests 138. Blacklisted for debug builds since",
- "these builds have duplicate classes checks which",
- "punt to interpreter"]
- },
- {
"tests": "138-duplicate-classes-check",
"variant": "ndebug",
"description": ["Turned on for debug builds since debug builds have",