ARM stack alignment on hand called functions

ARM stack alignment on hand called functions

[Kris Warkentin]

Hi,

I was chasing a bug that was uncovered by the gdb dejagnu regression suite
on QNX 6 with gdb 5.2.1.  The problem was in the file 'structs.c' which
follows the pattern below.

If I break on main and then do something like 'call fun1()', the inferior
would die with a SIGBUS.  As long as I called functions whose structures
were divisible by 4, like fun4(), fun12(), etc., it was fine.  I chased it
down to stack pointer alignment: the value stuffed into sp when it executed
the dummy frame was not aligned on a 4 byte boundary.

Initially I had tried defining STACK_ALIGN() but it seemed to cause other
problems to pop up.  For example, 'call Fun1(foo1)' would fail with a
SIGBUS.  My final fix which seems to work well was just to add sp = (sp + 3)
& ~3 at the end of arm_push_arguments() in arm-tdep.c just before it returns
sp.  Looking at the code for mips_push_arguments though, it seems like this
might be a little simplistic since there is quite a lot of alignment code in
there.

Can anyone comment on the correctness of this fix?

cheers,

Kris


struct struct1 { char a;};
struct struct2 { char a, b;};
struct struct3 { char a, b, c; };
...

struct struct1 foo1 = {'1'},  L1;
struct struct2 foo2 = { 'a', 'b'},  L2;
struct struct3 foo3 = { 'A', 'B', 'C'},  L3;
...

struct struct1  fun1()
{
  return foo1;
}
struct struct2  fun2()
{
  return foo2;
}
...

void Fun1(struct struct1 foo1)
{
  L1 = foo1;
}
void Fun2(struct struct2 foo2)
{
  L2 = foo2;
}
....

Re: ARM stack alignment on hand called functions

[Richard Earnshaw]

> Hi,
> 
> I was chasing a bug that was uncovered by the gdb dejagnu regression suite
> on QNX 6 with gdb 5.2.1.  The problem was in the file 'structs.c' which
> follows the pattern below.
> 
> If I break on main and then do something like 'call fun1()', the inferior
> would die with a SIGBUS.  As long as I called functions whose structures
> were divisible by 4, like fun4(), fun12(), etc., it was fine.  I chased it
> down to stack pointer alignment: the value stuffed into sp when it executed
> the dummy frame was not aligned on a 4 byte boundary.
> 
> Initially I had tried defining STACK_ALIGN() but it seemed to cause other
> problems to pop up.  For example, 'call Fun1(foo1)' would fail with a
> SIGBUS.  My final fix which seems to work well was just to add sp = (sp + 3)
> & ~3 at the end of arm_push_arguments() in arm-tdep.c just before it returns
> sp.  Looking at the code for mips_push_arguments though, it seems like this
> might be a little simplistic since there is quite a lot of alignment code in
> there.
> 
> Can anyone comment on the correctness of this fix?

No, I don't think this is correct, since it will mean that the structure 
starts at an unaligned address.  Instead the space allocated for the 
structure on the stack should be rounded up to a word and then the 
structure copied into that space with an aligned starting point.

R.

Re: ARM stack alignment on hand called functions

[Kris Warkentin]

----- Original Message -----
From: "Richard Earnshaw" <rearnsha@arm.com>
> No, I don't think this is correct, since it will mean that the structure
> starts at an unaligned address.  Instead the space allocated for the
> structure on the stack should be rounded up to a word and then the
> structure copied into that space with an aligned starting point.

But in this case, the issue isn't with passing arguments but rather with
returning them.  Earlier in hand_function_call() (in valops.c), there is
some code where if a structure is being returned, we enlarge the stack by
the size of the structure.  This is what wasn't aligned.  For example, if
you called 'fun2()', which returns a 2 byte structure, the type length was
'2' which is how much the stack pointer is out.  This way, when we write the
stack at the start of the called function, our sp is misaligned.  The frame
pointer is fine so the function gets the arguments alright, it's just
writing stack variables and return values to the stack that is buggered.
I'm looking at this and thinking, it just gives a little padding on the
stack to maintain alignment.

cheers,

Kris

Re: ARM stack alignment on hand called functions

[Richard Earnshaw]

