On Wed, 31 Mar 2021 14:32:23 GMT, Robbin Ehn <r...@openjdk.org> wrote:
>> 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;` ? Fixed ------------- PR: https://git.openjdk.java.net/jdk/pull/3191
