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

[Stefan Karlsson]

On Mon, 30 May 2022 17:09:35 GMT, Thomas Schatzl <tschatzl 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

It's unclear to me why this works. You've removed the "keep alive" property from the iterator, but it looks like the verification code still uses "keep alive" loads on the class loaders.

Could you explain how this patch prevents those class loaders from being kept alive?

src/hotspot/share/classfile/classLoaderDataGraph.cpp line 344:

> 342:     if (cld != NULL) {
> 343:       // Keep cld that is being returned alive.
> 344:       _holder = Handle(_thread, keep_alive ? cld->holder() : cld->holder_no_keepalive());

It's always scary when no_keepalive() read oops are stored, because then you need to make sure that they don't accidentally leak out over a safepoint. In this case we don't ever use _holder.

I wonder if it wouldn't be better to remove _handle and rewrite the code as:

if (keep_alive) {
  // Keep cld that is being returned alive.
  Handle(_thread, cld->holder);
}

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.

src/hotspot/share/classfile/classLoaderDataGraph.cpp line 665:

> 663:   while (ClassLoaderData* cld = iter.get_next()) {
> 664:     cld->verify();
> 665:   }

The same comment as for the verify_dictionary() function.

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

Changes requested by stefank (Reviewer).

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