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

["johnstonj at inn-soft dot com" <gcc-bugzilla at gcc dot gnu dot org>]

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.