8253081: G1 fails on stale objects in archived module graph in Open Archive regions

Change the handling of Open Archive areas, instead of assuming that everything in there is live always, a root containing references to all live root objects is provided. Adapt G1 to handle Open Archive regions as any other old region apart from never compacting or evacuating them.

Co-authored-by: Ioi Lam <iklam@openjdk.org>
Reviewed-by: kbarrett, sjohanss, redestad

Author: Thomas Schatzl

src/hotspot/share/classfile/classLoaderDataShared.cpp

@@ -59,6 +59,7 @@ class ArchivedClassLoaderData {
   }
 
   void restore(ClassLoaderData* loader_data, bool do_entries, bool do_oops);
+  void clear_archived_oops();
 };
 
 static ArchivedClassLoaderData _archived_boot_loader_data;
@@ -123,6 +124,15 @@ void ArchivedClassLoaderData::restore(ClassLoaderData* loader_data, bool do_entr
   }
 }
 
+void ArchivedClassLoaderData::clear_archived_oops() {
+  assert(UseSharedSpaces, "must be");
+  if (_modules != NULL) {
+    for (int i = 0; i < _modules->length(); i++) {
+      _modules->at(i)->clear_archived_oops();
+    }
+  }
+}
+
 // ------------------------------
 
 static ClassLoaderData* null_class_loader_data() {
@@ -183,6 +193,13 @@ void ClassLoaderDataShared::serialize(SerializeClosure* f) {
   }
 }
 
