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

[Daniel D.Daugherty]

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

>> The changes in CR2 are:
>> 
>> - Make Thread::is_JavaThread_protected() static and make it clear that
>>   it checks to see if the calling thread is protecting the target;
>>   added VMThread access as always protected.
>> - Revert changes in JvmtiTrace::safe_get_thread_name().
>> - Revert the JavaThread::get_thread_name() default_name parameter and
>>   don't create a ThreadsListHandle in the function; restore a simpler
>>   assert for the case where the target thread is not protected.
>> - Add Threads::threads_do() and Threads::java_threads_do() that work
>>   with a JavaThreadIteratorWithHandle.
>> - Update src/hotspot/share/services/management.cpp to switch from using
>>   the Threads_lock to using JavaThreadIteratorWithHandle.
>> - Redo compileBroker.cpp changes to use a ThreadsListHandle to protect 
>>   the CompilerThread/JavaThread.
>> - Remove a stale comment from src/hotspot/share/prims/jvm.cpp.
>> 
>> You'll notice some temporary testing code in JavaThread::get_thread_name():
>> 
>>   guarantee(false, "current_thread=" INTPTR_FORMAT
>>             " is not protecting this=" INTPTR_FORMAT,
>>             p2i(current_thread), p2i(this));
>> 
>> I'm doing my current Mach5 Tier[1-8] run with this guarantee() in place.
>> It hasn't fired in Mach5 Tier[1-7]; Tier8 is still running, but it hasn't
>> fired yet. What this means is that we don't currently have a test case that
>> always exercises the unprotected path; we might have a test case that 
>> sometimes exercises the unprotected path depending on timing and/or
>> options, but nothing so far. My intention with this fix is not to leave any
>> unprotected paths for JavaThread::get_thread_name() and these test results
>> give me confidence, but not complete assurance.
>
> Here's another thing that I noticed and want to discuss with the reviwers:
> 
> I added a new comment above the simpler assert that I restored into
> JavaThread::get_thread_name():
> 
>   // Note: Since this JavaThread isn't protected by a TLH, the call to
>   // this->is_handshake_safe_for() may crash, but we have debug bits so...
>   assert(SafepointSynchronize::is_at_safepoint() ||
>          this->is_handshake_safe_for(current_thread), "JavaThread="
>          INTPTR_FORMAT " is not protected, not at a safepoint and "
>          "not handshake safe.", p2i(this));
> 
> The baseline code used:
> 
>   `assert_locked_or_safepoint_or_handshake(Threads_lock, this);`
> 
> and that function looks like this:
> 
> void assert_locked_or_safepoint_or_handshake(const Mutex* lock, const JavaThread* thread) {
>   if (thread->is_handshake_safe_for(Thread::current())) return;
>   assert_locked_or_safepoint(lock);
> }
> 
> and is_handshake_safe_for() looks like this:
> 
>   // A JavaThread can always safely operate on it self and other threads
>   // can do it safely if they are the active handshaker.
>   bool is_handshake_safe_for(Thread* th) const {
>     return _handshake.active_handshaker() == th || this == th;
>   }
> 
> so the baseline code is also using "this" JavaThread without knowing that
> it is safe. The interesting thing about is_handshake_safe_for() is that it
> *is safe* if the target is the active handshaker for the current thread.
> However, if the target is not the active handshaker for the current thread,
> then calling `thread->is_handshake_safe_for()` and accessing the
> `_handshake` field in that JavaThread may not be safe.

The CR2 changes are being tested with Mach5 Tier[1-8]:


    Mach5 Tier[1245] - no failures
    Mach5 Tier6 - 2 unrelated, known failures
    Mach5 Tier7 - 3 unrelated, known failures
    Mach5 Tier8 - still running, no failures so far

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

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