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

[David Holmes]

On Thu, 14 Sep 2023 18:59:14 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> For non-parallel capable class loaders, the class is not put in the placeholders table, so a verification at the wrong time will hit this assert. This change removes the invalid assert.
> While studying this code, we also noticed that if an error occurs during class loading after the klass is put in the LoaderConstraints table but before adding to the dictionary, we weren't removing the klass from the LoaderConstraints table.  This change also fixes this.
> Tested with inserting a LoaderConstraintTable::verify while running jck tests.  Also tested with failure insertion after the addClass() call to throw OutOfMemoryError.  Also tested with tier1-4.

Simply removing the assertion rather than trying to always use the PHT seems a reasonable simplication.

I'm not really following the remove-on-error part for the constraint table though. It is isn't clear to me what order things are happening in with regard to the loader constraint table addition, the loading of the class (we managed to load it as we have the `instanceKlass` for it) and the subsequent determination that we shouldn't actually define it.

Thanks.

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

src/hotspot/share/classfile/loaderConstraints.cpp line 453:

> 451:   LoaderConstraint *p = find_loader_constraint(name, loader);
> 452:   if (p != nullptr && p->klass() != nullptr && p->klass() == klass) {
> 453:     log_info(class, loader, constraints)("removing klass %s failed to load", name->as_C_string());

Suggestion, either:

"removing klass %s: failed to load"

or

"removing klass %s as it failed to load"

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?

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

PR Review: https://git.openjdk.org/jdk/pull/15752#pullrequestreview-1628022696
PR Review Comment: https://git.openjdk.org/jdk/pull/15752#discussion_r1326637173
PR Review Comment: https://git.openjdk.org/jdk/pull/15752#discussion_r1326636566
PR Review Comment: https://git.openjdk.org/jdk/pull/15752#discussion_r1326639541