On Mon, 22 Feb 2021 01:37:25 GMT, David Holmes <[email protected]> wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address dholmes-ora CR1 comments.
>
> src/hotspot/share/compiler/compileBroker.cpp line 1115:
>
>> 1113: if (ct == NULL) break;
>> 1114: ThreadsListHandle tlh;
>> 1115: assert(tlh.includes(ct), "ct=" INTPTR_FORMAT " exited
>> unexpectedly.", p2i(ct));
>
> This seems to be the fully dynamic case. Here the TLH is potentially racing
> with thread termination and so may or may not capture ct. Rather than an
> assertion the call to `get_thread_name()` below should be guarded by
> `tlh.includes(ct)`.
Agreed. Here's the new block:
JavaThread *ct = make_thread(compiler_t, compiler2_object(i),
_c2_compile_queue, _compilers[1], THREAD);
if (ct == NULL) break;
ThreadsListHandle tlh;
if (!tlh.includes(ct)) break;
_compilers[1]->set_num_compiler_threads(i + 1);
So we "break" if the `tlh` doesn't include the JavaThread which is the same
behavior as when `ct == NULL)`. Please note that I followed the existing
code style of putting the if-statement break on a single line.
> src/hotspot/share/compiler/compileBroker.cpp line 1136:
>
>> 1134: if (ct == NULL) break;
>> 1135: ThreadsListHandle tlh;
>> 1136: assert(tlh.includes(ct), "ct=" INTPTR_FORMAT " exited
>> unexpectedly.", p2i(ct));
>
> Ditto - the assert is not appropriate for this case.
Ditto for the resolution.
> src/hotspot/share/runtime/thread.cpp line 485:
>
>> 483: #endif
>> 484:
>> 485: // Is the target JavaThread protected by the calling Thread:
>
> s/calling/current/ ?
>
> We could also expand this to clarify that even if a target JavaThread is not
> explicitly protected by the current thread it can still be safe to access if
> the two threads engage in a suitable protocol.
That comment used to be:
`// Is the target JavaThread protected by this Thread:`
so I changed it to "the calling Thread" because I wanted it to be clear that
we're checking to see if the Thread that's calling is_JavaThread_protected()
is protecting the target JavaThread. I didn't use "current" because I thought
"calling" would be more clear.
I could add more comments about the target JavaThread being possibly
protected by other protocols, but I thought that might raise questions
about why the code doesn't check for those things and I didn't want to
go down that rabbit hole...
> 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?
> 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...
-------------
PR: https://git.openjdk.java.net/jdk/pull/2535
