User account creation filtered due to spam.

Bug 27386 - AVR: wrong code generated when passing three uint64_t arguments to function
Summary: AVR: wrong code generated when passing three uint64_t arguments to function
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.2.0
: P3 normal
Target Milestone: 4.3.2
Assignee: Not yet assigned to anyone
URL:
Keywords: FIXME, wrong-code
: 21834 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-05-02 12:54 UTC by Joerg Wunsch
Modified: 2008-06-04 22:06 UTC (History)
5 users (show)

See Also:
Host: *-*-*
Target: avr-*-*
Build: *-*-*
Known to work:
Known to fail: 3.4.6 4.1.0 4.2.0
Last reconfirmed: 2007-05-30 20:09:37


Attachments
Testcase demonstrating the faulty code generated. (157 bytes, text/plain)
2006-05-02 12:54 UTC, Joerg Wunsch
Details
Faulty code generated by test case. (960 bytes, text/plain)
2006-05-02 12:57 UTC, Joerg Wunsch
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joerg Wunsch 2006-05-02 12:54:07 UTC
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).
Comment 1 Joerg Wunsch 2006-05-02 12:54:54 UTC
Created attachment 11358 [details]
Testcase demonstrating the faulty code generated.
Comment 2 Joerg Wunsch 2006-05-02 12:57:52 UTC
Created attachment 11359 [details]
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.
Comment 3 berndtrog 2006-05-02 15:19:17 UTC
Is this one related to PR21834?
Comment 4 Joerg Wunsch 2006-05-02 15:25:17 UTC
(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.
Comment 5 Joerg Wunsch 2006-05-02 15:33:47 UTC
(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.
Comment 6 Joerg Wunsch 2006-05-02 15:34:36 UTC
(Sorry, I hit <enter> a bit too fast.)
Comment 7 Joerg Wunsch 2006-05-02 15:37:40 UTC
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))
Comment 8 Eric Weddington 2006-09-01 21:44:36 UTC
Bugmasters: Please mark this bug as NEW. It is a valid bug.
Comment 9 Eric Weddington 2007-02-13 19:12:07 UTC
5 months later and this bug still needs to be marked as NEW. Will a bug master please do this?
Comment 10 Eric Weddington 2007-05-30 20:09:37 UTC
Test case fails for 4.3-20070525.
Comment 11 Eric Weddington 2007-05-30 20:29:01 UTC
*** Bug 21834 has been marked as a duplicate of this bug. ***
Comment 12 Rask Ingemann Lambertsen 2007-05-30 21:29:46 UTC
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.
Comment 13 Andy Hutchinson 2008-06-01 02:40:35 UTC
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!







    
Comment 14 Andy Hutchinson 2008-06-01 15:22:08 UTC
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!

Comment 15 Andy Hutchinson 2008-06-04 22:03:59 UTC
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

Comment 16 Andy Hutchinson 2008-06-04 22:06:32 UTC
Fixed 4.4
Comment 17 Andy Hutchinson 2008-06-07 15:49:07 UTC
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