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

[David Holmes]

On Tue, 23 Feb 2021 23:03:00 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> 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 ...
>
> ... that will tell us that we have a spot in the code where a TLH is
> missing once we've analyzed the crash. So are you okay with this
> assert() having a bug since the original assert has the same bug?

Yes

>> 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.
>
> Holding the Threads_lock is no longer being considered as
> proper protection for the JavaThread::get_thread_name() call
> so I had to change the code to use a JavaThreadIteratorWithHandle
> so that we have the protection of the TLH. That closure eventually
> gets us down to a JavaThread::get_thread_name() call...

But the code is now broken as it needs the Threads_lock to guard against non-JavaThreads being created or terminating.
The code also appears misleading because the passing of the JTIWH makes it look (to me) like it only deals with JavaThreads.
I think this code needs to be:
    JavaThreadIteratorWithHandle jtiwh;  // ensure JavaThreads are safe to access
    MutexLocker ml(Threads_lock);        // ensure non_JavaThreads are safe to access
    Threads::threads_do(&jtiwh, &vmtcc); // process all threads

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

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