Refactor code around the verifier.
- For classes that are not resolved, we were doing verification but were
not recording the outcome - fix that.
- Clear the verifier deps (for space reasons) only when a class is hard
fail or needs to be verified at runtime.
Test: test.py
Bug: 176960283
Change-Id: If19c0e6171ce945a6cd56ba4a42fbb8e2a5ccee7
diff --git a/dex2oat/dex2oat_vdex_test.cc b/dex2oat/dex2oat_vdex_test.cc
index ce82c5d..eb9ae3f 100644
--- a/dex2oat/dex2oat_vdex_test.cc
+++ b/dex2oat/dex2oat_vdex_test.cc
@@ -178,15 +178,12 @@
ASSERT_TRUE(HasVerifiedClass(deps, "LAccessPublicStaticMethod;", *dex_file));
ASSERT_TRUE(HasVerifiedClass(deps, "LAccessPublicStaticField;", *dex_file));
- // Verify NON public API usage. The classes should not ve verified.
- ASSERT_FALSE(HasVerifiedClass(deps, "LAccessNonPublicCtor;", *dex_file));
- ASSERT_FALSE(HasVerifiedClass(deps, "LAccessNonPublicMethod;", *dex_file));
- ASSERT_FALSE(HasVerifiedClass(deps, "LAccessNonPublicMethodFromParent;", *dex_file));
- ASSERT_FALSE(HasVerifiedClass(deps, "LAccessNonPublicStaticMethod;", *dex_file));
-
- // Accessing unresolved static fields do not lead to class verification
- // failures.
- // The linker will just throw NoSuchFieldError at runtime.
+ // Verify NON public API usage. The classes should be verified, but will run
+ // with access checks.
+ ASSERT_TRUE(HasVerifiedClass(deps, "LAccessNonPublicCtor;", *dex_file));
+ ASSERT_TRUE(HasVerifiedClass(deps, "LAccessNonPublicMethod;", *dex_file));
+ ASSERT_TRUE(HasVerifiedClass(deps, "LAccessNonPublicMethodFromParent;", *dex_file));
+ ASSERT_TRUE(HasVerifiedClass(deps, "LAccessNonPublicStaticMethod;", *dex_file));
ASSERT_TRUE(HasVerifiedClass(deps, "LAccessNonPublicStaticField;", *dex_file));
// Compile again without public API stubs but with the previously generated vdex.
@@ -208,10 +205,10 @@
ASSERT_TRUE(HasVerifiedClass(deps2, "LAccessPublicStaticMethod;", *dex_file));
ASSERT_TRUE(HasVerifiedClass(deps2, "LAccessPublicStaticField;", *dex_file));
- ASSERT_FALSE(HasVerifiedClass(deps2, "LAccessNonPublicCtor;", *dex_file)) << output_;
- ASSERT_FALSE(HasVerifiedClass(deps2, "LAccessNonPublicMethod;", *dex_file));
- ASSERT_FALSE(HasVerifiedClass(deps2, "LAccessNonPublicMethodFromParent;", *dex_file));
- ASSERT_FALSE(HasVerifiedClass(deps2, "LAccessNonPublicStaticMethod;", *dex_file));
+ ASSERT_TRUE(HasVerifiedClass(deps2, "LAccessNonPublicCtor;", *dex_file)) << output_;
+ ASSERT_TRUE(HasVerifiedClass(deps2, "LAccessNonPublicMethod;", *dex_file));
+ ASSERT_TRUE(HasVerifiedClass(deps2, "LAccessNonPublicMethodFromParent;", *dex_file));
+ ASSERT_TRUE(HasVerifiedClass(deps2, "LAccessNonPublicStaticMethod;", *dex_file));
ASSERT_TRUE(HasVerifiedClass(deps2, "LAccessNonPublicStaticField;", *dex_file));
}
diff --git a/dex2oat/driver/compiler_driver.cc b/dex2oat/driver/compiler_driver.cc
index a43562a..3175ccb 100644
--- a/dex2oat/driver/compiler_driver.cc
+++ b/dex2oat/driver/compiler_driver.cc
@@ -2014,6 +2014,7 @@
hs.NewHandle(soa.Decode<mirror::ClassLoader>(jclass_loader)));
Handle<mirror::Class> klass(
hs.NewHandle(class_linker->FindClass(soa.Self(), descriptor, class_loader)));
+ ClassReference ref(manager_->GetDexFile(), class_def_index);
verifier::FailureKind failure_kind;
if (klass == nullptr) {
CHECK(soa.Self()->IsExceptionPending());
@@ -2038,19 +2039,25 @@
log_level_,
sdk_version_,
&error_msg);
- if (failure_kind == verifier::FailureKind::kHardFailure) {
- LOG(ERROR) << "Verification failed on class " << PrettyDescriptor(descriptor)
- << " because: " << error_msg;
- manager_->GetCompiler()->SetHadHardVerifierFailure();
- } else if (failure_kind == verifier::FailureKind::kSoftFailure) {
- manager_->GetCompiler()->AddSoftVerifierFailure();
- } else {
- // Force a soft failure for the VerifierDeps. This is a validity measure, as
- // the vdex file already records that the class hasn't been resolved. It avoids
- // trying to do future verification optimizations when processing the vdex file.
- DCHECK(failure_kind == verifier::FailureKind::kNoFailure ||
- failure_kind == verifier::FailureKind::kAccessChecksFailure) << failure_kind;
- failure_kind = verifier::FailureKind::kSoftFailure;
+ switch (failure_kind) {
+ case verifier::FailureKind::kHardFailure: {
+ LOG(ERROR) << "Verification failed on class " << PrettyDescriptor(descriptor)
+ << " because: " << error_msg;
+ manager_->GetCompiler()->SetHadHardVerifierFailure();
+ break;
+ }
+ case verifier::FailureKind::kSoftFailure: {
+ manager_->GetCompiler()->AddSoftVerifierFailure();
+ break;
+ }
+ case verifier::FailureKind::kAccessChecksFailure: {
+ manager_->GetCompiler()->RecordClassStatus(ref, ClassStatus::kVerifiedNeedsAccessChecks);
+ break;
+ }
+ case verifier::FailureKind::kNoFailure: {
+ manager_->GetCompiler()->RecordClassStatus(ref, ClassStatus::kVerified);
+ break;
+ }
}
} else if (&klass->GetDexFile() != &dex_file) {
// Skip a duplicate class (as the resolved class is from another, earlier dex file).
@@ -2075,7 +2082,6 @@
<< klass->PrettyDescriptor() << ": state=" << klass->GetStatus();
// Class has a meaningful status for the compiler now, record it.
- ClassReference ref(manager_->GetDexFile(), class_def_index);
ClassStatus status = klass->GetStatus();
if (status == ClassStatus::kInitialized) {
// Initialized classes shall be visibly initialized when loaded from the image.
diff --git a/dex2oat/verifier_deps_test.cc b/dex2oat/verifier_deps_test.cc
index f937978..f37248d 100644
--- a/dex2oat/verifier_deps_test.cc
+++ b/dex2oat/verifier_deps_test.cc
@@ -252,13 +252,21 @@
return dex_file.GetIndexForClassDef(*class_def);
}
+ bool HasVerifiedClass(const std::string& cls) {
+ return HasVerifiedClass(cls, *primary_dex_file_);
+ }
+
bool HasUnverifiedClass(const std::string& cls) {
- return HasUnverifiedClass(cls, *primary_dex_file_);
+ return !HasVerifiedClass(cls, *primary_dex_file_);
}
bool HasUnverifiedClass(const std::string& cls, const DexFile& dex_file) {
+ return !HasVerifiedClass(cls, dex_file);
+ }
+
+ bool HasVerifiedClass(const std::string& cls, const DexFile& dex_file) {
uint16_t class_def_idx = GetClassDefIndex(cls, dex_file);
- return !verifier_deps_->GetVerifiedClasses(dex_file)[class_def_idx];
+ return verifier_deps_->GetVerifiedClasses(dex_file)[class_def_idx];
}
// Iterates over all assignability records and tries to find an entry which
@@ -526,10 +534,10 @@
ASSERT_TRUE(HasUnverifiedClass("LMain;"));
// Test that a class with hard failure is recorded.
ASSERT_TRUE(HasUnverifiedClass("LMyVerificationFailure;"));
- // Test that a class with unresolved super is recorded.
- ASSERT_TRUE(HasUnverifiedClass("LMyClassWithNoSuper;"));
// Test that a class with unresolved super and hard failure is recorded.
ASSERT_TRUE(HasUnverifiedClass("LMyClassWithNoSuperButFailures;"));
+ // Test that a class with unresolved super can be verified.
+ ASSERT_TRUE(HasVerifiedClass("LMyClassWithNoSuper;"));
}
TEST_F(VerifierDepsTest, UnverifiedOrder) {
diff --git a/runtime/module_exclusion_test.cc b/runtime/module_exclusion_test.cc
index 1f5f87e..b62fc01 100644
--- a/runtime/module_exclusion_test.cc
+++ b/runtime/module_exclusion_test.cc
@@ -178,14 +178,14 @@
jar_name,
&error_msg));
ASSERT_TRUE(odex_file != nullptr) << error_msg;
- // Check that no classes have been resolved.
+ // Check that no classes have been initialized.
for (const OatDexFile* oat_dex_file : odex_file->GetOatDexFiles()) {
std::unique_ptr<const DexFile> dex_file = oat_dex_file->OpenDexFile(&error_msg);
ASSERT_TRUE(dex_file != nullptr);
for (size_t i = 0, num_class_defs = dex_file->NumClassDefs(); i != num_class_defs; ++i) {
ClassStatus status = oat_dex_file->GetOatClass(i).GetStatus();
ASSERT_FALSE(mirror::Class::IsErroneous(status));
- ASSERT_LT(status, ClassStatus::kResolved);
+ ASSERT_LE(status, ClassStatus::kVerified);
}
}
}
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 8ab588e..a6c8817 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -5235,6 +5235,7 @@
}
bool set_dont_compile = false;
+ bool must_count_locks = false;
if (verifier.failures_.size() != 0) {
if (VLOG_IS_ON(verifier)) {
verifier.DumpFailures(VLOG_STREAM(verifier) << "Soft verification failures in "
@@ -5249,31 +5250,26 @@
} else {
result.kind = FailureKind::kSoftFailure;
}
- if (method != nullptr &&
- !CanCompilerHandleVerificationFailure(verifier.encountered_failure_types_)) {
+ if (!CanCompilerHandleVerificationFailure(verifier.encountered_failure_types_)) {
set_dont_compile = true;
}
- }
- if (method != nullptr) {
- if (verifier.HasInstructionThatWillThrow()) {
- set_dont_compile = true;
- if (aot_mode && (callbacks != nullptr) && !callbacks->IsBootImage()) {
- // When compiling apps, make HasInstructionThatWillThrow a soft error to trigger
- // re-verification at runtime.
- // The dead code after the throw is not verified and might be invalid. This may cause
- // the JIT compiler to crash since it assumes that all the code is valid.
- //
- // There's a strong assumption that the entire boot image is verified and all its dex
- // code is valid (even the dead and unverified one). As such this is done only for apps.
- // (CompilerDriver DCHECKs in VerifyClassVisitor that methods from boot image are
- // fully verified).
- result.kind = FailureKind::kSoftFailure;
- }
- }
- bool must_count_locks = false;
if ((verifier.encountered_failure_types_ & VerifyError::VERIFY_ERROR_LOCKING) != 0) {
must_count_locks = true;
}
+ }
+
+ if (verifier.HasInstructionThatWillThrow()) {
+ // The dead code after the throw is not verified and might be invalid. This may cause
+ // the JIT compiler to crash since it assumes that all the code is valid.
+ set_dont_compile = true;
+ if (aot_mode) {
+ // Make HasInstructionThatWillThrow a soft error to trigger
+ // re-verification at runtime.
+ result.kind = FailureKind::kSoftFailure;
+ }
+ }
+
+ if (method != nullptr) {
verifier_callback->SetDontCompile(method, set_dont_compile);
verifier_callback->SetMustCountLocks(method, must_count_locks);
}
diff --git a/runtime/verifier/verifier_deps.cc b/runtime/verifier/verifier_deps.cc
index 9132745..e104d34 100644
--- a/runtime/verifier/verifier_deps.cc
+++ b/runtime/verifier/verifier_deps.cc
@@ -290,12 +290,20 @@
FailureKind failure_kind) {
VerifierDeps* thread_deps = GetThreadLocalVerifierDeps();
if (thread_deps != nullptr) {
- if (failure_kind == FailureKind::kNoFailure) {
- thread_deps->RecordClassVerified(dex_file, class_def);
- } else {
- DexFileDeps* dex_deps = thread_deps->GetDexFileDeps(dex_file);
- uint16_t index = dex_file.GetIndexForClassDef(class_def);
- dex_deps->assignable_types_[index].clear();
+ switch (failure_kind) {
+ case verifier::FailureKind::kHardFailure:
+ case verifier::FailureKind::kSoftFailure: {
+ // Class will be verified at runtime.
+ DexFileDeps* dex_deps = thread_deps->GetDexFileDeps(dex_file);
+ uint16_t index = dex_file.GetIndexForClassDef(class_def);
+ dex_deps->assignable_types_[index].clear();
+ break;
+ }
+ case verifier::FailureKind::kAccessChecksFailure:
+ case verifier::FailureKind::kNoFailure: {
+ thread_deps->RecordClassVerified(dex_file, class_def);
+ break;
+ }
}
}
}