[Bug target/52129] New: wrong code to pass parameters to tail call function

[Bug target/52129] New: wrong code to pass parameters to tail call function

[carrot at google dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52129

             Bug #: 52129
           Summary: wrong code to pass parameters to tail call function
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: carrot@google.com
            Target: arm-unknown-linux-gnueabi


Created attachment 26578
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26578
test case

The attached test case is simplified from llvm.

Compile it with options -march=armv7-a -mthumb -O2 -finline-limit=64, GCC 4.7
generates:

...
 61         add     r2, sp, #44
 62         add     r3, r1, r3, lsl #2     // A
 63         mov     r0, r7                 // B
 64         add     r1, r1, r4, lsl #2
 65         str     r1, [sp, #48]          // C
 66         ldmia   r2, {r1, r2}           // D
 67         add     sp, sp, #20
 68         pop     {r4, r5, r6, r7, lr}
 69         add     sp, sp, #8
 70         b       _ZN15ARMDAGToDAGISel19SelectAddrModeImm12E7SDValueRS0_S1_

Instructions ABCD pass parameters to a tail call function, instruction C pass
one parameter through stack. Instruction D read out two words from [sp+44] and
[sp+48], it is also the passed in parameter N of the function itself. the
second word overlaps with instruction C, it is unexpected, so the read out
value of N is actually garbage, and cause runtime error.

The bug is introduced in expand pass

...

(insn 69 68 70 6 (set (mem:SI (plus:SI (reg/f:SI 128 virtual-incoming-args)
                (const_int 8 [0x8])) [0 S4 A32])
        (reg:SI 176)) ARMISelDAGToDAG2.ii:45 -1
     (nil))

(insn 70 69 71 6 (set (reg:SI 0 r0)
        (reg/f:SI 152 [ this ])) ARMISelDAGToDAG2.ii:45 -1
     (nil))

(insn 71 70 72 6 (set (reg:SI 177)
        (plus:SI (reg/f:SI 128 virtual-incoming-args)
            (const_int 4 [0x4]))) ARMISelDAGToDAG2.ii:45 -1
     (nil))

(insn 72 71 73 6 (parallel [
            (set (reg:SI 1 r1)
                (mem/c:SI (reg:SI 177) [4 N+0 S4 A32]))
            (set (reg:SI 2 r2)
                (mem/c:SI (plus:SI (reg:SI 177)
                        (const_int 4 [0x4])) [4 N+4 S4 A32]))
        ]) ARMISelDAGToDAG2.ii:45 -1
     (nil))

(insn 73 72 74 6 (set (reg:SI 3 r3)
        (reg:SI 170)) ARMISelDAGToDAG2.ii:45 -1
     (nil))

(call_insn/j 74 73 75 6 (parallel [
            (set (reg:SI 0 r0)
                (call (mem:SI (symbol_ref:SI
("_ZN15ARMDAGToDAGISel19SelectAddrModeImm12E7SDValueRS0_S1_") [flags 0x41] 
<function_decl 0x7fcefe992800 SelectAddrModeImm12>) [0 SelectAddrModeImm12 S4
A32])
                    (const_int 4 [0x4])))
            (return)
            (use (const_int 0 [0]))
        ]) ARMISelDAGToDAG2.ii:45 -1
     (nil)
    (expr_list:REG_CFA_WINDOW_SAVE (use (reg:SI 3 r3))
        (expr_list:REG_DEP_TRUE (use (reg:SI 2 r2))
            (expr_list:REG_DEP_TRUE (use (reg:SI 1 r1))
                (expr_list:REG_CFA_WINDOW_SAVE (use (reg:SI 0 r0))
                    (expr_list:REG_CFA_WINDOW_SAVE (use (mem/f:SI (plus:SI
(reg/f:SI 128 virtual-incoming-args)
                                    (const_int 8 [0x8])) [0 S4 A32]))
                        (nil)))))))

...

insn 74 is a tail call, insns 69 to 73 are part of the parameter passing
instructions. Because of large number of parameters, insn69 pass one parameter
through stack. But this function itself contains some passed in parameters in
the stack, and need to pass them again to the tail call function in insn 72. At
this time the content of the stack has been overwritten by insn 69, so insn 72
read out wrong value, and will cause runtime error.

The correct result should put insn 69 after insn 72 to avoid writing to the
memory before using it.

GCC 4.6 also contains this bug.

[Bug target/52129] wrong code to pass parameters to tail call function

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52129

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
      Known to fail|                            |4.6.0
           Severity|major                       |normal

[Bug target/52129] wrong code to pass parameters to tail call function

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52129

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-06 08:02:36 UTC ---
Reduced testcase (-march=armv7-a -mthumb -O2):

extern void abort (void);
struct S { void *p; unsigned int q; };
struct T { char a[64]; char b[64]; } t;

__attribute__((noinline, noclone)) int
foo (void *x, struct S s, void *y, void *z)
{
  if (x != &t.a[2] || s.p != &t.b[5] || s.q != 27 || y != &t.a[17] || z !=
&t.b[17])
    abort ();
  return 29;
}

__attribute__((noinline, noclone)) int
bar (void *x, void *y, void *z, struct S s, int t, struct T *u)
{
  return foo (x, s, &u->a[t], &u->b[t]);
}

int
main ()
{
  struct S s = { &t.b[5], 27 };
  if (bar (&t.a[2], (void *) 0, (void *) 0, s, 17, &t) != 29)
    abort ();
  return 0;
}

[Bug target/52129] wrong code to pass parameters to tail call function

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52129

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-02-06
         AssignedTo|unassigned at gcc dot       |jakub at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-06 09:46:59 UTC ---
Created attachment 26580
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26580
gcc47-pr52129.patch

Untested fix.

Seems on this testcase pretend_args_size is non-zero, and argblock is
initialized for sibcall sequences as:
          argblock = crtl->args.internal_arg_pointer;
          argblock
#ifdef STACK_GROWS_DOWNWARD
            = plus_constant (argblock, crtl->args.pretend_args_size);
#else
            = plus_constant (argblock, -crtl->args.pretend_args_size);
#endif
and what is stored into stored_args_map is relative to that argblock
(arg->locate.slot_offset.constant based), while the testing is done with
offsets relative to crtl->args.internal_arg_pointer.

[Bug target/52129] wrong code to pass parameters to tail call function

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52129

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-06 13:33:09 UTC ---
Author: jakub
Date: Mon Feb  6 13:33:05 2012
New Revision: 183933

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183933
Log:
    PR target/52129
    * calls.c (mem_overlaps_already_clobbered_arg_p): If val is
    CONST_INT_P, subtract resp. add crtl->args.pretend_args_size to it.

    * gcc.c-torture/execute/pr52129.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr52129.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/calls.c
    trunk/gcc/testsuite/ChangeLog

[Bug target/52129] wrong code to pass parameters to tail call function

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52129

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-06 13:35:18 UTC ---
Fixed on the trunk so far.

[Bug target/52129] wrong code to pass parameters to tail call function

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52129

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-09 17:27:34 UTC ---
Author: jakub
Date: Thu Feb  9 17:27:25 2012
New Revision: 184059

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=184059
Log:
    Backported from mainline
    2012-02-06  Jakub Jelinek  <jakub@redhat.com>

    PR target/52129
    * calls.c (mem_overlaps_already_clobbered_arg_p): If val is
    CONST_INT_P, subtract resp. add crtl->args.pretend_args_size to it.

    * gcc.c-torture/execute/pr52129.c: New test.

Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.c-torture/execute/pr52129.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/calls.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog

[Bug target/52129] wrong code to pass parameters to tail call function

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52129

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-09 17:48:15 UTC ---
Hopefully fixed now.

[Bug target/52129] wrong code to pass parameters to tail call function

[mikpe at it dot uu.se]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52129

Mikael Pettersson <mikpe at it dot uu.se> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikpe at it dot uu.se

--- Comment #7 from Mikael Pettersson <mikpe at it dot uu.se> 2012-02-18 13:24:21 UTC ---
This test case also fails with gcc-4.5 and 4.4 and -O2 -march=armv5te -marm.