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

[Frederic Parain]

Thank you Dan!

Fred

On 09/02/2016 02:48 PM, Daniel D. Daugherty wrote:
> 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;
>>>>>>>>>
>>>>>>
>>>
>