RFR: 8221207: Redo JDK-8218446 - SuspendAtExit hangs

[Daniel D. Daugherty]

On 3/21/19 4:47 AM, David Holmes wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8221207
>
> There was one small oversight in the original fix that led to crashes, 
> seen (randomly) in JDI tests. The safepoint check must not happen if 
> the thread-state is already _thread_in_native. I've checked the 
> thread-state on all call paths to confirm that.
>
> Incremental webrev from original fix: 
> http://cr.openjdk.java.net/~dholmes/8221207/webrev.inc/
>
> Full webrev: http://cr.openjdk.java.net/~dholmes/8221207/webrev/

src/hotspot/share/runtime/thread.hpp
     No comments.

src/hotspot/share/runtime/thread.cpp
     So here's the crashing stack:

     V [libjvm.so+0x14b1fee] SafepointSynchronize::block(JavaThread*)+0xae
     V [libjvm.so+0x14ba78d] 
SafepointMechanism::block_if_requested_slow(JavaThread*)+0x6d
     V [libjvm.so+0x1637207] 
JavaThread::handle_special_runtime_exit_condition(bool)+0x67
     V [libjvm.so+0x1073b1e] 
JvmtiJavaThreadEventTransition::JvmtiJavaThreadEventTransition(JavaThread*)+0x1ae
     V [libjvm.so+0x1069990] 
JvmtiExport::post_class_prepare(JavaThread*, Klass*)+0x1b0

     so we have a JvmtiJavaThreadEventTransition helper object to
     handle the transition from thread_in_vm -> thread_in_native:

     src/hotspot/share/prims/jvmtiExport.cpp

         class JvmtiJavaThreadEventTransition : StackObj {
         private:
           ResourceMark _rm;
           ThreadToNativeFromVM _transition;
           HandleMark _hm;

         public:
           JvmtiJavaThreadEventTransition(JavaThread *thread) :
             _rm(),
             _transition(thread),
             _hm(thread)  {};
         };

     so that's just a wrapper around ThreadToNativeFromVM:

     src/hotspot/share/runtime/interfaceSupport.inline.hpp

         class ThreadToNativeFromVM : public ThreadStateTransition {
          public:
           ThreadToNativeFromVM(JavaThread *thread) : 
ThreadStateTransition(thread) {
             // We are leaving the VM at this point and going directly 
to native code.
             // Block, if we are in the middle of a safepoint 
synchronization.
             assert(!thread->owns_locks(), "must release all locks when 
leaving VM");
             thread->frame_anchor()->make_walkable(thread);
             trans_and_fence(_thread_in_vm, _thread_in_native);
             // Check for pending. async. exceptions or suspends.
             if (_thread->has_special_runtime_exit_condition()) 
_thread->handle_special_runtime_exit_condition(false);
           }

           ~ThreadToNativeFromVM() {
             trans_from_native(_thread_in_vm);
             assert(!_thread->is_pending_jni_exception_check(), "Pending 
JNI Exception Check");
             // We don't need to clear_walkable because it will happen 
automagically when we return to java
           }
         };

     so trans_and_fence() calls transition_and_fence() which
     does this:

         static inline void transition_and_fence(JavaThread *thread, 
JavaThreadState from, JavaThreadState to) {
           assert(thread->thread_state() == from, "coming from wrong 
thread state");
           assert((from & 1) == 0 && (to & 1) == 0, "odd numbers are 
transitions states");
           // Change to transition state
           thread->set_thread_state((JavaThreadState)(from + 1));

InterfaceSupport::serialize_thread_state_with_handler(thread);

           SafepointMechanism::block_if_requested(thread);
           thread->set_thread_state(to);

CHECK_UNHANDLED_OOPS_ONLY(thread->clear_unhandled_oops();)
         }

     So for this use of handle_special_runtime_exit_condition(false),
     a safepoint is already handled by the previous transition_and_fence()
     with the thread still in the right thread state. However, if that
     handle_special_runtime_exit_condition() honors a self-suspend
     request and there's another safepoint, then we run the risk of
     the VMThread seeing _thread_blocked during the self-suspend
     phase of the thread and then the thread will go ahead into
     thread_native without stopping for the safepoint.

     Okay, but do we care? I don't think so. The thread will be off
     in native code and if it returns quickly and the safepoint is
     still active, then ~ThreadToNativeFromVM() should cause the
     thread to block for the safepoint.

     So this is a long winded way of saying I think the revised
     fix is okay. :-)

     You added this comment for the new if-statement:

     +// But it's more complicated than that as not all initial 
thread-states are suitable for
     +// doing safepoint checks. Fortunately, _thread_in_native is the 
only unsuitable state we
     +// can encounter based on our two callers.

     and I'm okay with it.

     Please consider adding this comment:

     +  if (state != _thread_in_native) {
          // _thread_in_native will block for a safepoint when it 
transitions back.
          SafepointMechanism::block_if_requested(this);
     +  }

Thumbs up!

Dan


>
> Re-tested in mach5 tiers 1-3 and com/sun/jdi tests (but they passed 
> last time too.).
>
> Thanks,
> David
>