diff mbox

[PR/backtrace,16721] Fix erroneous backtrace on AVR

Message ID 53288819.7080903@embecosm.com
State Committed
Headers

Commit Message

Pierre Langlois March 18, 2014, 5:53 p.m. UTC 
  Hi all,

Looking at stack unwinding on AVR, I noticed the current frame was not always correctly
detected from the function prologue.

This bug occurs only with the following prologue, referred to as "Method 2: Adjust stack pointer"
in GCC: gcc/config/avr/avr.c (avr_prologue_setup_frame).

--> push the old frame pointer
   push r28
   push r29

--> allocate new space
   rcall .+0
   push r1

--> move fp <- sp
   in r28, 0x3d
   in r29, 0x3e

GCC uses "rcall .+0" and "push r1" to adjust the stack pointer, "rcall" pushing
automatically 2 or 3 bytes on the stack, depending on the target.

GDB should scan this prologue and find out the size of the frame but it is incorrect by one
because it expects "push r0" and not "push r1".

I believe this register was changed in GCC withcommit 915f904be.

Best,

Pierre

2014-03-18  Pierre Langlois  <pierre.langlois@embecosm.com>

       * avr-tdep.c (avr_scan_prologue): Accept push r1 instruction for small
         stack allocation.

-----------------------------------------------------------------------------------------

GNU gdb (AVR 8-bit toolchain (built 20140310)) 7.7.50.20140318-cvs
Copyright (C) 2014 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "--host=x86_64-unknown-linux-gnu --target=avr".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.atmel.com>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word".
(gdb) file atmega-test.elf
Reading symbols from atmega-test.elf...done.
(gdb) target remote :51000
Remote debugging using :51000
0x00000116 in multiply (a=25, b=8) at main.c:4
4		return a * b;
(gdb) monitor reset
(gdb) load
Loading section .text, size 0x1a4 lma 0x0
Start address 0x0, load size 420
Transfer rate: 3360 bits in <1 sec, 210 bytes/write.
(gdb) b multiply
Breakpoint 1 at 0x114: file main.c, line 4.
(gdb) c
Continuing.

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00000116 in multiply (a=25, b=8) at main.c:4
4		return a * b;
(gdb) bt
#0  0x00000116 in multiply (a=25, b=8) at main.c:4
#1  0x01e00000 in ?? ()
(gdb) q
A debugging session is active.

	Inferior 1 [Remote target] will be killed.

Quit anyway? (y or n)

Comments

Tristan Gingold March 19, 2014, 8 a.m. UTC | #1 
On 18 Mar 2014, at 18:53, Pierre Langlois <pierre.langlois@embecosm.com> wrote:

> Hi all,
> 
> Looking at stack unwinding on AVR, I noticed the current frame was not always correctly
> detected from the function prologue.
> 
> This bug occurs only with the following prologue, referred to as "Method 2: Adjust stack pointer"
> in GCC: gcc/config/avr/avr.c (avr_prologue_setup_frame).
> 
> --> push the old frame pointer
>  push r28
>  push r29
> 
> --> allocate new space
>  rcall .+0
>  push r1
> 
> --> move fp <- sp
>  in r28, 0x3d
>  in r29, 0x3e
> 
> GCC uses "rcall .+0" and "push r1" to adjust the stack pointer, "rcall" pushing
> automatically 2 or 3 bytes on the stack, depending on the target.
> 
> GDB should scan this prologue and find out the size of the frame but it is incorrect by one
> because it expects "push r0" and not "push r1".
> 
> I believe this register was changed in GCC withcommit 915f904be.

Looks good to me.

Tristan (as former AVR maintainer)

