~ThreadInVMForHandshake() should call handle_special_runtime_exit_condition()

[Daniel D. Daugherty]

Richard,

Very nice catch!


On 5/9/19 6:05 AM, Reingruber, Richard wrote:
> Hi,
>
> could I please get reviews and sponsoring for
>
> Webrev: http://cr.openjdk.java.net/~rrich/webrevs/2019/8223572/webrev.1/

src/hotspot/share/runtime/interfaceSupport.inline.hpp
     Verified that your new conditional block for calling
     handle_special_runtime_exit_condition() and the
     conditions for the parameter match the code at the
     end of SafepointSynchronize::block(JavaThread *thread).

I had to walk through the target thread call sequence:

SafepointSynchronize::handle_polling_page_exception(JavaThread *thread)
   thread->safepoint_state()->handle_polling_page_exception
     SafepointMechanism::block_if_requested(thread())
       SafepointMechanism::block_if_requested_slow(thread)
         SafepointMechanism::block_or_handshake()
           // not a global poll
           thread->handshake_process_by_self()
             _handshake.process_by_self(this)
               process_self_inner(thread)
                 ThreadInVMForHandshake tivm(thread)
                 :  make_walkable()
                 :  set _thread_in_vm
                 _semaphore.wait_with_safepoint_check(thread)
                 : ThreadBlockInVM tbivm
                 :   make_walkable()
                 :   trans(_thread_in_vm, _thread_blocked)
                 : _impl.wait()
                 : <VM_thread signals>
                 : <VM_ThreadSuspend on target thread>
                 : ~ThreadBlockInVM
                 :   trans(_thread_blocked, _thread_in_vm)
                 :   : transition(_thread, from, to)
                 :   :   set _thread_blocked+1
                 :   : SafepointMechanism::block_if_requested(thread)
                 :   :   : 
SafepointMechanism::block_if_requested_slow(thread)
                 :   :   : SafepointMechanism::block_or_handshake()
                 :   :   :     if (global_poll()) {
                 :   :   : SafepointSynchronize::block(thread)
                 :   :   :         make_walkable()
                 :   :   :         set _thread_blocked
                 :   :   :         _wait_barrier->wait()
                 :   :   :         // wait is notified when the 
safepoint is over
                 :   :   :         restore _thread_blocked+1  // 
_thread_blocked_trans
                 :   :   :         skip the call to 
handle_special_runtime_exit_condition()
                 :   :   :           because state == _thread_blocked_trans
                 :   :   // return all the way back to transition()
                 :   :   set _thread_in_vm
                 ~ThreadInVMForHandshake
                   transition_back()
                     set _thread_in_vm_trans
                     SafepointMechanism::block_if_requested()
                     set _original_state
// Now we return all the way back to handle_polling_page_exception().
// If we came from a _thread_in_Java state, then we just returned to
// that state without self suspending.
// The fix adds this:
                     if (_original_state != _thread_blocked_trans &&
                         _original_state != _thread_in_vm_trans &&
_thread->has_special_runtime_exit_condition()) {
_thread->handle_special_runtime_exit_condition(
                           !_thread->is_at_poll_safepoint() &&
                           (_original_state != _thread_in_native_trans));

And handle_special_runtime_exit_condition() does this:

   bool do_self_suspend = is_external_suspend_with_lock();
   if (do_self_suspend && (!AllowJNIEnvProxy || this == 
JavaThread::current())) {
     frame_anchor()->make_walkable(this);
     java_suspend_self_with_safepoint_check();
   }

Okay. Now I grok where the bug is and how your fix handles it.
Thumbs up on the fix itself!

You don't mention how you tested this fix. You also don't say
whether you have a test that reproduces this issue.

Obviously, we also need to hear from Robbin on this thread.

Dan

> Bug:    https://bugs.openjdk.java.net/browse/JDK-8223572
>
> A java thread T can continue executing bytecodes after it was suspended using the vm operation
> VM_ThreadSuspend. This can happen in HandshakeState::process_self_inner() when T becomes safepoint
> safe calling _semaphore.wait_with_safepoint_check(thread).
>
> Fix: poll has_special_runtime_exit_condition() and conditionally call
> handle_special_runtime_exit_condition() as it is done in SafepointSynchronize::block(), too.
>
> Thanks, Richard.
>
> PS: see also mail thread https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-May/034144.html