RFR: 8310874: Runthese30m crashes with klass should be in the placeholders during verification [v2]

[Coleen Phillimore]

On Fri, 15 Sep 2023 00:08:09 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add comments about remove_failed_loaded_class.
>
> src/hotspot/share/classfile/loaderConstraints.cpp line 447:
> 
>> 445: 
>> 446: // Removes a class that was added to the table then class loading subsequently failed for this class.
>> 447: void LoaderConstraintTable::remove_failed_loaded_klass(InstanceKlass* klass,
> 
> This only seems to null out the klass ptr not remove the constraint. ??

Right.  The constraint is added when we are checking the class signatures (check_signature_loaders) - we add the constraint as class name and the class loader.  When the class is loaded in SystemDictionary::check_constraints, we add the InstanceKlass that was loaded in the loader constraint entry for the name.
Further checks are make sure that the klass in the constraint is the same as the class currently being loaded, ie: 

        ss.print("loader constraint violation: loader %s", loader_data->loader_name_and_id());
        ss.print(" wants to load %s %s.",
                 k->external_kind(), k->external_name());
        Klass *existing_klass = LoaderConstraintTable::find_constrained_klass(name, loader_data);
        if (existing_klass != nullptr && existing_klass->class_loader_data() != loader_data) {
          ss.print(" A different %s with the same name was previously loaded by %s. (%s)",


Ioi wanted me to add a comment that this isn't a leak.  It's not.  This constraint (name, loader) is valid as long as the class loader is loaded.  It is removed when the class loader unloads in SystemDictionary::do_unloading.

> src/hotspot/share/classfile/systemDictionary.cpp line 1530:
> 
>> 1528:     k->class_loader_data()->add_to_deallocate_list(k);
>> 1529: 
>> 1530:     // Also remove this InstanceKlass from the LoaderConstraintTable if added.
> 
> Why do we not do this in the non-exception case? If `k` was a duplicate class then there should be a constraint entry under `defined_k`, so any entry under `k` should also be removed shouldn't it?

This non-error path is for -XX:+AllowParallelDefine case.  In this case, the class that we're removing is from the second thread.  The second thread waits on the SystemDictionary_lock and the first thread for this class does the constraint checking, possibly adding it to the LoaderConstraintsTable, then adding it to the dictionary.  Then the second class returns the InstanceKlass from the first thread.  Therefore, the second InstanceKlass needs to be removed but it's never going to be in the LoaderConstraintTable.

https://github.com/openjdk/jdk/blob/bd32fa4cc3c76c159764ed1494d47cde22e617df/src/hotspot/share/classfile/systemDictionary.cpp#L1478

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15752#discussion_r1326674851
PR Review Comment: https://git.openjdk.org/jdk/pull/15752#discussion_r1326680888