8280454: G1: ClassLoaderData verification keeps CLDs live that causes problems with VerifyDuringGC during Remark

Reviewed-by: stefank, coleenp

src/hotspot/share/classfile/classLoaderData.hpp

@@ -98,7 +98,8 @@ class ClassLoaderData : public CHeapObj<mtClass> {
   };
 
   friend class ClassLoaderDataGraph;
-  friend class ClassLoaderDataGraphIterator;
+  template <bool keep_alive>
+  friend class ClassLoaderDataGraphIteratorBase;
   friend class ClassLoaderDataGraphKlassIteratorAtomic;
   friend class ClassLoaderDataGraphKlassIteratorStatic;
   friend class ClassLoaderDataGraphMetaspaceIterator;
@@ -253,6 +254,7 @@ class ClassLoaderData : public CHeapObj<mtClass> {
   ClassLoaderMetaspace* metaspace_non_null();
 
   inline oop class_loader() const;
+  inline oop class_loader_no_keepalive() const;
 
   // Returns true if this class loader data is for a loader going away.
   // Note that this is only safe after the GC has computed if the CLD is

src/hotspot/share/classfile/classLoaderData.inline.hpp

@@ -39,6 +39,12 @@ inline oop ClassLoaderData::class_loader() const {
   return _class_loader.resolve();
 }
 
+inline oop ClassLoaderData::class_loader_no_keepalive() const {
+  assert(!_unloading, "This oop is not available to unloading class loader data");
+  assert(_holder.is_null() || holder_no_keepalive() != NULL , "This class loader data holder must be alive");
+  return _class_loader.peek();
+}
+
 inline bool ClassLoaderData::is_boot_class_loader_data() const {
   return this == _the_null_class_loader_data || class_loader() == NULL;
 }

src/hotspot/share/classfile/classLoaderDataGraph.cpp

@@ -314,18 +314,21 @@ LockedClassesDo::~LockedClassesDo() {
 
 // Iterating over the CLDG needs to be locked because
 // unloading can remove entries concurrently soon.
-class ClassLoaderDataGraphIterator : public StackObj {
+template <bool keep_alive = true>
+class ClassLoaderDataGraphIteratorBase : public StackObj {
   ClassLoaderData* _next;
   Thread*          _thread;
   HandleMark       _hm;  // clean up handles when this is done.
-  Handle           _holder;
   NoSafepointVerifier _nsv; // No safepoints allowed in this scope
                             // unless verifying at a safepoint.
 
 public:
-  ClassLoaderDataGraphIterator() : _next(ClassLoaderDataGraph::_head), _thread(Thread::current()), _hm(_thread) {
-    _thread = Thread::current();
-    assert_locked_or_safepoint(ClassLoaderDataGraph_lock);
+  ClassLoaderDataGraphIteratorBase() : _next(ClassLoaderDataGraph::_head), _thread(Thread::current()), _hm(_thread) {
+    if (keep_alive) {
+      assert_locked_or_safepoint(ClassLoaderDataGraph_lock);
+    } else {
+      assert_at_safepoint();
+    }
   }
 
   ClassLoaderData* get_next() {
@@ -335,8 +338,10 @@ public:
       cld = cld->next();
     }
     if (cld != NULL) {
-      // Keep cld that is being returned alive.
-      _holder = Handle(_thread, cld->holder());
+      if (keep_alive) {
+        // Keep cld that is being returned alive.
+        Handle(_thread, cld->holder());
+      }
       _next = cld->next();
     } else {
       _next = NULL;
@@ -345,6 +350,9 @@ public:
   }
 };
 
+using ClassLoaderDataGraphIterator = ClassLoaderDataGraphIteratorBase<true /* keep_alive */>;
+using ClassLoaderDataGraphIteratorNoKeepAlive = ClassLoaderDataGraphIteratorBase<false /* keep_alive */>;
+
 void ClassLoaderDataGraph::loaded_cld_do(CLDClosure* cl) {
   ClassLoaderDataGraphIterator iter;
   while (ClassLoaderData* cld = iter.get_next()) {
@@ -422,16 +430,19 @@ void ClassLoaderDataGraph::classes_unloading_do(void f(Klass* const)) {
   }
 }
 
+void ClassLoaderDataGraph::verify_dictionary() {
+  ClassLoaderDataGraphIteratorNoKeepAlive iter;
+  while (ClassLoaderData* cld = iter.get_next()) {
+    if (cld->dictionary() != nullptr) {
+      cld->dictionary()->verify();
+    }
+  }
+}
+
 #define FOR_ALL_DICTIONARY(X)   ClassLoaderDataGraphIterator iter; \
                                 while (ClassLoaderData* X = iter.get_next()) \
                                   if (X->dictionary() != NULL)
 
-void ClassLoaderDataGraph::verify_dictionary() {
-  FOR_ALL_DICTIONARY(cld) {
-    cld->dictionary()->verify();
-  }
-}
-
 void ClassLoaderDataGraph::print_dictionary(outputStream* st) {
   FOR_ALL_DICTIONARY(cld) {
     st->print("Dictionary for ");
@@ -648,7 +659,7 @@ Klass* ClassLoaderDataGraphKlassIteratorAtomic::next_klass() {
 }
 
 void ClassLoaderDataGraph::verify() {
-  ClassLoaderDataGraphIterator iter;
+  ClassLoaderDataGraphIteratorNoKeepAlive iter;
   while (ClassLoaderData* cld = iter.get_next()) {
     cld->verify();
   }

src/hotspot/share/classfile/classLoaderDataGraph.hpp

@@ -37,7 +37,8 @@ class ClassLoaderDataGraph : public AllStatic {
   friend class ClassLoaderDataGraphMetaspaceIterator;
   friend class ClassLoaderDataGraphKlassIteratorAtomic;
   friend class ClassLoaderDataGraphKlassIteratorStatic;
-  friend class ClassLoaderDataGraphIterator;
+  template <bool keep_alive>
+  friend class ClassLoaderDataGraphIteratorBase;
   friend class VMStructs;
  private:
   // All CLDs (except the null CLD) can be reached by walking _head->_next->...

src/hotspot/share/classfile/dictionary.cpp

@@ -633,7 +633,7 @@ void Dictionary::verify() {
   // class loader must be present;  a null class loader is the
   // bootstrap loader
   guarantee(cld != NULL &&
-            (cld->the_null_class_loader_data() || cld->class_loader()->is_instance()),
+            (cld->is_the_null_class_loader_data() || cld->class_loader_no_keepalive()->is_instance()),
             "checking type of class_loader");
 
   ResourceMark rm;

test/hotspot/jtreg/runtime/ClassUnload/UnloadTestWithVerifyDuringGC.java

@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2022, 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 Class unloading test with concurrent mark
+ * @summary Make sure that verification during gc does not prevent class unloading
+ * @bug 8280454
+ * @requires vm.gc.G1
+ * @requires vm.opt.final.ClassUnloading
+ * @requires vm.opt.final.ClassUnloadingWithConcurrentMark
+ * @modules java.base/jdk.internal.misc
+ * @library /test/lib
+ * @library classes
+ * @build sun.hotspot.WhiteBox test.Empty
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
+ * @run main/othervm -Xbootclasspath/a:. -Xmn8m -XX:+UseG1GC -XX:+VerifyDuringGC -XX:+AlwaysTenure -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xlog:gc,class+unload=debug UnloadTestWithVerifyDuringGC
+ */
+import sun.hotspot.WhiteBox;
+import jdk.test.lib.classloader.ClassUnloadCommon;
+
+/**
+ * Test that verifies that classes are unloaded using concurrent mark with G1 when they are no
+ * longer reachable even when -XX:+VerifyDuringGC is enabled
+ *
+ * The test creates a class loader, uses the loader to load a class and creates an instance
+ * of that class. The it nulls out all the references to the instance, class and class loader
+ * and tries to trigger class unloading using a concurrent mark. Then it verifies that the class
+ * is no longer loaded by the VM.
+ */
+public class UnloadTestWithVerifyDuringGC {
+    private static final WhiteBox wb = WhiteBox.getWhiteBox();
+
+    private static void waitUntilConcMarkFinished() throws Exception {
+        while (wb.g1InConcurrentMark()) {
+            try {
+                Thread.sleep(1);
+            } catch (InterruptedException e) {
+                System.out.println("Got InterruptedException while waiting for concurrent mark to finish");
+                throw e;
+            }
+        }
+    }
+
+    private static void triggerUnloadingWithConcurrentMark() throws Exception {
+        // Try to unload classes using concurrent mark. First wait for any currently running concurrent
+        // cycle.
+        waitUntilConcMarkFinished();
+        wb.g1StartConcMarkCycle();
+        waitUntilConcMarkFinished();
+    }
+
+    private static String className = "test.Empty";
+
+    public static void main(String... args) throws Exception {
+        ClassUnloadCommon.failIf(wb.isClassAlive(className), "is not expected to be alive yet");
+
+        ClassLoader cl = ClassUnloadCommon.newClassLoader();
+        Class<?> c = cl.loadClass(className);
+        Object o = c.newInstance();
+
+        ClassUnloadCommon.failIf(!wb.isClassAlive(className), "should be live here");
+
+        String loaderName = cl.getName();
+        int loadedRefcount = wb.getSymbolRefcount(loaderName);
+        System.out.println("Refcount of symbol " + loaderName + " is " + loadedRefcount);
+
+        // Move everything into the old gen so that concurrent mark can unload.
+        wb.youngGC();
+        cl = null; c = null; o = null;
+        triggerUnloadingWithConcurrentMark();
+
+        ClassUnloadCommon.failIf(wb.isClassAlive(className), "should have been unloaded");
+
+        int unloadedRefcount = wb.getSymbolRefcount(loaderName);
+        System.out.println("Refcount of symbol " + loaderName + " is " + unloadedRefcount);
+        ClassUnloadCommon.failIf(unloadedRefcount != (loadedRefcount - 1), "Refcount must be decremented");
+   }
+}