[patch] Enable dwarf unwind for AVR target

[patch] Enable dwarf unwind for AVR target

[Sivanupandi, Pitchumani]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

Hi All,

When analyzing regression failures for AVR target I found one unwinder issue
with.
Example: gdb.base/break.exp: step over breakpoint

AVR target in gdb has avr-frame unwinder and it doesn't enable dwarf unwinder.
Without dwarf unwinder, gdb could not unwind the stack and PC reliably.

Example:
1 int volatile a;
2 void main ()
3 {
4   a = 10;
5   printf ("%d\n", a);
6   a = 12;
7 }

avr-gcc test.c -mmcu=atmega1280 -g

When next command on line 5, gdb should move pc to line 6. But it stops at
printf function. GDB could not unwind frame from which the call printf
initiated and stops at printf as the source line changed.

CFA information is part of dwarf debug information that can be used to unwind
the stack and PC. But avr target do not enable the dwarf unwinder.

Attached patch enables the dwarf unwinder for avr target. Reported test case
and many 'step over' tests (such as break.exp) are passed now. There are few
regressions that are not analyzed. Before that I want to get the community
opinion about this fix.

Could you please review the patch?

Regards,
Pitchumani

[-- Attachment #2: avr-enable-dwarf-unwind.patch --]
[-- Type: application/octet-stream, Size: 5771 bytes --]

diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
index ccc8b45..73272c8 100644
--- a/gdb/avr-tdep.c
+++ b/gdb/avr-tdep.c
@@ -29,6 +29,7 @@
 #include "trad-frame.h"
 #include "gdbcmd.h"
 #include "gdbcore.h"
+#include "dwarf2-frame.h"
 #include "gdbtypes.h"
 #include "inferior.h"
 #include "symfile.h"
@@ -97,7 +98,8 @@ enum
 
   /* Pseudo registers.  */
   AVR_PSEUDO_PC_REGNUM = 35,
-  AVR_NUM_PSEUDO_REGS = 1,
+  AVR_DWARF2_PC_REGNUM = 36 /*LR*/,
+  AVR_NUM_PSEUDO_REGS = 2,
 
   AVR_PC_REG_INDEX = 35,	/* index into array of registers */
 
@@ -211,7 +213,7 @@ avr_register_name (struct gdbarch *gdbarch, int regnum)
     "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
     "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
     "SREG", "SP", "PC2",
-    "pc"
+    "pc", "LR"
   };
   if (regnum < 0)
     return NULL;
@@ -230,6 +232,8 @@ avr_register_type (struct gdbarch *gdbarch, int reg_nr)
     return builtin_type (gdbarch)->builtin_uint32;
   if (reg_nr == AVR_PSEUDO_PC_REGNUM)
     return gdbarch_tdep (gdbarch)->pc_type;
+  if (reg_nr == AVR_DWARF2_PC_REGNUM)
+    return gdbarch_tdep (gdbarch)->pc_type;
   if (reg_nr == AVR_SP_REGNUM)
     return builtin_type (gdbarch)->builtin_data_ptr;
   return builtin_type (gdbarch)->builtin_uint8;
@@ -395,6 +399,8 @@ avr_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
       val >>= 1;
       store_unsigned_integer (buf, 4, gdbarch_byte_order (gdbarch), val);
       return status;
+    case AVR_DWARF2_PC_REGNUM:
+      return REG_UNAVAILABLE;
     default:
       internal_error (__FILE__, __LINE__, _("invalid regnum"));
     }
@@ -413,6 +419,9 @@ avr_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
       val <<= 1;
       regcache_raw_write_unsigned (regcache, AVR_PC_REGNUM, val);
       break;
+    case AVR_DWARF2_PC_REGNUM:
+      /* Do nothing.  */
+      break;
     default:
       internal_error (__FILE__, __LINE__, _("invalid regnum"));
     }
@@ -1355,6 +1364,34 @@ avr_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   return sp + call_length;
 }
 
