On Tue, 28 Nov 2023 18:36:00 GMT, Jiangli Zhou <jian...@openjdk.org> wrote:
>> This also was caught by my eyes. :) >> With the lines 99-101 in place the only case when `thread_oop` can be equal >> to `nullptr` is when `thread->threadObj() == nullptr`. My understanding is >> that it can be for a detached thread only. >> I would suggest to add an assert after the line 101: ` assert(thread_oop != >> nullptr, "sanity check");` >> Full testing with this assert should help to identify if it can be fired. If >> such cases are found then they need to be fixed. Then we can remove the >> check at the line 104. >> The `JvmtiThreadState` constructor also allows for `thread_oop` to be >> `nullptr`. >> Some cleanup will be needed to get rid of unneeded checks there as well. > > @sspitsyn For the above suggestions, it seems cleaner/safer to handle the > clean-ups in a separate RFE with full testing including the vthread cases. > There are additional comments in > https://github.com/openjdk/jdk/pull/16642#issuecomment-1815379890 related to > this as well. Those could be handled together and require through testing > including the vthread support. Okay, thanks! I was about to suggest the same as it will simplify the process. I've filed the following issue and assigned to myself: [8320925](https://bugs.openjdk.org/browse/JDK-8320925 )`assert and cleanup in JvmtiThreadState::state_for_while_locked for thread_oop != nullptr` I'll prepare a fix for 22 and hope to get you as a reviewer. :) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1408394435
