8210422: runtime/modules/ModuleStress/ExportModuleStressTest.java - assertion failed: address not aligned: 0x00000008baadbabe

CLDG_lock caused safepoint in inconsistent state

Reviewed-by: lfoltan, eosterlund, kbarrett

src/hotspot/share/classfile/classLoaderData.cpp

@@ -157,6 +157,7 @@ ClassLoaderData::ClassLoaderData(Handle h_class_loader, bool is_unsafe_anonymous
   if (!h_class_loader.is_null()) {
     _class_loader = _handles.add(h_class_loader());
     _class_loader_klass = h_class_loader->klass();
+    initialize_name(h_class_loader);
   }
 
   if (!is_unsafe_anonymous) {
@@ -1077,33 +1078,38 @@ bool ClassLoaderDataGraph::_metaspace_oom = false;
 // ClassLoaderData into the java/lang/ClassLoader object as a hidden field
 ClassLoaderData* ClassLoaderDataGraph::add_to_graph(Handle loader, bool is_unsafe_anonymous) {
 
+  assert_lock_strong(ClassLoaderDataGraph_lock);
 
   ClassLoaderData* cld;
-  {
-    NoSafepointVerifier no_safepoints; // we mustn't GC until we've installed the
-                                       // ClassLoaderData in the loader since the CLD
-                                       // contains oops in _handles that must be walked.
-                                       // GC will find the CLD through the loader after this.
 
-    cld = new ClassLoaderData(loader, is_unsafe_anonymous);
-
-    if (!is_unsafe_anonymous) {
-      // First, Atomically set it
-      ClassLoaderData* old = java_lang_ClassLoader::cmpxchg_loader_data(cld, loader(), NULL);
-      if (old != NULL) {
-        delete cld;
-        // Returns the data.
-        return old;
-      }
+  // First check if another thread beat us to creating the CLD and installing
+  // it into the loader while we were waiting for the lock.
+  if (!is_unsafe_anonymous && loader.not_null()) {
+    cld = java_lang_ClassLoader::loader_data_acquire(loader());
+    if (cld != NULL) {
+      return cld;
     }
   }
 
-  MutexLocker ml(ClassLoaderDataGraph_lock);
+  // We mustn't GC until we've installed the ClassLoaderData in the Graph since the CLD
+  // contains oops in _handles that must be walked.  GC doesn't walk CLD from the
+  // loader oop in all collections, particularly young collections.
+  NoSafepointVerifier no_safepoints;
 
-  // We won the race, and therefore the task of adding the data to the list of
-  // class loader data
+  cld = new ClassLoaderData(loader, is_unsafe_anonymous);
+
+  // First install the new CLD to the Graph.
   cld->set_next(_head);
   _head = cld;
+
+  // Next associate with the class_loader.
+  if (!is_unsafe_anonymous) {
+    // Use OrderAccess, since readers need to get the loader_data only after
+    // it's added to the Graph
+    java_lang_ClassLoader::release_set_loader_data(loader(), cld);
+  }
+
+  // Lastly log, if requested
   LogTarget(Trace, class, loader, data) lt;
   if (lt.is_enabled()) {
     ResourceMark rm;
@@ -1116,12 +1122,8 @@ ClassLoaderData* ClassLoaderDataGraph::add_to_graph(Handle loader, bool is_unsaf
 }
 
 ClassLoaderData* ClassLoaderDataGraph::add(Handle loader, bool is_unsafe_anonymous) {
+  MutexLocker ml(ClassLoaderDataGraph_lock);
   ClassLoaderData* loader_data = add_to_graph(loader, is_unsafe_anonymous);
-  // Initialize _name and _name_and_id after the loader data is added to the
-  // CLDG because adding the Symbol for _name and _name_and_id might safepoint.
-  if (loader.not_null()) {
-    loader_data->initialize_name(loader);
-  }
   return loader_data;
 }
 

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

@@ -46,7 +46,7 @@ inline ClassLoaderData* ClassLoaderData::class_loader_data_or_null(oop loader) {
   if (loader == NULL) {
     return ClassLoaderData::the_null_class_loader_data();
   }
-  return java_lang_ClassLoader::loader_data(loader);
+  return java_lang_ClassLoader::loader_data_acquire(loader);
 }
 
 inline ClassLoaderData* ClassLoaderData::class_loader_data(oop loader) {
@@ -60,7 +60,7 @@ inline ClassLoaderData *ClassLoaderDataGraph::find_or_create(Handle loader) {
   guarantee(loader() != NULL && oopDesc::is_oop(loader()), "Loader must be oop");
   // Gets the class loader data out of the java/lang/ClassLoader object, if non-null
   // it's already in the loader_data, so no need to add
-  ClassLoaderData* loader_data= java_lang_ClassLoader::loader_data(loader());
+  ClassLoaderData* loader_data= java_lang_ClassLoader::loader_data_acquire(loader());
   if (loader_data) {
      return loader_data;
   }

src/hotspot/share/classfile/classLoaderStats.cpp

@@ -139,7 +139,7 @@ void ClassLoaderStatsClosure::print() {
 
 
 void ClassLoaderStatsClosure::addEmptyParents(oop cl) {
-  while (cl != NULL && java_lang_ClassLoader::loader_data(cl) == NULL) {
+  while (cl != NULL && java_lang_ClassLoader::loader_data_acquire(cl) == NULL) {
     // This classloader has not loaded any classes
     ClassLoaderStats** cls_ptr = _stats->get(cl);
     if (cls_ptr == NULL) {

src/hotspot/share/classfile/javaClasses.cpp

@@ -4027,9 +4027,9 @@ int  java_lang_ClassLoader::name_offset = -1;
 int  java_lang_ClassLoader::nameAndId_offset = -1;
 int  java_lang_ClassLoader::unnamedModule_offset = -1;
 
-ClassLoaderData* java_lang_ClassLoader::loader_data(oop loader) {
+ClassLoaderData* java_lang_ClassLoader::loader_data_acquire(oop loader) {
   assert(loader != NULL && oopDesc::is_oop(loader), "loader must be oop");
-  return HeapAccess<>::load_at(loader, _loader_data_offset);
+  return HeapAccess<MO_ACQUIRE>::load_at(loader, _loader_data_offset);
 }
 
 ClassLoaderData* java_lang_ClassLoader::loader_data_raw(oop loader) {
@@ -4037,9 +4037,9 @@ ClassLoaderData* java_lang_ClassLoader::loader_data_raw(oop loader) {
   return RawAccess<>::load_at(loader, _loader_data_offset);
 }
 
-ClassLoaderData* java_lang_ClassLoader::cmpxchg_loader_data(ClassLoaderData* new_data, oop loader, ClassLoaderData* expected_data) {
+void java_lang_ClassLoader::release_set_loader_data(oop loader, ClassLoaderData* new_data) {
   assert(loader != NULL && oopDesc::is_oop(loader), "loader must be oop");
-  return HeapAccess<>::atomic_cmpxchg_at(new_data, loader, _loader_data_offset, expected_data);
+  HeapAccess<MO_RELEASE>::store_at(loader, _loader_data_offset, new_data);
 }
 
 #define CLASSLOADER_FIELDS_DO(macro) \

src/hotspot/share/classfile/javaClasses.hpp

@@ -1322,9 +1322,9 @@ class java_lang_ClassLoader : AllStatic {
   static void compute_offsets();
   static void serialize_offsets(SerializeClosure* f) NOT_CDS_RETURN;
 
-  static ClassLoaderData* loader_data(oop loader);
+  static ClassLoaderData* loader_data_acquire(oop loader);
   static ClassLoaderData* loader_data_raw(oop loader);
-  static ClassLoaderData* cmpxchg_loader_data(ClassLoaderData* new_data, oop loader, ClassLoaderData* expected_data);
+  static void release_set_loader_data(oop loader, ClassLoaderData* new_data);
 
   static oop parent(oop loader);
   static oop name(oop loader);

src/hotspot/share/gc/parallel/psCompactionManager.cpp

@@ -183,7 +183,7 @@ void InstanceMirrorKlass::oop_pc_follow_contents(oop obj, ParCompactionManager*
 void InstanceClassLoaderKlass::oop_pc_follow_contents(oop obj, ParCompactionManager* cm) {
   InstanceKlass::oop_pc_follow_contents(obj, cm);
 
-  ClassLoaderData * const loader_data = java_lang_ClassLoader::loader_data(obj);
+  ClassLoaderData * const loader_data = java_lang_ClassLoader::loader_data_acquire(obj);
   if (loader_data != NULL) {
     cm->follow_class_loader(loader_data);
   }

src/hotspot/share/jfr/periodic/jfrPeriodic.cpp

@@ -466,9 +466,9 @@ public:
 
   bool do_entry(oop const& key, ClassLoaderStats* const& cls) {
     const ClassLoaderData* this_cld = cls->_class_loader != NULL ?
-      java_lang_ClassLoader::loader_data(cls->_class_loader) : (ClassLoaderData*)NULL;
+      java_lang_ClassLoader::loader_data_acquire(cls->_class_loader) : NULL;
     const ClassLoaderData* parent_cld = cls->_parent != NULL ?
-      java_lang_ClassLoader::loader_data(cls->_parent) : (ClassLoaderData*)NULL;
+      java_lang_ClassLoader::loader_data_acquire(cls->_parent) : NULL;
     EventClassLoaderStatistics event;
     event.set_classLoader(this_cld);
     event.set_parentClassLoader(parent_cld);

src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp

@@ -129,7 +129,7 @@ JvmtiGetLoadedClasses::getClassLoaderClasses(JvmtiEnv *env, jobject initiatingLo
     // requested, so only need to walk this loader's ClassLoaderData
     // dictionary, or the NULL ClassLoaderData dictionary for bootstrap loader.
     if (loader != NULL) {
-      ClassLoaderData* data = java_lang_ClassLoader::loader_data(loader);
+      ClassLoaderData* data = java_lang_ClassLoader::loader_data_acquire(loader);
       // ClassLoader may not be used yet for loading.
       if (data != NULL && data->dictionary() != NULL) {
         data->dictionary()->all_entries_do(&closure);

src/hotspot/share/prims/whitebox.cpp

@@ -1504,7 +1504,7 @@ WB_ENTRY(jlong, WB_AllocateMetaspace(JNIEnv* env, jobject wb, jobject class_load
 
   oop class_loader_oop = JNIHandles::resolve(class_loader);
   ClassLoaderData* cld = class_loader_oop != NULL
-      ? java_lang_ClassLoader::loader_data(class_loader_oop)
+      ? java_lang_ClassLoader::loader_data_acquire(class_loader_oop)
       : ClassLoaderData::the_null_class_loader_data();
 
   void* metadata = MetadataFactory::new_array<u1>(cld, WhiteBox::array_bytes_to_length((size_t)size), thread);
@@ -1515,7 +1515,7 @@ WB_END
 WB_ENTRY(void, WB_FreeMetaspace(JNIEnv* env, jobject wb, jobject class_loader, jlong addr, jlong size))
   oop class_loader_oop = JNIHandles::resolve(class_loader);
   ClassLoaderData* cld = class_loader_oop != NULL
-      ? java_lang_ClassLoader::loader_data(class_loader_oop)
+      ? java_lang_ClassLoader::loader_data_acquire(class_loader_oop)
       : ClassLoaderData::the_null_class_loader_data();
 
   MetadataFactory::free_array(cld, (Array<u1>*)(uintptr_t)addr);