[rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 177 bytes --]

[mailing list changed]

The attached implements the theory.  Doesn't show any regressions.  Then 
again, I don't know if it fixes the reported problem either :-/

Kris?

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 1076 bytes --]

2002-11-27  Andrew Cagney  <cagney@redhat.com>

	* arm-tdep.c (arm_frame_align): New function.
	(arm_gdbarch_init): Set frame_align.

Index: arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.78
diff -u -r1.78 arm-tdep.c
--- arm-tdep.c	24 Nov 2002 18:23:37 -0000	1.78
+++ arm-tdep.c	27 Nov 2002 18:26:44 -0000
@@ -1279,6 +1279,15 @@
   return sp;
 }
 
+/* Ensure that the ARM's stack pointer has the correct alignment for a
+   new frame.  */
+static CORE_ADDR
+arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  return (addr & -16);
+}
+
+
 /* Push an empty stack frame, to record the current PC, etc.  */
 
 static void
@@ -2857,6 +2866,7 @@
   set_gdbarch_frame_args_skip (gdbarch, 0);
   set_gdbarch_frame_init_saved_regs (gdbarch, arm_frame_init_saved_regs);
   set_gdbarch_push_dummy_frame (gdbarch, generic_push_dummy_frame);
+  set_gdbarch_frame_align (gdbarch, arm_frame_align);
   set_gdbarch_pop_frame (gdbarch, arm_pop_frame);
 
   /* Address manipulation.  */

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Richard Earnshaw]

> +/* Ensure that the ARM's stack pointer has the correct alignment for a
> +   new frame.  */
> +static CORE_ADDR
> +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
> +{
> +  return (addr & -16);
> +}

Yuck, two's complement assumption.  I much prefer ~0xf in this case.

But why so much.  The maximum stack alignment you'll see on an ARM is 8 
bytes.

R.

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Andreas Schwab]

Richard Earnshaw <rearnsha@arm.com> writes:

|> > +/* Ensure that the ARM's stack pointer has the correct alignment for a
|> > +   new frame.  */
|> > +static CORE_ADDR
|> > +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
|> > +{
|> > +  return (addr & -16);
|> > +}
|> 
|> Yuck, two's complement assumption.

No, -16 is implicitly cast to bfd_vma, which is unsigned, and this
operation is completely defined independent of the representation of
signed integers.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Richard Earnshaw]

> Richard Earnshaw <rearnsha@arm.com> writes:
> 
> |> > +/* Ensure that the ARM's stack pointer has the correct alignment for a
> |> > +   new frame.  */
> |> > +static CORE_ADDR
> |> > +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
> |> > +{
> |> > +  return (addr & -16);
> |> > +}
> |> 
> |> Yuck, two's complement assumption.
> 
> No, -16 is implicitly cast to bfd_vma, which is unsigned, and this
> operation is completely defined independent of the representation of
> signed integers.

Hmm, strictly speaking you are correct.  I don't have to like it though, 
and it means that the code is heavily dependent on the non-obvious fact 
that addr is an unsigned type to get the correct behaviour (if bfd_vma 
were to be changed to a signed type then this code would quietly break).

So I'll change my comment to:

Yuck, implicit cast of negative number to unsigned.

R.

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Andreas Schwab]

Richard Earnshaw <rearnsha@arm.com> writes:

|> > Richard Earnshaw <rearnsha@arm.com> writes:
|> > 
|> > |> > +/* Ensure that the ARM's stack pointer has the correct alignment for a
|> > |> > +   new frame.  */
|> > |> > +static CORE_ADDR
|> > |> > +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
|> > |> > +{
|> > |> > +  return (addr & -16);
|> > |> > +}
|> > |> 
|> > |> Yuck, two's complement assumption.
|> > 
|> > No, -16 is implicitly cast to bfd_vma, which is unsigned, and this
|> > operation is completely defined independent of the representation of
|> > signed integers.
|> 
|> Hmm, strictly speaking you are correct.  I don't have to like it though, 
|> and it means that the code is heavily dependent on the non-obvious fact 
|> that addr is an unsigned type to get the correct behaviour (if bfd_vma 
|> were to be changed to a signed type then this code would quietly break).
|> 
|> So I'll change my comment to:
|> 
|> Yuck, implicit cast of negative number to unsigned.

Note that ~0xf is even worse.  If sizeof(int) < sizeof(bfd_vma) then you
get the wrong value, because ~0xf is _zero_ extended to bfd_vma.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Richard Earnshaw]

