RFR: 8261921: ClassListParser::current should be used only by main thread

[David Holmes]

On Thu, 18 Feb 2021 02:25:38 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.
> 
> The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.
> 
> I also did some renaming and comment cleaning.

Hi Ioi,

Thanks for the side-bar explanations on the background here.

Changes seem fine, but a couple of comments below.

Thanks,
David

src/hotspot/share/classfile/classListParser.cpp line 80:

> 78:   assert(_instance == NULL, "must be singleton");
> 79:   _instance = this;
> 80:   Atomic::store(&_parsing_thread, Thread::current());

The use of atomics here seems overkill as we have no issues with atomicity, tearing or ordering, when checking if the field is the current thread. You can't get a false positive and the current thread must always see the value it set.

src/hotspot/share/classfile/classListParser.cpp line 94:

> 92:   Atomic::store(&_parsing_thread, (Thread*)NULL);
> 93:   _instance = NULL;
> 94: }

This gives the impression that we can create a new ClassListParser later, potentially in a different thread - is that really the case?

src/hotspot/share/classfile/classListParser.hpp line 86:

> 84:   };
> 85: 
> 86:   static Thread* _parsing_thread;    // the thread that created _instance

I forget whether we still want to use `volatile` here or not.

src/hotspot/share/classfile/systemDictionaryShared.hpp line 251:

> 249:   static bool add_unregistered_class(InstanceKlass* k, TRAPS);
> 250:   static InstanceKlass* lookup_super_for_unregistered_class(Symbol* class_name,
> 251:                             Symbol* super_name,  bool is_superclass);

Indentation is ad-hoc here, please align Symbol

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

Marked as reviewed by dholmes (Reviewer).

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