On Wed, 31 Mar 2021 10:48:11 GMT, Robbin Ehn <r...@openjdk.org> wrote:
>> 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. I got some offline input from David, there seem to be an issue with signal being delivered after the ThreadsListHandle destructor. If that is the case a ThreadsList doesn't help here. So I suggested we keep this out of this change-set and just take another suitable field to mirror what we have today. `ParkEvent * _ParkEvent;` ? ------------- PR: https://git.openjdk.java.net/jdk/pull/3191
