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

[Daniel D. Daugherty]

On Wed, 15 Jun 2022 23:48:44 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> A trivial 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.
>
> src/hotspot/share/runtime/thread.cpp line 3601:
> 
>> 3599:     // requiring the GC state to be alive.
>> 3600:     BarrierSet::barrier_set()->on_thread_detach(p);
>> 3601:     if (p->is_exiting()) {
> 
> I don't understand why this needs to be conditional?

The "normal" transitions for a JavaThread's _terminated field are:

  _not_terminated => _thread_exiting => _thread_gc_barrier_detached => _thread_terminated

The transitions for a JavaThread that failed to attach are:

  _not_terminated => _thread_terminated

because that JavaThread does not calls JavaThread::exit(). If we try to take
the attach failing JavaThread thru _thread_gc_barrier_detached, then some
of the counter code will be unhappy and we'll see assertion failures.

> src/hotspot/share/runtime/thread.hpp line 1180:
> 
>> 1178:   bool is_exiting() const;
>> 1179:   // thread's GC barrier is detached or thread is terminated
>> 1180:   bool is_gc_barrier_detached() const;
> 
> Can we name this based on what it allows (access to oops) rather than what it relates to deep down? e.g. `can_access_oops_safely` or `is_oop_safe` ?

We can definitely rename, but naming is hard... :-)

Those names you proposed are inversions of the current name and would
require that the logic of the function be inverted.

The other names I came up before I settled on is_gc_barrier_detached():


    can_no_longer_access_oops()
    oops_are_no_longer_safe()
    cannot_safely_access_oops()


The closest match between your proposed names and mine are:

`can_access_oops_safely`  and `cannot_safely_access_oops`

Other opinions are welcome.

> src/hotspot/share/runtime/thread.hpp line 1186:
> 
>> 1184:   bool check_is_terminated(TerminatedTypes l_terminated) const {
>> 1185:     return l_terminated != _not_terminated && l_terminated != _thread_exiting &&
>> 1186:            l_terminated != _thread_gc_barrier_detached;
> 
> Existing: I don't think I buy this argument that we need to check all the states we don't want to see rather than the one we do. If the JavaThread has been freed we could see anything - and we should never be executing this code in the first place.

The logic style of this function is a carry over from before we had Thread-SMR
and we tried to detect a freed JavaThread. With Thread-SMR, we don't need to
do the check this way at all.

Do we really want to change that as part of this fix for JDK19?

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

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