On Fri, 19 Nov 2021 10:14:23 GMT, Serguei Spitsyn <sspit...@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
