RFR: 8280454: G1: ClassLoaderData verification keeps CLDs live that causes problems with VerifyDuringGC during Remark

[Thomas Schatzl]

On Wed, 1 Jun 2022 06:09:06 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Hi all,
>> 
>>   can I have reviews for this change that makes a few CLDG iterations no-keepalive during GC pauses?
>> 
>> This fixes a bug where the `VerifyDuringGC` verification otherwise kept alive all classes, effectively disabling class unloading during (g1) concurrent mark.
>> 
>> Afaict this verification code is only every called during GC pauses, so the change should be fine.
>> 
>> As for the change itself, I tried to avoid cluttering the code with `ClassLoaderDataGraphIterator<>` by using typedefs - unfortunately only C++ 17 allows omitting the `<>` if all template parameters are defaulted.
>> 
>> There is also a new test case for just this.
>> 
>> Testing: tier1-5
>> 
>> Thanks,
>>   Thomas
>
> src/hotspot/share/classfile/classLoaderDataGraph.cpp line 437:
> 
>> 435:   while (ClassLoaderData* cld = iter.get_next()) {
>> 436:     if (cld->dictionary() != nullptr) {
>> 437:       cld->dictionary()->verify();
> 
> If I'm reading the code correctly, it seems like this function keeps the class loader alive:
> 
> void Dictionary::verify() {
>   guarantee(number_of_entries() >= 0, "Verify of dictionary failed");
> 
>   ClassLoaderData* cld = loader_data();
>   // class loader must be present;  a null class loader is the
>   // bootstrap loader
>   guarantee(cld != NULL &&
>             (cld->the_null_class_loader_data() || cld->class_loader()->is_instance()),
>             "checking type of class_loader");
> 
> 
> The `cld->class_loader()` call resolves the OopHandle, which should keep it alive.

That never happens because afaict `cld->the_null_class_loader_data()` is always non-NULL except during very early bootstrapping. I do not think it can ever be NULL for a java instantiated class loader, so the second clause is never executed there. Some additional logging in the test proved that.

I can find three where this idiom is used to see whether the null CLD is initialized - I could add a static `is_null_class_loader_data_initialized()` predicate to `ClassLoaderData` - preferably in a separate CR.

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

PR: https://git.openjdk.java.net/jdk/pull/8949