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

[Daniel D. Daugherty]

On 9/2/16 12:39 PM, Frederic Parain wrote:
>
>
> On 09/02/2016 01:02 PM, 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 '('.
>
> Fixed.
>
>> 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 case of transition from native to Java is currently handled in
> the native call wrapper, in which yellow pages are unconditionally
> restored when the thread return to Java.
> This is unfortunate to have the yellow pages restoration managed
> at two different locations. Coleen and I discussed a clean up of
> this, by removing yellow page management from native call wrappers
> and exception propagation code and move everything into the thread
> state transition code. But it looks too dangerous to make this
> changes for JDK9, the JDK10 time frame would be a better choice
> to do it.
>
> The transition_and_fence() is used when the thread has no Java frame
> on its call stack, so it doesn't make sense to me to check and
> restore yellow pages in this case.
>
> Karen was concerned about the cost of an additional test in thread
> state transition, this is why I've tried to put the code only in
> paths where the thread was transitioning to _thread_in_Java, and
> keep other transitions (ThreadBlockInVM and others) untouched.

Sounds very reasonable to me. Thumbs up on your latest version...

Dan

>
> Regards,
>
> Fred
>
>>     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;
>>>>>>>>
>>>>>
>>