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

[Daniel D.Daugherty]

On Fri, 3 Jun 2022 09:45:31 GMT, Johan Sjölén <duke at openjdk.java.net> wrote:

>> Please review this PR for fixing JDK-8287281.
>> 
>> If a thread is handshake safe we immediately execute the closure, instead of going through the regular Handshake process. 
>> 
>> Finally:  Should `VirtualThreadGetThreadClosure` and its `do_thread()` body be inlined instead? We can do this in this PR, imho, but I'm hoping to get some input on this.
>> 
>> 
>> Passes tier1. Running tier2-5.
>
> 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

This bug/PR is specifically about this block of code:

  if (tlh == nullptr) {
    guarantee(Thread::is_JavaThread_protected_by_TLH(target),
              "missing ThreadsListHandle in calling context.");
    target->handshake_state()->add_operation(&op);

and the bug makes the claim that we need to adjust this
guarantee(). Okay, but this proposed fix is indirectly changing
the guarantee() by inserting this block of code before the
guarantee():

  if (target->is_handshake_safe_for(self)) {
    hs_cl->do_thread(target);
    return;
}

so we still have the original guarantee() that checks a
specific state with respect to ThreadsListHandles and
we replace it with a check, the `is_handshake_safe_for()`
call, that has nothing to do with ThreadsListHandles!

The original purpose of this logic block:

  if (tlh == nullptr) {
    guarantee(Thread::is_JavaThread_protected_by_TLH(target),
              "missing ThreadsListHandle in calling context.");
    target->handshake_state()->add_operation(&op);

is to require a protecting ThreadsListHandle to be in place
*somewhere* in the calling context since we have not
passed in a ThreadsListHandle from the calling context.

When I added the above block of code, I intentionally
updated all of the call sites that reached the new strict
check with ThreadsListHandles. This included calls sites
where the caller was the current thread. This was an
intentional change on my part to make sure that all the
JavaThreads being operated (including current) on are
protected by ThreadsListHandles.

When the Loom project was being developed, a number
of these carefully placed ThreadsListHandles were moved
and unprotected code paths were introduced. We believe
that these unprotected code paths are safe because we
believe that they are only used by the current thread and
the current thread does not really need a ThreadsListHandle.
That might be true, but it certainly complicates the reasoning
about the code paths.

The bug talks about adjusting the guarantee() to allow the
current thread to be unprotected by a ThreadsListHandle, but
the logic that we have switched to:

  // A JavaThread can always safely operate on it self and other threads
  // can do it safely if they are the active handshaker.
  bool is_handshake_safe_for(Thread* th) const {
    return _handshake.active_handshaker() == th || this == th;
  }

does more than that. It also allows the target to be unprotected
by a ThreadsListHandle if the calling thread is the active handshaker.
I'm not (yet) convinced that is a good policy.

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

Changes requested by dcubed (Reviewer).

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