RFR: 8338693: assert(Atomic::add(&ik->_shared_class_load_count, 1) == 1) failed: shared class loaded more than once [v2]

[Ioi Lam]

On Tue, 17 Sep 2024 19:52:38 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> The test vmTestbase/nsk/sysdict/vm/stress/chain/chain007/chain007.java  occasionally fails due to the number of shared classes being incremented even though the shared class is not loaded. This patch moves the assert to only increment once loading is possible.
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Ioi suggestion

After looking at the code further, I think my previous suggestion (getting rid of SharedClassLoadingMark) won't work.

The CDS class loading code has this expectation:

- After we start loading a class `k` from `SystemDictionary::load_shared_class()`, if we run into *any* failure before `k` is added to the system dictionary, we must mark `k` failed, so that we will never try to load it again.

`SystemDictionary::load_shared_class()` only gets us halfway: the InstanceKlass and its mirror are restored (as if an InstanceKlass has been returned by the ClassFileParser), but we haven't tried to insert it into the system dictionary yet.

Later, we do this to insert it into the system dictionary


      CDS_ONLY(SharedClassLoadingMark slm(THREAD, k);)
      k = find_or_define_instance_class(class_name, class_loader, k, CHECK_NULL);


I think almost the paths will throw an exception on failure, which will be caught by SharedClassLoadingMark.

The only places we don't throw exceptions are the two check at the top of `SystemDictionary::load_shared_class()` that return null to indicate that the shared class cannot be used due to "normal" conditions.

So to fix the symptoms reported by the bug, I think we can do this:


 InstanceKlass* SystemDictionary::load_shared_class(InstanceKlass* ik, ...) {
   if (!is_shared_class_visible(class_name, ik, pkg_entry, class_loader)) {
+    ik->set_shared_loading_failed();
     return nullptr;
   }

   if (!check_shared_class_super_types(ik, class_loader, protection_domain, THREAD)) {
+    ik->set_shared_loading_failed();
     return nullptr;
   }


But we probably should redesign the error checking mechanism to make it more reliable and easier to understand.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/20955#issuecomment-2357416255