On Thu, 30 Mar 2023 16:33:01 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> 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. > >> 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. > > Agreed, thanks. In fact, I've already experimented with it. > As you noted, this correction is wrong for unmount transition. Also, we don't > need to correct it for mount transition because it will be corrected by the > `JvmtiVTMSTransitionDisabler::VTMS_mount_end()` as it makes a call > `thread->rebind_to_jvmti_thread_state_of(vt)`. > The only thing that bothers me here is that we fight non-real problems as > this code is needed only to support artificial disabling for testing purposes > to be able to repeat enable again. :-) The fix to only restore the state outside a transition looks good. Yes, the added testing methods are making this harder :) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1153670911