+static struct value *
+avr_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
+			  int regnum)
+{
+  struct gdbarch * gdbarch = get_frame_arch (this_frame);
+  struct gdbarch_tdep * tdep = gdbarch_tdep (gdbarch);
+  CORE_ADDR addr;
+  ULONGEST pc;
+  int i;
+  gdb_byte buf[3];
+
+  switch (regnum)
+    {
+    case AVR_PC_REGNUM:
+      addr = dwarf2_frame_addr (this_cache, AVR_DWARF2_PC_REGNUM);
+      read_memory (addr, buf, tdep->call_length);
+      pc = 0;
+      for (i = 0; i < tdep->call_length; i++)
+        pc = (pc << 8) | buf[i];
+      /* Change it to byte address. */
+      pc <<= 1;
+      return frame_unwind_got_constant (this_frame, regnum, pc);
+    default:
+      internal_error (__FILE__, __LINE__,
+		      _("Unexpected register %d"), regnum);
+    }
+}
+
 /* Unfortunately dwarf2 register for SP is 32.  */
 
 static int
@@ -1362,11 +1399,34 @@ avr_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
 {
   if (reg >= 0 && reg < 32)
     return reg;
-  if (reg == 32)
-    return AVR_SP_REGNUM;
+  switch (reg)
+  {
+    case 32:
+      return AVR_SP_REGNUM;
+    case 34: // AVR_PC_REGNUM
+    case 36: // AVR_DWARF2_PC_REGNUM
+      return reg;
+  }
   return -1;
 }
 
+static void
+avr_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
+                           struct dwarf2_frame_state_reg *reg,
+                           struct frame_info *this_frame)
+{
+  switch (regnum)
+  {
+    case AVR_PC_REGNUM:
+      reg->how = DWARF2_FRAME_REG_FN;
+      reg->loc.fn = avr_dwarf2_prev_register;
+      break;
+    case AVR_SP_REGNUM:
+      reg->how = DWARF2_FRAME_REG_CFA;
+      break;
+  }
+}
+
 /* Implementation of `address_class_type_flags' gdbarch method.
 
    This method maps DW_AT_address_class attributes to a
@@ -1478,6 +1538,8 @@ avr_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_ptr_bit (gdbarch, 2 * TARGET_CHAR_BIT);
   set_gdbarch_addr_bit (gdbarch, 32);
 
+  set_gdbarch_dwarf2_addr_size (gdbarch, 4);
+
   set_gdbarch_float_bit (gdbarch, 4 * TARGET_CHAR_BIT);
   set_gdbarch_double_bit (gdbarch, 4 * TARGET_CHAR_BIT);
   set_gdbarch_long_double_bit (gdbarch, 4 * TARGET_CHAR_BIT);
@@ -1517,6 +1579,9 @@ avr_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   set_gdbarch_breakpoint_from_pc (gdbarch, avr_breakpoint_from_pc);
 
+  dwarf2_frame_set_init_reg (gdbarch, avr_dwarf2_frame_init_reg);
+  dwarf2_append_unwinders (gdbarch);
+
   frame_unwind_append_unwinder (gdbarch, &avr_frame_unwind);
   frame_base_set_default (gdbarch, &avr_frame_base);
 
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index a640c26..54bab17 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1524,6 +1524,21 @@ dwarf2_frame_cfa (struct frame_info *this_frame)
 
   return get_frame_base (this_frame);
 }
+
+/* Compute the frame address.  */
+CORE_ADDR
+dwarf2_frame_addr (void ** this_cache, int regnum)
+{
+  struct dwarf2_frame_cache *cache = *this_cache;
+  CORE_ADDR addr;
+
+  gdb_assert (cache != NULL);
+  gdb_assert (cache->reg[regnum].how == DWARF2_FRAME_REG_SAVED_OFFSET);
+
+  addr = cache->cfa + cache->reg[regnum].loc.offset;
+
+  return addr;
+}
 \f
 const struct objfile_data *dwarf2_frame_objfile_data;
 
diff --git a/gdb/dwarf2-frame.h b/gdb/dwarf2-frame.h
index b739744..cde88e5 100644
--- a/gdb/dwarf2-frame.h
+++ b/gdb/dwarf2-frame.h
@@ -119,6 +119,7 @@ extern const struct frame_base *
 /* Compute the DWARF CFA for a frame.  */
 
 CORE_ADDR dwarf2_frame_cfa (struct frame_info *this_frame);
