8240481: Remove CDS usage of InstanceKlass::is_in_error_state

Track the classes which fail verification during CDS dumping in DumpTimeSharedClassInfo.

Reviewed-by: iklam, minqi

src/hotspot/share/classfile/systemDictionaryShared.cpp

@@ -76,6 +76,7 @@ class DumpTimeSharedClassInfo: public CHeapObj<mtClass> {
   };
 
   InstanceKlass*               _klass;
+  bool                         _failed_verification;
   int                          _id;
   int                          _clsfile_size;
   int                          _clsfile_crc32;
@@ -84,6 +85,7 @@ class DumpTimeSharedClassInfo: public CHeapObj<mtClass> {
 
   DumpTimeSharedClassInfo() {
     _klass = NULL;
+    _failed_verification = false;
     _id = -1;
     _clsfile_size = -1;
     _clsfile_crc32 = -1;
@@ -124,7 +126,15 @@ class DumpTimeSharedClassInfo: public CHeapObj<mtClass> {
 
   bool is_excluded() {
     // _klass may become NULL due to DynamicArchiveBuilder::set_to_null
-    return _excluded || _klass == NULL;
+    return _excluded || _failed_verification || _klass == NULL;
+  }
+
+  void set_failed_verification() {
+    _failed_verification = true;
+  }
+
+  bool failed_verification() {
+    return _failed_verification;
   }
 };
 
@@ -1108,17 +1118,21 @@ bool SystemDictionaryShared::should_be_excluded(InstanceKlass* k) {
     return true;
   }
   if (k->init_state() < InstanceKlass::linked) {
-    // In static dumping, we will attempt to link all classes. Those that fail to link will
-    // be marked as in error state.
-    assert(DynamicDumpSharedSpaces, "must be");
+    // In CDS dumping, we will attempt to link all classes. Those that fail to link will
+    // be recorded in DumpTimeSharedClassInfo.
+    Arguments::assert_is_dumping_archive();
 
     // TODO -- rethink how this can be handled.
     // We should try to link ik, however, we can't do it here because
     // 1. We are at VM exit
     // 2. linking a class may cause other classes to be loaded, which means
     //    a custom ClassLoader.loadClass() may be called, at a point where the
     //    class loader doesn't expect it.
-    warn_excluded(k, "Not linked");
+    if (has_class_failed_verification(k)) {
+      warn_excluded(k, "Failed verification");
+    } else {
+      warn_excluded(k, "Not linked");
+    }
     return true;
   }
   if (k->major_version() < 50 /*JAVA_6_VERSION*/) {
@@ -1187,6 +1201,22 @@ bool SystemDictionaryShared::is_excluded_class(InstanceKlass* k) {
   return find_or_allocate_info_for(k)->is_excluded();
 }
 
+void SystemDictionaryShared::set_class_has_failed_verification(InstanceKlass* ik) {
+  Arguments::assert_is_dumping_archive();
+  find_or_allocate_info_for(ik)->set_failed_verification();
+}
+
+bool SystemDictionaryShared::has_class_failed_verification(InstanceKlass* ik) {
+  Arguments::assert_is_dumping_archive();
+  if (_dumptime_table == NULL) {
+    assert(DynamicDumpSharedSpaces, "sanity");
+    assert(ik->is_shared(), "must be a shared class in the static archive");
+    return false;
+  }
+  DumpTimeSharedClassInfo* p = _dumptime_table->get(ik);
+  return (p == NULL) ? false : p->failed_verification();
+}
+
 class IterateDumpTimeSharedClassTable : StackObj {
   MetaspaceClosure *_it;
 public:

src/hotspot/share/classfile/systemDictionaryShared.hpp

@@ -292,6 +292,8 @@ class SystemDictionaryShared: public SystemDictionary {
                   bool from_is_array, bool from_is_object) NOT_CDS_RETURN_(false);
   static void check_verification_constraints(InstanceKlass* klass,
                                              TRAPS) NOT_CDS_RETURN;
+  static void set_class_has_failed_verification(InstanceKlass* ik) NOT_CDS_RETURN;
+  static bool has_class_failed_verification(InstanceKlass* ik) NOT_CDS_RETURN_(false);
   static bool is_builtin(InstanceKlass* k) {
     return (k->shared_classpath_index() != UNREGISTERED_INDEX);
   }

src/hotspot/share/memory/metaspaceShared.cpp

@@ -1713,20 +1713,6 @@ class LinkSharedClassesClosure : public KlassClosure {
   }
 };
 
-class CheckSharedClassesClosure : public KlassClosure {
-  bool    _made_progress;
- public:
-  CheckSharedClassesClosure() : _made_progress(false) {}
-
-  void reset()               { _made_progress = false; }
-  bool made_progress() const { return _made_progress; }
-  void do_klass(Klass* k) {
-    if (k->is_instance_klass() && InstanceKlass::cast(k)->check_sharing_error_state()) {
-      _made_progress = true;
-    }
-  }
-};
-
 void MetaspaceShared::link_and_cleanup_shared_classes(TRAPS) {
   // We need to iterate because verification may cause additional classes
   // to be loaded.
@@ -1736,18 +1722,6 @@ void MetaspaceShared::link_and_cleanup_shared_classes(TRAPS) {
     ClassLoaderDataGraph::unlocked_loaded_classes_do(&link_closure);
     guarantee(!HAS_PENDING_EXCEPTION, "exception in link_class");
   } while (link_closure.made_progress());
-
-  if (_has_error_classes) {
-    // Mark all classes whose super class or interfaces failed verification.
-    CheckSharedClassesClosure check_closure;
-    do {
-      // Not completely sure if we need to do this iteratively. Anyway,
-      // we should come here only if there are unverifiable classes, which
-      // shouldn't happen in normal cases. So better safe than sorry.
-      check_closure.reset();
-      ClassLoaderDataGraph::unlocked_loaded_classes_do(&check_closure);
-    } while (check_closure.made_progress());
-  }
 }
 
 void MetaspaceShared::prepare_for_dumping() {
@@ -1875,7 +1849,8 @@ int MetaspaceShared::preload_classes(const char* class_list_path, TRAPS) {
 // Returns true if the class's status has changed
 bool MetaspaceShared::try_link_class(InstanceKlass* ik, TRAPS) {
   assert(DumpSharedSpaces, "should only be called during dumping");
-  if (ik->init_state() < InstanceKlass::linked) {
+  if (ik->init_state() < InstanceKlass::linked &&
+      !SystemDictionaryShared::has_class_failed_verification(ik)) {
     bool saved = BytecodeVerificationLocal;
     if (ik->loader_type() == 0 && ik->class_loader() == NULL) {
       // The verification decision is based on BytecodeVerificationRemote
@@ -1893,7 +1868,7 @@ bool MetaspaceShared::try_link_class(InstanceKlass* ik, TRAPS) {
       log_warning(cds)("Preload Warning: Verification failed for %s",
                     ik->external_name());
       CLEAR_PENDING_EXCEPTION;
-      ik->set_in_error_state();
+      SystemDictionaryShared::set_class_has_failed_verification(ik);
       _has_error_classes = true;
     }
     BytecodeVerificationLocal = saved;

src/hotspot/share/oops/instanceKlass.cpp

@@ -763,7 +763,7 @@ bool InstanceKlass::link_class_or_fail(TRAPS) {
 }
 
 bool InstanceKlass::link_class_impl(TRAPS) {
-  if (DumpSharedSpaces && is_in_error_state()) {
+  if (DumpSharedSpaces && SystemDictionaryShared::has_class_failed_verification(this)) {
     // This is for CDS dumping phase only -- we use the in_error_state to indicate that
     // the class has failed verification. Throwing the NoClassDefFoundError here is just
     // a convenient way to stop repeat attempts to verify the same (bad) class.
@@ -2373,12 +2373,10 @@ void InstanceKlass::metaspace_pointers_do(MetaspaceClosure* it) {
 void InstanceKlass::remove_unshareable_info() {
   Klass::remove_unshareable_info();
 
-  if (is_in_error_state()) {
+  if (SystemDictionaryShared::has_class_failed_verification(this)) {
     // Classes are attempted to link during dumping and may fail,
     // but these classes are still in the dictionary and class list in CLD.
-    // Check in_error state first because in_error is > linked state, so
-    // is_linked() is true.
-    // If there's a linking error, there is nothing else to remove.
+    // If the class has failed verification, there is nothing else to remove.
     return;
   }
 
@@ -2473,38 +2471,6 @@ void InstanceKlass::restore_unshareable_info(ClassLoaderData* loader_data, Handl
   }
 }
 
-// returns true IFF is_in_error_state() has been changed as a result of this call.
-bool InstanceKlass::check_sharing_error_state() {
-  assert(DumpSharedSpaces, "should only be called during dumping");
-  bool old_state = is_in_error_state();
-
-  if (!is_in_error_state()) {
-    bool bad = false;
-    for (InstanceKlass* sup = java_super(); sup; sup = sup->java_super()) {
-      if (sup->is_in_error_state()) {
-        bad = true;
-        break;
-      }
-    }
-    if (!bad) {
-      Array<InstanceKlass*>* interfaces = transitive_interfaces();
-      for (int i = 0; i < interfaces->length(); i++) {
-        InstanceKlass* iface = interfaces->at(i);
-        if (iface->is_in_error_state()) {
-          bad = true;
-          break;
-        }
-      }
-    }
-
-    if (bad) {
-      set_in_error_state();
-    }
-  }
-
-  return (old_state != is_in_error_state());
-}
-
 void InstanceKlass::set_class_loader_type(s2 loader_type) {
   switch (loader_type) {
   case ClassLoader::BOOT_LOADER:

src/hotspot/share/oops/instanceKlass.hpp

@@ -1268,7 +1268,6 @@ class InstanceKlass: public Klass {
     assert(DumpSharedSpaces, "only call this when dumping archive");
     _init_state = initialization_error;
   }
-  bool check_sharing_error_state();
 
 private:
   // initialization state