On Wed, 29 Nov 2023 21:05:31 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: add comment for new ThreadsListHandle use
>
> I'm going to resurrect the failing guarantee() code and part of the stack
> trace that was removed
> and yack a bit about this code path.
>
> Here's the location of the failing guarantee():
>
> void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh,
> JavaThread* target) {
> . . .
> guarantee(target != nullptr, "must be");
> if (tlh == nullptr) {
> guarantee(Thread::is_JavaThread_protected_by_TLH(target),
> "missing ThreadsListHandle in calling context.");
>
>
> and here's part of the stack trace that got us here:
>
> V [libjvm.so+0x117937d]
> JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState*)+0x45d
> (jvmtiEventController.cpp:402)
> V [libjvm.so+0x1179520]
> JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState*)
> [clone .part.0]+0x190 (jvmtiEventController.cpp:632)
> V [libjvm.so+0x117a1e1]
> JvmtiEventControllerPrivate::thread_started(JavaThread*)+0x351
> (jvmtiEventController.cpp:1174)
> V [libjvm.so+0x117e608]
> JvmtiExport::get_jvmti_thread_state(JavaThread*)+0x98 (jvmtiExport.cpp:424)
> V [libjvm.so+0x118a86c] JvmtiExport::post_field_access(JavaThread*,
> Method*, unsigned char*, Klass*, Handle, _jfieldID*)+0x6c
> (jvmtiExport.cpp:2214)
>
>
> This must have been a stack trace from a build with some optimizations
> enabled because
> when I look at last night's code base, I see 8 frames from the
> JvmtiExport::get_jvmti_thread_state()
> call to Handshake::execute() with three params:
>
>
> src/hotspot/share/prims/jvmtiExport.cpp:
> JvmtiExport::get_jvmti_thread_state(JavaThread *thread) {
> assert(thread == JavaThread::current(), "must be current thread");
> if (thread->is_vthread_mounted() && thread->jvmti_thread_state() ==
> nullptr) {
> JvmtiEventController::thread_started(thread);
> }
>
> The above code asserts that the `thread` parameter is the current thread so
> we know that the calling thread is operating on itself.
>
>
> src/hotspot/share/prims/jvmtiEventController.cpp
> JvmtiEventControllerPrivate::thread_started(JavaThread *thread) {
> assert(thread == Thread::current(), "must be current thread");
> <snip>
> // if we have any thread filtered events globally enabled,
> create/update the thread state
> if (is_any_thread_filtered_event_enabled_globally()) { // intentionally
> racy
> JvmtiThreadState::state_for(thread);
>
> The above code asserts that the `thread` parameter is the current thread so
> we know that the calling thread is operating on itself. Note that we're
> calling
> the single parameter vers...
@dcubed-ojdk
Thank you for the analysis. I agree with it. It is why I've removed this stack
trace and posted my statement:
> I've removed a part of this comment with stack traces as my traps were not
> fully correct, need to double check everything. > This issue is not well
> reproducible but I'm still trying to reproduce it again.
Initially, I added a trap to catch the issue with the JVMTI
SetEventNotificationMode related code path and mistakenly thought that I've
caught it. But it was a different code path that you just described. So. I've
removed this part of my comment as it was wrong. Then I tried to reproduce the
code path I wanted to catch but had no luck to reproduce it.
Now, I'm going to remove this line of code from my PR we are discussing.
I'm suggesting to investigate the issue with guarantee separately from this PR.
Is it okay with you?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/16686#issuecomment-1832970099
