RFR: 8288139: JavaThread touches oop after GC barrier is detached

[David Holmes]

On Thu, 16 Jun 2022 00:04:04 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> src/hotspot/share/runtime/sharedRuntime.cpp line 1003:
>> 
>>> 1001:     guarantee(current != thread || !JavaThread::cast(thread)->is_gc_barrier_detached(),
>>> 1002:               "current cannot touch oops after its GC barrier is detached.");
>>> 1003:     oop obj = JavaThread::cast(thread)->threadObj();
>> 
>> I think the oop-touching-safety check should be done in `threadObj()` itself so that all callers are protected.
>
> The placement in SharedRuntime::get_java_tid() is because that is the failure
> mode that this bug is about. My next stress testing experiment will be to put
> a similar guarantee() in threadObj(), but I expect there to be more fan-out
> from that placement.
> 
> This fix is tightly focued on the bug as it is reported because I plan to fix
> it in JDK19.

Yeah ... hmmm ... threadObj is still the better place for any safety check ... arguably it should be even deeper in OopHandle.

I'm not sure what fanout you are concerned about. If we find there are other invalid oop accesses then we would want to fix them in 19 - no?

>> src/hotspot/share/runtime/thread.cpp line 3603:
>> 
>>> 3601:       ThreadIdTable::remove_thread(tid);
>>> 3602:     }
>>> 3603: 
>> 
>> I don't like exposing `ThreadSMR` internal details like this. Can we at least make this a little `ThreadSMRSupport` method that we call from here.
>> 
>> Or ... can we just move `ThreadsSMRSupport::remove_thread(p);` to before `on_thread_detach`?
>> 
>> Or can we grab the pid before `on_thread_detach` and pass it to `ThreadsSMRSupport::remove_thread`?
>
> Ummmm... The code that I moved is not a TheadSMR internal detail. That code
> is part of the M&M support that was added by Daniil in:
> 
> JDK-8185005 Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
> https://bugs.openjdk.org/browse/JDK-8185005
> 
> I don't remember why Daniil put that code in ThreadsSMRSupport::remove_thread(p),
> but it is just as appropriate to put it in Threads::remove(). That M&M cleanup code
> is part of the cleanup necessary when a JavaThread is removed. The important part
> is to call that code after the Threads_lock is grabbed.

Thanks Dan, I had forgotten about that and just assumed it was ThreadSMR related. Moving it as you did is now not a concern.

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

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