[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[j at uriah dot heep dot sax dot de]

------- Comment #1 from j at uriah dot heep dot sax dot de  2006-05-02 12:54 -------
Created an attachment (id=11358)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=11358&action=view)
Testcase demonstrating the faulty code generated.


-- 


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

[Bug target/27386] New: AVR: wrong code generated when passing three uint64_t arguments to function

[j at uriah dot heep dot sax dot de]

When passing three uint64_t arguments to a function, AVR-GCC
generates completely wrong code.

The first two uint64_t arguments are passed in registers, and
thus not affected.

The caller then allocates 8 bytes on the stack for the fourth
argument, but writes to 8 bytes beyond the allocated area
(destroying the current function's stack).

The callee tries to fetch the values from the locations on the
stack where they ought to be (but actually aren't).

This bug appears in all tested versions of GCC (3.4.6, 4.1.0,
SVN trunk).


-- 
           Summary: AVR: wrong code generated when passing three uint64_t
                    arguments to function
           Product: gcc
           Version: 4.2.0
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: j at uriah dot heep dot sax dot de
 GCC build triplet: *-*-*
  GCC host triplet: *-*-*
GCC target triplet: avr-*-*


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[j at uriah dot heep dot sax dot de]

------- Comment #2 from j at uriah dot heep dot sax dot de  2006-05-02 12:57 -------
Created an attachment (id=11359)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=11359&action=view)
Faulty code generated by test case.

The faulty code is in lines 136...160.

First, 8 bytes of space are allocated on the stack,
but the access uses Z+8 through Z+15 instead of
Z+1 through Z+8.


-- 


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[berndtrog at yahoo dot com]

------- Comment #3 from berndtrog at yahoo dot com  2006-05-02 15:19 -------
Is this one related to PR21834?


-- 


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[j at uriah dot heep dot sax dot de]

------- Comment #4 from j at uriah dot heep dot sax dot de  2006-05-02 15:25 -------
(In reply to comment #3)
> Is this one related to PR21834?

Possible, yes.  The symptoms are similar.  Sorry for missing that
one at my prior search.

Anyway, my report has a preprocessed source file attached, so it
might be more useful to non-AVR aware GCC developers.


-- 


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[pinskia at gcc dot gnu dot org]

-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|critical                    |normal


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[j at uriah dot heep dot sax dot de]

------- Comment #5 from j at uriah dot heep dot sax dot de  2006-05-02 15:33 -------
(In reply to comment #4)

> > Is this one related to PR21834?

> Anyway, my report has a preprocessed source file attached, so it
> might be more useful to non-AVR aware GCC developers.

Perhaps Richard (Cc'ed) can add an xref to this bug in 21834 (and
correct the target triplet to "avr-*-*"), then I'll resolve this
bug as a dup.


-- 

j at uriah dot heep dot sax dot de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |critical


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[j at uriah dot heep dot sax dot de]

------- Comment #6 from j at uriah dot heep dot sax dot de  2006-05-02 15:34 -------
(Sorry, I hit <enter> a bit too fast.)


-- 

j at uriah dot heep dot sax dot de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|critical                    |normal


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[j at uriah dot heep dot sax dot de]

------- Comment #7 from j at uriah dot heep dot sax dot de  2006-05-02 15:37 -------
Forwarding this comment on behalf of Bjoern Haase:

Preliminary analysis of the RTL generated without optimization shows that
the problem is present already directly after expand. Maybe the problem
is triggered by the fact that the avr port does not have move patterns
for DI, however, there seems to be a general problem in the mid-end.:

Excerpt of test.c.01.sibling. Personal comments marked with. #

;; Function main

(note 2 0 5 NOTE_INSN_DELETED)

(note 5 2 3 0 [bb 0] NOTE_INSN_BASIC_BLOCK)

(note 3 5 6 0 NOTE_INSN_FUNCTION_BEG)

(note 6 3 8 1 [bb 1] NOTE_INSN_BASIC_BLOCK)

(insn 8 6 9 1 (set (reg:HI 44)
        (const_int 42 [0x2a])) -1 (nil)
    (nil))

#push int parameter on stack (last parameter of function call)
(insn 9 8 10 1 (set (mem/i:HI (post_dec:HI (reg/f:HI 32 __SPL__)) [0 S2 A8])
        (reg:HI 44)) -1 (nil)
    (nil))

#allocate 8 Bytes on the stack for the uint64_t var, i.e. stack -= 8
(insn 10 9 11 1 (set (reg/f:HI 32 __SPL__)
        (plus:HI (reg/f:HI 32 __SPL__)
            (const_int -8 [0xfffffff8]))) -1 (nil)
    (nil))

#load first data byte in reg
(insn 11 10 12 1 (set (reg:QI 45)
        (const_int -16 [0xfffffff0])) -1 (nil)
    (nil))

#write first data byte to stack + 8. instead of stack + 0 !!!! BUG !!!!
(insn 12 11 13 1 (set (mem/i:QI (plus:HI (reg/f:HI 32 __SPL__)
                (const_int 8 [0x8])) [0 S1 A8])
        (reg:QI 45)) -1 (nil)
    (nil))

#load second data byte in reg
(insn 13 12 14 1 (set (reg:QI 46)
        (const_int -34 [0xffffffde])) -1 (nil)
    (nil))

#write second data byte to stack + 9. instead of stack + 1 : !!!! BUG !!!!
(insn 14 13 15 1 (set (mem/i:QI (plus:HI (reg/f:HI 32 __SPL__)
                (const_int 9 [0x9])) [0 S1 A8])
        (reg:QI 46)) -1 (nil)
    (nil))


-- 


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[eweddington at cso dot atmel dot com]

------- Comment #8 from eweddington at cso dot atmel dot com  2006-09-01 21:44 -------
Bugmasters: Please mark this bug as NEW. It is a valid bug.


-- 


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[eweddington at cso dot atmel dot com]

------- Comment #9 from eweddington at cso dot atmel dot com  2007-02-13 19:12 -------
5 months later and this bug still needs to be marked as NEW. Will a bug master
please do this?


-- 


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[eweddington at cso dot atmel dot com]

-- 

eweddington at cso dot atmel dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2007-04-09 21:47:11
               date|                            |


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[eweddington at cso dot atmel dot com]

------- Comment #10 from eweddington at cso dot atmel dot com  2007-05-30 20:09 -------
Test case fails for 4.3-20070525.


-- 

eweddington at cso dot atmel dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|2007-04-09 21:47:11         |2007-05-30 20:09:37
               date|                            |


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[eweddington at cso dot atmel dot com]

------- Comment #11 from eweddington at cso dot atmel dot com  2007-05-30 20:29 -------
*** Bug 21834 has been marked as a duplicate of this bug. ***


-- 

eweddington at cso dot atmel dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  BugsThisDependsOn|21834                       |


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[rask at sygehus dot dk]

------- Comment #12 from rask at sygehus dot dk  2007-05-30 21:29 -------
The AVR is unusual in having

#define STACK_PUSH_CODE POST_DEC
#define STACK_GROWS_DOWNWARD

where most targets would have PRE_DEC when the stack grows downward. The
middle-end tries to synthesize the missing pushdi pattern, but while the
offsets seem to be wrong in any case, the instruction order would be correct
for PRE_DEC. In expr.c/emit_single_push_insn(), there is this comment:

#ifdef STACK_GROWS_DOWNWARD
      /* ??? This seems wrong if STACK_PUSH_CODE == POST_DEC.  */
      dest_addr = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
                                GEN_INT (-(HOST_WIDE_INT) rounded_size));
#else

Someone needs to set a breakpoint on emit_push_insn() and friends to find out
exactly where the problem is.


-- 


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[hutchinsonandy at aim dot com]

------- Comment #13 from hutchinsonandy at aim dot com  2008-06-01 02:40 -------
expr.c appears all messed up on emit_single_push_insn.

This bad code gets executed when there is no push instruction available.

As well as getting address of the mem created completely wrong, it does not
account for any offset between SP and Top/Bottom of Stack aka
STACK_POINTER_OFFSET

Any comment before I try and fix this mess?


First example, ironically without the warning mentioned in latter code.

 else if (FUNCTION_ARG_PADDING (mode, type) == downward)
    {
      unsigned padding_size = rounded_size - GET_MODE_SIZE (mode);
      HOST_WIDE_INT offset;

      emit_move_insn (stack_pointer_rtx,
                      expand_binop (Pmode,
#ifdef STACK_GROWS_DOWNWARD
                                    sub_optab,
#else
                                    add_optab,
#endif
                                    stack_pointer_rtx,
                                    GEN_INT (rounded_size),
                                    NULL_RTX, 0, OPTAB_LIB_WIDEN));

      offset = (HOST_WIDE_INT) padding_size;
#ifdef STACK_GROWS_DOWNWARD
      if (STACK_PUSH_CODE == POST_DEC)
        /* We have already decremented the stack pointer, so get the
           previous value.  */
///NEXT LINE IS WRONG We are pointing just below value so we need SP +
STACK_POINTER_OFFSET
        offset += (HOST_WIDE_INT) rounded_size;
//For PRE_DEC we already point directly to mem so code OK
#else
      if (STACK_PUSH_CODE == POST_INC)
        /* We have already incremented the stack pointer, so get the
           previous value.  */
//NEXT LINE IS CORRECT
        offset -= (HOST_WIDE_INT) rounded_size;
//For PRE_INC we now add STACK_POINTER_OFFSET or  SP will be one lower than mem
address
#endif
      dest_addr = gen_rtx_PLUS (Pmode, stack_pointer_rtx, GEN_INT (offset));
    }
  else

The rest of code is even worse!










-- 

hutchinsonandy at aim dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hutchinsonandy at aim dot
                   |                            |com


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[hutchinsonandy at aim dot com]

------- Comment #14 from hutchinsonandy at aim dot com  2008-06-01 15:22 -------
It appears emit_single_push_insn() is BROKEN for targets with:

a)STACK_GROWS_DOWNWARDS+POST_DEC push 
b)Upwards+POST_INC push.