> Richard Earnshaw <rearnsha@arm.com> writes:
> 
> |> > Richard Earnshaw <rearnsha@arm.com> writes:
> |> > 
> |> > |> > +/* Ensure that the ARM's stack pointer has the correct alignment for a
> |> > |> > +   new frame.  */
> |> > |> > +static CORE_ADDR
> |> > |> > +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
> |> > |> > +{
> |> > |> > +  return (addr & -16);
> |> > |> > +}
> |> > |> 
> |> > |> Yuck, two's complement assumption.
> |> > 
> |> > No, -16 is implicitly cast to bfd_vma, which is unsigned, and this
> |> > operation is completely defined independent of the representation of
> |> > signed integers.
> |> 
> |> Hmm, strictly speaking you are correct.  I don't have to like it though, 
> |> and it means that the code is heavily dependent on the non-obvious fact 
> |> that addr is an unsigned type to get the correct behaviour (if bfd_vma 
> |> were to be changed to a signed type then this code would quietly break).
> |> 
> |> So I'll change my comment to:
> |> 
> |> Yuck, implicit cast of negative number to unsigned.
> 
> Note that ~0xf is even worse.  If sizeof(int) < sizeof(bfd_vma) then you
> get the wrong value, because ~0xf is _zero_ extended to bfd_vma.

But not if you write

	~(bfd_vma)0xf

R.

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Keith Walker]

At 10:40 28/11/2002 +0000, Richard Earnshaw wrote:
> > Richard Earnshaw <rearnsha@arm.com> writes:
> >
> > |> > +/* Ensure that the ARM's stack pointer has the correct alignment 
> for a
> > |> > +   new frame.  */
> > |> > +static CORE_ADDR
> > |> > +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
> > |> > +{
> > |> > +  return (addr & -16);
> > |> > +}
> > |>
> > |> Yuck, two's complement assumption.
> >
> > No, -16 is implicitly cast to bfd_vma, which is unsigned, and this
> > operation is completely defined independent of the representation of
> > signed integers.
>
>Hmm, strictly speaking you are correct.  I don't have to like it though,
>and it means that the code is heavily dependent on the non-obvious fact
>that addr is an unsigned type to get the correct behaviour (if bfd_vma
>were to be changed to a signed type then this code would quietly break).
>
>So I'll change my comment to:
>
>Yuck, implicit cast of negative number to unsigned.

I notice that the h8300-tdep.c file defines some macros to do this sort of 
rounding ... round_up() and round_down().     As aligning addresses is a 
reasonable common thing to do maybe these macros should be moved to a more 
generic place and used whenever such rounding is required.    It doesn't 
fix the implicit cast issue, but would at least make finding where such 
alignment occurs easier.

Keith

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Andrew Cagney]

>> +/* Ensure that the ARM's stack pointer has the correct alignment for a
>> +   new frame.  */
>> +static CORE_ADDR
>> +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>> +{
>> +  return (addr & -16);
>> +}
> 
> 
> Yuck, two's complement assumption.  I much prefer ~0xf in this case.
> 
> But why so much.  The maximum stack alignment you'll see on an ARM is 8 
> bytes.

The function was lifted from the PPC code.  And, hey, what's a few extra 
bytes between friends? :-)

Does (addr & 0x7) look better?

Andrew

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Andrew Cagney]

> At 10:40 28/11/2002 +0000, Richard Earnshaw wrote:
>> Richard Earnshaw <rearnsha@arm.com> writes:
>>
>> |> > +/* Ensure that the ARM's stack pointer has the correct alignment for a
>> |> > +   new frame.  */
>> |> > +static CORE_ADDR
>> |> > +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>> |> > +{
>> |> > +  return (addr & -16);
>> |> > +}
>> |>
>> |> Yuck, two's complement assumption.
>>
>> No, -16 is implicitly cast to bfd_vma, which is unsigned, and this
>> operation is completely defined independent of the representation of
>> signed integers.
> 
> Hmm, strictly speaking you are correct.  I don't have to like it though,
> and it means that the code is heavily dependent on the non-obvious fact
> that addr is an unsigned type to get the correct behaviour (if bfd_vma
> were to be changed to a signed type then this code would quietly break).
> 
> So I'll change my comment to:
> 
> Yuck, implicit cast of negative number to unsigned.
> 
> I notice that the h8300-tdep.c file defines some macros to do this sort of rounding ... round_up() and round_down().     As aligning addresses is a reasonable common thing to do maybe these macros should be moved to a more generic place and used whenever such rounding is required.    It doesn't fix the implicit cast issue, but would at least make finding where such alignment occurs easier.

Hmm, the've been breeding.  So does the MIPS and s390 ....
Equivalent functions that spell out each step vis:

	ULONGEST mask = -(ULONGEST)(n);
	ULONGEST maddr = (addr & mask);
	return maddr;

and have a 10:1 comment:code ratio sound like a good idea.

A GDB goal can be to debug this at -O3 :-)

Andrew

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Richard Earnshaw]

