[Bug target/60991] New: [avr] Stack corruption when using 24-bit integers __int24 or __memx pointers in large stack frame

[Bug target/60991] New: [avr] Stack corruption when using 24-bit integers __int24 or __memx pointers in large stack frame

[johnstonj@inn-soft.com]

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

            Bug ID: 60991
           Summary: [avr] Stack corruption when using 24-bit integers
                    __int24 or __memx pointers in large stack frame
           Product: gcc
           Version: 4.8.1
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: johnstonj@inn-soft.com

Both avr-gcc 4.8.1 and 4.7.2 corrupt the stack when using 24-bit integers in a
large stack frame (>= approximately 64 bytes) where the integer is stored at
the end of the stack frame such that it is normally out of reach of the "STD
Y+q" instruction.  The below example demonstrates the problem.  While the
example appears pedantic, it becomes a real problem when a program is
aggressively inlined through multiple layers of complicated functions that are
called only once, or when an array is allocated on the stack.

Note this problem exists with both __int24 data type and __memx pointers.

Steps I used to reproduce:

1.  Create new AVR GCC C Executable project in Atmel Studio 6.1 or 6.2 beta. 
6.2 beta uses avr-gcc 4.8.1 and 6.1 uses avr-gcc 4.7.2.
2.  Target is ATxmega256A3U but I should think any target will work (but
untested).  I use default compiler settings which gave command lines of:

"C:\Program Files (x86)\Atmel\Atmel Toolchain\AVR8
GCC\Native\3.4.1051\avr8-gnu-toolchain\bin\avr-gcc.exe"  -x c -funsigned-char
-funsigned-bitfields -DDEBUG  -O1 -ffunction-sections -fdata-sections
-fpack-struct -fshort-enums -mrelax -g2 -Wall -mmcu=atxmega256a3u -c -std=gnu99
-MD -MP -MF "CrashTest.d" -MT"CrashTest.d" -MT"CrashTest.o"   -o "CrashTest.o"
".././CrashTest.c" 

"C:\Program Files (x86)\Atmel\Atmel Toolchain\AVR8
GCC\Native\3.4.1051\avr8-gnu-toolchain\bin\avr-gcc.exe" -o CrashTest.elf 
CrashTest.o   -Wl,-Map="CrashTest.map" -Wl,--start-group -Wl,-lm 
-Wl,--end-group -Wl,--gc-sections -mrelax -mmcu=atxmega256a3u  

3.  Use this code:

#include <avr/wdt.h>

__attribute__((__noinline__)) void Blowup(void) {
    #define SZ 62
    volatile char junk[SZ];
    junk[0] = 5;
    junk[SZ-1] = 6;
    volatile __int24 staticConfig = 0;
} // BUG:  Instruction pointer register will change to a bogus value when
returning.

int main(void)
{
    Blowup();
    // BUG: This loop will never be reached because Blowup() won't return
properly.
    while(1) { wdt_reset(); }
}

4.  The emitted assembly for Blowup function is problematic as follows, from
the LSS file:

    volatile __int24 staticConfig = 0;
 22a:    22 96           adiw    r28, 0x02    ; 2
 22c:    1d ae           std    Y+61, r1    ; 0x3d
 22e:    1e ae           std    Y+62, r1    ; 0x3e
 230:    1f ae           std    Y+63, r1    ; 0x3f
 232:    23 97           sbiw    r28, 0x03    ; 3

AVR-GCC appears to hold the frame pointer in Y register pair: r28..r29.  This
code corrupts that pointer, which is later adjusted again by the frame size and
stored in SPH/SPL, such that the subsequent "ret" instruction jumps to invalid
location.

The STD instruction is limited to a 6-bit immediate offset ("Y+q" syntax).  The
"staticConfig" variable was stored in the frame normally beyond the reach of
this instruction, so AVR-GCC appears to try to use "STD Y+q" anyway by
temporary adding and then subtracting the appropriate offset to the frame
pointer.  Except, that AVR-GCC appears to be incorrectly doing this - as we can
see the added value in "ADIW" of 0x02 is not the same as the subtracted value
in "SBIW" of 0x03.  Examining the earlier assembly code seems to suggest that
the correct value for "SBIW" would be 0x02 also.

Because the "SBIW" instruction screwed up the "Y" register pair which is later
stored in SPH/SPL, the "RET" call jumps to bad memory.  Also, I would imagine
subsequent variable accesses in the "Blowup" function to the stack would not
work properly.

I also suspect that reads from the "staticConfig" variable using "LDD Y+q"
instruction could have this same problem, but I did not test.  "LDD Y+q" has
similar 6-bit limitation, so I suspect AVR-GCC would be using the same "ADIW" /
"SBIW" trick to work around that which failed here.

[Bug target/60991] [avr] Stack corruption when using 24-bit integers __int24 or __memx pointers in large stack frame

[johnstonj@inn-soft.com]

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

--- Comment #1 from johnstonj@inn-soft.com ---
I don't really know GCC source code well enough to feel comfortable changing
and testing this myself, but I suspect the problem lies with this code?

gcc/config/avr/avr.c: avr_out_store_psi function: revision 209767:

3992                          return avr_asm_len ("adiw r28,%o0-61" CR_TAB
3993                                              "std Y+61,%A1"    CR_TAB
3994                                              "std Y+62,%B1"    CR_TAB
3995                                              "std Y+63,%C1"    CR_TAB
3996                                              "sbiw r28,%o0-60", op, plen,
-5);