> ----- Original Message -----
> From: "Richard Earnshaw" <rearnsha@arm.com>
> > No, I don't think this is correct, since it will mean that the structure
> > starts at an unaligned address.  Instead the space allocated for the
> > structure on the stack should be rounded up to a word and then the
> > structure copied into that space with an aligned starting point.
> 
> But in this case, the issue isn't with passing arguments but rather with
> returning them.  Earlier in hand_function_call() (in valops.c), there is
> some code where if a structure is being returned, we enlarge the stack by
> the size of the structure.  This is what wasn't aligned.  For example, if
> you called 'fun2()', which returns a 2 byte structure, the type length was
> '2' which is how much the stack pointer is out.  This way, when we write the
> stack at the start of the called function, our sp is misaligned.  The frame
> pointer is fine so the function gets the arguments alright, it's just
> writing stack variables and return values to the stack that is buggered.
> I'm looking at this and thinking, it just gives a little padding on the
> stack to maintain alignment.
> 

Then I'm not sure I understand exactly what your problem is.  If you show 
me the actual patch, rather than just trying to describe it, then perhaps 
that will be more clear.

R.

Re: ARM stack alignment on hand called functions

[Kris Warkentin]

The problem arises only with functions which return structures whose size is
not evenly divisible by 4.  Below is what I did to solve it.

Index: arm-tdep.c
===================================================================
RCS file: /product/tools/gdb/gdb/arm-tdep.c,v
retrieving revision 1.9
retrieving revision 1.10
diff -c -r1.9 -r1.10
*** arm-tdep.c  20 Sep 2002 17:11:31 -0000      1.9
--- arm-tdep.c  19 Nov 2002 18:33:37 -0000      1.10
***************
*** 1480,1485 ****
--- 1480,1486 ----
        }
      }

+   sp = (sp + 3) & ~3;
    /* Return adjusted stack pointer.  */
    return sp;
  }

