Improve codegen for referrer's class...
... for unresolved compiling class.
Test: Update test 727-checker-unresolved-class.
Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing
Test: testrunner.py --host --optimizing --interpreter --jvm -t 727
Bug: 161898207
Change-Id: I1a931179060ae435ca52d5a6eca3c641b9356c03
diff --git a/compiler/optimizing/instruction_builder.cc b/compiler/optimizing/instruction_builder.cc
index 3fe51ea..f917500 100644
--- a/compiler/optimizing/instruction_builder.cc
+++ b/compiler/optimizing/instruction_builder.cc
@@ -1253,7 +1253,10 @@
// need access checks, then we haven't resolved the method and the class may
// again be finalizable.
QuickEntrypointEnum entrypoint = kQuickAllocObjectInitialized;
- if (load_class->NeedsAccessCheck() || klass->IsFinalizable() || !klass->IsInstantiable()) {
+ if (load_class->NeedsAccessCheck() ||
+ klass == nullptr || // Finalizable/instantiable is unknown.
+ klass->IsFinalizable() ||
+ !klass->IsInstantiable()) {
entrypoint = kQuickAllocObjectWithChecks;
}
// We will always be able to resolve the string class since it is in the BCP.
@@ -2375,7 +2378,7 @@
ScopedObjectAccess soa(Thread::Current());
const DexFile& dex_file = *dex_compilation_unit_->GetDexFile();
Handle<mirror::Class> klass = ResolveClass(soa, type_index);
- bool needs_access_check = LoadClassNeedsAccessCheck(klass.Get());
+ bool needs_access_check = LoadClassNeedsAccessCheck(type_index, klass.Get());
return BuildLoadClass(type_index, dex_file, klass, dex_pc, needs_access_check);
}
@@ -2395,9 +2398,15 @@
}
}
- // Note: `klass` must be from `graph_->GetHandleCache()`.
+ // We cannot use the referrer's class load kind if we need to do an access check.
+ // If the `klass` is unresolved, we need access check with the exception of the referrer's
+ // class, see LoadClassNeedsAccessCheck(), so the `!needs_access_check` check is enough.
+ // Otherwise, also check if the `klass` is the same as the compiling class, which also
+ // conveniently rejects the case of unresolved compiling class.
bool is_referrers_class =
- (klass != nullptr) && (outer_compilation_unit_->GetCompilingClass().Get() == klass.Get());
+ !needs_access_check &&
+ (klass == nullptr || outer_compilation_unit_->GetCompilingClass().Get() == klass.Get());
+ // Note: `klass` must be from `graph_->GetHandleCache()`.
HLoadClass* load_class = new (allocator_) HLoadClass(
graph_->GetCurrentMethod(),
type_index,
@@ -2438,9 +2447,56 @@
return h_klass;
}
-bool HInstructionBuilder::LoadClassNeedsAccessCheck(ObjPtr<mirror::Class> klass) {
+bool HInstructionBuilder::LoadClassNeedsAccessCheck(dex::TypeIndex type_index,
+ ObjPtr<mirror::Class> klass) {
if (klass == nullptr) {
- return true;
+ // If the class is unresolved, we can avoid access checks only for references to
+ // the compiling class as determined by checking the descriptor and ClassLoader.
+ if (outer_compilation_unit_->GetCompilingClass() != nullptr) {
+ // Compiling class is resolved, so different from the unresolved class.
+ return true;
+ }
+ if (dex_compilation_unit_->GetClassLoader().Get() !=
+ outer_compilation_unit_->GetClassLoader().Get()) {
+ // Resolving the same descriptor in a different ClassLoader than the
+ // defining loader of the compiling class shall either fail to find
+ // the class definition, or find a different one.
+ // (Assuming no custom ClassLoader hierarchy with circular delegation.)
+ return true;
+ }
+ // Check if the class is the outer method's class.
+ // For the same dex file compare type indexes, otherwise descriptors.
+ const DexFile* outer_dex_file = outer_compilation_unit_->GetDexFile();
+ const DexFile* inner_dex_file = dex_compilation_unit_->GetDexFile();
+ const dex::ClassDef& outer_class_def =
+ outer_dex_file->GetClassDef(outer_compilation_unit_->GetClassDefIndex());
+ if (IsSameDexFile(*inner_dex_file, *outer_dex_file)) {
+ if (type_index != outer_class_def.class_idx_) {
+ return true;
+ }
+ } else {
+ uint32_t outer_utf16_length;
+ const char* outer_descriptor =
+ outer_dex_file->StringByTypeIdx(outer_class_def.class_idx_, &outer_utf16_length);
+ uint32_t target_utf16_length;
+ const char* target_descriptor =
+ inner_dex_file->StringByTypeIdx(type_index, &target_utf16_length);
+ if (outer_utf16_length != target_utf16_length ||
+ strcmp(outer_descriptor, target_descriptor) != 0) {
+ return true;
+ }
+ }
+ // For inlined methods we also need to check if the compiling class
+ // is public or in the same package as the inlined method's class.
+ if (dex_compilation_unit_ != outer_compilation_unit_ &&
+ (outer_class_def.access_flags_ & kAccPublic) == 0) {
+ DCHECK(dex_compilation_unit_->GetCompilingClass() != nullptr);
+ SamePackageCompare same_package(*outer_compilation_unit_);
+ if (!same_package(dex_compilation_unit_->GetCompilingClass().Get())) {
+ return true;
+ }
+ }
+ return false;
} else if (klass->IsPublic()) {
return false;
} else if (dex_compilation_unit_->GetCompilingClass() != nullptr) {
@@ -2472,7 +2528,7 @@
ScopedObjectAccess soa(Thread::Current());
const DexFile& dex_file = *dex_compilation_unit_->GetDexFile();
Handle<mirror::Class> klass = ResolveClass(soa, type_index);
- bool needs_access_check = LoadClassNeedsAccessCheck(klass.Get());
+ bool needs_access_check = LoadClassNeedsAccessCheck(type_index, klass.Get());
TypeCheckKind check_kind = HSharpening::ComputeTypeCheckKind(
klass.Get(), code_generator_, needs_access_check);
diff --git a/compiler/optimizing/instruction_builder.h b/compiler/optimizing/instruction_builder.h
index e75fa52..52d4bfc 100644
--- a/compiler/optimizing/instruction_builder.h
+++ b/compiler/optimizing/instruction_builder.h
@@ -232,7 +232,7 @@
Handle<mirror::Class> ResolveClass(ScopedObjectAccess& soa, dex::TypeIndex type_index)
REQUIRES_SHARED(Locks::mutator_lock_);
- bool LoadClassNeedsAccessCheck(ObjPtr<mirror::Class> klass)
+ bool LoadClassNeedsAccessCheck(dex::TypeIndex type_index, ObjPtr<mirror::Class> klass)
REQUIRES_SHARED(Locks::mutator_lock_);
// Builds a `HLoadMethodHandle` loading the given `method_handle_index`.
diff --git a/test/727-checker-unresolved-class/src-ex/resolved/PublicSubclassOfUnresolvedClass2.java b/test/727-checker-unresolved-class/src-ex/resolved/PublicSubclassOfUnresolvedClass2.java
index 8dcc2e4..887196a 100644
--- a/test/727-checker-unresolved-class/src-ex/resolved/PublicSubclassOfUnresolvedClass2.java
+++ b/test/727-checker-unresolved-class/src-ex/resolved/PublicSubclassOfUnresolvedClass2.java
@@ -49,7 +49,7 @@
}
/// CHECK-START: void resolved.PublicSubclassOfUnresolvedClass2.$noinline$testReferrersClass() builder (after)
- // CHECK: LoadClass class_name:resolved.PublicSubclassOfUnresolvedClass2 needs_access_check:false
+ /// CHECK: LoadClass class_name:resolved.PublicSubclassOfUnresolvedClass2 needs_access_check:false
static void $noinline$testReferrersClass() {
Class<?> c = PublicSubclassOfUnresolvedClass2.class;
}
diff --git a/test/727-checker-unresolved-class/src-ex2/resolved/PackagePrivateSubclassOfUnresolvedClass2.java b/test/727-checker-unresolved-class/src-ex2/resolved/PackagePrivateSubclassOfUnresolvedClass2.java
index 52e2448..a0c61ce 100644
--- a/test/727-checker-unresolved-class/src-ex2/resolved/PackagePrivateSubclassOfUnresolvedClass2.java
+++ b/test/727-checker-unresolved-class/src-ex2/resolved/PackagePrivateSubclassOfUnresolvedClass2.java
@@ -32,7 +32,7 @@
}
/// CHECK-START: void resolved.PackagePrivateSubclassOfUnresolvedClass2.$noinline$testReferrersClass() builder (after)
- // CHECK: LoadClass class_name:resolved.PackagePrivateSubclassOfUnresolvedClass2 needs_access_check:false
+ /// CHECK: LoadClass class_name:resolved.PackagePrivateSubclassOfUnresolvedClass2 needs_access_check:false
static void $noinline$testReferrersClass() {
Class<?> c = PackagePrivateSubclassOfUnresolvedClass2.class;
}
diff --git a/test/727-checker-unresolved-class/src-multidex/getters/GetUnresolvedPublicClassFromDifferentDexFile.java b/test/727-checker-unresolved-class/src-multidex/getters/GetUnresolvedPublicClassFromDifferentDexFile.java
new file mode 100644
index 0000000..fd63ced
--- /dev/null
+++ b/test/727-checker-unresolved-class/src-multidex/getters/GetUnresolvedPublicClassFromDifferentDexFile.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2020 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 getters;
+
+import resolved.PublicSubclassOfUnresolvedClass;
+import unresolved.UnresolvedPublicClass;
+import unresolved.UnresolvedPublicClazz;
+
+public class GetUnresolvedPublicClassFromDifferentDexFile {
+ // TODO: Make $inline$ when we relax the verifier. b/28313047
+ public static Class<?> get() {
+ return UnresolvedPublicClass.class;
+ }
+
+ // TODO: Make $inline$ when we relax the verifier. b/28313047
+ public static Class<?> getOtherClass() {
+ return PublicSubclassOfUnresolvedClass.class;
+ }
+
+ // TODO: Make $inline$ when we relax the verifier. b/28313047
+ public static Class<?> getOtherClassWithSameDescriptorLength() {
+ return UnresolvedPublicClazz.class;
+ }
+}
diff --git a/test/727-checker-unresolved-class/src/resolved/PublicSubclassOfUnresolvedClass.java b/test/727-checker-unresolved-class/src/resolved/PublicSubclassOfUnresolvedClass.java
index 168853c..2b8cbf8 100644
--- a/test/727-checker-unresolved-class/src/resolved/PublicSubclassOfUnresolvedClass.java
+++ b/test/727-checker-unresolved-class/src/resolved/PublicSubclassOfUnresolvedClass.java
@@ -46,7 +46,7 @@
}
/// CHECK-START: void resolved.PublicSubclassOfUnresolvedClass.$noinline$testReferrersClass() builder (after)
- // CHECK: LoadClass class_name:resolved.PublicSubclassOfUnresolvedClass needs_access_check:false
+ /// CHECK: LoadClass class_name:resolved.PublicSubclassOfUnresolvedClass needs_access_check:false
static void $noinline$testReferrersClass() {
Class<?> c = PublicSubclassOfUnresolvedClass.class;
}
diff --git a/test/727-checker-unresolved-class/src/unresolved/UnresolvedPublicClass.java b/test/727-checker-unresolved-class/src/unresolved/UnresolvedPublicClass.java
index 8bbe4f6..073b03e 100644
--- a/test/727-checker-unresolved-class/src/unresolved/UnresolvedPublicClass.java
+++ b/test/727-checker-unresolved-class/src/unresolved/UnresolvedPublicClass.java
@@ -17,6 +17,7 @@
package unresolved;
import getters.GetUnresolvedPublicClass;
+import getters.GetUnresolvedPublicClassFromDifferentDexFile;
import resolved.ResolvedPackagePrivateClass;
import resolved.ResolvedPublicSubclassOfPackagePrivateClass;
@@ -24,6 +25,9 @@
public static void $noinline$main() {
$noinline$testReferrersClass();
$noinline$testInlinedReferrersClass();
+ $noinline$testInlinedReferrersClassFromDifferentDexFile();
+ $noinline$testInlinedClassDescriptorCompare1();
+ $noinline$testInlinedClassDescriptorCompare2();
$noinline$testResolvedPublicClass();
$noinline$testResolvedPackagePrivateClass();
@@ -46,7 +50,7 @@
}
/// CHECK-START: void unresolved.UnresolvedPublicClass.$noinline$testReferrersClass() builder (after)
- // CHECK: LoadClass class_name:unresolved.UnresolvedPublicClass needs_access_check:false
+ /// CHECK: LoadClass class_name:unresolved.UnresolvedPublicClass needs_access_check:false
static void $noinline$testReferrersClass() {
Class<?> c = UnresolvedPublicClass.class;
}
@@ -58,6 +62,33 @@
Class<?> c = GetUnresolvedPublicClass.get();
}
+ /// CHECK-START: void unresolved.UnresolvedPublicClass.$noinline$testInlinedReferrersClassFromDifferentDexFile() inliner (after)
+ // CHECK: LoadClass class_name:unresolved.UnresolvedPublicClass needs_access_check:false
+ static void $noinline$testInlinedReferrersClassFromDifferentDexFile() {
+ // TODO: Make $inline$ and enable CHECK above when we relax the verifier. b/28313047
+ Class<?> c = GetUnresolvedPublicClassFromDifferentDexFile.get();
+ }
+
+ /// CHECK-START: void unresolved.UnresolvedPublicClass.$noinline$testInlinedClassDescriptorCompare1() inliner (after)
+ // CHECK: LoadClass class_name:resolved.PublicSubclassOfUnresolvedClass needs_access_check:true
+ static void $noinline$testInlinedClassDescriptorCompare1() {
+ // TODO: Make $inline$ and enable CHECK above when we relax the verifier. b/28313047
+ Class<?> c =
+ GetUnresolvedPublicClassFromDifferentDexFile.getOtherClass();
+ }
+
+ /// CHECK-START: void unresolved.UnresolvedPublicClass.$noinline$testInlinedClassDescriptorCompare2() inliner (after)
+ // CHECK: LoadClass class_name:unresolved.UnresolvedPublicClazz needs_access_check:true
+ static void $noinline$testInlinedClassDescriptorCompare2() {
+ // This is useful for code coverage of descriptor comparison
+ // implemented by first comparing the utf16 lengths and then
+ // checking strcmp(). Using these classes we cover the path
+ // where utf16 lengths match but string contents differ.
+ // TODO: Make $inline$ and enable CHECK above when we relax the verifier. b/28313047
+ Class<?> c =
+ GetUnresolvedPublicClassFromDifferentDexFile.getOtherClassWithSameDescriptorLength();
+ }
+
/// CHECK-START: void unresolved.UnresolvedPublicClass.$noinline$testResolvedPublicClass() builder (after)
/// CHECK: LoadClass class_name:resolved.ResolvedPublicSubclassOfPackagePrivateClass needs_access_check:false
static void $noinline$testResolvedPublicClass() {
diff --git a/test/727-checker-unresolved-class/src/unresolved/UnresolvedPublicClazz.java b/test/727-checker-unresolved-class/src/unresolved/UnresolvedPublicClazz.java
new file mode 100644
index 0000000..8fc8dd6
--- /dev/null
+++ b/test/727-checker-unresolved-class/src/unresolved/UnresolvedPublicClazz.java
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2020 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 unresolved;
+
+// Class with the same descriptor length as UnresolvedPublicClass.
+public class UnresolvedPublicClazz {
+}
diff --git a/test/727-checker-unresolved-class/src2/resolved/PackagePrivateSubclassOfUnresolvedClass.java b/test/727-checker-unresolved-class/src2/resolved/PackagePrivateSubclassOfUnresolvedClass.java
index ce94cce..2f8526d 100644
--- a/test/727-checker-unresolved-class/src2/resolved/PackagePrivateSubclassOfUnresolvedClass.java
+++ b/test/727-checker-unresolved-class/src2/resolved/PackagePrivateSubclassOfUnresolvedClass.java
@@ -29,7 +29,7 @@
}
/// CHECK-START: void resolved.PackagePrivateSubclassOfUnresolvedClass.$noinline$testReferrersClass() builder (after)
- // CHECK: LoadClass class_name:resolved.PackagePrivateSubclassOfUnresolvedClass needs_access_check:false
+ /// CHECK: LoadClass class_name:resolved.PackagePrivateSubclassOfUnresolvedClass needs_access_check:false
static void $noinline$testReferrersClass() {
Class<?> c = PackagePrivateSubclassOfUnresolvedClass.class;
}
@@ -47,6 +47,7 @@
/// CHECK-START: void resolved.PackagePrivateSubclassOfUnresolvedClass.$noinline$testInlinedReferrersClassFromSamePackage() inliner (after)
// CHECK: LoadClass class_name:resolved.PackagePrivateSubclassOfUnresolvedClass needs_access_check:false
static void $noinline$testInlinedReferrersClassFromSamePackage() {
+ // TODO: Make $inline$ and enable CHECK above when we relax the verifier. b/28313047
Class<?> c = GetPackagePrivateSubclassOfUnresolvedClassFromSamePackage.get();
}
}
diff --git a/test/727-checker-unresolved-class/src2/unresolved/UnresolvedPackagePrivateClass.java b/test/727-checker-unresolved-class/src2/unresolved/UnresolvedPackagePrivateClass.java
index 81a76a1..5c3df25 100644
--- a/test/727-checker-unresolved-class/src2/unresolved/UnresolvedPackagePrivateClass.java
+++ b/test/727-checker-unresolved-class/src2/unresolved/UnresolvedPackagePrivateClass.java
@@ -27,7 +27,7 @@
}
/// CHECK-START: void unresolved.UnresolvedPackagePrivateClass.$noinline$testReferrersClass() builder (after)
- // CHECK: LoadClass class_name:unresolved.UnresolvedPackagePrivateClass needs_access_check:false
+ /// CHECK: LoadClass class_name:unresolved.UnresolvedPackagePrivateClass needs_access_check:false
static void $noinline$testReferrersClass() {
Class<?> c = UnresolvedPackagePrivateClass.class;
}
diff --git a/test/952-invoke-custom/build b/test/952-invoke-custom/build
index a70fc20..de7e91d 100755
--- a/test/952-invoke-custom/build
+++ b/test/952-invoke-custom/build
@@ -27,14 +27,13 @@
set -e # Stop on error - the caller script may not have this set.
# Update arguments to add transformer and ASM to the compiler classpath.
- local args=()
local classpath="./transformer.jar:$ASM_JAR"
+ local args=(-cp $classpath)
while [ $# -ne 0 ] ; do
case $1 in
-cp|-classpath|--class-path)
shift
shift
- args+=(-cp $classpath)
;;
*)
args+=("$1")
diff --git a/test/etc/default-build b/test/etc/default-build
index 03e029f..7f9c818 100755
--- a/test/etc/default-build
+++ b/test/etc/default-build
@@ -379,19 +379,31 @@
make_dex classes
fi
else
+ if [ "${HAS_SRC}" = "true" -a "${HAS_SRC_MULTIDEX}" = "true" ]; then
+ # To allow circular references, compile src/ and src-multidex/ together
+ # and pass the output as class path argument. Replacement sources
+ # in src-art/ can replace symbols used by src-multidex but everything
+ # needed to compile src-multidex should be present in src/.
+ mkdir classes-tmp-all
+ javac_with_bootclasspath -implicit:none -d classes-tmp-all \
+ `find src -name '*.java'` \
+ `find src-multidex -name '*.java'`
+ src_tmp_all="-cp classes-tmp-all"
+ fi
+
if [ "${HAS_SRC}" = "true" ]; then
mkdir -p classes
- javac_with_bootclasspath -implicit:none -classpath src-multidex -d classes `find src -name '*.java'`
+ javac_with_bootclasspath -implicit:none $src_tmp_all -d classes `find src -name '*.java'`
fi
if [ "${HAS_SRC_ART}" = "true" ]; then
mkdir -p classes
- javac_with_bootclasspath -implicit:none -classpath src-multidex -d classes `find src-art -name '*.java'`
+ javac_with_bootclasspath -implicit:none $src_tmp_all -d classes `find src-art -name '*.java'`
fi
if [ "${HAS_SRC_MULTIDEX}" = "true" ]; then
mkdir classes2
- javac_with_bootclasspath -implicit:none -classpath src -d classes2 `find src-multidex -name '*.java'`
+ javac_with_bootclasspath -implicit:none $src_tmp_all -d classes2 `find src-multidex -name '*.java'`
if [ ${NEED_DEX} = "true" ]; then
make_dex classes2
fi
@@ -464,8 +476,14 @@
mkdir -p classes-tmp-for-ex
src_tmp_for_ex="-cp classes-tmp-for-ex"
fi
- if [[ "${HAS_SRC}" == "true" ]]; then
+ if [ "${HAS_SRC}" = "true" -a "${HAS_SRC_MULTIDEX}" = "true" ]; then
+ javac_with_bootclasspath -d classes-tmp-for-ex \
+ `find src -name '*.java'` \
+ `find src-multidex -name '*.java'`
+ elif [[ "${HAS_SRC}" == "true" ]]; then
javac_with_bootclasspath -d classes-tmp-for-ex `find src -name '*.java'`
+ elif [[ "${HAS_SRC_MULTIDEX}" == "true" ]]; then
+ javac_with_bootclasspath -d classes-tmp-for-ex `find src-multidex -name '*.java'`
fi
if [[ "${HAS_SRC_ART}" == "true" ]]; then
javac_with_bootclasspath -d classes-tmp-for-ex `find src-art -name '*.java'`