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

[Daniel D. Daugherty]

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 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;
>>>>>>
>>>