RFR: 8221207: Redo JDK-8218446 - SuspendAtExit hangs

[David Holmes]

Hi Dan,

Thanks again for the eagle-eyed analysis ...

On 22/03/2019 5:57 am, Daniel D. Daugherty wrote:
> 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.

The way I looked at this is that we are trying to ensure that the 
VMThread doesn't see _thread_blocked just before we restore the true 
state, and allow the thread to escape the safepoint. In this case the 
true state is _thread_in_native, which is safepoint-safe just as 
_thread_blocked is, so it doesn't matter if the VMThread sees the true 
state or _thread_blocked as the result is the same. Hence for 
_thread_in_native we don't need to do the additional safepoint check.

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

Okay - thanks :)

>      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);
>      +  }

It's a correct statement but I'm not sure its really relevant to the 
context. But I didn't explain in the code why _thread_in_native is 
special. My "But it's more complicated ..." comment doesn't explain 
things properly. How about:

// However, not all initial-states are allowed when performing a
// safepoint check, as we should never be blocking at a safepoint
// whilst in those states. Of these 'bad' states only _thread_in_native
// is possible when executing this code (based on our two callers).
// A thread that is _thread_in_native is already safepoint-safe and so
// it doesn't matter whether the VMThread sees the _thread_blocked
// state, or the _thread_in_native state, and so we don't need the
// explicit safepoint check.

?

Thanks,
David
-----

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