On Tue, 23 Feb 2021 23:03:00 GMT, Daniel D. Daugherty <[email protected]>
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
