Don't fail fast verification if one class fails.

Just mark that class as unverified, and verify it at runtime.

Test: test.py
Bug: 289910991
Change-Id: I1a2f593d0f3a9495ea6dc69cf11a40277d5cd5ca

5 files changed, 53 insertions, 73 deletions

diff --git a/dex2oat/driver/compiler_driver.cc b/dex2oat/driver/compiler_driver.cc
index f7f86ed4fc..6423cc3ead 100644
--- a/dex2oat/driver/compiler_driver.cc
+++ b/dex2oat/driver/compiler_driver.cc
@@ -1844,17 +1844,10 @@ bool CompilerDriver::FastVerify(jobject jclass_loader,
       hs.NewHandle(soa.Decode<mirror::ClassLoader>(jclass_loader)));
   std::string error_msg;
 
-  if (!verifier_deps->ValidateDependencies(
+  verifier_deps->ValidateDependenciesAndUpdateStatus(
       soa.Self(),
       class_loader,
-      dex_files,
-      &error_msg)) {
-    // Clear the information we have as we are going to re-verify and we do not
-    // want to keep that a class is verified.
-    verifier_deps->ClearData(dex_files);
-    LOG(WARNING) << "Fast verification failed: " << error_msg;
-    return false;
-  }
+      dex_files);
 
   bool compiler_only_verifies =
       !GetCompilerOptions().IsAnyCompilationEnabled() &&