> 
> Best,
> 
> Pierre
> 
> 2014-03-18  Pierre Langlois  <pierre.langlois@embecosm.com>
> 
>      * avr-tdep.c (avr_scan_prologue): Accept push r1 instruction for small
>        stack allocation.
> 
> -----------------------------------------------------------------------------------------
> 
> GNU gdb (AVR 8-bit toolchain (built 20140310)) 7.7.50.20140318-cvs
> Copyright (C) 2014 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "--host=x86_64-unknown-linux-gnu --target=avr".
> Type "show configuration" for configuration details.
> For bug reporting instructions, please see:
> <http://www.atmel.com>.
> Find the GDB manual and other documentation resources online at:
> <http://www.gnu.org/software/gdb/documentation/>.
> For help, type "help".
> Type "apropos word" to search for commands related to "word".
> (gdb) file atmega-test.elf
> Reading symbols from atmega-test.elf...done.
> (gdb) target remote :51000
> Remote debugging using :51000
> 0x00000116 in multiply (a=25, b=8) at main.c:4
> 4		return a * b;
> (gdb) monitor reset
> (gdb) load
> Loading section .text, size 0x1a4 lma 0x0
> Start address 0x0, load size 420
> Transfer rate: 3360 bits in <1 sec, 210 bytes/write.
> (gdb) b multiply
> Breakpoint 1 at 0x114: file main.c, line 4.
> (gdb) c
> Continuing.
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> 0x00000116 in multiply (a=25, b=8) at main.c:4
> 4		return a * b;
> (gdb) bt
> #0  0x00000116 in multiply (a=25, b=8) at main.c:4
> #1  0x01e00000 in ?? ()
> (gdb) q
> A debugging session is active.
> 
> 	Inferior 1 [Remote target] will be killed.
> 
> Quit anyway? (y or n)
> 
> <pr-16721.patch>
Joel Brobecker March 19, 2014, 12:34 p.m. UTC | #2 
> 2014-03-18  Pierre Langlois  <pierre.langlois@embecosm.com>
> 
>       * avr-tdep.c (avr_scan_prologue): Accept push r1 instruction for small
>         stack allocation.

Now that Tristan approved the patch, do you have an FSF copyright
assignment on file? This patch is small that we can push it in without;
but if you are planning on sending more, you'll need one. Just let me
know and I will get you a message to get you started (it takes a while
to complete, so the sooner the better).

One remark on the ChangeLog; the formatting is off a bit. It should be
indented by one tab, and the indentation should be the same, regardless
of whether the line starts with a '*' or not - no extra indentation
for continuation lines. Thus:

	* avr-tdep.c (avr_scan_prologue): Accept push r1 instruction for small
	stack allocation.

(in emails, it's not important to be using 8 spaces or 1 tab, but in
the CL, we tend to be picky about it).

Thanks!
Pierre Langlois March 21, 2014, 1:23 p.m. UTC | #3 
Hi Joel,

Sorry for the late reply.

> Now that Tristan approved the patch, do you have an FSF copyright
> assignment on file? This patch is small that we can push it in without;
> but if you are planning on sending more, you'll need one. Just let me
> know and I will get you a message to get you started (it takes a while
> to complete, so the sooner the better).

I am doing this work on behalf of Embecosm, we have an FSF copyright assignment
on file.

> One remark on the ChangeLog; the formatting is off a bit. It should be
> indented by one tab, and the indentation should be the same, regardless
> of whether the line starts with a '*' or not - no extra indentation
> for continuation lines. Thus:
>
> 	* avr-tdep.c (avr_scan_prologue): Accept push r1 instruction for small
> 	stack allocation.
>
> (in emails, it's not important to be using 8 spaces or 1 tab, but in
> the CL, we tend to be picky about it).

I'll make sure my email client is properly configured next time.

Thank you very much!

Pierre
Joel Brobecker March 21, 2014, 5:19 p.m. UTC | #4 
> Sorry for the late reply.

No worries.

> I am doing this work on behalf of Embecosm, we have an FSF copyright
> assignment on file.

Perfect!

> >	* avr-tdep.c (avr_scan_prologue): Accept push r1 instruction for small
> >	stack allocation.

I can push this patch for you, but you now also meet the requirements
for write access to the GDB repository (FSF assignment + 1 good patch).
If you are planning on contributing more patches, I suggest we get you
write access and let you commit yourself. WDYT? If not interested, just
let me know and I will push the patch.  Otherwise, can you contact me
in private for further instructions on how to achieve that?

Thanks,
diff mbox

Patch

diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
index 6e58f04..7fb16d1 100644
--- a/gdb/avr-tdep.c
+++ b/gdb/avr-tdep.c
@@ -720,7 +720,7 @@  avr_scan_prologue (struct gdbarch *gdbarch, CORE_ADDR pc_beg, CORE_ADDR pc_end,
           info->size += gdbarch_tdep (gdbarch)->call_length;
           vpc += 2;
         }
-      else if (insn == 0x920f)  /* push r0 */
+      else if (insn == 0x920f || insn == 0x921f)  /* push r0 or push r1 */
         {
           info->size += 1;
           vpc += 2;