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

[David Holmes]

On Fri, 14 Jul 2023 14:00:13 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> This is a better fix and cleanup to make placeholder use for non-parallel capable class loaders the same as the other sort of class loaders, and fixes potential problems with thread safety in the definer field of the PlaceholderEntry.   It's not easy to backport, which is why I chose the other fix for this bug.
> Tested with tier1-4 with and without injected LoaderConstrantTable::verify code.

src/hotspot/share/classfile/placeholders.cpp line 330:

> 328:   if (definer_acquire() != nullptr) {
> 329:     st->print(", definer ");
> 330:     definer_acquire()->print_value_on(st);

Ignoring whether we need acquire semantics at all for definer, you never need to issue two acquires like this. The first acquire performs any necessary memory synchronization related to the associated release. And if the definer could actually be changing concurrently at this point then your null check is ineffective.

src/hotspot/share/classfile/placeholders.hpp line 121:

> 119: 
> 120:   JavaThread*        definer_acquire()     const { return Atomic::load_acquire(&_definer); }
> 121:   void               release_set_definer(JavaThread* definer) { Atomic::release_store(&_definer, definer); }

I cannot see any lock-free access to the definer thread that would require these acquire/release semantics. AFAICS all reading and writing of the definer happen under the SD lock - even the `print_on` usage is ultimately under the SD lock.

src/hotspot/share/classfile/systemDictionary.cpp line 901:

> 899:   // class loaders, handle parallel define requests. In this case, find_or_define_instance_class may return a different
> 900:   // InstanceKlass, in which case the old k would be deallocated
> 901:   k = find_or_define_instance_class(h_name, class_loader, k, CHECK_NULL);

Now that you call this for parallel-capable and non-parallel-capable loaders the comments preceding the definition of `find_or_define_instance_class` no longer make complete sense.

src/hotspot/share/classfile/systemDictionary.cpp line 1375:

> 1373:   // If a class loader calls define_instance_class instead of
> 1374:   // find_or_define_instance_class to get here, we have a timing
> 1375:   // hole with systemDictionary updates and check_constraints

AFAICS the only call to `define_instance_class` is now through `find_or_define_instance_class` so this comment no longer makes much sense to me.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14889#discussion_r1278773731
PR Review Comment: https://git.openjdk.org/jdk/pull/14889#discussion_r1278776678
PR Review Comment: https://git.openjdk.org/jdk/pull/14889#discussion_r1278784608
PR Review Comment: https://git.openjdk.org/jdk/pull/14889#discussion_r1278779985