So if any target has this combo and #define PUSH_ROUNDING - it is broken.

Fortunately for AVR the whole mess goes away when we #undef PUSH_ROUNDING-
which appears so far to be uneccesary. It also cleared some compat test
failures!


For reference here what I did sort out for emit_single_push_insn

There are two problems.

1) For downwards padding, MISTAKE in original (H8) change that for (a) adds
size of slot to SP to get address - that would be highest byte of mem! This
crashes AVR.

        /* We have already decremented the stack pointer, so get the
           previous value.  */
        offset += (HOST_WIDE_INT) rounded_size; I AM VERY WRONG

POST_DEC leaves stack pointer at BOS-1.  We must add smallest addressable unit
of stack (byte,word?) to get address of MEM. Or perhaps use
STACK_POINTER_OFFSET. So above becomes.

offset += (HOST_WIDE_INT) STACK_POINTER_OFFSET; 

2) For Upwards padding both POST_INC and POST_DEC have PRE_MODIFY sequences
created for upwards padding. This is questioned in code and clearly incorrect.
I assume no target has this combo.

One possible way to fix issue is to use POST_MODIFY. However, that would assume
final instructions would not be split and cause stack corruption during
interrupt. This matter I have not checked.

Someone might consider adding some asserts here!


-- 


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[hutchinsonandy at gcc dot gnu dot org]

