RFR(S): JDK-8137035 tests got EXCEPTION_STACK_OVERFLOW on Windows 64 bit

[David Holmes]

Following up on a "Yikes"  :) ...

On 3/09/2016 3:02 AM, Daniel D. Daugherty wrote:
> On 9/2/16 9:20 AM, Frederic Parain wrote:
>> Hi David,
>>
>> Here's an updated version of the fix:
>> http://cr.openjdk.java.net/~fparain/8137035/webrev.02/
>
> src/cpu/x86/vm/globals_x86.hpp
>     No comments.
>
> src/os/windows/vm/os_windows.cpp
>     L2507:     if(t != NULL && t->is_Java_thread()) {
>         Missing space between 'if' and '('.
>
> src/share/vm/runtime/interfaceSupport.hpp
>     I worry that we're potentially missing enabling the yellow zone
>     on other transitions back into Java...
>
>     The transition(JavaThread *thread, JavaThreadState from,
> JavaThreadState to)
>     function is how both ~ThreadInVMfromJava and
> ~ThreadInVMfromJavaNoAsyncException
>     get the thread state back to _thread_in_Java. It looks like
>     transition_and_fence() and transition_from_native() can also
>     have a 'to' thread state of _thread_in_Java...
>
>     The only thing that everyone has in common is:
>
>     thread->set_thread_state(to);
>
>     Do we perhaps want to check and enable the yellow zone whether
>     set_thread_state(to) is called with to == _thread_in_Java?
>
>     Yikes. JavaThread::set_thread_state() is defined in two different
>     places depending on platform... sigh... on PPC64 and AARCH64, it
>     is set via a OrderAccess::release_store() and fetched via a
>     JavaThreadState JavaThread::thread_state(). Makes sense, here's
>     the non-PPC64 and AARCH64 versions:
>
>     hotspot/src/share/vm/runtime/thread.hpp:
>
>       // Safepoint support
>      public:                                         // Expose
> _thread_state for SafeFetchInt()
>       volatile JavaThreadState _thread_state;
>
>      <snip>
>
>       // Safepoint support
>     #if !(defined(PPC64) || defined(AARCH64))
>       JavaThreadState thread_state() const           { return
> _thread_state; }
>       void set_thread_state(JavaThreadState s)       { _thread_state =
> s;    }
>     #else
>
>     The above assumes TSO so PPC64 and AARCH64 need the OrderAccess ops...

I don't believe there are any TSO assumptions. Ignore the prehistoric 
comments by Urs:

  118     // Change to transition state (assumes total store ordering! 
-Urs)

We usually change the state in two steps (trans state then final state) 
and there is a whopping great synchronization point in between them. The 
thread states are only read by the thread itself and the VMThread (when 
a safepoint has been initiated). There is no other state that needs to 
be visible before the thread-state is visible, and if there were it is 
usually only the trans-state that can be reordered due to the "whopping 
great synchronization point". And the trans-state is not an issue 
because the VMThread will loop until the target thread moves out of a 
trans-state, which is on the other side of the "whopping great 
synchronization point". :)

The exception is transition_from_java where we do move directly to the 
final "to" state. But again this is not an issue in the safepoint 
protocol as we only move from in_java to in_vm, both of which of not 
"safepoint safe" states. If we see a stale _thread_state of "in java" 
the VMThread will loop on it until it transitions to another state. 
Again there is no other 'state' that needs to be ordered with the 
setting of the _thread_state.

As to why PPC64 and AARCH64 do what they do ... I think I called them on 
it years ago. It may relate to those stale "this assumes TSO" comments 
in the code.

Cheers,
David

>     I could be overly paranoid here so it's possible that those
>     other code paths do not allow _thread_in_Java to be set...
>
> Dan
>
>
>>
>> Regards,
>>
>> Fred
>>
>> On 09/01/2016 03:29 AM, David Holmes wrote:
>>> On 31/08/2016 9:04 AM, Frederic Parain wrote:
>>>> Hi David,
>>>>
>>>> On 08/29/2016 11:10 PM, David Holmes wrote:
>>>>> Hi Fred,
>>>>
>>>>>>> While examining the thread state logic in the exception handler I
>>>>>>> noticed some pre-existing bugs:
>>>>>>>
>>>>>>> 2506   if (exception_code == EXCEPTION_ACCESS_VIOLATION) {
>>>>>>> 2507     JavaThread* thread = (JavaThread*) t;
>>>>>>>
>>>>>>> there is no check that t is in fact a JavaThread, or even that t is
>>>>>>> non-NULL. Such checks occur slightly later:
>>>>>>
>>>>>> I've investigated this issue, and it is currently harmless.
>>>>>> The casted pointer is only used to call a method requiring
>>>>>> a JavaThread* pointer and the only usage of its argument it's
>>>>>> a NULL check. Unfortunately, fixing this issue would require
>>>>>> to modify the prototype of os::is_memory_serialize_page()
>>>>>> and propagate the change across all platforms using it.
>>>>>> It's a wider scope fix than JDK-8137035.
>>>>>
>>>>> I was only expecting you to move (if the scoping allows it, else copy)
>>>>> the later:
>>>>
>>>> The scoping doesn't allow his move, and adding the test would
>>>> potentially change the behavior (I'm expecting that only JavaThreads
>>>> could be blocked on the serialization page, but I might be wrong).
>>>
>>> If we were to find that a non-JavaThread were executing that code that
>>> would be a significant bug I think. I really think this needs to be
>>> fixed to ensure it is a JavaThread - even if you only add an assert.
>>>
>>> Thanks,
>>> David
>>>
>>>> Fred
>>>>
>>>>> if (t != NULL && t->is_Java_thread()) {
>>>>>
>>>>> check. ;)
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> I've added a comment the unsafe cast in os_windows.cpp file,
>>>>>> highlighting the fact it was unsafe, and explaining why it
>>>>>> is currently harmless.
>>>>>>
>>>>>>>
>>>>>>> 2523   if (t != NULL && t->is_Java_thread()) {
>>>>>>> 2524     JavaThread* thread = (JavaThread*) t;
>>>>>>>
>>>>
>