RFR: 8288497: add support for JavaThread::cannot_access_oops_safely()

[Robbin Ehn]

On Thu, 16 Jun 2022 07:29:34 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> A fix to add support for JavaThread::is_gc_barrier_detached() which allows
>> us to add checks to detect failures like:
>> 
>>     JDK-8288139 JavaThread touches oop after GC barrier is detached
>>     https://bugs.openjdk.org/browse/JDK-8288139
>> 
>> This fix along with the fix for JDK-8288139 has been tested in Mach5 Tier[1-8].
>> There are no related failures in Mach5 Tier[1-7]; Mach5 Tier8 is still running.
>
> Hi,
> 
> Except for the few lines of code between:
> BarrierSet::barrier_set()->on_thread_detach(p) and ThreadsSMRSupport::remove();
> 
> This new state is identical to the result of "ThreadsSMRSupport::get_java_thread_list()->includes(p)".
> The JavaThread is attached to the barrier set when we add it to the ThreadsList and detached when we remove it.
> 
> If we have a safepoint poll between (done with Threads_lock held):
> BarrierSet::barrier_set()->on_thread_detach(p);
> ->
> p->set_terminated(JavaThread::_thread_terminated);
> 
> We have a bug. This means that this new state is not even possible to observe from another thread (with target safe).
> So for any other thread there is no difference, which means we have new global state just for a few lines of code while we are terminating with Threads_lock held.
> 
> So I don't think this one place needs a new global state.
> 
> Note that for a safe target thread the terminated flag gives the same result as:
> "ThreadsSMRSupport::get_java_thread_list()->includes(p)"
> 
> So jt->is_terminated() can be implemented with "ThreadsSMRSupport::get_java_thread_list()->includes(p)". (again assuming we are not doing some racy, and only asking for threads that are safe/stable)

> @robehn's comment from above:
> 
> > Except for the few lines of code between:
> > BarrierSet::barrier_set()->on_thread_detach(p) and ThreadsSMRSupport::remove();
> > This new state is identical to the result of "ThreadsSMRSupport::get_java_thread_list()->includes(p)".
> 
> No it's not equivalent. If I replace is_gc_barrier_attached()'s implementation with your check, then it would NOT catch the code that's in the wrong place and is being fixed by JDK-8288139. Setting the new _terminated field state exactly where I do so in this fix is what allowed me to prove that the code that I moved in JDK-8288139 is in the wrong place.

That's why I said except between these lines :) 

If you say this is the correct way to handle it for now, then it is.
Looks fine, thanks!

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

PR: https://git.openjdk.org/jdk19/pull/20