Re: Problem with ARM_DOUBLEWORD_ALIGN on ARM

Re: Problem with ARM_DOUBLEWORD_ALIGN on ARM

[Geert Bosch]

On Nov 21, 2007, at 16:34, Daniel Jacobowitz wrote:
>Did you mean to send this to the list?  If so, feel free to reply
>there.
Indeed, I did. I now Cc-ed the list as well.
>>Here's the first few lines of a-textio.adb disassembly:
>>Dump of assembler code for function ada__text_io__put_line:
>>0x800150ac <ada__text_io__put_line+0>:  mov     r12, sp
>>0x800150b0 <ada__text_io__put_line+4>:  stmdb   sp!, {r4, r5, r6, r7, r8, r9, 
>>r10, r11, r12, lr, pc}
>>0x800150b4 <ada__text_io__put_line+8>:  sub     r11, r12, #4    ; 0x4
>>0x800150b8 <ada__text_io__put_line+12>: sub     sp, sp, #20     ; 0x14
>>
>>So, stack pointer is not properly aligned.

>Sure it's aligned.  The stmdb decrements it by 11 * 4, the sub
>decrements it by 5 * 4, that adds up to a nice 16 * 4 frame size.

Ah, I missed that. Turns out the (or: at least one) problem was 
with nested functions indeed. See below for a rough patch against
GCC 4.1 that seems to fix the issue we're seeing (thanks
Richard, for the help). However,  we're not quite sure if this is
right.  If you or other ARM-knowledgeable people have any feedback,
that would be most welcome. I'll then make an updated patch against
head and submit for review after testing etc.

>>I guess, the biggest question is how things are supposed
>>to work and what the invariants are that should be kept to ensure
>>proper stack aligment.

>I can't answer this for you, except that our extensive arm-eabi and
>arm-linux-gnueabi testing has not turned up problems with this
>alignment in quite a while, so it mostly does work.

Nested functions aren't used that much in C indeed... :)

  -Geert

--- arm.c.orig  2007-11-20 16:27:04.000000000 -0500
+++ arm.c       2007-11-21 18:15:18.000000000 -0500
@@ -10448,6 +10448,14 @@ arm_get_frame_offsets (void)
   /* Saved registers include the stack frame.  */
   offsets->saved_regs = offsets->saved_args + saved;
   offsets->soft_frame = offsets->saved_regs + CALLER_INTERWORKING_SLOT_SIZE;
+
+  /* Allow for storage of static chain when it needs its own space in the
+     frame. */
+  if (IS_NESTED (arm_current_func_type ())
+      && regs_ever_live[3]
+      && offsets->saved_args == 0)
+    offsets->soft_frame += 4;
+
   /* A leaf function does not need any stack alignment if it has nothing
      on the stack.  */
   if (leaf && frame_size == 0)
@@ -10569,6 +10577,7 @@ arm_expand_prologue (void)
   unsigned long live_regs_mask;
   unsigned long func_type;
   int fp_offset = 0;
+  int static_chain_size = 0;
   int saved_pretend_args = 0;
   int saved_regs = 0;
   unsigned HOST_WIDE_INT args_to_push;
@@ -10643,6 +10652,7 @@ arm_expand_prologue (void)
              insn = emit_insn (insn);
 
              fp_offset = 4;
+             static_chain_size = 4;
 
              /* Just tell the dwarf backend that we adjusted SP.  */
              dwarf = gen_rtx_SET (VOIDmode, stack_pointer_rtx,
@@ -10836,14 +10846,15 @@ arm_expand_prologue (void)
     }
 
   offsets = arm_get_frame_offsets ();
-  if (offsets->outgoing_args != offsets->saved_args + saved_regs)
+  if (offsets->outgoing_args != offsets->saved_args + saved_regs
+      + static_chain_size)
     {
       /* This add can produce multiple insns for a large constant, so we
         need to get tricky.  */
       rtx last = get_last_insn ();
 
       amount = GEN_INT (offsets->saved_args + saved_regs
-                       - offsets->outgoing_args);
+                        + static_chain_size - offsets->outgoing_args);
 
       insn = emit_insn (gen_addsi3 (stack_pointer_rtx, stack_pointer_rtx,
                                    amount));

Re: Problem with ARM_DOUBLEWORD_ALIGN on ARM

[Daniel Jacobowitz]

On Wed, Nov 21, 2007 at 06:32:22PM -0500, Geert Bosch wrote:
> Richard, for the help). However,  we're not quite sure if this is
> right.  If you or other ARM-knowledgeable people have any feedback,
> that would be most welcome. I'll then make an updated patch against
> head and submit for review after testing etc.

Won't be me, I'm afraid - I don't know how this code works, just that
it does in our testing.  Sorry.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Problem with ARM_DOUBLEWORD_ALIGN on ARM

[Mark Mitchell]

Geert Bosch wrote:

> Nested functions aren't used that much in C indeed... :)

Paul, would you please review this patch?

