On Wed, 29 Mar 2023 21:36:04 GMT, Patricio Chilano Mateo <pchilanom...@openjdk.org> wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: updated correction of jt->jvmti_thread_state() links in >> VM_SetNotifyJvmtiEventsMode > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1554: > >> 1552: } >> 1553: // Correct jt->jvmti_thread_state() and jt->jvmti_vthread() if >> necessary. >> 1554: // It was not maintained while notifyJvmti was disabled. > > While trying to understand which exact situation we are trying to guard > against with this code, I run the test without the sleeps and without this > restore code and I got a crash when deleting a JvmtiThreadState (null > dereference of _thread in the ~()). Probably the same crash you mentioned you > had. But when debugging the crash I see that the problem is that the > assumption that disabling the flag is done when no virtual threads are > running is not guaranteed (see my comment there). So I think we are trying to > address a case that shouldn't happen in the first place. Also not sure if > applying this restore in all cases will be correct, since we might be > somewhere at a transition. For example, a thread could have blocked right in > the return from notifyJvmtiUnmount() in yieldContinuation(). It will looked > like virtual because unmount() was not executed yet, and the > jvmti_thread_state should be that of the platform thread because we never > changed it when mounting. We should leave the state as is but in here we would change it to the virtual thread's jvmti state. The only case I think it makes sense to do this restore steps when enabling the flag is for those threads that are outside a transition with a mounted virtual thread, since we want to adjust the jvmti_thread_state so that it looks right on the next unmount. > But in any case this is also only needed when using the WhiteBox methods > right? In the intended case (no WhiteBox method used), after we execute this > operation to enable the events, we will create the JvmtiThreadStates later in > JvmtiExport::get_jvmti_interface() and the correct jvmti_thread_state and > jvmti_vthread will be already set for each JavaThread. In that case can we > only execute this restore code when using the WhiteBox API? I've just pushed my update with refactoring of these corrections in `VM_SetNotifyJvmtiEventsMode` after discussion of this code with Leonid. I hope it resolved at least part of your concerns. This kind of problem exists only when we disable+enable notifyJvmti events multiple times for testing purposes. This code with corrections of jt->jvmti_thread_state() is not needed when we enable notifyJvmti events just once. Your are correct that the ability to disable notifyJvmti events is implemented with using WhiteBox API. My last update has a comment explaining this. > In that case can we only execute this restore code when using the WhiteBox > API? This is a good suggestion which we already discussed privately with Leonid. I've got an idea how to implement this check. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1152641723
