On Fri, 15 Oct 2021 18:20:12 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8249004.cr1.patch > > src/hotspot/share/runtime/handshake.cpp line 358: > >> 356: bool target_is_dead = false; >> 357: if (target == nullptr) { >> 358: target_is_dead = true; > > Why would you pass a NULL target thread to Handshake::execute? Why would the > caller not check if the target is dead? The `NULL` target thread being passed in is actually handled by the baseline code: ThreadsListHandle tlh; if (tlh.includes(target)) { `tlh.includes(target)` returns `false` when `target` is `NULL/nullptr`. I just made the already handled situation more explicit. > Why would the caller not check if the target is dead? Hmmm... It's hard for me to answer that question since I didn't write the original code. The test code that calls `WB_HandshakeWalkStack()` or `WB_AsyncHandshakeWalkStack()` can call those functions with a `thread_handle` that translates into a `thread_oop` that returns a `NULL` `JavaThread*`. See the comment that I added to `WB_AsyncHandshakeWalkStack()` above. > src/hotspot/share/runtime/thread.cpp line 497: > >> 495: // placement somewhere in the calling context. >> 496: bool Thread::is_JavaThread_protected_by_my_ThreadsList(const >> JavaThread* p) { >> 497: Thread* current_thread = Thread::current(); > > Shouldn't you call this on the current thread as "this" argument? I modeled the new check after the existing: bool Thread::is_JavaThread_protected(const JavaThread* p) { which is also a static function. ------------- PR: https://git.openjdk.java.net/jdk/pull/4677