On Fri, 12 Apr 2024 01:22:04 GMT, Serguei Spitsyn <[email protected]> wrote:
>> Good question, thanks.
>> The `JvmtiVTMSTransitionDisabler` is supposed to be installed in the
>> caller's context if needed.
>> However, it is not easy to make sure it is always the case.
>> At least, I see a couple of contexts when the `JvmtiVTMSTransitionDisabler`
>> is not being installed.
>> But it is not clear if it is really needed there. Let me do some extra
>> analysis there.
>
> Okay. The class `GetCurrentLocationClosure` is used by the
> `reset_current_location` only. It is called for the SINGLE_STEP and REAKPOINT
> event types as the following assert is placed at the function start:
>
> void JvmtiEnvThreadState::reset_current_location(jvmtiEvent event_type, bool
> enabled) {
> assert(event_type == JVMTI_EVENT_SINGLE_STEP || event_type ==
> JVMTI_EVENT_BREAKPOINT,
> "must be single-step or breakpoint event");
> . . .
>
> Also, this is the only two places where this function is called:
>
> JvmtiEventControllerPrivate::recompute_env_thread_enabled(JvmtiEnvThreadState*
> ets, JvmtiThreadState* state) {
> . . .
> if (changed & SINGLE_STEP_BIT) {
> ets->reset_current_location(JVMTI_EVENT_SINGLE_STEP, (now_enabled &
> SINGLE_STEP_BIT) != 0);
> }
> if (changed & BREAKPOINT_BIT) {
> ets->reset_current_location(JVMTI_EVENT_BREAKPOINT, (now_enabled &
> BREAKPOINT_BIT) != 0);
> }
>
> The `reset_current_location` is called called in the context of the
> `SetEventNotificationMode` where a JvmtiVTMSTransitionDisabler is present.
>
> Theoretically, it can be also triggered by the `SetEventCallbacks` (if
> callbacks are for SINGLE_STEP or REAKPOINT event type). But it also has a
> J`vmtiVTMSTransitionDisabler` in place:
>
> JvmtiEnv::SetEventCallbacks(const jvmtiEventCallbacks* callbacks, jint
> size_of_callbacks) {
> JvmtiVTMSTransitionDisabler disabler;
> JvmtiEventController::set_event_callbacks(this, callbacks,
> size_of_callbacks);
> return JVMTI_ERROR_NONE;
> } /* end SetEventCallbacks */
Thanks for the investigation! Maybe we should have an assert that
current->is_VTMS_transition_disabler() here or even in the
JvmtiHandshake::execute() that expects we have one in scope? I see that we have
some conditions where JvmtiVTMSTransitionDisabler is a no-op though so we would
have to include does as well. Or maybe set the boolean even when it is a no-op.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18630#discussion_r1562813538
