RFR: 8287281: adjust guarantee in Handshake::execute for the case of target thread being current [v4]

[Daniel D.Daugherty]

On Mon, 6 Jun 2022 07:17:15 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Johan Sjölén has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - do_thread(target) not self
>>  - Remove checks for is_handshake_for, instead call Handshake::execute
>>  - Switch order of handshake check
>
> src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 401:
> 
>> 399:       Thread *current = Thread::current();
>> 400:       Handshake::execute(&op, thread);
>> 401:       guarantee(op.completed(), "Handshake failed. Target thread is not alive?");
> 
> I much prefer that the current-thread case is internalised by `Handshake::execute` now. The code creating the handshake op shouldn't have to worry about current thread or not.

Having `Handshake::execute()` handle the current-thread case will certainly
allow us to make the code consistent in all the callers of `Handshake::execute()`.

> src/hotspot/share/runtime/handshake.cpp line 357:
> 
>> 355:   if (target->is_handshake_safe_for(self)) {
>> 356:     hs_cl->do_thread(target);
>> 357:     return;
> 
> I like the idea of doing this, but I can't quite convince myself that it will always be safe when the target is not the current thread. ??

Because we're pushing the special case handling for current-thread down
into the three parameter version of `Handshake::execute()`, we'll also
directly execute the closure's `do_thread()` function in other calls to the
three parameter version of `Handshake::execute()` where we didn't change
the calling code site in this patch:

- src/hotspot/share/classfile/javaClasses.cpp: async_get_stack_trace()
- src/hotspot/share/prims/jvmtiExtensions.cpp: GetCarrierThread()
- src/hotspot/share/prims/whitebox.cpp: WB_HandshakeReadMonitors(), WB_HandshakeWalkStack()
- src/hotspot/share/runtime/handshake.cpp: execute(HandshakeClosure* hs_cl, JavaThread* target)
     Of course, since the two parameter version of `Handshake::execute()` is
     now a changed code path, that means that all callers to the two parameter
     version of `Handshake::execute()` are also affected. No, I'm not going to
     list all those call sites.

This is a change in behavior and I'm not saying that this is wrong, but it's
not clear to me that the repercussions are understood and discussed in
this PR.

What I'm mumbling about here might be the same thing that @dholmes-ora is
worried about, but I'm just being more verbose about it. :-)

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

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