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

[Ioi Lam]

On Wed, 11 Sep 2024 21:24:53 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.

We have this pattern (in two different functions) for handling loading failure of `load_shared_class()`:


if (ik != nullptr && ik->is_shared_boot_class() && !ik->shared_loading_failed()) {
  SharedClassLoadingMark slm(THREAD, ik);
  k = load_shared_class(ik, class_loader, Handle(), nullptr,  pkg_entry, CHECK_NULL);
}
....
~SharedClassLoadingMark() {
  if (HAS_PENDING_EXCEPTION) {
    if (_klass->is_shared()) {
      _klass->set_shared_loading_failed();
    }
  }
}


As Matias discovered, `load_shared_class` could "fail" by returning `nullptr` without throwing an exception. Although moving `Atomic::add` a few lines down catches one of such cases, it's not guaranteed that all subsequent failures inside `load_shared_class()` will always throw an exception.

(We cannot make `load_shared_class()` to always throw exception on failure, as there are normal reasons where a shared class cannot be used).

Also, this pattern forces the caller to know that `load_shared_class` must be used with `SharedClassLoadingMark`, so too much implementation details are leaked to the caller. In fact, we seem to have a bug where `SystemDictionary::load_shared_lambda_proxy_class()` calls `load_shared_class` without using `SharedClassLoadingMark`.

I think it's better to remove `SharedClassLoadingMark`, and change `load_shared_class` to this:


InstanceKlass* load_shared_class(InstanceKlass* ik,..., TRAPS) {
  InstanceKlass* loaded_ik = load_shared_class_impl(ik, ...., THREAD);  <-- old body of load_shared_class()
  // we *may* have a pending exception here
  if (loaded_ik != ik) {
    assert(loaded_ik == nullptr, "cannot load alternative class");
    ik->set_shared_loading_failed(); // ik may be partially updated. Never load it again.
  }
  return loaded_ik;
}


@coleenp: `load_shared_class()` needs exclusive access to the class and it assumes that the exclusivity is provided by the placeholder mechanism. For sanity, Matias is looking at the core dump to confirm that we don't have two threads calling `load_shared_class()` concurrently.

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

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