The code in valops.c : hand_function_call() that was causing the problem was
this:

  /* Reserve space for the return structure to be written on the
     stack, if necessary */
  if (struct_return)
    {
      int len = TYPE_LENGTH (value_type);
      if (STACK_ALIGN_P ())
 /* MVS 11/22/96: I think at least some of this stack_align
    code is really broken.  Better to let PUSH_ARGUMENTS adjust
    the stack in a target-defined manner.  */
 len = STACK_ALIGN (len);
      if (INNER_THAN (1, 2))
 {
   /* stack grows downward */
   sp -= len;
   struct_addr = sp;
 }

So what I did was to make sure that arm_push_arguments would always return
an aligned stack pointer.  I think we can safely agree that
arm_push_arguments should NEVER return an unaligned stack pointer right?

cheers,

Kris

Re: ARM stack alignment on hand called functions

[Andrew Cagney]

> The problem arises only with functions which return structures whose size is
> not evenly divisible by 4.  Below is what I did to solve it.

Kris, FYI,
It's very important to always reproduce problems using the current GDB 
sources.  That way, any confusion arrising from either local 
modifications (does QNX 6 ship an un-modified GDB 5.2.1?) or out-of-date 
sources (has the problem been fixed?).

For the problem at hand, I suspect the post 5.3 architecture method - 
gdbarch_frame_align() - is needed.  That method is used to align each 
element's stack address (e.g., struct return) and not the size of each 
element.

(I think I might add a few more juicy comments to valops.c so its 
clearer what the fix to your problem is :-).

Andrew

(1) BTW, note:
http://sources.redhat.com/gdb/current/onlinedocs/gdbint_15.html#SEC130
To avoid version conflicts, vendors are expected to modify the file 
`gdb/version.in' to include a vendor unique alphabetic identifier (an 
official GDB release never uses alphabetic characters in its version 
identifer).



> Index: arm-tdep.c
> ===================================================================
> RCS file: /product/tools/gdb/gdb/arm-tdep.c,v
> retrieving revision 1.9
> retrieving revision 1.10
> diff -c -r1.9 -r1.10
> *** arm-tdep.c  20 Sep 2002 17:11:31 -0000      1.9
> --- arm-tdep.c  19 Nov 2002 18:33:37 -0000      1.10
> ***************
> *** 1480,1485 ****
> --- 1480,1486 ----
>         }
>       }
> 
> +   sp = (sp + 3) & ~3;
>     /* Return adjusted stack pointer.  */
>     return sp;
>   }
> 
> The code in valops.c : hand_function_call() that was causing the problem was
> this:
> 
>   /* Reserve space for the return structure to be written on the
>      stack, if necessary */
>   if (struct_return)
>     {
>       int len = TYPE_LENGTH (value_type);
>       if (STACK_ALIGN_P ())
>  /* MVS 11/22/96: I think at least some of this stack_align
>     code is really broken.  Better to let PUSH_ARGUMENTS adjust
>     the stack in a target-defined manner.  */
>  len = STACK_ALIGN (len);
>       if (INNER_THAN (1, 2))
>  {
>    /* stack grows downward */
>    sp -= len;
>    struct_addr = sp;
>  }
> 
> So what I did was to make sure that arm_push_arguments would always return
> an aligned stack pointer.  I think we can safely agree that
> arm_push_arguments should NEVER return an unaligned stack pointer right?
> 
> cheers,
> 
> Kris
> 
> 

Re: ARM stack alignment on hand called functions

[Kris Warkentin]

----- Original Message -----
From: "Andrew Cagney" <ac131313@redhat.com>
To: "Kris Warkentin" <kewarken@qnx.com>
Cc: <Richard.Earnshaw@arm.com>; <gdb@sources.redhat.com>
Sent: Wednesday, November 20, 2002 12:18 PM
Subject: Re: ARM stack alignment on hand called functions


> > The problem arises only with functions which return structures whose
size is
> > not evenly divisible by 4.  Below is what I did to solve it.
>
> Kris, FYI,
> It's very important to always reproduce problems using the current GDB
> sources.  That way, any confusion arrising from either local
> modifications (does QNX 6 ship an un-modified GDB 5.2.1?) or out-of-date
> sources (has the problem been fixed?).

We had to pick a snapshot for our current release so we chose 5.2.1 which is
the current, stable gdb release.  Our modifications mostly lie in the procfs
interface and the remote communications ends of things.

> For the problem at hand, I suspect the post 5.3 architecture method -
> gdbarch_frame_align() - is needed.  That method is used to align each
> element's stack address (e.g., struct return) and not the size of each
> element.

So perhaps this is a FIXME for us for later.  The thing is, there isn't any
QNX specific code down this path so I suspect this alignment issue is a
generic gdb problem.  Once we finish up with this release, the plan is to
port our QNX modifications to the gdb head branch for eventual inclusion in
the generic release.

> (I think I might add a few more juicy comments to valops.c so its
> clearer what the fix to your problem is :-).

Thanks Andrew.

Kris

Re: ARM stack alignment on hand called functions

[Richard Earnshaw]

> The problem arises only with functions which return structures whose size is
> not evenly divisible by 4.  Below is what I did to solve it.
> 
> Index: arm-tdep.c
> ===================================================================
> RCS file: /product/tools/gdb/gdb/arm-tdep.c,v
> retrieving revision 1.9
> retrieving revision 1.10
> diff -c -r1.9 -r1.10
> *** arm-tdep.c  20 Sep 2002 17:11:31 -0000      1.9
> --- arm-tdep.c  19 Nov 2002 18:33:37 -0000      1.10
> ***************
> *** 1480,1485 ****
> --- 1480,1486 ----
>         }
>       }
> 
> +   sp = (sp + 3) & ~3;
>     /* Return adjusted stack pointer.  */
>     return sp;
>   }
> 

OK, but with this change the alignment is being done *after* any arguments 
that have to go onto the stack have been pushed.  It should happen 
*before*.  What happens if you have?

struct f { char a; char b; char c;};

struct f g = {1,2,3};

struct f h (int a, int b, int c, int d, int e)
{
  g.c = e;
  return g;
}

and then call h from within the debugger.  Is g.c set correctly?

My guess is that it won't, because the integer value for e will have been 
pushed onto the stack incorrectly.

R.

Re: ARM stack alignment on hand called functions

[Kris Warkentin]

> OK, but with this change the alignment is being done *after* any arguments
> that have to go onto the stack have been pushed.  It should happen
> *before*.  What happens if you have?
>
> struct f { char a; char b; char c;};
>
> struct f g = {1,2,3};
>
> struct f h (int a, int b, int c, int d, int e)
> {
>   g.c = e;
>   return g;
> }
>
> and then call h from within the debugger.  Is g.c set correctly?
>
> My guess is that it won't, because the integer value for e will have been
> pushed onto the stack incorrectly.
>

Looks like you might be right:  When I 'call h(1,2,3,4,5)', I get:

Breakpoint 2, h (a=1, b=2, c=3, d=1280, e=-31946752) at armstack.c:8

Hmm.... Looks like some further investigation is needed.

Kris

Re: ARM stack alignment on hand called functions

[Kris Warkentin]

> > OK, but with this change the alignment is being done *after* any
arguments
> > that have to go onto the stack have been pushed.  It should happen
> > *before*.  What happens if you have?

You're right.  If I put the "sp = (sp + 3) & ~3" at the top of
arm_push_arguments(), everything works.  I see that a lot of stuff in
arm-tdep.c has been changed extensively on your head branch, including
arm_push_arguments().  Probably once we move to the head branch, everything
will work fine.

cheers,

Kris

Re: ARM stack alignment on hand called functions

[Richard Earnshaw]

> > > OK, but with this change the alignment is being done *after* any
> arguments
> > > that have to go onto the stack have been pushed.  It should happen
> > > *before*.  What happens if you have?
> 
> You're right.  If I put the "sp = (sp + 3) & ~3" at the top of
> arm_push_arguments(), everything works.  I see that a lot of stuff in
> arm-tdep.c has been changed extensively on your head branch, including
> arm_push_arguments().  Probably once we move to the head branch, everything
> will work fine.
> 

Don't forget that the stack is a "full-descending" stack; so you should be 
rounding the value down, not up.

R.

Re: ARM stack alignment on hand called functions

[Andrew Cagney]

> So perhaps this is a FIXME for us for later.  The thing is, there isn't any
> QNX specific code down this path so I suspect this alignment issue is a
> generic gdb problem.  Once we finish up with this release, the plan is to
> port our QNX modifications to the gdb head branch for eventual inclusion in
> the generic release.

Can I suggest downloading a current GDB snap and find out if the problem 
is still present (and if it isn't, follow through with a patch)? 
Otherwize this will slip through the cracks, only to re-bite you (and 
others) later :-(

Andrew

Re: ARM stack alignment on hand called functions

[Richard Earnshaw]

> 
> > So perhaps this is a FIXME for us for later.  The thing is, there isn't any
> > QNX specific code down this path so I suspect this alignment issue is a
> > generic gdb problem.  Once we finish up with this release, the plan is to
> > port our QNX modifications to the gdb head branch for eventual inclusion in
> > the generic release.
> 
> Can I suggest downloading a current GDB snap and find out if the problem 
> is still present (and if it isn't, follow through with a patch)? 
> Otherwize this will slip through the cracks, only to re-bite you (and 
> others) later :-(
> 
> Andrew
> 
> 

I can see no evidence that the current code is correct either, so perhaps 
we should file a PR with all this info.

R.

Re: ARM stack alignment on hand called functions

[Kris Warkentin]

> Forwarded From: Richard Earnshaw <rearnsha@arm.com>
> > > Can I suggest downloading a current GDB snap and find out if the
problem
> > > is still present (and if it isn't, follow through with a patch)?
> > > Otherwize this will slip through the cracks, only to re-bite you (and
> > > others) later :-(
> > >
> > > Andrew
> > >
> > >
> >
> > I can see no evidence that the current code is correct either, so
perhaps
> > we should file a PR with all this info.
> >
> > R.

Sorry about the late reply...I had missed this in purging my email.  I used
the arm_push_arguments() : arm-tdep.c from your head branch in isolation and
observed the same problem.  When I forced an sp alignment at the top of
arm_push_arguments(), it worked fine.  Now I wonder, however, if " sp =
(sp - 3) & ~3 " is too simplistic of a fix?  Can we assume that arm will
always have 4 byte alignment?  2) Can we assume that arm stacks will always
grow downward?  I'm thinking yes to the direction but no to the alignment so
perhaps we should do a

#define STACK_ALIGN_SIZE 4

in tm-arm.h and then do:

sp = (sp - STACK_ALIGN_SIZE + 1) & ~(STACK_ALIGN_SIZE - 1);

????

Kris