RFR: 8273597: Rectify Thread::is_ConcurrentGC_thread()

[Coleen Phillimore]

On Fri, 10 Sep 2021 12:39:14 GMT, Per Liden <pliden at openjdk.org> wrote:

> `Thread::is_ConcurrentGC_thread()` behaves differently to all other `Thread::is_xxx_thread()` functions, in the sense that it doesn't directly map to a distinct `Thread` sub-class. Instead, `is_ConcurrentGC_thread()` can today return true for both `ConcurrentGCThread` and `GangWorker`. These two classes have no super/sub-class relation. This is confusing and and potentially dangerous.
> 
> It would be reasonable to think that code like this would be correct:
> 
> 
>   if (thread->is_ConcurrentGC_thread()) {
>     conc_thread = static_cast<ConcurrentGCThread*>(thread);
>     ...
>   }
> 
> 
> but it's not, since we might try to cast a `GangWorker` to a `ConcurrentGCThread`. And again, these two classes have no super/sub-class relation.
> 
> I propose that we clean this up, so that `is_ConcurrentGCThread()` only returns true for threads inheriting from `ConcurrentGCThread`. The main side-effect is that a handful of asserts need to be adjusted. In return, the code example above would become legal, and we can also remove some cruft from `WorkGang`/`GangWorker`.

Nice cleanup.  One comment and suggestion.

src/hotspot/share/gc/z/zCollectedHeap.cpp line 84:

> 82:   virtual void do_thread(Thread* thread) {
> 83:     if (thread->is_ConcurrentGC_thread()) {
> 84:       static_cast<ConcurrentGCThread*>(thread)->stop();

Should you have a cast function like JavaThread does  ?
  static JavaThread* cast(Thread* t) {
    assert(t->is_Java_thread(), "incorrect cast to JavaThread");
    return static_cast<JavaThread*>(t);
  }

At one point @dholmes replaced all the thread->as_Java_thread() with JavaThread::cast() so this would be consistent with that and nice.

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

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