On Wed, 31 Mar 2021 02:43:21 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> src/hotspot/os/posix/signals_posix.cpp line 1587:
>>
>>> 1585: // destructor has completed.
>>> 1586:
>>> 1587: if (thread->is_Java_thread() &&
>>> thread->as_Java_thread()->is_terminated()) {
>>
>> We need @dholmes-ora to verify that this version of the code will
>> still solve the bug he was fixing when he added old L1610.
>
> Thanks @dcubed-ojdk - no it won't. The problem is that the signal can hit
> very late in a thread's termination process, after the JavaThread destructor
> has executed, so no query of JavaThread state is possible. So we needed
> something in the Thread state and the SR_lock was a good enough proxy for
> that. It may be possible to use a different Thread field (perhaps _ParkEvent
> but encapsulated in a Thread::has_terminated() helper method).
SR_handler is used for OS-level suspend/resume (not to conflict with this
change-set).
This feature is only used by JFR (AFAIK), and JFR only samples threads on it's
ThreadsList.
This means the JavaThread can never be terminated, hence this code will always
pass.
If someone else is using OS-level suspend/resume without a ThreadsList, the bug
is there is no ThreadsList AFAICT.
Since ThreadLocalStorage::thread() is cleared last in ~Thread with
Thread::clear_thread_current(); may be in the ~Thread destructor.
So the argument is that would be safe to read stuff from Thread but not
JavaThread?
Since the compiler (and CPU) may reorder and optimize away code, so I argue
reading from a half destroyed object is not a great idea.
E.g. Monitor* _SR_lock; is not a volatile pointer; since reading from this
memory is UB after destruction, compiler is free to remove or not publish the
store to NULL.
So I suggest either to remove this check, since the only user is using a
ThreadsList and any other should also be using that too.
Or call Thread::clear_thread_current() before the JavaThread destructor is
called, that way we can be certain that there is no UB.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3191
