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

[Daniel D.Daugherty]

On Mon, 22 Feb 2021 01:37:25 GMT, David Holmes <dholmes at openjdk.org> 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