RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935

[Jiangli Zhou]

On Mon, 4 Dec 2023 20:52:46 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>>> @jianglizhou - Thanks for doing the testing. Can you also do a review? We need two reviewers for this change.
>> 
>> Complete test run finished. Checking the results, looks like there's no issue related to JVMTIThreadState change. 
>> 
>> @dcubed-ojdk Reducing the check to `(thread->threadObj() == nullptr && thread->is_attaching_via_jni())` looks ok. 
>> 
>> I just rechecked all usages of setup_jvmti_thread_state(). Currently it's used in three cases:
>> - JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector()
>> - JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector()
>> - JvmtiSampledObjectAllocEventCollector::start()
>> 
>> JDK-8319935 ran into issue with JvmtiSampledObjectAllocEventCollector::start() call path. We changed JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample() to avoid sampling if there is no thread obj allocated for the attaching thread. We also changed JvmtiThreadState::state_for_while_locked to handle the attaching case and return null. @dcubed-ojdk and @dholmes-ora, is there a case JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() might also see an attaching thread without the thread obj allocated? 
>> 
>> JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() call path probably is not affected by this case.
>
> @jianglizhou and @sspitsyn - Thanks for the reviews.
> 
> In the interest of reducing the noise in the Mach5 CI, I'm going ahead with
> integrating this fix without waiting for a reply to my comment above. If there
> are remaining issues, then we'll deal with them in a follow-up bug/RFE.

> > @dcubed-ojdk and @dholmes-ora, is there a case
> > JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector()
> > might also see an attaching thread without the thread obj allocated?
> 
> JvmtiVMObjectAllocEventCollector is the code path that we ran into with the internal stress test, but in the case that tripped, we had a thread obj allocated. If we ran the same code path without the thread obj allocated, then we would return `nullptr` which was the goal of your original fix.
> 
> I don't know if that specific test case is actually reached by any of our tests, but if such a case occurred, it did not result in the guarantee() firing or any other failure mode that I saw. The `guarantee(state != nullptr) failed: exiting thread called setup_jvmti_thread_state` has not been seen in Mach5 Tier[1-8] testing with this fix in place and I didn't see any other test failures that could be tracked back to problems with this code.
> 
> A JvmtiVMObjectAllocEventCollector object is created in 34 places in jvm.cpp alone and I haven't checked all of those. I only checked out the one use in JVM_GetStackAccessControlContext.

Thanks. I was wondering if we would need further work to handle `JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector()` with something similar to the JvmtiSampledObjectAllocEventCollector change, if it's possible to trigger the `guarantee(state != nullptr)` for attaching thread with no allocated thread obj.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839626256