+CORE_ADDR dwarf2_frame_addr (void ** this_cache, int regnum);
 
 /* Find the CFA information for PC.
 

Re: [patch] Enable dwarf unwind for AVR target

[Pedro Alves]

On 01/04/2016 08:01 AM, Sivanupandi, Pitchumani wrote:
> -  AVR_NUM_PSEUDO_REGS = 1,
> +  AVR_DWARF2_PC_REGNUM = 36 /*LR*/,

It's hard to comment without some explanation for the design choices
in the patch, and/or some understanding of what this register is.
Seems odd that you create the register and then avr_pseudo_register_read
already returns <unavailable>?  Why did you need to add it?  Is this a
real machine register?  What's LR?

> +  AVR_NUM_PSEUDO_REGS = 2,
>  
>    AVR_PC_REG_INDEX = 35,	/* index into array of registers */
>  
> @@ -211,7 +213,7 @@ avr_register_name (struct gdbarch *gdbarch, int regnum)
>      "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
>      "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
>      "SREG", "SP", "PC2",
> -    "pc"
> +    "pc", "LR"
>    };

Thanks,
Pedro Alves

RE: [patch] Enable dwarf unwind for AVR target

[Sivanupandi, Pitchumani]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: 04 January 2016 19:28
> To: Sivanupandi, Pitchumani <Pitchumani.Sivanupandi@atmel.com>;
> troth@openavr.org; brobecker@adacore.com; gdb-patches@sourceware.org
> Subject: Re: [patch] Enable dwarf unwind for AVR target
> 
> On 01/04/2016 08:01 AM, Sivanupandi, Pitchumani wrote:
> > -  AVR_NUM_PSEUDO_REGS = 1,
> > +  AVR_DWARF2_PC_REGNUM = 36 /*LR*/,
> 
> It's hard to comment without some explanation for the design choices in the
> patch, and/or some understanding of what this register is.
> Seems odd that you create the register and then avr_pseudo_register_read
> already returns <unavailable>?  Why did you need to add it?  Is this a real
> machine register?  What's LR?
> 
> > +  AVR_NUM_PSEUDO_REGS = 2,
> >
> >    AVR_PC_REG_INDEX = 35,	/* index into array of registers */
> >
> > @@ -211,7 +213,7 @@ avr_register_name (struct gdbarch *gdbarch, int
> regnum)
> >      "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
> >      "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
> >      "SREG", "SP", "PC2",
> > -    "pc"
> > +    "pc", "LR"
> >    };

AVR has only 32 registers. GDB maps status register, SP and PC to numbers 32,
33 and 34 respectively.

Dwarf debug info generated by avr-gcc denotes the return address by register
36 which is not an actual register.
e.g. .debug_frame
(--snip--)
00000000 00000010 ffffffff CIE
  Version:               1
  Augmentation:          ""
  Code alignment factor: 2
  Data alignment factor: -1
  Return address column: 36

  DW_CFA_def_cfa: r32 ofs 3
  DW_CFA_offset: r36 at cfa-2
(--snip--)

The fix is to add a pseudo register (36 - AVR_DWARF2_PC_REGNUM) to gdb to map
return address register in .debug_frame. Register name is "LR" (link register).
When dwarf frame unwind asks for PC, target function should find return address
value (AVR_DWARF2_PC_REGNUM).

Dwarf frame reader records all registers from the .debug_frame section.
When gdb asks for PC value to unwind, avr_dwarf2_prev_register finds the frame
address recorded for AVR_DWARF2_PC_REGNUM and read value from that memory.
(avr_dwarf2_prev_register implementation is similar to existing 
avr_frame_prev_register function)

I assumed pseudo register read/write for this register not mandatory for dwarf
frame unwind. So, left it with dummy implementation as of now.

NOTE:
* Added extern function dwarf2_frame_addr to dwarf2-frame.c to find the frame
address for argument register from dwarf frame cache.
* dwarf2 address size set to 4.

Regards,
Pitchumani