RFR: 8329674: JvmtiEnvThreadState::reset_current_location function should use JvmtiHandshake [v2]

[Patricio Chilano Mateo]

On Fri, 12 Apr 2024 01:22:04 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> 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