+void ClassLoaderDataShared::clear_archived_oops() {
+  assert(UseSharedSpaces && !MetaspaceShared::use_full_module_graph(), "must be");
+  _archived_boot_loader_data.clear_archived_oops();
+  _archived_platform_loader_data.clear_archived_oops();
+  _archived_system_loader_data.clear_archived_oops();
+}
+
 oop ClassLoaderDataShared::restore_archived_oops_for_null_class_loader_data() {
   assert(UseSharedSpaces && MetaspaceShared::use_full_module_graph(), "must be");
   _archived_boot_loader_data.restore(null_class_loader_data(), false, true);

src/hotspot/share/classfile/classLoaderDataShared.hpp

@@ -39,6 +39,7 @@ class ClassLoaderDataShared : AllStatic {
   static void init_archived_tables();
   static void init_archived_oops();
   static void serialize(SerializeClosure* f);
+  static void clear_archived_oops();
   static oop  restore_archived_oops_for_null_class_loader_data();
   static void restore_java_platform_loader_from_archive(ClassLoaderData* loader_data);
   static void restore_java_system_loader_from_archive(ClassLoaderData* loader_data);

src/hotspot/share/classfile/javaClasses.cpp

@@ -890,14 +890,14 @@ void java_lang_Class::fixup_mirror(Klass* k, TRAPS) {
     }
   }
 
-  if (k->is_shared() && k->has_raw_archived_mirror()) {
+  if (k->is_shared() && k->has_archived_mirror_index()) {
     if (HeapShared::open_archive_heap_region_mapped()) {
       bool present = restore_archived_mirror(k, Handle(), Handle(), Handle(), CHECK);
       assert(present, "Missing archived mirror for %s", k->external_name());
       return;
     } else {
       k->clear_java_mirror_handle();
-      k->clear_has_raw_archived_mirror();
+      k->clear_archived_mirror_index();
     }
   }
   create_mirror(k, Handle(), Handle(), Handle(), Handle(), CHECK);
@@ -1163,9 +1163,9 @@ oop java_lang_Class::archive_mirror(Klass* k, TRAPS) {
          "HeapShared::is_heap_object_archiving_allowed() must be true");
 
   // Mirror is already archived
-  if (k->has_raw_archived_mirror()) {
-    assert(k->archived_java_mirror_raw() != NULL, "no archived mirror");
-    return k->archived_java_mirror_raw();
+  if (k->has_archived_mirror_index()) {
+    assert(k->archived_java_mirror() != NULL, "no archived mirror");
+    return k->archived_java_mirror();
   }
 
   // No mirror
@@ -1197,9 +1197,7 @@ oop java_lang_Class::archive_mirror(Klass* k, TRAPS) {
     return NULL;
   }
 
-  k->set_archived_java_mirror_raw(archived_mirror);
-
-  k->set_has_raw_archived_mirror();
+  k->set_archived_java_mirror(archived_mirror);
 
   ResourceMark rm;
   log_trace(cds, heap, mirror)(
@@ -1317,10 +1315,11 @@ bool java_lang_Class::restore_archived_mirror(Klass *k,
     return true;
   }
 
-  oop m = HeapShared::materialize_archived_object(k->archived_java_mirror_raw_narrow());
-  if (m == NULL) {
-    return false;
-  }
+  oop m = k->archived_java_mirror();
+  assert(m != NULL, "must have stored non-null archived mirror");
+
+  // Sanity: clear it now to prevent re-initialization if any of the following fails
+  k->clear_archived_mirror_index();
 
   // mirror is archived, restore
   log_debug(cds, mirror)("Archived mirror is: " PTR_FORMAT, p2i(m));
@@ -1346,7 +1345,6 @@ bool java_lang_Class::restore_archived_mirror(Klass *k,
   }
 
   k->set_java_mirror(mirror);
-  k->clear_has_raw_archived_mirror();
 
   set_mirror_module_field(k, mirror, module, THREAD);
 

src/hotspot/share/classfile/moduleEntry.cpp

@@ -467,7 +467,7 @@ void ModuleEntry::init_archived_oops() {
   if (module_obj != NULL) {
     oop m = HeapShared::find_archived_heap_object(module_obj);
     assert(m != NULL, "sanity");
-    _archived_module_narrow_oop = CompressedOops::encode(m);
+    _archived_module_index = HeapShared::append_root(m);
   }
   assert(shared_protection_domain() == NULL, "never set during -Xshare:dump");
   // Clear handles and restore at run time. Handles cannot be archived.
@@ -481,8 +481,8 @@ void ModuleEntry::load_from_archive(ClassLoaderData* loader_data) {
   JFR_ONLY(INIT_ID(this);)
 }
 
-void ModuleEntry::restore_archive_oops(ClassLoaderData* loader_data) {
-  Handle module_handle(Thread::current(), HeapShared::materialize_archived_object(_archived_module_narrow_oop));
+void ModuleEntry::restore_archived_oops(ClassLoaderData* loader_data) {
+  Handle module_handle(Thread::current(), HeapShared::get_root(_archived_module_index, /*clear=*/true));
   assert(module_handle.not_null(), "huh");
   set_module(loader_data->add_handle(module_handle));
 
@@ -495,6 +495,10 @@ void ModuleEntry::restore_archive_oops(ClassLoaderData* loader_data) {
   }
 }
 
+void ModuleEntry::clear_archived_oops() {
+  HeapShared::clear_root(_archived_module_index);
+}
+
 static int compare_module_by_name(ModuleEntry* a, ModuleEntry* b) {
   assert(a == b || a->name() != b->name(), "no duplicated names");
   return a->name()->fast_compare(b->name());
@@ -561,7 +565,7 @@ void ModuleEntryTable::restore_archived_oops(ClassLoaderData* loader_data, Array
   assert(UseSharedSpaces, "runtime only");
   for (int i = 0; i < archived_modules->length(); i++) {
     ModuleEntry* archived_entry = archived_modules->at(i);
-    archived_entry->restore_archive_oops(loader_data);
+    archived_entry->restore_archived_oops(loader_data);
   }
 }
 #endif // INCLUDE_CDS_JAVA_HEAP

src/hotspot/share/classfile/moduleEntry.hpp

@@ -76,7 +76,7 @@ class ModuleEntry : public HashtableEntry<Symbol*, mtModule> {
   bool _must_walk_reads;               // walk module's reads list at GC safepoints to purge out dead modules
   bool _is_open;                       // whether the packages in the module are all unqualifiedly exported
   bool _is_patched;                    // whether the module is patched via --patch-module
-  CDS_JAVA_HEAP_ONLY(narrowOop _archived_module_narrow_oop;)
+  CDS_JAVA_HEAP_ONLY(int _archived_module_index;)
 
   JFR_ONLY(DEFINE_TRACE_ID_FIELD;)
   enum {MODULE_READS_SIZE = 101};      // Initial size of list of modules that the module can read.
@@ -201,7 +201,8 @@ class ModuleEntry : public HashtableEntry<Symbol*, mtModule> {
   static Array<ModuleEntry*>* write_growable_array(GrowableArray<ModuleEntry*>* array);
   static GrowableArray<ModuleEntry*>* restore_growable_array(Array<ModuleEntry*>* archived_array);
   void load_from_archive(ClassLoaderData* loader_data);
-  void restore_archive_oops(ClassLoaderData* loader_data);
+  void restore_archived_oops(ClassLoaderData* loader_data);
+  void clear_archived_oops();
 #endif
 };
 

src/hotspot/share/classfile/systemDictionary.cpp

@@ -1979,8 +1979,12 @@ void SystemDictionary::initialize(TRAPS) {
   oop lock_obj = oopFactory::new_intArray(0, CHECK);
   _system_loader_lock_obj = OopHandle(Universe::vm_global(), lock_obj);
 
-  // Initialize basic classes
+  // Resolve basic classes
   resolve_well_known_classes(CHECK);
+  // Resolve classes used by archived heap objects
+  if (UseSharedSpaces) {
+    HeapShared::resolve_classes(CHECK);
+  }
 }
 
 // Compact table of directions on the initialization of klasses:

src/hotspot/share/classfile/systemDictionaryShared.cpp

@@ -75,6 +75,7 @@ bool SystemDictionaryShared::_dump_in_progress = false;
 
 class DumpTimeSharedClassInfo: public CHeapObj<mtClass> {
   bool                         _excluded;
+  bool                         _is_early_klass;
 public:
   struct DTLoaderConstraint {
     Symbol* _name;
@@ -121,6 +122,7 @@ class DumpTimeSharedClassInfo: public CHeapObj<mtClass> {
     _clsfile_size = -1;
     _clsfile_crc32 = -1;
     _excluded = false;
+    _is_early_klass = JvmtiExport::is_early_phase();
     _verifier_constraints = NULL;
     _verifier_constraint_flags = NULL;
     _loader_constraints = NULL;
@@ -177,6 +179,11 @@ class DumpTimeSharedClassInfo: public CHeapObj<mtClass> {
     return _excluded || _failed_verification || _klass == NULL;
   }
 
+  // Was this class loaded while JvmtiExport::is_early_phase()==true
+  bool is_early_klass() {
+    return _is_early_klass;
+  }
+
   void set_failed_verification() {
     _failed_verification = true;
   }
@@ -1325,6 +1332,11 @@ bool SystemDictionaryShared::is_hidden_lambda_proxy(InstanceKlass* ik) {
   }
 }
 
+bool SystemDictionaryShared::is_early_klass(InstanceKlass* ik) {
+  DumpTimeSharedClassInfo* info = _dumptime_table->get(ik);
+  return (info != NULL) ? info->is_early_klass() : false;
+}
+
 void SystemDictionaryShared::warn_excluded(InstanceKlass* k, const char* reason) {
   ResourceMark rm;
   log_warning(cds)("Skipping %s: %s", k->name()->as_C_string(), reason);
@@ -2302,8 +2314,8 @@ bool SystemDictionaryShared::empty_dumptime_table() {
 class ArchivedMirrorPatcher {
 protected:
   static void update(Klass* k) {
-    if (k->has_raw_archived_mirror()) {
-      oop m = HeapShared::materialize_archived_object(k->archived_java_mirror_raw_narrow());
+    if (k->has_archived_mirror_index()) {
+      oop m = k->archived_java_mirror();
       if (m != NULL) {
         java_lang_Class::update_archived_mirror_native_pointers(m);
       }

src/hotspot/share/classfile/systemDictionaryShared.hpp

@@ -229,6 +229,7 @@ class SystemDictionaryShared: public SystemDictionary {
 
 public:
   static bool is_hidden_lambda_proxy(InstanceKlass* ik);
+  static bool is_early_klass(InstanceKlass* k);   // Was k loaded while JvmtiExport::is_early_phase()==true
   static Handle init_security_info(Handle class_loader, InstanceKlass* ik, PackageEntry* pkg_entry, TRAPS);
   static InstanceKlass* find_builtin_class(Symbol* class_name);
 

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -804,17 +804,6 @@ void G1CollectedHeap::dealloc_archive_regions(MemRegion* ranges, size_t count) {
   decrease_used(size_used);
 }
 
-oop G1CollectedHeap::materialize_archived_object(oop obj) {
-  assert(is_archived_object(obj), "not an archived obj");
-
-  // Loading an archived object makes it strongly reachable. If it is
-  // loaded during concurrent marking, it must be enqueued to the SATB
-  // queue, shading the previously white object gray.
-  G1BarrierSet::enqueue(obj);
-
-  return obj;
-}
-
 HeapWord* G1CollectedHeap::attempt_allocation_humongous(size_t word_size) {
   ResourceMark rm; // For retrieving the thread names in log messages.
 
@@ -4021,11 +4010,13 @@ void G1CollectedHeap::free_humongous_region(HeapRegion* hr,
   free_region(hr, free_list);
 }
 
-void G1CollectedHeap::remove_from_old_sets(const uint old_regions_removed,
-                                           const uint humongous_regions_removed) {
-  if (old_regions_removed > 0 || humongous_regions_removed > 0) {
+void G1CollectedHeap::remove_from_old_gen_sets(const uint old_regions_removed,
+                                               const uint archive_regions_removed,
+                                               const uint humongous_regions_removed) {
+  if (old_regions_removed > 0 || archive_regions_removed > 0 || humongous_regions_removed > 0) {
     MutexLocker x(OldSets_lock, Mutex::_no_safepoint_check_flag);
     _old_set.bulk_remove(old_regions_removed);
+    _archive_set.bulk_remove(archive_regions_removed);
     _humongous_set.bulk_remove(humongous_regions_removed);
   }
 
@@ -4453,7 +4444,7 @@ void G1CollectedHeap::eagerly_reclaim_humongous_regions() {
   G1FreeHumongousRegionClosure cl(&local_cleanup_list);
   heap_region_iterate(&cl);
 
-  remove_from_old_sets(0, cl.humongous_regions_reclaimed());
+  remove_from_old_gen_sets(0, 0, cl.humongous_regions_reclaimed());
 
   G1HRPrinter* hrp = hr_printer();
   if (hrp->is_active()) {
@@ -4528,22 +4519,23 @@ bool G1CollectedHeap::check_young_list_empty() {
 
 #endif // ASSERT
 
-  // Remove the given HeapRegion from the appropriate region set.
+// Remove the given HeapRegion from the appropriate region set.
 void G1CollectedHeap::prepare_region_for_full_compaction(HeapRegion* hr) {
-  if (hr->is_old()) {
+   if (hr->is_archive()) {
+    _archive_set.remove(hr);
+  } else if (hr->is_humongous()) {
+    _humongous_set.remove(hr);
+  } else if (hr->is_old()) {
     _old_set.remove(hr);
   } else if (hr->is_young()) {
-    // Note that emptying the _young_list is postponed and instead done as
-    // the first step when rebuilding the regions sets again. The reason for
-    // this is that during a full GC string deduplication needs to know if
+    // Note that emptying the eden and survivor lists is postponed and instead
+    // done as the first step when rebuilding the regions sets again. The reason
+    // for this is that during a full GC string deduplication needs to know if
     // a collected region was young or old when the full GC was initiated.
     hr->uninstall_surv_rate_group();
   } else {
     // We ignore free regions, we'll empty the free list afterwards.
-    // We ignore humongous and archive regions, we're not tearing down these
-    // sets.
-    assert(hr->is_archive() || hr->is_free() || hr->is_humongous(),
-           "it cannot be another type");
+    assert(hr->is_free(), "it cannot be another type");
   }
 }
 
@@ -4567,19 +4559,26 @@ class RebuildRegionSetsClosure : public HeapRegionClosure {
   bool _free_list_only;
 
   HeapRegionSet* _old_set;
+  HeapRegionSet* _archive_set;
+  HeapRegionSet* _humongous_set;
+
   HeapRegionManager* _hrm;
 
   size_t _total_used;
 
 public:
   RebuildRegionSetsClosure(bool free_list_only,
                            HeapRegionSet* old_set,
+                           HeapRegionSet* archive_set,
+                           HeapRegionSet* humongous_set,
                            HeapRegionManager* hrm) :
-    _free_list_only(free_list_only),
-    _old_set(old_set), _hrm(hrm), _total_used(0) {
+    _free_list_only(free_list_only), _old_set(old_set), _archive_set(archive_set),
+    _humongous_set(humongous_set), _hrm(hrm), _total_used(0) {
     assert(_hrm->num_free_regions() == 0, "pre-condition");
     if (!free_list_only) {
       assert(_old_set->is_empty(), "pre-condition");
+      assert(_archive_set->is_empty(), "pre-condition");
+      assert(_humongous_set->is_empty(), "pre-condition");
     }
   }
 
@@ -4592,11 +4591,14 @@ class RebuildRegionSetsClosure : public HeapRegionClosure {
     } else if (!_free_list_only) {
       assert(r->rem_set()->is_empty(), "At this point remembered sets must have been cleared.");
 
-      if (r->is_archive() || r->is_humongous()) {
-        // We ignore archive and humongous regions. We left these sets unchanged.
+      if (r->is_humongous()) {
+        _humongous_set->add(r);
+      } else if (r->is_archive()) {
+        _archive_set->add(r);
       } else {
         assert(r->is_young() || r->is_free() || r->is_old(), "invariant");
-        // We now move all (non-humongous, non-old, non-archive) regions to old gen, and register them as such.
+        // We now move all (non-humongous, non-old, non-archive) regions to old gen,
+        // and register them as such.
         r->move_to_old();
         _old_set->add(r);
       }
@@ -4619,7 +4621,9 @@ void G1CollectedHeap::rebuild_region_sets(bool free_list_only) {
     _survivor.clear();
   }
 
-  RebuildRegionSetsClosure cl(free_list_only, &_old_set, &_hrm);
+  RebuildRegionSetsClosure cl(free_list_only,
+                              &_old_set, &_archive_set, &_humongous_set,
+                              &_hrm);
   heap_region_iterate(&cl);
 
   if (!free_list_only) {