------- Comment #15 from hutchinsonandy at gcc dot gnu dot org  2008-06-04 22:03 -------
Subject: Bug 27386

Author: hutchinsonandy
Date: Wed Jun  4 22:02:57 2008
New Revision: 136377

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=136377
Log:
PR target/27386
* config/avr/avr.h: (PUSH_ROUNDING): Remove.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.h


-- 


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[hutchinsonandy at gcc dot gnu dot org]

------- Comment #16 from hutchinsonandy at gcc dot gnu dot org  2008-06-04 22:06 -------
Fixed 4.4


-- 

hutchinsonandy at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.3.2


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

[Bug target/27386] AVR: wrong code generated when passing three uint64_t arguments to function

[hutchinsonandy at gcc dot gnu dot org]

------- Comment #17 from hutchinsonandy at gcc dot gnu dot org  2008-06-07 15:49 -------
Subject: Bug 27386

Author: hutchinsonandy
Date: Sat Jun  7 15:48:25 2008
New Revision: 136531

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=136531
Log:
Backports from 4.4
PR target/27386
* config/avr/avr.h: (PUSH_ROUNDING): Remove.

PR target/30243
* builtins.c (expand_builtin_signbit): Don't take lowpart when
register is already smaller or equal to required mode.  

PR target/34932
* config/avr/avr.md (*addhi3_zero_extend2): Remove.

* config/avr/avr.h (MAX_OFILE_ALIGNMENT): Define.

Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/builtins.c
    branches/gcc-4_3-branch/gcc/config/avr/avr.h
    branches/gcc-4_3-branch/gcc/config/avr/avr.md


-- 


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