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

[Matias Saavedra Silva]

On Wed, 18 Sep 2024 03:23:37 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> 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.

Thanks for the reviews and comments @iklam @dholmes-ora @coleenp and @kimbarrett!

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

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