On Tue, Apr 23, 2013 at 03:10:04PM +0200, Landry Breuil wrote:
> On Tue, Apr 23, 2013 at 08:57:31AM -0400, Kenneth R Westerback wrote:
> > On Tue, Apr 23, 2013 at 03:40:58AM -0600, Landry Breuil wrote:
> > > CVSROOT: /cvs
> > > Module name: ports
> > > Changes by: [email protected] 2013/04/23 03:40:58
> > >
> > > Modified files:
> > > www/webkit : Makefile
> > > Added files:
> > > www/webkit/patches: patch-Source_WTF_wtf_StackBounds_cpp
> > >
> > > Log message:
> > > Add a patch to fix stack bounds computation, from David Hill (also
> > > pushed upstream as https://bugs.webkit.org/show_bug.cgi?id=114978)
> > >
> > > Seems to fix xombrero/surf crashes on amd64, and seems to magically make
> > > webkit sort-of usable (ie dead-slow, but js seems to work) on powerpc.
> > > (symptom : ** Message: console message: undefined @0: RangeError: Maximum
> > > call stack size exceeded.)
> > >
> > > Note : webkit still fails to build on mips64* and hppa. sigh.
> > >
> > > ok jasper@
> > >
> >
> > So the logic was not checked/fixed where the bounds are actually
> > checked on architectures that have upward growing stacks? Where
> > m_bound and m_origin have their meanings swapped?
>
> The previous logic was doing (note the lovely FIXME) :
>
> 67 // FIXME: remove this! - this code unsafely guesses at stack sizes!
> 68 static const ptrdiff_t estimatedStackSize = 128 * sizeof(void*) * 1024;
> 69 // This method assumes the stack is growing downwards.
> 70 static void* estimateStackBound(void* origin)
> 71 {
> 72 return static_cast<char*>(origin) - estimatedStackSize;
> 73 }
>
> 122 void StackBounds::initialize()
> 123 {
> 124 pthread_t thread = pthread_self();
> 125 stack_t stack;
> 126 pthread_stackseg_np(thread, &stack);
> 127 m_origin = stack.ss_sp;
> 128 m_bound = estimateStackBound(m_origin);
> 129 }
>
> The new logic does :
>
> 122 void StackBounds::initialize()
> 123 {
> 124 pthread_t thread = pthread_self();
> 125 stack_t stack;
> 126 pthread_stackseg_np(thread, &stack);
> 127 m_origin = stack.ss_sp;
> 128 #if defined(__hppa__) || defined(__hppa64__)
> 129 // hppa's stack grows up
> 130 m_bound = static_cast<char*>(m_origin) + stack.ss_size;
> 131 #else
> 132 m_bound = static_cast<char*>(m_origin) - stack.ss_size;
> 133 #endif
> 134 }
I am familiar with the patch. My concern is that on upward growing
stack machines, if the code is doing things like
if (new_stack_pos > m_bound)
life_is_good()
else
stack_go_boom()
having a better value for m_bound doesn't fully fix the problem. If the
code doesn't make such assumptions then I guess things are fine.
.... Ken
>
> > I only see MACHINE_STACK_GROWS_UP defined on hppa and hppa64. Are
> > these really the only archs with upward growing stacks?
>
> It seems so. Do you mean we should pull that #define from a header
> instead of hardcoding hppa* here ?
>
> Landry
