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

[Daniel D.Daugherty]

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

>> @dholmes-ora - I'm really glad that I waited for your review! Thanks for taking the time.
>
> 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.

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

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