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

[David Holmes]

On Fri, 19 Nov 2021 17:04:42 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> 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.

The `is_exiting` check changes the behaviour from reporting JVMTI_ERROR_THREAD_NOT_SUSPENDED to JVMTI_ERROR_THREAD_NOT_ALIVE. Arguably it is a more precise answer, but it is somewhat splitting hairs. To me it might be clearer to the developer what their logic error is if they get NOT_SUSPENDED rather than NOT_ALIVE. Either way this change is not needed to fix any known bug and the change is behaviour seems questionable.

>> 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.

Again this introduces a more precise state check but also changes the behaviour by now reporting NOT_ALIVE instead of NOT_SUSPENDED. The assertion failure can be fixed by simply moving the assertion to after the suspension check.

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

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