RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]

[Daniel D.Daugherty]

On Fri, 19 Nov 2021 10:14:23 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1401:
>> 
>>> 1399:   if (!self) {
>>> 1400:     if (!java_thread->is_suspended()) {
>>> 1401:       _result = JVMTI_ERROR_THREAD_NOT_SUSPENDED;
>> 
>> I don't see an obvious reason for this `is_exiting()` check.
>
> Okay. I see similar check in the `force_early_return()` function:
> 
>   if (state == NULL) {
>     return JVMTI_ERROR_THREAD_NOT_ALIVE;
>   }
> 
> Would it better to replace it with this check instead? :
> 
>   if (java_thread->is_exiting()) {
>     return JVMTI_ERROR_THREAD_NOT_ALIVE;
>   }
> 
> Removing this check and keep the one inside the handshake would be even better.
> 
> I would also add this line for symmetry with two other cases:
> 
> +  MutexLocker mu(JvmtiThreadState_lock);
>   SetForceEarlyReturn op(state, value, tos);

My point is that I don't see why you added the `is_exiting()` check
since I don't see a race in that function, i.e., there's no `assert()` in
this function that you need to protect.

As for adding the `MutexLocker mu(JvmtiThreadState_lock)`, you'll
have to analyze and justify why you would need to add that lock grab
independent of this fix. I'm not seeing a bug there, but I haven't looked
very closely.

>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1533:
>> 
>>> 1531:     return; /* JVMTI_ERROR_THREAD_NOT_ALIVE (default) */
>>> 1532:   }
>>> 1533:   assert(java_thread == _state->get_thread(), "Must be");
>> 
>> This `assert()` is the site of the original test failure. I haven't yet
>> looked at the locations of the other changes.
>> 
>> The `is_exiting()` check is made under the protection of the
>> `JvmtiThreadState_lock` so an unsuspended target thread that is
>> exiting cannot reach the point where the `_state` is updated to
>> clear the `JavaThread*` so we can't fail the `assert()` if the
>> `is_exiting()` check has returned `false`.
>
> Dan,
> Thank you for reviewing this!
> I'm not sure, I correctly understand you here.
> Are you saying that you agree with this change?
> In fact, the thread state can not be changed (and the assert fired) after the `is_exiting()` check is made even without `JvmtiThreadState_lock` protection because it is inside of a handshake execution.

I agree with the `is_exiting()` check addition.

I forgot that we're executing a Handshake `doit()` function. So we have a couple
of reasons why an unsuspended target thread can't change from `!is_exiting()`
to `is_exiting()` while we are in this function.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6440