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

[Coleen Phillimore]

On Mon, 31 Jul 2023 04:09:43 GMT, David Holmes <dholmes 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.

Since I added the acquire version, I wanted to remove the non-acquire version to avoid mistakes.

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

You're right.  I thought this access required an acquire but it is always after you relock the SystemDictionary_lock, so it will refetch the field from probe ?


    // Acquire define token for this class/classloader
    PlaceholderEntry* probe = PlaceholderTable::find_and_add(name_h, loader_data,
                                                             PlaceholderTable::DEFINE_CLASS, nullptr, THREAD);
...
    while (probe->definer_acquire() != nullptr) {
      SystemDictionary_lock->wait();
    }

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

This comment is still true.  non bootstrap parallel-capable class loaders might return a different k.

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

It seems that the comment was describing a bug someone once had.  I can remove it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14889#discussion_r1279433601
PR Review Comment: https://git.openjdk.org/jdk/pull/14889#discussion_r1279405800
PR Review Comment: https://git.openjdk.org/jdk/pull/14889#discussion_r1279412723
PR Review Comment: https://git.openjdk.org/jdk/pull/14889#discussion_r1279410493