RFR: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread [v5]

[Jiangli Zhou]

On Tue, 28 Nov 2023 05:08:58 GMT, Serguei Spitsyn <sspitsyn at 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