On Tue, 28 Nov 2023 05:08:58 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 98:
>>
>>> 96: state->get_thread_oop() != thread_oop)) {
>>> 97: // Check if java_lang_Thread already has a link to the
>>> JvmtiThreadState.
>>> 98: if (thread_oop != nullptr) { // thread_oop can be null during
>>> early VMStart.
>>
>> This comment is another case of `state->get_thread_oop()` being null. We
>> should merge this comment with the new comment about attaching native thread.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1408228229
