On Mon, 4 Dec 2023 20:52:46 GMT, Daniel D. Daugherty <dcu...@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
