From 7cc3ae5705416bd8fc4b7096904e2871aa761e73 Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Tue, 7 Mar 2017 14:33:37 +0000 Subject: [PATCH] Return the right value in VerifyClass. We used to return kNoFailure when a class was already processed. But it could have had soft failures. Also remove IsCompileTimeVerified to avoid future confusions and introduce ShouldVerifyAtRuntime. Add some more checks to make sure we record the right things in the vdex file. bug: 33845394 test: verifier_deps_test test-art-host Change-Id: Iff11a96e825c85db416083413761981515f405b7 --- build/Android.gtest.mk | 11 +++++++++- compiler/driver/compiler_driver.cc | 22 ++++++++++++++++---- compiler/verifier_deps_test.cc | 23 ++++++++++++++++++--- runtime/class_linker.cc | 15 +++++++++----- runtime/mirror/class.h | 6 +++--- .../MySub1SoftVerificationFailure.smali | 16 +++++++++++++++ .../MySub2SoftVerificationFailure.smali | 16 +++++++++++++++ .../MySoftVerificationFailure.smali | 24 ++++++++++++++++++++++ 8 files changed, 117 insertions(+), 16 deletions(-) create mode 100644 test/VerifierDeps/MySub1SoftVerificationFailure.smali create mode 100644 test/VerifierDeps/MySub2SoftVerificationFailure.smali create mode 100644 test/VerifierDepsMulti/MySoftVerificationFailure.smali diff --git a/build/Android.gtest.mk b/build/Android.gtest.mk index b661e001c..c27f8dbe4 100644 --- a/build/Android.gtest.mk +++ b/build/Android.gtest.mk @@ -75,8 +75,11 @@ $(ART_TEST_TARGET_GTEST_MainStripped_DEX): $(ART_TEST_TARGET_GTEST_Main_DEX) $(call dexpreopt-remove-classes.dex,$@) ART_TEST_GTEST_VerifierDeps_SRC := $(abspath $(wildcard $(LOCAL_PATH)/VerifierDeps/*.smali)) +ART_TEST_GTEST_VerifierDepsMulti_SRC := $(abspath $(wildcard $(LOCAL_PATH)/VerifierDepsMulti/*.smali)) ART_TEST_HOST_GTEST_VerifierDeps_DEX := $(dir $(ART_TEST_HOST_GTEST_Main_DEX))$(subst Main,VerifierDeps,$(basename $(notdir $(ART_TEST_HOST_GTEST_Main_DEX))))$(suffix $(ART_TEST_HOST_GTEST_Main_DEX)) ART_TEST_TARGET_GTEST_VerifierDeps_DEX := $(dir $(ART_TEST_TARGET_GTEST_Main_DEX))$(subst Main,VerifierDeps,$(basename $(notdir $(ART_TEST_TARGET_GTEST_Main_DEX))))$(suffix $(ART_TEST_TARGET_GTEST_Main_DEX)) +ART_TEST_HOST_GTEST_VerifierDepsMulti_DEX := $(dir $(ART_TEST_HOST_GTEST_Main_DEX))$(subst Main,VerifierDepsMulti,$(basename $(notdir $(ART_TEST_HOST_GTEST_Main_DEX))))$(suffix $(ART_TEST_HOST_GTEST_Main_DEX)) +ART_TEST_TARGET_GTEST_VerifierDepsMulti_DEX := $(dir $(ART_TEST_TARGET_GTEST_Main_DEX))$(subst Main,VerifierDepsMulti,$(basename $(notdir $(ART_TEST_TARGET_GTEST_Main_DEX))))$(suffix $(ART_TEST_TARGET_GTEST_Main_DEX)) $(ART_TEST_HOST_GTEST_VerifierDeps_DEX): $(ART_TEST_GTEST_VerifierDeps_SRC) $(HOST_OUT_EXECUTABLES)/smali $(HOST_OUT_EXECUTABLES)/smali --output=$@ $(filter %.smali,$^) @@ -84,6 +87,12 @@ $(ART_TEST_HOST_GTEST_VerifierDeps_DEX): $(ART_TEST_GTEST_VerifierDeps_SRC) $(HO $(ART_TEST_TARGET_GTEST_VerifierDeps_DEX): $(ART_TEST_GTEST_VerifierDeps_SRC) $(HOST_OUT_EXECUTABLES)/smali $(HOST_OUT_EXECUTABLES)/smali --output=$@ $(filter %.smali,$^) +$(ART_TEST_HOST_GTEST_VerifierDepsMulti_DEX): $(ART_TEST_GTEST_VerifierDepsMulti_SRC) $(HOST_OUT_EXECUTABLES)/smali + $(HOST_OUT_EXECUTABLES)/smali --output=$@ $(filter %.smali,$^) + +$(ART_TEST_TARGET_GTEST_VerifierDepsMulti_DEX): $(ART_TEST_GTEST_VerifierDepsMulti_SRC) $(HOST_OUT_EXECUTABLES)/smali + $(HOST_OUT_EXECUTABLES)/smali --output=$@ $(filter %.smali,$^) + # Dex file dependencies for each gtest. ART_GTEST_dex2oat_environment_tests_DEX_DEPS := Main MainStripped MultiDex MultiDexModifiedSecondary Nested @@ -115,7 +124,7 @@ ART_GTEST_stub_test_DEX_DEPS := AllFields ART_GTEST_transaction_test_DEX_DEPS := Transaction ART_GTEST_type_lookup_table_test_DEX_DEPS := Lookup ART_GTEST_unstarted_runtime_test_DEX_DEPS := Nested -ART_GTEST_verifier_deps_test_DEX_DEPS := VerifierDeps MultiDex +ART_GTEST_verifier_deps_test_DEX_DEPS := VerifierDeps VerifierDepsMulti MultiDex ART_GTEST_dex_to_dex_decompiler_test_DEX_DEPS := VerifierDeps DexToDexDecompiler # The elf writer test has dependencies on core.oat. diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc index a5e4cb087..057e3c996 100644 --- a/compiler/driver/compiler_driver.cc +++ b/compiler/driver/compiler_driver.cc @@ -535,9 +535,8 @@ static optimizer::DexToDexCompilationLevel GetDexToDexCompilationLevel( if (klass->IsVerified()) { // Class is verified so we can enable DEX-to-DEX compilation for performance. return max_level; - } else if (klass->IsCompileTimeVerified()) { + } else if (klass->ShouldVerifyAtRuntime()) { // Class verification has soft-failed. Anyway, ensure at least correctness. - DCHECK_EQ(klass->GetStatus(), mirror::Class::kStatusRetryVerificationAtRuntime); return optimizer::DexToDexCompilationLevel::kRequired; } else { // Class verification has failed: do not run DEX-to-DEX compilation. @@ -964,7 +963,7 @@ static void EnsureVerifiedOrVerifyAtRuntime(jobject jclass_loader, if (cls == nullptr) { soa.Self()->ClearException(); } else if (&cls->GetDexFile() == dex_file) { - DCHECK(cls->IsErroneous() || cls->IsVerified() || cls->IsCompileTimeVerified()) + DCHECK(cls->IsErroneous() || cls->IsVerified() || cls->ShouldVerifyAtRuntime()) << cls->PrettyClass() << " " << cls->GetStatus(); } @@ -2160,6 +2159,14 @@ class VerifyClassVisitor : public CompilationVisitor { LOG(ERROR) << "Verification failed on class " << PrettyDescriptor(descriptor) << " because: " << error_msg; manager_->GetCompiler()->SetHadHardVerifierFailure(); + } else { + // Force a soft failure for the VerifierDeps. This is a sanity 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::MethodVerifier::kSoftFailure || + failure_kind == verifier::MethodVerifier::kNoFailure) + << failure_kind; + failure_kind = verifier::MethodVerifier::kSoftFailure; } } else if (!SkipClass(jclass_loader, dex_file, klass.Get())) { CHECK(klass->IsResolved()) << klass->PrettyClass(); @@ -2172,7 +2179,7 @@ class VerifyClassVisitor : public CompilationVisitor { manager_->GetCompiler()->SetHadHardVerifierFailure(); } - CHECK(klass->IsCompileTimeVerified() || klass->IsErroneous()) + CHECK(klass->ShouldVerifyAtRuntime() || klass->IsVerified() || klass->IsErroneous()) << klass->PrettyDescriptor() << ": state=" << klass->GetStatus(); // It is *very* problematic if there are verification errors in the boot classpath. For example, @@ -2186,6 +2193,13 @@ class VerifyClassVisitor : public CompilationVisitor { DCHECK(klass->IsVerified()) << "Boot classpath class " << klass->PrettyClass() << " failed to fully verify: state= " << klass->GetStatus(); } + if (klass->IsVerified()) { + DCHECK_EQ(failure_kind, verifier::MethodVerifier::kNoFailure); + } else if (klass->ShouldVerifyAtRuntime()) { + DCHECK_EQ(failure_kind, verifier::MethodVerifier::kSoftFailure); + } else { + DCHECK_EQ(failure_kind, verifier::MethodVerifier::kHardFailure); + } } } else { // Make the skip a soft failure, essentially being considered as verify at runtime. diff --git a/compiler/verifier_deps_test.cc b/compiler/verifier_deps_test.cc index c892b25ed..01c33591e 100644 --- a/compiler/verifier_deps_test.cc +++ b/compiler/verifier_deps_test.cc @@ -246,9 +246,13 @@ class VerifierDepsTest : public CommonCompilerTest { } bool HasUnverifiedClass(const std::string& cls) { - const DexFile::TypeId* type_id = primary_dex_file_->FindTypeId(cls.c_str()); + return HasUnverifiedClass(cls, *primary_dex_file_); + } + + bool HasUnverifiedClass(const std::string& cls, const DexFile& dex_file) { + const DexFile::TypeId* type_id = dex_file.FindTypeId(cls.c_str()); DCHECK(type_id != nullptr); - dex::TypeIndex index = primary_dex_file_->GetIndexForTypeId(*type_id); + dex::TypeIndex index = dex_file.GetIndexForTypeId(*type_id); for (const auto& dex_dep : verifier_deps_->dex_deps_) { for (dex::TypeIndex entry : dex_dep.second->unverified_classes_) { if (index == entry) { @@ -1141,7 +1145,7 @@ TEST_F(VerifierDepsTest, UnverifiedClasses) { // Test that a class with hard failure is recorded. ASSERT_TRUE(HasUnverifiedClass("LMyVerificationFailure;")); // Test that a class with unresolved super is recorded. - ASSERT_FALSE(HasUnverifiedClass("LMyClassWithNoSuper;")); + ASSERT_TRUE(HasUnverifiedClass("LMyClassWithNoSuper;")); // Test that a class with unresolved super and hard failure is recorded. ASSERT_TRUE(HasUnverifiedClass("LMyClassWithNoSuperButFailures;")); } @@ -1511,5 +1515,18 @@ TEST_F(VerifierDepsTest, CompilerDriver) { } } +TEST_F(VerifierDepsTest, MultiDexVerification) { + VerifyDexFile("VerifierDepsMulti"); + ASSERT_EQ(NumberOfCompiledDexFiles(), 2u); + + ASSERT_TRUE(HasUnverifiedClass("LMySoftVerificationFailure;", *dex_files_[1])); + ASSERT_TRUE(HasUnverifiedClass("LMySub1SoftVerificationFailure;", *dex_files_[0])); + ASSERT_TRUE(HasUnverifiedClass("LMySub2SoftVerificationFailure;", *dex_files_[0])); + + std::vector buffer; + verifier_deps_->Encode(dex_files_, &buffer); + ASSERT_FALSE(buffer.empty()); +} + } // namespace verifier } // namespace art diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc index d23271433..360d4aba0 100644 --- a/runtime/class_linker.cc +++ b/runtime/class_linker.cc @@ -3900,8 +3900,10 @@ bool ClassLinker::AttemptSupertypeVerification(Thread* self, if (!supertype->IsVerified() && !supertype->IsErroneous()) { VerifyClass(self, supertype); } - if (supertype->IsCompileTimeVerified()) { - // Either we are verified or we soft failed and need to retry at runtime. + + if (supertype->IsVerified() || supertype->ShouldVerifyAtRuntime()) { + // The supertype is either verified, or we soft failed at AOT time. + DCHECK(supertype->IsVerified() || Runtime::Current()->IsAotCompiler()); return true; } // If we got this far then we have a hard failure. @@ -3967,13 +3969,16 @@ verifier::MethodVerifier::FailureKind ClassLinker::VerifyClass( return verifier::MethodVerifier::kHardFailure; } - // Don't attempt to re-verify if already sufficiently verified. + // Don't attempt to re-verify if already verified. if (klass->IsVerified()) { EnsureSkipAccessChecksMethods(klass, image_pointer_size_); return verifier::MethodVerifier::kNoFailure; } - if (klass->IsCompileTimeVerified() && Runtime::Current()->IsAotCompiler()) { - return verifier::MethodVerifier::kNoFailure; + + // For AOT, don't attempt to re-verify if we have already found we should + // verify at runtime. + if (Runtime::Current()->IsAotCompiler() && klass->ShouldVerifyAtRuntime()) { + return verifier::MethodVerifier::kSoftFailure; } if (klass->GetStatus() == mirror::Class::kStatusResolved) { diff --git a/runtime/mirror/class.h b/runtime/mirror/class.h index d34f09c72..b68eedcb1 100644 --- a/runtime/mirror/class.h +++ b/runtime/mirror/class.h @@ -206,10 +206,10 @@ class MANAGED Class FINAL : public Object { return status >= kStatusResolved || status == kStatusErrorResolved; } - // Returns true if the class was compile-time verified. + // Returns true if the class should be verified at runtime. template - bool IsCompileTimeVerified() REQUIRES_SHARED(Locks::mutator_lock_) { - return GetStatus() >= kStatusRetryVerificationAtRuntime; + bool ShouldVerifyAtRuntime() REQUIRES_SHARED(Locks::mutator_lock_) { + return GetStatus() == kStatusRetryVerificationAtRuntime; } // Returns true if the class has been verified. diff --git a/test/VerifierDeps/MySub1SoftVerificationFailure.smali b/test/VerifierDeps/MySub1SoftVerificationFailure.smali new file mode 100644 index 000000000..8123394e8 --- /dev/null +++ b/test/VerifierDeps/MySub1SoftVerificationFailure.smali @@ -0,0 +1,16 @@ +# Copyright (C) 2017 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 LMySub1SoftVerificationFailure; +.super LMySoftVerificationFailure; diff --git a/test/VerifierDeps/MySub2SoftVerificationFailure.smali b/test/VerifierDeps/MySub2SoftVerificationFailure.smali new file mode 100644 index 000000000..8d003236c --- /dev/null +++ b/test/VerifierDeps/MySub2SoftVerificationFailure.smali @@ -0,0 +1,16 @@ +# Copyright (C) 2017 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 LMySub2SoftVerificationFailure; +.super LMySoftVerificationFailure; diff --git a/test/VerifierDepsMulti/MySoftVerificationFailure.smali b/test/VerifierDepsMulti/MySoftVerificationFailure.smali new file mode 100644 index 000000000..6b56a3b6d --- /dev/null +++ b/test/VerifierDepsMulti/MySoftVerificationFailure.smali @@ -0,0 +1,24 @@ +# Copyright (C) 2017 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 LMySoftVerificationFailure; +.super Ljava/lang/Object; + +.method public final foo()V + .registers 1 + sget-object v0, LMySoftVerificationFailure;->error:LUnknownType; + throw v0 +.end method + +.field public static error:LUnknownType; -- 2.11.0