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

Frederic Parain frederic.parain at oracle.com
Fri Sep 2 18:39:17 UTC 2016



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.

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


More information about the hotspot-runtime-dev mailing list