> >> +/* Ensure that the ARM's stack pointer has the correct alignment for a
> >> +   new frame.  */
> >> +static CORE_ADDR
> >> +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
> >> +{
> >> +  return (addr & -16);
> >> +}
> > 
> > 
> > Yuck, two's complement assumption.  I much prefer ~0xf in this case.
> > 
> > But why so much.  The maximum stack alignment you'll see on an ARM is 8 
> > bytes.
> 
> The function was lifted from the PPC code.  And, hey, what's a few extra 
> bytes between friends? :-)
> 
> Does (addr & 0x7) look better?
> 
> Andrew
> 
> 

As discussed elsewhere, the best solution (IMO) is

	 ~(CORE_ADDR)0x7

R.

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Andrew Cagney]

>> >> +/* Ensure that the ARM's stack pointer has the correct alignment for a
>> >> +   new frame.  */
>> >> +static CORE_ADDR
>> >> +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>> >> +{
>> >> +  return (addr & -16);
>> >> +}
> 
>> > 
>> > 
>> > Yuck, two's complement assumption.  I much prefer ~0xf in this case.
>> > 
>> > But why so much.  The maximum stack alignment you'll see on an ARM is 8 
>> > bytes.
> 
>> 
>> The function was lifted from the PPC code.  And, hey, what's a few extra 
>> bytes between friends? :-)
>> 
>> Does (addr & 0x7) look better?
>> 
>> Andrew
>> 
>> 
> 
> 
> As discussed elsewhere, the best solution (IMO) is
> 
> 	 ~(CORE_ADDR)0x7

You mean ~(ULONGEST)0x7?

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Richard Earnshaw]

> >> >> +/* Ensure that the ARM's stack pointer has the correct alignment for a
> >> >> +   new frame.  */
> >> >> +static CORE_ADDR
> >> >> +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
> >> >> +{
> >> >> +  return (addr & -16);
> >> >> +}
> > 
> >> > 
> >> > 
> >> > Yuck, two's complement assumption.  I much prefer ~0xf in this case.
> >> > 
> >> > But why so much.  The maximum stack alignment you'll see on an ARM is 8 
> >> > bytes.
> > 
> >> 
> >> The function was lifted from the PPC code.  And, hey, what's a few extra 
> >> bytes between friends? :-)
> >> 
> >> Does (addr & 0x7) look better?
> >> 
> >> Andrew
> >> 
> >> 
> > 
> > 
> > As discussed elsewhere, the best solution (IMO) is
> > 
> > 	 ~(CORE_ADDR)0x7
> 
> You mean ~(ULONGEST)0x7?
> 
> 

I mean ~(typeof(addr)) 0x7.

R.

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Andrew Cagney]

>> >> >> +/* Ensure that the ARM's stack pointer has the correct alignment for a
>> >> >> +   new frame.  */
>> >> >> +static CORE_ADDR
>> >> >> +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>> >> >> +{
>> >> >> +  return (addr & -16);
>> >> >> +}

>> You mean ~(ULONGEST)0x7?

> I mean ~(typeof(addr)) 0x7.

M'kay (finally gets brain around what that expression is doing), back to 
the patch.

Does it fix the bug?

Andrew

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Andrew Cagney]

>>> >> +/* Ensure that the ARM's stack pointer has the correct alignment for a
>>> >> +   new frame.  */
>>> >> +static CORE_ADDR
>>> >> +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>>> >> +{
>>> >> +  return (addr & -16);
>>> >> +}
> 
> You mean ~(ULONGEST)0x7?
> 
> I mean ~(typeof(addr)) 0x7.
> 
> M'kay (finally gets brain around what that expression is doing), back to the patch.
> 
> Does it fix the bug?

Anyone?

Andrew

Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand called functions

[Kris Warkentin]

Seems to be okay on this end.

Kris
----- Original Message -----
From: "Andrew Cagney" <ac131313@redhat.com>
To: "Andrew Cagney" <ac131313@redhat.com>
Cc: <Richard.Earnshaw@arm.com>; "Kris Warkentin" <kewarken@qnx.com>;
<gdb-patches@sources.redhat.com>
Sent: Tuesday, January 28, 2003 7:34 PM
Subject: Re: [rfa?] Add frame_align(); Was: ARM stack alignment on hand
called functions


> >>> >> +/* Ensure that the ARM's stack pointer has the correct alignment
for a
> >>> >> +   new frame.  */
> >>> >> +static CORE_ADDR
> >>> >> +arm_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
> >>> >> +{
> >>> >> +  return (addr & -16);
> >>> >> +}
> >
> > You mean ~(ULONGEST)0x7?
> >
> > I mean ~(typeof(addr)) 0x7.
> >
> > M'kay (finally gets brain around what that expression is doing), back to
the patch.
> >
> > Does it fix the bug?
>
> Anyone?
>
> Andrew
>
>
>