Don't wrap VerifyError into NoClassDefFoundError.

Follow RI behavior by returning the VerifyError. NoClassDefFoundError
only wraps initializer errors.

Also rename the field in ClassExt from verifyError to
erroneousStateError for better clarity.

And remove now unused feature of storing a class in the verifyError
field.

Test: test.py
Test: 824-verification-rethrow
Bug: 28313047
Change-Id: I19383f7b74f22a62ab1e0b8a13bea75a14c7b33f

dex2oat/linker/image_writer.cc

@@ -955,7 +955,7 @@ bool ImageWriter::PruneImageClassInternal(
     result = true;
   } else {
     ObjPtr<mirror::ClassExt> ext(klass->GetExtData());
-    CHECK(ext.IsNull() || ext->GetVerifyError() == nullptr) << klass->PrettyClass();
+    CHECK(ext.IsNull() || ext->GetErroneousStateError() == nullptr) << klass->PrettyClass();
   }
   if (!result) {
     // Check interfaces since these wont be visited through VisitReferences.)

runtime/class_linker.cc

@@ -171,63 +171,35 @@ static void ThrowNoClassDefFoundError(const char* fmt, ...) {
   va_end(args);
 }
 
-static bool HasInitWithString(Thread* self, ClassLinker* class_linker, const char* descriptor)
-    REQUIRES_SHARED(Locks::mutator_lock_) {
-  ArtMethod* method = self->GetCurrentMethod(nullptr);
-  StackHandleScope<1> hs(self);
-  Handle<mirror::ClassLoader> class_loader(hs.NewHandle(method != nullptr ?
-      method->GetDeclaringClass()->GetClassLoader() : nullptr));
-  ObjPtr<mirror::Class> exception_class = class_linker->FindClass(self, descriptor, class_loader);
-
-  if (exception_class == nullptr) {
-    // No exc class ~ no <init>-with-string.
-    CHECK(self->IsExceptionPending());
-    self->ClearException();
-    return false;
-  }
-
-  ArtMethod* exception_init_method = exception_class->FindConstructor(
-      "(Ljava/lang/String;)V", class_linker->GetImagePointerSize());
-  return exception_init_method != nullptr;
-}
-
-static ObjPtr<mirror::Object> GetVerifyError(ObjPtr<mirror::Class> c)
+static ObjPtr<mirror::Object> GetErroneousStateError(ObjPtr<mirror::Class> c)
     REQUIRES_SHARED(Locks::mutator_lock_) {
   ObjPtr<mirror::ClassExt> ext(c->GetExtData());
   if (ext == nullptr) {
     return nullptr;
   } else {
-    return ext->GetVerifyError();
+    return ext->GetErroneousStateError();
   }
 }
 
+static bool IsVerifyError(ObjPtr<mirror::Object> obj)
+    REQUIRES_SHARED(Locks::mutator_lock_) {
+  // This is slow, but we only use it for rethrowing an error, and for DCHECK.
+  return obj->GetClass()->DescriptorEquals("Ljava/lang/VerifyError;");
+}
+
 // Helper for ThrowEarlierClassFailure. Throws the stored error.
-static void HandleEarlierVerifyError(Thread* self,
-                                     ClassLinker* class_linker,
-                                     ObjPtr<mirror::Class> c)
+static void HandleEarlierErroneousStateError(Thread* self,
+                                             ClassLinker* class_linker,
+                                             ObjPtr<mirror::Class> c)
     REQUIRES_SHARED(Locks::mutator_lock_) {
-  ObjPtr<mirror::Object> obj = GetVerifyError(c);
+  ObjPtr<mirror::Object> obj = GetErroneousStateError(c);
   DCHECK(obj != nullptr);
   self->AssertNoPendingException();
-  if (obj->IsClass()) {
-    // Previous error has been stored as class. Create a new exception of that type.
-
-    // It's possible the exception doesn't have a <init>(String).
-    std::string temp;
-    const char* descriptor = obj->AsClass()->GetDescriptor(&temp);
-
-    if (HasInitWithString(self, class_linker, descriptor)) {
-      self->ThrowNewException(descriptor, c->PrettyDescriptor().c_str());
-    } else {
-      self->ThrowNewException(descriptor, nullptr);
-    }
-  } else {
-    // Previous error has been stored as an instance. Just rethrow.
-    ObjPtr<mirror::Class> throwable_class = GetClassRoot<mirror::Throwable>(class_linker);
-    ObjPtr<mirror::Class> error_class = obj->GetClass();
-    CHECK(throwable_class->IsAssignableFrom(error_class));
-    self->SetException(obj->AsThrowable());
-  }
+  DCHECK(!obj->IsClass());
+  ObjPtr<mirror::Class> throwable_class = GetClassRoot<mirror::Throwable>(class_linker);
+  ObjPtr<mirror::Class> error_class = obj->GetClass();
+  CHECK(throwable_class->IsAssignableFrom(error_class));
+  self->SetException(obj->AsThrowable());
   self->AssertPendingException();
 }
 
@@ -530,13 +502,10 @@ void ClassLinker::ThrowEarlierClassFailure(ObjPtr<mirror::Class> c,
   Runtime* const runtime = Runtime::Current();
   if (!runtime->IsAotCompiler()) {  // Give info if this occurs at runtime.
     std::string extra;
-    ObjPtr<mirror::Object> verify_error = GetVerifyError(c);
+    ObjPtr<mirror::Object> verify_error = GetErroneousStateError(c);
     if (verify_error != nullptr) {
-      if (verify_error->IsClass()) {
-        extra = mirror::Class::PrettyDescriptor(verify_error->AsClass());
-      } else {
-        extra = verify_error->AsThrowable()->Dump();
-      }
+      DCHECK(!verify_error->IsClass());
+      extra = verify_error->AsThrowable()->Dump();
     }
     if (log) {
       LOG(INFO) << "Rejecting re-init on previously-failed class " << c->PrettyClass()
@@ -551,15 +520,16 @@ void ClassLinker::ThrowEarlierClassFailure(ObjPtr<mirror::Class> c,
     ObjPtr<mirror::Throwable> pre_allocated = runtime->GetPreAllocatedNoClassDefFoundError();
     self->SetException(pre_allocated);
   } else {
-    ObjPtr<mirror::Object> verify_error = GetVerifyError(c);
-    if (verify_error != nullptr) {
+    ObjPtr<mirror::Object> erroneous_state_error = GetErroneousStateError(c);
+    if (erroneous_state_error != nullptr) {
       // Rethrow stored error.
-      HandleEarlierVerifyError(self, this, c);
+      HandleEarlierErroneousStateError(self, this, c);
     }
     // TODO This might be wrong if we hit an OOME while allocating the ClassExt. In that case we
     // might have meant to go down the earlier if statement with the original error but it got
     // swallowed by the OOM so we end up here.
-    if (verify_error == nullptr || wrap_in_no_class_def) {
+    if (erroneous_state_error == nullptr ||
+        (wrap_in_no_class_def && !IsVerifyError(erroneous_state_error))) {
       // If there isn't a recorded earlier error, or this is a repeat throw from initialization,
       // the top-level exception must be a NoClassDefFoundError. The potentially already pending
       // exception will be a cause.
@@ -5296,8 +5266,7 @@ bool ClassLinker::InitializeClass(Thread* self,
           // whether an exception is already pending.
           if (self->IsExceptionPending()) {
             // Check that it's a VerifyError.
-            DCHECK_EQ("java.lang.Class<java.lang.VerifyError>",
-                      mirror::Class::PrettyClass(self->GetException()->GetClass()));
+            DCHECK(IsVerifyError(self->GetException()));
           } else {
             // Check that another thread attempted initialization.
             DCHECK_NE(0, klass->GetClinitThreadId());

runtime/class_linker_test.cc

@@ -613,6 +613,7 @@ struct ClassOffsets : public CheckOffsets<mirror::Class> {
 
 struct ClassExtOffsets : public CheckOffsets<mirror::ClassExt> {
   ClassExtOffsets() : CheckOffsets<mirror::ClassExt>(false, "Ldalvik/system/ClassExt;") {
+    addOffset(OFFSETOF_MEMBER(mirror::ClassExt, erroneous_state_error_), "erroneousStateError");
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, instance_jfield_ids_), "instanceJfieldIDs");
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, jmethod_ids_), "jmethodIDs");
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, obsolete_class_), "obsoleteClass");
@@ -624,7 +625,6 @@ struct ClassExtOffsets : public CheckOffsets<mirror::ClassExt> {
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, pre_redefine_dex_file_ptr_),
               "preRedefineDexFilePtr");
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, static_jfield_ids_), "staticJfieldIDs");
-    addOffset(OFFSETOF_MEMBER(mirror::ClassExt, verify_error_), "verifyError");
   }
 };
 

runtime/mirror/class.cc

@@ -232,7 +232,7 @@ void Class::SetStatus(Handle<Class> h_this, ClassStatus new_status, Thread* self
     ObjPtr<ClassExt> ext(EnsureExtDataPresent(h_this, self));
     if (!ext.IsNull()) {
       self->AssertPendingException();
-      ext->SetVerifyError(self->GetException());
+      ext->SetErroneousStateError(self->GetException());
     } else {
       self->AssertPendingOOMException();
     }

runtime/mirror/class_ext-inl.h

@@ -144,9 +144,8 @@ inline bool ClassExt::HasMethodPointerIdMarker() {
   return !arr.IsNull() && !arr->IsArrayInstance();
 }
 
-
-inline ObjPtr<Object> ClassExt::GetVerifyError() {
-  return GetFieldObject<ClassExt>(OFFSET_OF_OBJECT_MEMBER(ClassExt, verify_error_));
+inline ObjPtr<Throwable> ClassExt::GetErroneousStateError() {
+  return GetFieldObject<Throwable>(OFFSET_OF_OBJECT_MEMBER(ClassExt, erroneous_state_error_));
 }
 
 inline ObjPtr<ObjectArray<DexCache>> ClassExt::GetObsoleteDexCaches() {

runtime/mirror/class_ext.cc

@@ -118,11 +118,11 @@ ObjPtr<ClassExt> ClassExt::Alloc(Thread* self) {
   return ObjPtr<ClassExt>::DownCast(GetClassRoot<ClassExt>()->AllocObject(self));
 }
 
-void ClassExt::SetVerifyError(ObjPtr<Object> err) {
+void ClassExt::SetErroneousStateError(ObjPtr<Throwable> err) {
   if (Runtime::Current()->IsActiveTransaction()) {
-    SetFieldObject<true>(OFFSET_OF_OBJECT_MEMBER(ClassExt, verify_error_), err);
+    SetFieldObject<true>(OFFSET_OF_OBJECT_MEMBER(ClassExt, erroneous_state_error_), err);
   } else {
-    SetFieldObject<false>(OFFSET_OF_OBJECT_MEMBER(ClassExt, verify_error_), err);
+    SetFieldObject<false>(OFFSET_OF_OBJECT_MEMBER(ClassExt, erroneous_state_error_), err);
   }
 }
 

runtime/mirror/class_ext.h

@@ -42,9 +42,9 @@ class MANAGED ClassExt : public Object {
     return sizeof(ClassExt);
   }
 
-  void SetVerifyError(ObjPtr<Object> obj) REQUIRES_SHARED(Locks::mutator_lock_);
+  void SetErroneousStateError(ObjPtr<Throwable> obj) REQUIRES_SHARED(Locks::mutator_lock_);
 
-  ObjPtr<Object> GetVerifyError() REQUIRES_SHARED(Locks::mutator_lock_);
+  ObjPtr<Throwable> GetErroneousStateError() REQUIRES_SHARED(Locks::mutator_lock_);
 
   ObjPtr<ObjectArray<DexCache>> GetObsoleteDexCaches() REQUIRES_SHARED(Locks::mutator_lock_);
 
@@ -156,6 +156,9 @@ class MANAGED ClassExt : public Object {
   bool EnsureJniIdsArrayPresent(MemberOffset off, size_t count)
       REQUIRES_SHARED(Locks::mutator_lock_);
 
+  // The saved error for this class being erroneous.
+  HeapReference<Throwable> erroneous_state_error_;
+
   // Field order required by test "ValidateFieldOrderOfJavaCppUnionClasses".
   // An array containing the jfieldIDs assigned to each field in the corresponding position in the
   // classes ifields_ array or '0' if no id has been assigned to that field yet.
@@ -178,9 +181,6 @@ class MANAGED ClassExt : public Object {
   // classes sfields_ array or '0' if no id has been assigned to that field yet.
   HeapReference<PointerArray> static_jfield_ids_;
 
-  // The saved verification error of this class.
-  HeapReference<Object> verify_error_;
-
   // Native pointer to DexFile and ClassDef index of this class before it was JVMTI-redefined.
   int64_t pre_redefine_dex_file_ptr_;
   int32_t pre_redefine_class_def_index_;

test/142-classloader2/expected-stdout.txt

@@ -1,5 +1,5 @@
 Loaded class B.
 Caught VerifyError.
 Loaded class B.
-Caught wrapped VerifyError.
+Caught existing VerifyError.
 Everything OK.

test/142-classloader2/src/Main.java

@@ -72,13 +72,15 @@ public static void main(String[] args) throws Exception {
         }
 
         // Try to load a dex file with bad dex code. Use new instance to force verification.
+        VerifyError existing = null;
         try {
           Class<?> badClass = Main.class.getClassLoader().loadClass("B");
           System.out.println("Loaded class B.");
           badClass.newInstance();
           System.out.println("Should not be able to instantiate B with bad dex bytecode.");
         } catch (VerifyError e) {
           System.out.println("Caught VerifyError.");
+          existing = e;
         }
 
         // Make sure the same error is rethrown when reloading the bad class.
@@ -87,9 +89,9 @@ public static void main(String[] args) throws Exception {
           System.out.println("Loaded class B.");
           badClass.newInstance();
           System.out.println("Should not be able to instantiate B with bad dex bytecode.");
-        } catch (NoClassDefFoundError e) {
-          if (e.getCause() instanceof VerifyError) {
-            System.out.println("Caught wrapped VerifyError.");
+        } catch (VerifyError e) {
+          if (e == existing) {
+            System.out.println("Caught existing VerifyError.");
           } else {
             e.printStackTrace(System.out);
           }

test/824-verification-rethrow/info.txt

@@ -0,0 +1,2 @@
+Regression test that a VerifyError does not get wrapped in a
+NoClassDefFoundError.

test/824-verification-rethrow/jasmin/Bar.j

@@ -0,0 +1,24 @@
+; Copyright (C) 2021 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 Bar
+.super                   java/lang/Object
+
+; Missing super constructor call will trigger a verification error.
+.method                  public <init>()V
+   .limit stack          1
+   .limit locals         1
+   return
+.end method
+

test/824-verification-rethrow/src/Main.java

@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2021 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 Main {
+  public static void main(String[] args) throws Exception {
+    // When calling resolution of a failed class twice, we expect a VerifyError to be
+    // thrown and not wrapped through a NoClassDefFoundError.
+    forName();
+    forName();
+  }
+
+  public static void forName() throws Exception {
+    try {
+      Class.forName("Bar");
+      throw new Error("Expected VerifyError");
+    } catch (VerifyError e) {
+      // Expected
+    }
+  }
+}