RFR: 8241403: JavaThread::get_thread_name() should be ThreadSMR-aware [v4]

[David Holmes]

On Sat, 20 Feb 2021 17:50:01 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> A minor fix to add a new function:
>> 
>>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>> 
>> that returns true when the target JavaThread* is protected and false
>> otherwise. Update JavaThread::get_thread_name() to create a
>> ThreadsListHandle and use the new is_JavaThread_protected(). Also
>> update JvmtiTrace::safe_get_thread_name() to use the new
>> is_JavaThread_protected().
>> 
>> This fix is tested via a Mach5 Tier[1-8] run.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Address dholmes-ora CR1 comments.

Hi Dan,

Still a few comments/concerns - see below.

Thanks,
David

src/hotspot/share/compiler/compileBroker.cpp line 1007:

> 1005:       assert(ct != NULL, "should have been handled for initial thread");
> 1006:       ThreadsListHandle tlh;
> 1007:       assert(tlh.includes(ct), "ct=" INTPTR_FORMAT " exited unexpectedly.", p2i(ct));

This case seems to be one where we are creating an immortal compiler thread (either because they are all immortal, or this is the first compiler thread and it is always immortal (verify that?)) - so the assert seems fine.

src/hotspot/share/compiler/compileBroker.cpp line 1028:

> 1026:       assert(ct != NULL, "should have been handled for initial thread");
> 1027:       ThreadsListHandle tlh;
> 1028:       assert(tlh.includes(ct), "ct=" INTPTR_FORMAT " exited unexpectedly.", p2i(ct));

Ditto

src/hotspot/share/compiler/compileBroker.cpp line 1115:

> 1113:       if (ct == NULL) break;
> 1114:       ThreadsListHandle tlh;
> 1115:       assert(tlh.includes(ct), "ct=" INTPTR_FORMAT " exited unexpectedly.", p2i(ct));

This seems to be the fully dynamic case. Here the TLH is potentially racing with thread termination and so may or may not capture ct. Rather than an assertion the call to  `get_thread_name()` below should be guarded by `tlh.includes(ct)`.

src/hotspot/share/compiler/compileBroker.cpp line 1136:

> 1134:       if (ct == NULL) break;
> 1135:       ThreadsListHandle tlh;
> 1136:       assert(tlh.includes(ct), "ct=" INTPTR_FORMAT " exited unexpectedly.", p2i(ct));

Ditto - the assert is not appropriate for this case.

src/hotspot/share/runtime/thread.cpp line 485:

> 483: #endif
> 484: 
> 485: // Is the target JavaThread protected by the calling Thread:

s/calling/current/ ?

We could also expand this to clarify that even if a target JavaThread is not explicitly protected by the current thread it can still be safe to access if the two threads engage in a suitable protocol.

src/hotspot/share/runtime/thread.cpp line 2568:

> 2566:   // this->is_handshake_safe_for() may crash, but we have debug bits so...
> 2567:   assert(SafepointSynchronize::is_at_safepoint() ||
> 2568:          this->is_handshake_safe_for(current_thread), "JavaThread="

I agree this is a pre-existing bug as accessing `this` may not be safe if the target is not protected. But if we crash attempting the access that implies the assert failed so ...

src/hotspot/share/services/management.cpp line 848:

> 846:   {
> 847:     JavaThreadIteratorWithHandle jtiwh;
> 848:     Threads::threads_do(&jtiwh, &vmtcc);

I'm not sure this is at all necessary. Threads::add and Threads::remove still use the Threads_lock so this code is safe as it stands. Switching to  use JTIWH may be an alternative, and may even be desirable, but that hasn't been established, and is not necessary for safety and so seems outside the scope of this bug IMO.

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

Changes requested by dholmes (Reviewer).

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