8267189: Remove duplicated unregistered classes from dynamic archive

Reviewed-by: ccheung, minqi

src/hotspot/share/cds/classListParser.cpp

@@ -466,7 +466,7 @@ InstanceKlass* ClassListParser::load_class_from_source(Symbol* class_name, TRAPS
           _interfaces->length(), k->local_interfaces()->length());
   }
 
-  bool added = SystemDictionaryShared::add_unregistered_class(THREAD, k);
+  bool added = SystemDictionaryShared::add_unregistered_class_for_static_archive(THREAD, k);
   if (!added) {
     // We allow only a single unregistered class for each unique name.
     error("Duplicated class %s", _class_name);

src/hotspot/share/classfile/systemDictionaryShared.cpp

@@ -1165,29 +1165,43 @@ InstanceKlass* SystemDictionaryShared::acquire_class_for_current_thread(
   return shared_klass;
 }
 
-class LoadedUnregisteredClassesTable : public ResourceHashtable<
-  Symbol*, bool,
+class UnregisteredClassesTable : public ResourceHashtable<
+  Symbol*, InstanceKlass*,
   primitive_hash<Symbol*>,
   primitive_equals<Symbol*>,
-  6661,                             // prime number
+  15889, // prime number
   ResourceObj::C_HEAP> {};
 
-static LoadedUnregisteredClassesTable* _loaded_unregistered_classes = NULL;
+static UnregisteredClassesTable* _unregistered_classes_table = NULL;
 
-bool SystemDictionaryShared::add_unregistered_class(Thread* current, InstanceKlass* k) {
-  // We don't allow duplicated unregistered classes of the same name.
-  assert(DumpSharedSpaces, "only when dumping");
-  Symbol* name = k->name();
-  if (_loaded_unregistered_classes == NULL) {
-    _loaded_unregistered_classes = new (ResourceObj::C_HEAP, mtClass)LoadedUnregisteredClassesTable();
+bool SystemDictionaryShared::add_unregistered_class(Thread* current, InstanceKlass* klass) {
+  // We don't allow duplicated unregistered classes with the same name.
+  // We only archive the first class with that name that succeeds putting
+  // itself into the table.
+  Arguments::assert_is_dumping_archive();
+  MutexLocker ml(current, UnregisteredClassesTable_lock);
+  Symbol* name = klass->name();
+  if (_unregistered_classes_table == NULL) {
+    _unregistered_classes_table = new (ResourceObj::C_HEAP, mtClass)UnregisteredClassesTable();
   }
-  bool created = false;
-  _loaded_unregistered_classes->put_if_absent(name, true, &created);
+  bool created;
+  InstanceKlass** v = _unregistered_classes_table->put_if_absent(name, klass, &created);
   if (created) {
+    name->increment_refcount();
+  }
+  return (klass == *v);
+}
+
+// true == class was successfully added; false == a duplicated class (with the same name) already exists.
+bool SystemDictionaryShared::add_unregistered_class_for_static_archive(Thread* current, InstanceKlass* k) {
+  assert(DumpSharedSpaces, "only when dumping");
+  if (add_unregistered_class(current, k)) {
     MutexLocker mu_r(current, Compile_lock); // add_to_hierarchy asserts this.
     SystemDictionary::add_to_hierarchy(k);
+    return true;
+  } else {
+    return false;
   }
-  return created;
 }
 
 // This function is called to lookup the super/interfaces of shared classes for
@@ -1295,6 +1309,21 @@ void SystemDictionaryShared::remove_dumptime_info(InstanceKlass* k) {
   _dumptime_table->remove(k);
 }
 
+void SystemDictionaryShared::handle_class_unloading(InstanceKlass* klass) {
+  remove_dumptime_info(klass);
+
+  if (_unregistered_classes_table != NULL) {
+    // Remove the class from _unregistered_classes_table: keep the entry but
+    // set it to NULL. This ensure no classes with the same name can be
+    // added again.
+    MutexLocker ml(Thread::current(), UnregisteredClassesTable_lock);
+    InstanceKlass** v = _unregistered_classes_table->get(klass->name());
+    if (v != NULL) {
+      *v = NULL;
+    }
+  }
+}
+
 bool SystemDictionaryShared::is_jfr_event_class(InstanceKlass *k) {
   while (k) {
     if (k->name()->equals("jdk/internal/event/Event")) {
@@ -1476,6 +1505,48 @@ void SystemDictionaryShared::validate_before_archiving(InstanceKlass* k) {
   }
 }
 
+class UnregisteredClassesDuplicationChecker : StackObj {
+  GrowableArray<InstanceKlass*> _list;
+  Thread* _thread;
+public:
+  UnregisteredClassesDuplicationChecker() : _thread(Thread::current()) {}
+
+  bool do_entry(InstanceKlass* k, DumpTimeSharedClassInfo& info) {
+    if (!SystemDictionaryShared::is_builtin(k)) {
+      _list.append(k);
+    }
+    return true;  // keep on iterating
+  }
+
+  static int compare_by_loader(InstanceKlass** a, InstanceKlass** b) {
+    ClassLoaderData* loader_a = a[0]->class_loader_data();
+    ClassLoaderData* loader_b = b[0]->class_loader_data();
+
+    if (loader_a != loader_b) {
+      return intx(loader_a) - intx(loader_b);
+    } else {
+      return intx(a[0]) - intx(b[0]);
+    }
+  }
+
+  void mark_duplicated_classes() {
+    // Two loaders may load two identical or similar hierarchies of classes. If we
+    // check for duplication in random order, we may end up excluding important base classes
+    // in both hierarchies, causing most of the classes to be excluded.
+    // We sort the classes by their loaders. This way we're likely to archive
+    // all classes in the one of the two hierarchies.
+    _list.sort(compare_by_loader);
+    for (int i = 0; i < _list.length(); i++) {
+      InstanceKlass* k = _list.at(i);
+      bool i_am_first = SystemDictionaryShared::add_unregistered_class(_thread, k);
+      if (!i_am_first) {
+        SystemDictionaryShared::warn_excluded(k, "Duplicated unregistered class");
+        SystemDictionaryShared::set_excluded_locked(k);
+      }
+    }
+  }
+};
+
 class ExcludeDumpTimeSharedClasses : StackObj {
 public:
   bool do_entry(InstanceKlass* k, DumpTimeSharedClassInfo& info) {
@@ -1487,6 +1558,16 @@ public:
 void SystemDictionaryShared::check_excluded_classes() {
   assert(no_class_loading_should_happen(), "sanity");
   assert_lock_strong(DumpTimeTable_lock);
+
+  if (DynamicDumpSharedSpaces) {
+    // Do this first -- if a base class is excluded due to duplication,
+    // all of its subclasses will also be excluded by ExcludeDumpTimeSharedClasses
+    ResourceMark rm;
+    UnregisteredClassesDuplicationChecker dup_checker;
+    _dumptime_table->iterate(&dup_checker);
+    dup_checker.mark_duplicated_classes();
+  }
+
   ExcludeDumpTimeSharedClasses excl;
   _dumptime_table->iterate(&excl);
   _dumptime_table->update_counts();
@@ -1500,6 +1581,15 @@ bool SystemDictionaryShared::is_excluded_class(InstanceKlass* k) {
   return (p == NULL) ? true : p->is_excluded();
 }
 
+void SystemDictionaryShared::set_excluded_locked(InstanceKlass* k) {
+  assert_lock_strong(DumpTimeTable_lock);
+  Arguments::assert_is_dumping_archive();
+  DumpTimeSharedClassInfo* info = find_or_allocate_info_for_locked(k);
+  if (info != NULL) {
+    info->set_excluded();
+  }
+}
+
 void SystemDictionaryShared::set_excluded(InstanceKlass* k) {
   Arguments::assert_is_dumping_archive();
   DumpTimeSharedClassInfo* info = find_or_allocate_info_for(k);

src/hotspot/share/classfile/systemDictionaryShared.hpp

@@ -230,8 +230,8 @@ private:
   static void write_lambda_proxy_class_dictionary(LambdaProxyClassDictionary* dictionary);
   static bool is_jfr_event_class(InstanceKlass *k);
   static bool is_registered_lambda_proxy_class(InstanceKlass* ik);
-  static bool warn_excluded(InstanceKlass* k, const char* reason);
   static bool check_for_exclusion_impl(InstanceKlass* k);
+  static void remove_dumptime_info(InstanceKlass* k) NOT_CDS_RETURN;
   static bool has_been_redefined(InstanceKlass* k);
 
   static bool _dump_in_progress;
@@ -265,12 +265,12 @@ public:
   // Check if sharing is supported for the class loader.
   static bool is_sharing_possible(ClassLoaderData* loader_data);
 
-  static bool add_unregistered_class(Thread* current, InstanceKlass* k);
+  static bool add_unregistered_class_for_static_archive(Thread* current, InstanceKlass* k);
   static InstanceKlass* lookup_super_for_unregistered_class(Symbol* class_name,
                                                             Symbol* super_name,  bool is_superclass);
 
   static void init_dumptime_info(InstanceKlass* k) NOT_CDS_RETURN;
-  static void remove_dumptime_info(InstanceKlass* k) NOT_CDS_RETURN;
+  static void handle_class_unloading(InstanceKlass* k) NOT_CDS_RETURN;
 
   static Dictionary* boot_loader_dictionary() {
     return ClassLoaderData::the_null_class_loader_data()->dictionary();
@@ -322,11 +322,14 @@ public:
   static bool is_builtin(InstanceKlass* k) {
     return (k->shared_classpath_index() != UNREGISTERED_INDEX);
   }
+  static bool add_unregistered_class(Thread* current, InstanceKlass* k);
   static void check_excluded_classes();
   static bool check_for_exclusion(InstanceKlass* k, DumpTimeSharedClassInfo* info);
   static void validate_before_archiving(InstanceKlass* k);
   static bool is_excluded_class(InstanceKlass* k);
   static void set_excluded(InstanceKlass* k);
+  static void set_excluded_locked(InstanceKlass* k);
+  static bool warn_excluded(InstanceKlass* k, const char* reason);
   static void dumptime_classes_do(class MetaspaceClosure* it);
   static size_t estimate_size_for_archive();
   static void write_to_archive(bool is_static_archive = true);

src/hotspot/share/oops/instanceKlass.cpp

@@ -683,7 +683,7 @@ void InstanceKlass::deallocate_contents(ClassLoaderData* loader_data) {
   set_annotations(NULL);
 
   if (Arguments::is_dumping_archive()) {
-    SystemDictionaryShared::remove_dumptime_info(this);
+    SystemDictionaryShared::handle_class_unloading(this);
   }
 }
 
@@ -2613,7 +2613,7 @@ void InstanceKlass::unload_class(InstanceKlass* ik) {
   ClassLoadingService::notify_class_unloaded(ik);
 
   if (Arguments::is_dumping_archive()) {
-    SystemDictionaryShared::remove_dumptime_info(ik);
+    SystemDictionaryShared::handle_class_unloading(ik);
   }
 
   if (log_is_enabled(Info, class, unload)) {

src/hotspot/share/runtime/mutexLocker.cpp

@@ -154,6 +154,7 @@ Mutex*   DumpTimeTable_lock           = NULL;
 Mutex*   CDSLambda_lock               = NULL;
 Mutex*   DumpRegion_lock              = NULL;
 Mutex*   ClassListFile_lock           = NULL;
+Mutex*   UnregisteredClassesTable_lock= NULL;
 Mutex*   LambdaFormInvokers_lock      = NULL;
 #endif // INCLUDE_CDS
 Mutex*   Bootclasspath_lock           = NULL;

src/hotspot/share/runtime/mutexLocker.hpp

@@ -132,6 +132,7 @@ extern Mutex*   DumpTimeTable_lock;              // SystemDictionaryShared::find
 extern Mutex*   CDSLambda_lock;                  // SystemDictionaryShared::get_shared_lambda_proxy_class
 extern Mutex*   DumpRegion_lock;                 // Symbol::operator new(size_t sz, int len)
 extern Mutex*   ClassListFile_lock;              // ClassListWriter()
+extern Mutex*   UnregisteredClassesTable_lock;   // UnregisteredClassesTableTable
 extern Mutex*   LambdaFormInvokers_lock;         // Protecting LambdaFormInvokers::_lambdaform_lines
 #endif // INCLUDE_CDS
 #if INCLUDE_JFR

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DuplicatedCustomTest.java

@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+/*
+ * @test
+ * @summary Handling of duplicated classes in dynamic archive with custom loader
+ * @requires vm.cds
+ * @library /test/lib
+ *          /test/hotspot/jtreg/runtime/cds/appcds
+ *          /test/hotspot/jtreg/runtime/cds/appcds/customLoader/test-classes
+ *          /test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/test-classes
+ * @build DuplicatedCustomApp CustomLoadee CustomLoadee2 CustomLoadee3 CustomLoadee3Child
+ * @build sun.hotspot.WhiteBox
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar app.jar DuplicatedCustomApp
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar custom.jar CustomLoadee
+ *                  CustomLoadee2 CustomInterface2_ia CustomInterface2_ib
+ *                  CustomLoadee3 CustomLoadee3Child
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar WhiteBox.jar sun.hotspot.WhiteBox
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xbootclasspath/a:./WhiteBox.jar DuplicatedCustomTest
+ */
+
+import java.io.File;
+import jdk.test.lib.cds.CDSTestUtils;
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.helpers.ClassFileInstaller;
+
+public class DuplicatedCustomTest extends DynamicArchiveTestBase {
+    private static final String ARCHIVE_NAME = CDSTestUtils.getOutputFileName("top.jsa");
+
+    public static void main(String[] args) throws Exception {
+        runTest(DuplicatedCustomTest::testDefaultBase);
+    }
+
+    private static void testDefaultBase() throws Exception {
+        String wbJar = ClassFileInstaller.getJarPath("WhiteBox.jar");
+        String use_whitebox_jar = "-Xbootclasspath/a:" + wbJar;
+        String appJar = ClassFileInstaller.getJarPath("app.jar");
+        String customJarPath = ClassFileInstaller.getJarPath("custom.jar");
+        String mainAppClass = "DuplicatedCustomApp";
+        String numberOfLoops = "2";
+
+        dump(ARCHIVE_NAME,
+            use_whitebox_jar,
+            "-XX:+UnlockDiagnosticVMOptions",
+            "-XX:+WhiteBoxAPI",
+            "-Xlog:cds",
+            "-Xlog:cds+dynamic=debug",
+            "-cp", appJar,
+            mainAppClass, customJarPath, numberOfLoops)
+            .assertNormalExit(output -> {
+                output.shouldContain("Written dynamic archive 0x")
+                      .shouldContain("Skipping CustomLoadee: Duplicated unregistered class")
+                      .shouldHaveExitValue(0);
+                });
+
+        run(ARCHIVE_NAME,
+            use_whitebox_jar,
+            "-XX:+UnlockDiagnosticVMOptions",
+            "-XX:+WhiteBoxAPI",
+            "-Xlog:class+load",
+            "-Xlog:cds=debug",
+            "-Xlog:cds+dynamic=info",
+            "-cp", appJar,
+            mainAppClass, customJarPath, numberOfLoops)
+            .assertNormalExit(output -> {
+                output.shouldContain("DuplicatedCustomApp source: shared objects file (top)")
+                      .shouldContain("CustomLoadee source: shared objects file (top)")
+                      .shouldHaveExitValue(0);
+                });
+    }
+}

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/test-classes/DuplicatedCustomApp.java

@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+import java.io.*;
+import java.net.*;
+import sun.hotspot.WhiteBox;
+
+public class DuplicatedCustomApp {
+    static WhiteBox wb = WhiteBox.getWhiteBox();
+    static URLClassLoader loaders[];
+
+    // If DuplicatedCustomApp.class is loaded from JAR file, it means we are dumping the
+    // dynamic archive.
+    static boolean is_dynamic_dumping = !wb.isSharedClass(DuplicatedCustomApp.class);
+    static boolean is_running_with_dynamic_archive = !is_dynamic_dumping;
+
+    public static void main(String args[]) throws Exception {
+        String path = args[0];
+        URL url = new File(path).toURI().toURL();
+        URL[] urls = new URL[] {url};
+        System.out.println(path);
+        System.out.println(url);
+
+        int num_loops = 1;
+        if (args.length > 1) {
+            num_loops = Integer.parseInt(args[1]);
+        }
+        loaders = new URLClassLoader[num_loops];
+        for (int i = 0; i < num_loops; i++) {
+            loaders[i] = new URLClassLoader(urls);
+        }
+
+        if (is_dynamic_dumping) {
+            // Try to load the super interfaces of CustomLoadee2 in different orders
+            for (int i = 0; i < num_loops; i++) {
+                int a = (i + 1) % num_loops;
+                loaders[a].loadClass("CustomInterface2_ia");
+            }
+            for (int i = 0; i < num_loops; i++) {
+                int a = (i + 2) % num_loops;
+                loaders[a].loadClass("CustomInterface2_ib");
+            }
+        }
+
+        for (int i = 0; i < num_loops; i++) {
+            System.out.println("============================ LOOP = " + i);
+            URLClassLoader urlClassLoader = loaders[i];
+            test(i, urlClassLoader, "CustomLoadee");
+            test(i, urlClassLoader, "CustomInterface2_ia");
+            test(i, urlClassLoader, "CustomInterface2_ib");
+            test(i, urlClassLoader, "CustomLoadee2");
+            test(i, urlClassLoader, "CustomLoadee3");
+            test(i, urlClassLoader, "CustomLoadee3Child");
+        }
+    }
+
+    private static void test(int i, URLClassLoader urlClassLoader, String name) throws Exception {
+        Class c = urlClassLoader.loadClass(name);
+        try {
+            c.newInstance(); // make sure the class is linked so it can be archived
+        } catch (Throwable t) {}
+        boolean is_shared = wb.isSharedClass(c);
+
+        System.out.println("Class = " + c + ", loaded from " + (is_shared ? "CDS" : "Jar"));
+        System.out.println("Loader = " + c.getClassLoader());
+
+        // [1] Check that the loaded class is defined by the correct loader
+        if (c.getClassLoader() != urlClassLoader) {
+            throw new RuntimeException("c.getClassLoader() == " + c.getClassLoader() +
+                                       ", expected == " + urlClassLoader);
+        }
+
+        if (is_running_with_dynamic_archive) {
+            // There's only one copy of the shared class of <name> in the
+            // CDS archive.
+            if (i == 0) {
+                // The first time we must be able to load it from CDS.
+                if (!is_shared) {
+                    throw new RuntimeException("Must be loaded from CDS");
+                }
+            } else {
+                // All subsequent times, we must load this from JAR file.
+                if (is_shared) {
+                    throw new RuntimeException("Must be loaded from JAR");
+                }
+            }
+        }
+    }
+}