diff --git a/dex2oat/verifier_deps_test.cc b/dex2oat/verifier_deps_test.cc
index a1e93b750e..1ede9e07bc 100644
--- a/dex2oat/verifier_deps_test.cc
+++ b/dex2oat/verifier_deps_test.cc
@@ -310,7 +310,7 @@ class VerifierDepsTest : public CommonCompilerDriverTest {
   // Load the dex file again with a new class loader, decode the VerifierDeps
   // in `buffer`, allow the caller to modify the deps and then run validation.
   template<typename Fn>
-  bool RunValidation(Fn fn, const std::vector<uint8_t>& buffer, std::string* error_msg) {
+  bool RunValidation(Fn fn, const std::vector<uint8_t>& buffer) {
     ScopedObjectAccess soa(Thread::Current());
 
     jobject second_loader = LoadDex("VerifierDeps");
@@ -329,10 +329,9 @@ class VerifierDepsTest : public CommonCompilerDriverTest {
     Handle<mirror::ClassLoader> new_class_loader =
         hs.NewHandle<mirror::ClassLoader>(soa.Decode<mirror::ClassLoader>(second_loader));
 
-    return decoded_deps.ValidateDependencies(soa.Self(),
-                                             new_class_loader,
-                                             second_dex_files,
-                                             error_msg);
+    return decoded_deps.ValidateDependenciesAndUpdateStatus(soa.Self(),
+                                                            new_class_loader,
+                                                            second_dex_files);
   }
 
   std::unique_ptr<verifier::VerifierDeps> verifier_deps_;
@@ -564,8 +563,6 @@ TEST_F(VerifierDepsTest, UnverifiedOrder) {
 }
 
 TEST_F(VerifierDepsTest, VerifyDeps) {
-  std::string error_msg;
-
   VerifyDexFile();
   ASSERT_EQ(1u, NumberOfCompiledDexFiles());
   ASSERT_TRUE(HasEachKindOfRecord());
@@ -579,8 +576,7 @@ TEST_F(VerifierDepsTest, VerifyDeps) {
   ASSERT_FALSE(buffer.empty());
 
   // Check that dependencies are satisfied after decoding `buffer`.
-  ASSERT_TRUE(RunValidation([](VerifierDeps::DexFileDeps&) {}, buffer, &error_msg))
-      << error_msg;
+  ASSERT_TRUE(RunValidation([](VerifierDeps::DexFileDeps&) {}, buffer));
 }
 
 TEST_F(VerifierDepsTest, CompilerDriver) {
diff --git a/runtime/verifier/reg_type-inl.h b/runtime/verifier/reg_type-inl.h
index 8db2fe669a..c5311e6e62 100644
--- a/runtime/verifier/reg_type-inl.h
+++ b/runtime/verifier/reg_type-inl.h
@@ -127,6 +127,10 @@ inline bool RegType::AssignableFrom(const RegType& lhs,
           // For unresolved types, we don't know if they are assignable, and the
           // verifier will continue assuming they are. We need to record that.
           if (verifier != nullptr) {
+            // Note that if `rhs` is an interface type, `lhs` may be j.l.Object
+            // and if the assignability check is not strict, then this should be
+            // OK. However we don't encode strictness in the verifier deps, and
+            // such a situation will force a full verification.
             VerifierDeps::MaybeRecordAssignability(verifier->GetVerifierDeps(),
                                                    verifier->GetDexFile(),
                                                    verifier->GetClassDef(),
diff --git a/runtime/verifier/verifier_deps.cc b/runtime/verifier/verifier_deps.cc
index e09e007d93..f5df8ddf72 100644
--- a/runtime/verifier/verifier_deps.cc
+++ b/runtime/verifier/verifier_deps.cc
@@ -673,17 +673,18 @@ void VerifierDeps::Dump(VariableIndentationOutputStream* vios) const {
   }
 }
 
-bool VerifierDeps::ValidateDependencies(Thread* self,
-                                        Handle<mirror::ClassLoader> class_loader,
-                                        const std::vector<const DexFile*>& dex_files,
-                                        /* out */ std::string* error_msg) const {
+bool VerifierDeps::ValidateDependenciesAndUpdateStatus(
+    Thread* self,
+    Handle<mirror::ClassLoader> class_loader,
+    const std::vector<const DexFile*>& dex_files) {
+  bool all_validated = true;
   for (const auto* dex_file : dex_files) {
-    const DexFileDeps* my_deps = GetDexFileDeps(*dex_file);
-    if (!VerifyDexFile(class_loader, *dex_file, *my_deps, self, error_msg)) {
-      return false;
+    DexFileDeps* my_deps = GetDexFileDeps(*dex_file);
+    if (!VerifyDexFileAndUpdateStatus(class_loader, *dex_file, *my_deps, self)) {
+      all_validated = false;
     }
   }
-  return true;
+  return all_validated;
 }
 
 // TODO: share that helper with other parts of the compiler that have
@@ -701,16 +702,21 @@ static ObjPtr<mirror::Class> FindClassAndClearException(ClassLinker* class_linke
   return result;
 }
 
-bool VerifierDeps::VerifyAssignability(Handle<mirror::ClassLoader> class_loader,
-                                       const DexFile& dex_file,
-                                       const std::vector<std::set<TypeAssignability>>& assignables,
-                                       Thread* self,
-                                       /* out */ std::string* error_msg) const {
+bool VerifierDeps::VerifyDexFileAndUpdateStatus(
+    Handle<mirror::ClassLoader> class_loader,
+    const DexFile& dex_file,
+    DexFileDeps& deps,
+    Thread* self) {
   StackHandleScope<2> hs(self);
+  const std::vector<std::set<TypeAssignability>>& assignables = deps.assignable_types_;
   ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
   MutableHandle<mirror::Class> source(hs.NewHandle<mirror::Class>(nullptr));
   MutableHandle<mirror::Class> destination(hs.NewHandle<mirror::Class>(nullptr));
 
+  uint32_t class_def_index = 0u;
+  bool all_validated = true;
+  uint32_t number_of_warnings = 0;
+  static constexpr uint32_t kMaxWarnings = 5;
   for (const auto& vec : assignables) {
     for (const auto& entry : vec) {
       const std::string& destination_desc = GetStringFromId(dex_file, entry.GetDestination());
@@ -728,31 +734,20 @@ bool VerifierDeps::VerifyAssignability(Handle<mirror::ClassLoader> class_loader,
 
       DCHECK(destination->IsResolved() && source->IsResolved());
       if (!destination->IsAssignableFrom(source.Get())) {
-        *error_msg = ART_FORMAT("Class {} not assignable from {}", destination_desc, source_desc);
-        return false;
+        deps.verified_classes_[class_def_index] = false;
+        all_validated = false;
+        if (number_of_warnings++ < kMaxWarnings) {
+          LOG(WARNING) << "Class "
+                       << dex_file.PrettyType(dex_file.GetClassDef(class_def_index).class_idx_)
+                       << " could not be fast verified because one of its methods wrongly expected "
+                       << destination_desc << " to be assignable from " << source_desc;
+        }
+        break;
       }
     }
+    class_def_index++;
   }
-  return true;
-}
-
-void VerifierDeps::ClearData(const std::vector<const DexFile*>& dex_files) {
-  for (const DexFile* dex_file : dex_files) {
-    auto it = dex_deps_.find(dex_file);
-    if (it == dex_deps_.end()) {
-      continue;
-    }
-    std::unique_ptr<DexFileDeps> deps(new DexFileDeps(dex_file->NumClassDefs()));
-    it->second.swap(deps);
-  }
-}
-
-bool VerifierDeps::VerifyDexFile(Handle<mirror::ClassLoader> class_loader,
-                                 const DexFile& dex_file,
-                                 const DexFileDeps& deps,
-                                 Thread* self,
-                                 /* out */ std::string* error_msg) const {
-  return VerifyAssignability(class_loader, dex_file, deps.assignable_types_, self, error_msg);
+  return all_validated;
 }
 
 }  // namespace verifier
diff --git a/runtime/verifier/verifier_deps.h b/runtime/verifier/verifier_deps.h
index dfa13ea112..5e3fb63c2d 100644
--- a/runtime/verifier/verifier_deps.h
+++ b/runtime/verifier/verifier_deps.h
@@ -119,11 +119,12 @@ class VerifierDeps {
 
   EXPORT void Dump(VariableIndentationOutputStream* vios) const;
 
-  // Verify the encoded dependencies of this `VerifierDeps` are still valid.
-  EXPORT bool ValidateDependencies(Thread* self,
-                                   Handle<mirror::ClassLoader> class_loader,
-                                   const std::vector<const DexFile*>& dex_files,
-                                   /* out */ std::string* error_msg) const
+  // Verifies the encoded dependencies of this `VerifierDeps` are still valid.
+  // Returns whether all dependencies were validated.
+  EXPORT bool ValidateDependenciesAndUpdateStatus(
+      Thread* self,
+      Handle<mirror::ClassLoader> class_loader,
+      const std::vector<const DexFile*>& dex_files)
       REQUIRES_SHARED(Locks::mutator_lock_);
 
   const std::vector<bool>& GetVerifiedClasses(const DexFile& dex_file) const {
@@ -142,9 +143,6 @@ class VerifierDeps {
     return GetDexFileDeps(dex_file) != nullptr;
   }
 
-  // Resets the data related to the given dex files.
-  EXPORT void ClearData(const std::vector<const DexFile*>& dex_files);
-
   // Parses raw VerifierDeps data to extract bitvectors of which class def indices
   // were verified or not. The given `dex_files` must match the order and count of
   // dex files used to create the VerifierDeps.
@@ -234,11 +232,12 @@ class VerifierDeps {
   // Verify `dex_file` according to the `deps`, that is going over each
   // `DexFileDeps` field, and checking that the recorded information still
   // holds.
-  bool VerifyDexFile(Handle<mirror::ClassLoader> class_loader,
-                     const DexFile& dex_file,
-                     const DexFileDeps& deps,
-                     Thread* self,
-                     /* out */ std::string* error_msg) const
+  // Returns whether all dependencies were validated.
+  bool VerifyDexFileAndUpdateStatus(
+      Handle<mirror::ClassLoader> class_loader,
+      const DexFile& dex_file,
+      DexFileDeps& deps,
+      Thread* self)
       REQUIRES_SHARED(Locks::mutator_lock_);
 
   // Iterates over `dex_files` and tries to find a class def matching `descriptor`.
@@ -248,13 +247,6 @@ class VerifierDeps {
                     const std::vector<const DexFile*>& dex_files,
                     /* out */ const DexFile** cp_dex_file) const;
 
-  bool VerifyAssignability(Handle<mirror::ClassLoader> class_loader,
-                           const DexFile& dex_file,
-                           const std::vector<std::set<TypeAssignability>>& assignables,
-                           Thread* self,
-                           /* out */ std::string* error_msg) const
-      REQUIRES_SHARED(Locks::mutator_lock_);
-
   // Map from DexFiles into dependencies collected from verification of their methods.
   std::map<const DexFile*, std::unique_ptr<DexFileDeps>> dex_deps_;