RFR: 8257831: Suspend with handshakes [v2]

[Robbin Ehn]

On Wed, 31 Mar 2021 02:43:21 GMT, David Holmes <dholmes at 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