Notice the top line has "%o0-61" and bottom line has "%o0-60", which must be
some kind of offset.  I suspect this code was copied and pasted from the
out_movsi_mr_r function, while forgetting to update the last line here since it
is 24-bit integer and not the 32-bit integer it was copied from.

It appears that this code was introduced with the new __int24 support and
nothing changed since then.

[Bug target/60991] [avr] Stack corruption when using 24-bit integers __int24 or __memx pointers in large stack frame

[senthil_kumar.selvaraj at atmel dot com]

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

Senthil Kumar Selvaraj <senthil_kumar.selvaraj at atmel dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |senthil_kumar.selvaraj@atme
                   |                            |l.com

--- Comment #2 from Senthil Kumar Selvaraj <senthil_kumar.selvaraj at atmel dot com> ---
Here's a simplified dejagnu testcase.

/* { dg-do run } */
/* { dg-options "-O1" } */

/* This testcase (simplified from the original bug report) exposes 
   PR60991. The code generated for writing the __int24 value corrupts
   the frame pointer if the offset is <= 63 + MAX_LD_OFFSET */

#include <stdlib.h>

int main(void)
{
    volatile char junk[62];
    junk[0] = 5;
    volatile __int24 staticConfig = 0;

    if (junk[0] != 5)
      abort();

    exit(0);
    return 0;
}

[Bug target/60991] [avr] Stack corruption when using 24-bit integers __int24 or __memx pointers in large stack frame

[senthil_kumar.selvaraj at atmel dot com]

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

--- Comment #3 from Senthil Kumar Selvaraj <senthil_kumar.selvaraj at atmel dot com> ---
The OP's suspicion/analysis was right. Here's a "trivial" patch that fixes the
problem.

diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index 2edc78a..e96691e 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -3993,7 +3993,7 @@ avr_out_store_psi (rtx insn, rtx *op, int *plen)
                                 "std Y+61,%A1"    CR_TAB
                                 "std Y+62,%B1"    CR_TAB
                                 "std Y+63,%C1"    CR_TAB
-                                "sbiw r28,%o0-60", op, plen, -5);
+                                "sbiw r28,%o0-61", op, plen, -5);

           return avr_asm_len ("subi r28,lo8(-%o0)" CR_TAB
                               "sbci r29,hi8(-%o0)" CR_TAB

[Bug target/60991] [avr] Stack corruption when using 24-bit integers __int24 or __memx pointers in large stack frame

[denisc at gcc dot gnu.org]

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

--- Comment #4 from denisc at gcc dot gnu.org ---
Author: denisc
Date: Mon May 12 15:33:00 2014
New Revision: 210325

URL: http://gcc.gnu.org/viewcvs?rev=210325&root=gcc&view=rev
Log:
gcc/ChangeLog

    PR target/60991
    * config/avr/avr.c (avr_out_store_psi): Use correct constant
    to restore Y.

gcc/testsuite/ChangeLog

    PR target/60991
    * gcc.target/avr/pr60991.c: New testcase.


Added:
    trunk/gcc/testsuite/gcc.target/avr/pr60991.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.c
    trunk/gcc/testsuite/ChangeLog

[Bug target/60991] [avr] Stack corruption when using 24-bit integers __int24 or __memx pointers in large stack frame

[denisc at gcc dot gnu.org]

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

--- Comment #6 from denisc at gcc dot gnu.org ---
Author: denisc
Date: Mon May 12 16:07:44 2014
New Revision: 210328

URL: http://gcc.gnu.org/viewcvs?rev=210328&root=gcc&view=rev
Log:
    Backport from mainline
    2014-05-12  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

    PR target/60991
    * config/avr/avr.c (avr_out_store_psi): Use correct constant
    to restore Y.


    Backport from mainline
    2014-05-12  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

    PR target/60991
    * gcc.target/avr/pr60991.c: New testcase.


Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.target/avr/pr60991.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/avr/avr.c
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog

[Bug target/60991] [avr] Stack corruption when using 24-bit integers __int24 or __memx pointers in large stack frame

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60991

--- Comment #7 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Author: gjl
Date: Tue May 20 08:37:50 2014
New Revision: 210635

URL: http://gcc.gnu.org/viewcvs?rev=210635&root=gcc&view=rev
Log:
gcc/
2014-05-20  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

    Backport from mainline r210325
    2014-05-12  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

    PR target/60991
    * config/avr/avr.c (avr_out_store_psi): Use correct constant
    to restore Y.

gcc/testsuite/
2014-05-20  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

    Backport from mainline r210325
    2014-05-12  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

    PR target/60991
    * gcc.target/avr/pr60991.c: New testcase.


Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.target/avr/pr60991.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/avr/avr.c
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog

[Bug target/60991] [avr] Stack corruption when using 24-bit integers __int24 or __memx pointers in large stack frame

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60991

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4
             Status|UNCONFIRMED                 |RESOLVED
      Known to work|                            |4.7.4, 4.8.3, 4.9.1
           Keywords|                            |wrong-code
         Resolution|---                         |FIXED
   Target Milestone|---                         |4.9.1
      Known to fail|4.7.2, 4.8.1                |4.7.3, 4.8.2, 4.9.0
           Severity|critical                    |normal

--- Comment #8 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Fixed in 4.7.4, 4.8.3, 4.9.1