On Wed, 17 Feb 2021 17:33:27 GMT, Daniel D. Daugherty <[email protected]>
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