> --- arm.c.orig  2007-11-20 16:27:04.000000000 -0500
> +++ arm.c       2007-11-21 18:15:18.000000000 -0500
> @@ -10448,6 +10448,14 @@ arm_get_frame_offsets (void)
>    /* Saved registers include the stack frame.  */
>    offsets->saved_regs = offsets->saved_args + saved;
>    offsets->soft_frame = offsets->saved_regs + CALLER_INTERWORKING_SLOT_SIZE;
> +
> +  /* Allow for storage of static chain when it needs its own space in the
> +     frame. */
> +  if (IS_NESTED (arm_current_func_type ())
> +      && regs_ever_live[3]
> +      && offsets->saved_args == 0)
> +    offsets->soft_frame += 4;
> +
>    /* A leaf function does not need any stack alignment if it has nothing
>       on the stack.  */
>    if (leaf && frame_size == 0)
> @@ -10569,6 +10577,7 @@ arm_expand_prologue (void)
>    unsigned long live_regs_mask;
>    unsigned long func_type;
>    int fp_offset = 0;
> +  int static_chain_size = 0;
>    int saved_pretend_args = 0;
>    int saved_regs = 0;
>    unsigned HOST_WIDE_INT args_to_push;
> @@ -10643,6 +10652,7 @@ arm_expand_prologue (void)
>               insn = emit_insn (insn);
>  
>               fp_offset = 4;
> +             static_chain_size = 4;
>  
>               /* Just tell the dwarf backend that we adjusted SP.  */
>               dwarf = gen_rtx_SET (VOIDmode, stack_pointer_rtx,
> @@ -10836,14 +10846,15 @@ arm_expand_prologue (void)
>      }
>  
>    offsets = arm_get_frame_offsets ();
> -  if (offsets->outgoing_args != offsets->saved_args + saved_regs)
> +  if (offsets->outgoing_args != offsets->saved_args + saved_regs
> +      + static_chain_size)
>      {
>        /* This add can produce multiple insns for a large constant, so we
>          need to get tricky.  */
>        rtx last = get_last_insn ();
>  
>        amount = GEN_INT (offsets->saved_args + saved_regs
> -                       - offsets->outgoing_args);
> +                        + static_chain_size - offsets->outgoing_args);
>  
>        insn = emit_insn (gen_addsi3 (stack_pointer_rtx, stack_pointer_rtx,
>                                     amount));
> 


-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Problem with ARM_DOUBLEWORD_ALIGN on ARM

[Geert Bosch]

On Dec 26, 2007, at 14:24, Mark Mitchell wrote:

> Geert Bosch wrote:
>
>> Nested functions aren't used that much in C indeed... :)
>
> Paul, would you please review this patch?

This patch isn't good. While it addressed the alignment
issue, it didn't correctly adjust all necessary offset
computations.

One of the problems is that this port has grown many
magic adjustments and mostly similar recalculations.
Olivier Hainque has refactored some of the code and
has a proper patch that has had a good amount of testing
now. We also have a testcase in C.

We'll submit this for review soon. In the meantime,
please consider this patch withdrawm.

   -Geert

Re: Problem with ARM_DOUBLEWORD_ALIGN on ARM

[Richard Kenner]

> Do you have any test cases?  I'm pretty sure this works, at least in
> the usual cases.

They're going to be hard to construct since they are sensitive to such
things as the number of registers saved.

> A wild guess says that you're doing this in Ada.  It may be something
> specific to the nested case then.

It is in Ada, but I don't think the nested case is the cause of the
particular problem I ran into (it does need to be fixed anyway, though,
since it might bite later).  I suspect the case we're running into is
either the args to push or the registers saved.

Here's a description of the miscompilation we saw:

> Specifically, in Ada.Text_IO.Put_Line, we end up calling memcpy
> with a buffer allocated using "alloca". The alloca result
> is aligned to a multiple of 8, but the corresponding operation
> on the stack pointer is eliminated because combine assumes
> the stack is aligned already.
> 
> Specifically, in the example below, the assignment to
> Buffer (1 .. Ilen) is done through memcpy, but
> memcpy overwrites the return address.
> 
>  From a-textio.adb, around line 1450:
> 
>        --  Now prepare the string with its terminator
> 
>        declare
>           Buffer : String (1 .. Ilen + 2);
>           Plen   : size_t;
> 
>        begin
>           Buffer (1 .. Ilen) := Item (Istart .. Item'Last);
>           Buffer (Ilen + 1) := Character'Val (LM);

When you look at the disassembled code, we saw an update to the stack
pointer in the prologue of a number that wasn't a multiple of 8 (it
was 36, if I remember correctly).

Re: Problem with ARM_DOUBLEWORD_ALIGN on ARM

[Daniel Jacobowitz]

On Wed, Nov 21, 2007 at 03:56:02PM -0500, Richard Kenner wrote:
> When that option is enabled, STACK_BOUNDARY is set to 64.
> 
> But when you look at arm_expand_prologue, it appears that very little
> effort is made to respect that alignment.  Three specific cases I see
> are the IS_NESTED case of pushing ip_rtx and, the lack of checking the
> size of args_to_push, and no attempt to ensure that an even number of
> registers are saved.  But there may well be other cases I haven't found.
> 
> I'm not familiar with the ABI of that machine to know how these should
> be changed.  Does anybody who knows the ABI know how to fix this?

Do you have any test cases?  I'm pretty sure this works, at least in
the usual cases.

A wild guess says that you're doing this in Ada.  It may be something
specific to the nested case then.

-- 
Daniel Jacobowitz
CodeSourcery

Problem with ARM_DOUBLEWORD_ALIGN on ARM

[Richard Kenner]

When that option is enabled, STACK_BOUNDARY is set to 64.

But when you look at arm_expand_prologue, it appears that very little
effort is made to respect that alignment.  Three specific cases I see
are the IS_NESTED case of pushing ip_rtx and, the lack of checking the
size of args_to_push, and no attempt to ensure that an even number of
registers are saved.  But there may well be other cases I haven't found.

I'm not familiar with the ABI of that machine to know how these should
be changed.  Does anybody who knows the ABI know how to fix this?