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

[Daniel D.Daugherty]

On Wed, 17 Feb 2021 17:33:27 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Hi Dan,
>> 
>> Sorry but I have a lot of issues with this. After thinking about it a lot I don't think the current approach is what is needed. To repeat what I wrote in one of the comments I think the simple fix here is to replace the use of Threads_lock in the caller with suitable use of a TLH, and then replace the assert_locked_or_safepoint(Threads_lock) with assert(is_JavaThread_protected(t)) where is_JavaThread_protected checks the target for either being the current thread or included in a TLH of the current thread. I don't think get_thread_name() should try to protect the target as that is the responsibility of the caller.
>> 
>> Thanks,
>> David
>
> @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.

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

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