[rfc] Unwind the ARM CPSR

[rfc] Unwind the ARM CPSR

[Daniel Jacobowitz]

This patch is based on a suggestion that Mark made around March 2006,
and another related problem that Paul Brook and Vladimir Prus
discovered.  I would love comments on it; it works, but there is
one place that I'm unhappy with (in frame unwinding and the DWARF-2
unwinder).

Many ARM processors support two execution modes (ARM and Thumb) with
different instruction sets.  We can use the symbol table to figure out
which mode a function requires, but there are cases where it's more
useful to consult the processor state.  The state is normally stored
in the CPSR (processor status register).  But the state can change
across function calls / returns, so it is also encoded on the stack
as the low bit of LR.

We treat the stack copy of LR as the resume address.  This isn't quite
right, because that low bit is only valid in LR or on the stack; it's
never set in the PC register.  The GDB simulator lets you set it and
still works more or less OK.  Another simulator we test with (QEMU)
does not allow this.  Nor does the architecture.  So going up a frame,
printing $pc, and seeing its low bit set is surprising.  It also
causes trouble for the "return" command.

Most of CPSR is not saved by normal function calls.  But the nicest
way by far to handle Thumb-ness checking for frames is to unwind that
frame's CPSR and check its T (Thumb state) bit, which requires the
unwinders to synthesize a saved CPSR from the next frame's CPSR
and the saved return address.  Pretty straightforward for the prologue
unwinder, but harder for the DWARF-2 frame unwinder.

To handle that, I added a new DWARF2_FRAME_REG_FN which supplies a
default unwinder.  For the PC, I originally added
DWARF2_FRAME_REG_RA_MASK instead to mask out the low bit, but
that wasn't enough for the CPSR.  And I discovered that
there is no easy way, given the arguments to an unwinder's
prev_register method, to request the unwound value of another
register from the same frame!  That's why the patch below had
to make dwarf2_frame_prev_register global, and pass its opaque
THIS_CACHE to the function supplied by DWARF2_FRAME_REG_FN.

During a prev_register method there are three frames of interest.  The
NEXT frame is passed; the CURRENT frame's cache is passed and some
knowledge of the CURRENT frame is implicit in the called function; and
the PREVIOUS frame's register value is calculated.  Given a frame
there are functions to get its register values or the values of the
previous frame; since we start with NEXT that means we can use generic
frame interfaces to get only NEXT or CURRENT registers.  But to
calculate PREVIOUS's CPSR we need PREVIOUS's LR so we have to call
dwarf2_frame_prev_register directly.

Most prev_register methods work by looking at their cache, and if they
can not find anything there, using an unwind method on the NEXT frame.
That returns the CURRENT frame's registers, i.e., the right values if
this frame did not save the register.  We're never interested in using
the other set of methods, which given the NEXT frame return NEXT's
registers.  And we can not use get_prev_frame (NEXT) to get CURRENT,
because in some cases that may return NULL, like if we're around main
where the backtrace cuts off.  But that frame does always exist.

So why shouldn't the argument be the CURRENT (i.e. THIS) frame
instead?  Then we can call frame_register (CURRENT) instead of
frame_unwind_register (NEXT) to get the same result, plus we'll
have the option of calling frame_unwind_register (CURRENT) when
we need it.

Of course this would be a pain to change all at once since there are
so many unwinders.  I'd introduce a new method instead
(unwind->prev_register_this?).  What do you think?  Is there some
reason I didn't think of why it would be naughty to use the current
frame instead of the next one?

Meanwhile the patch below has been tested on arm-sim/-mthumb, both
with and without the DWARF-2 unwinder.

-- 
Daniel Jacobowitz
CodeSourcery

2007-10-11  Daniel Jacobowitz  <dan@codesourcery.com>

	* arm-linux-tdep.h (ARM_CPSR_REGNUM): Delete definition.
	* arm-tdep.c (arm_pc_is_thumb): Clarify comment.
	(arm_frame_is_thumb): New.
	(arm_addr_bits_remove): Only remove one bit.
	(thumb_analyze_prologue): Remove PC special case.
	(thumb_scan_prologue): Take a block_addr argument.  Use it for
	find_pc_partial_function.  Remove unused variables.
	(arm_scan_prologue): Use arm_frame_is_thumb.  Use the unwound
	block address for find_pc_partial_function.  Remove PC special
	case.
	(arm_prologue_prev_register): Add special handling for PC and LR.
	(arm_dwarf2_prev_register, arm_dwarf2_frame_init_reg): New.
	(arm_get_next_pc): Use arm_frame_is_thumb.
	(arm_write_pc): Use CPSR_T instead of 0x20.
	(arm_gdbarch_init): Call dwarf2_frame_set_init_reg.
	* arm-tdep.h (enum gdb_regnum): Add ARM_CPSR_REGNUM.
	(CPSR_T): Define.
	* dwarf2-frame.c (dwarf2_frame_prev_register): Make global.
	Handle DWARF2_FRAME_REG_FN.
	* dwarf2-frame.h (enum dwarf2_frame_reg_rule): Add
	DWARF2_FRAME_REG_FN.
	(struct dwarf2_frame_state_reg): Add FN to loc union.
	(dwarf2_frame_prev_register): Declare.

2007-10-11  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.arch/thumb-prologue.exp: Do not expect a saved PC.

Index: arm-linux-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/arm-linux-tdep.h,v
retrieving revision 1.3
diff -u -p -r1.3 arm-linux-tdep.h
--- arm-linux-tdep.h	23 Aug 2007 18:08:26 -0000	1.3
+++ arm-linux-tdep.h	11 Oct 2007 14:16:49 -0000
@@ -20,8 +20,6 @@
 struct regset;
 struct regcache;
 
-#define		ARM_CPSR_REGNUM		16
-
 #define ARM_LINUX_SIZEOF_NWFPE (8 * FP_REGISTER_SIZE \
 				+ 2 * INT_REGISTER_SIZE \
 				+ 8 + INT_REGISTER_SIZE)
Index: arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.245
diff -u -p -r1.245 arm-tdep.c
--- arm-tdep.c	10 Oct 2007 14:04:53 -0000	1.245
+++ arm-tdep.c	11 Oct 2007 14:16:49 -0000
@@ -211,7 +211,8 @@ struct arm_prologue_cache
 int arm_apcs_32 = 1;
 
 /* Determine if the program counter specified in MEMADDR is in a Thumb
-   function.  */
+   function.  This function should be called for addresses unrelated to
+   any executing frame; otherwise, prefer arm_frame_is_thumb.  */
 
 static int
 arm_pc_is_thumb (CORE_ADDR memaddr)
@@ -234,12 +235,33 @@ arm_pc_is_thumb (CORE_ADDR memaddr)
     }
 }
 
+/* Determine if FRAME is executing in Thumb mode.  It does not have to
+   be the current frame.  If UNWIND is non-zero, check the frame previous
+   to FRAME instead.  */
+
+static int
+arm_frame_is_thumb (struct frame_info *frame, int unwind)
+{
+  CORE_ADDR cpsr;
+
+  /* Every ARM frame unwinder can unwind the CPSR, either directly
+     (from a signal frame or dummy frame) or by interpreting the saved
+     LR (from a prologue or DWARF frame).  So consult it and trust
+     the unwinders.  */
+  if (unwind)
+    cpsr = frame_unwind_register_unsigned (frame, ARM_PS_REGNUM);
+  else
+    cpsr = get_frame_register_unsigned (frame, ARM_PS_REGNUM);
+
+  return (cpsr & CPSR_T) != 0;
+}
+
 /* Remove useless bits from addresses in a running program.  */
 static CORE_ADDR
 arm_addr_bits_remove (CORE_ADDR val)
 {
   if (arm_apcs_32)
-    return (val & (arm_pc_is_thumb (val) ? 0xfffffffe : 0xfffffffc));
+    return UNMAKE_THUMB_ADDR (val);
   else
     return (val & 0x03fffffc);
 }
@@ -272,14 +294,6 @@ thumb_analyze_prologue (struct gdbarch *
   stack = make_pv_area (ARM_SP_REGNUM);
   back_to = make_cleanup_free_pv_area (stack);
 
-  /* The call instruction saved PC in LR, and the current PC is not
-     interesting.  Due to this file's conventions, we want the value
-     of LR at this function's entry, not at the call site, so we do
-     not record the save of the PC - when the ARM prologue analyzer
-     has also been converted to the pv mechanism, we could record the
-     save here and remove the hack in prev_register.  */
-  regs[ARM_PC_REGNUM] = pv_unknown ();
-
   while (start < limit)
     {
       unsigned short insn;
@@ -533,22 +547,15 @@ arm_skip_prologue (CORE_ADDR pc)
 /* *INDENT-ON* */
 
 static void
-thumb_scan_prologue (CORE_ADDR prev_pc, struct arm_prologue_cache *cache)
+thumb_scan_prologue (CORE_ADDR prev_pc, CORE_ADDR block_addr,
+		     struct arm_prologue_cache *cache)
 {
   CORE_ADDR prologue_start;
   CORE_ADDR prologue_end;
   CORE_ADDR current_pc;
-  /* Which register has been copied to register n?  */
-  int saved_reg[16];
-  /* findmask:
-     bit 0 - push { rlist }
-     bit 1 - mov r7, sp  OR  add r7, sp, #imm  (setting of r7)
-     bit 2 - sub sp, #simm  OR  add sp, #simm  (adjusting of sp)
-  */
-  int findmask = 0;
-  int i;
 
-  if (find_pc_partial_function (prev_pc, NULL, &prologue_start, &prologue_end))
+  if (find_pc_partial_function (block_addr, NULL, &prologue_start,
+				&prologue_end))
     {
       struct symtab_and_line sal = find_pc_line (prologue_start, 0);
 
@@ -643,6 +650,8 @@ arm_scan_prologue (struct frame_info *ne
   int regno;
   CORE_ADDR prologue_start, prologue_end, current_pc;
   CORE_ADDR prev_pc = frame_pc_unwind (next_frame);
+  CORE_ADDR block_addr = frame_unwind_address_in_block (next_frame,
+							NORMAL_FRAME);
   pv_t regs[ARM_FPS_REGNUM];
   struct pv_area *stack;
   struct cleanup *back_to;
@@ -653,15 +662,16 @@ arm_scan_prologue (struct frame_info *ne
   cache->framesize = 0;
 
   /* Check for Thumb prologue.  */
-  if (arm_pc_is_thumb (prev_pc))
+  if (arm_frame_is_thumb (next_frame, 1))
     {
-      thumb_scan_prologue (prev_pc, cache);
+      thumb_scan_prologue (prev_pc, block_addr, cache);
       return;
     }
 
   /* Find the function prologue.  If we can't find the function in
      the symbol table, peek in the stack frame to find the PC.  */
-  if (find_pc_partial_function (prev_pc, NULL, &prologue_start, &prologue_end))
+  if (find_pc_partial_function (block_addr, NULL, &prologue_start,
+				&prologue_end))
     {
       /* One way to find the end of the prologue (which works well
          for unoptimized code) is to do the following:
@@ -750,8 +760,6 @@ arm_scan_prologue (struct frame_info *ne
   stack = make_pv_area (ARM_SP_REGNUM);
   back_to = make_cleanup_free_pv_area (stack);
 
-  regs[ARM_PC_REGNUM] = pv_unknown ();
-
   for (current_pc = prologue_start;
        current_pc < prologue_end;
        current_pc += 4)
@@ -989,10 +997,31 @@ arm_prologue_prev_register (struct frame
   cache = *this_cache;
 
   /* If we are asked to unwind the PC, then we need to return the LR
-     instead.  The saved value of PC points into this frame's
-     prologue, not the next frame's resume location.  */
+     instead.  The prologue may save PC, but it will point into this
+     frame's prologue, not the next frame's resume location.  Also
+     strip the saved T bit.  A valid LR may have the low bit set, but
+     a valid PC never does.  */
   if (prev_regnum == ARM_PC_REGNUM)
-    prev_regnum = ARM_LR_REGNUM;
+    {
+      if (valuep)
+	{
+	  CORE_ADDR pc;
+
+	  trad_frame_get_prev_register (next_frame, cache->saved_regs,
+					ARM_LR_REGNUM, optimized, lvalp, addrp,
+					realnump, valuep);
+	  pc = extract_unsigned_integer (valuep, INT_REGISTER_SIZE);
+	  pc = arm_addr_bits_remove (pc);
+	  store_unsigned_integer (valuep, INT_REGISTER_SIZE, pc);
+	}
+
+      /* The PC is never an lvalue.  */
+      *optimized = 0;
+      *lvalp = not_lval;
+      *addrp = 0;
+      *realnump = -1;
+      return;
+    }
 
   /* SP is generally not saved to the stack, but this frame is
      identified by NEXT_FRAME's stack pointer at the time of the call.
@@ -1005,6 +1034,39 @@ arm_prologue_prev_register (struct frame
       return;
     }
 
+  /* The CPSR may have been changed by the call instruction and by the
+     called function.  The only bit we can reconstruct is the T bit,
+     by checking the low bit of LR as of the call.  This is a reliable
+     indicator of Thumb-ness except for some ARM v4T pre-interworking
+     Thumb code, which could get away with a clear low bit as long as
+     the called function did not use bx.  Guess that all other
+     bits are unchanged; the condition flags are presumably lost,
+     but the processor status is likely valid.  */
+  if (prev_regnum == ARM_PS_REGNUM)
+    {
+      if (valuep)
+	{
+	  CORE_ADDR lr, cpsr;
+
+	  cpsr = frame_unwind_register_unsigned (next_frame, prev_regnum);
+
+	  trad_frame_get_prev_register (next_frame, cache->saved_regs,
+					ARM_LR_REGNUM, optimized, lvalp, addrp,
+					realnump, valuep);
+	  lr = extract_unsigned_integer (valuep, INT_REGISTER_SIZE);
+	  if (IS_THUMB_ADDR (lr))
+	    cpsr |= CPSR_T;
+	  else
+	    cpsr &= ~CPSR_T;
+	  store_unsigned_integer (valuep, INT_REGISTER_SIZE, cpsr);
+	}
+
+      *optimized = 0;
+      *lvalp = not_lval;
+      *addrp = 0;
+      *realnump = -1;
+    }
+
   trad_frame_get_prev_register (next_frame, cache->saved_regs, prev_regnum,
 				optimized, lvalp, addrp, realnump, valuep);
 }
@@ -1123,6 +1185,93 @@ arm_unwind_sp (struct gdbarch *gdbarch, 
   return frame_unwind_register_unsigned (this_frame, ARM_SP_REGNUM);
 }
 
+static void
+arm_dwarf2_prev_register (struct frame_info *next_frame, void **this_cache,
+			  int regnum, int *optimized, enum lval_type *lvalp,
+			  CORE_ADDR *addrp, int *realnump,
+			  gdb_byte *valuep)
+{
+  switch (regnum)
+    {
+    case ARM_PC_REGNUM:
+      /* The PC is normally copied from the return column, which
+	 describes saves of LR.  However, that version may have an
+	 extra bit set to indicate Thumb state.  The bit is not
+	 part of the PC.  */
+      if (valuep)
+	{
+	  CORE_ADDR pc;
+
+	  dwarf2_frame_prev_register (next_frame, this_cache,
+				      ARM_LR_REGNUM, optimized, lvalp,
+				      addrp, realnump, valuep);
+	  pc = extract_unsigned_integer (valuep, INT_REGISTER_SIZE);
+	  pc = arm_addr_bits_remove (pc);
+	  store_unsigned_integer (valuep, INT_REGISTER_SIZE, pc);
+	}
+
+      *optimized = 0;
+      *lvalp = not_lval;
+      *addrp = 0;
+      *realnump = -1;
+      break;
+
+    case ARM_PS_REGNUM:
+      /* The CPSR may have been changed by the call instruction and by
+	 the called function.  The only bit we can reconstruct is the
+	 T bit, by checking the low bit of LR as of the call.  This is
+	 a reliable indicator of Thumb-ness except for some ARM v4T
+	 pre-interworking Thumb code, which could get away with a
+	 clear low bit as long as the called function did not use bx.
+	 Guess that all other bits are unchanged; the condition flags
+	 are presumably lost, but the processor status is likely
+	 valid.  */
+      if (valuep)
+	{
+	  CORE_ADDR lr, cpsr;
+
+	  cpsr = frame_unwind_register_unsigned (next_frame, ARM_PS_REGNUM);
+	  dwarf2_frame_prev_register (next_frame, this_cache,
+				      ARM_LR_REGNUM, optimized, lvalp,
+				      addrp, realnump, valuep);
+	  lr = extract_unsigned_integer (valuep, INT_REGISTER_SIZE);
+	  if (IS_THUMB_ADDR (lr))
+	    cpsr |= CPSR_T;
+	  else
+	    cpsr &= ~CPSR_T;
+	  store_unsigned_integer (valuep, INT_REGISTER_SIZE, cpsr);
+	}
+
+      *optimized = 0;
+      *lvalp = not_lval;
+      *addrp = 0;
+      *realnump = -1;
+      break;
+
+    default:
+      internal_error (__FILE__, __LINE__,
+		      _("Unexpected register %d"), regnum);
+    }
+}
+
+static void
+arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
+			   struct dwarf2_frame_state_reg *reg,
+			   struct frame_info *next_frame)
+{
+  switch (regnum)
+    {
+    case ARM_PC_REGNUM:
+    case ARM_PS_REGNUM:
+      reg->how = DWARF2_FRAME_REG_FN;
+      reg->loc.fn = arm_dwarf2_prev_register;
+      break;
+    case ARM_SP_REGNUM:
+      reg->how = DWARF2_FRAME_REG_CFA;
+      break;
+    }
+}
+
 /* When arguments must be pushed onto the stack, they go on in reverse
    order.  The code below implements a FILO (stack) to do this.  */
 
@@ -1701,7 +1850,7 @@ arm_get_next_pc (struct frame_info *fram
   unsigned long status;
   CORE_ADDR nextpc;
 
-  if (arm_pc_is_thumb (pc))
+  if (arm_frame_is_thumb (frame, 0))
     return thumb_get_next_pc (frame, pc);
 
   pc_val = (unsigned long) pc;
@@ -2669,10 +2818,10 @@ arm_write_pc (struct regcache *regcache,
       ULONGEST val;
       regcache_cooked_read_unsigned (regcache, ARM_PS_REGNUM, &val);
       if (arm_pc_is_thumb (pc))
-	regcache_cooked_write_unsigned (regcache, ARM_PS_REGNUM, val | 0x20);
+	regcache_cooked_write_unsigned (regcache, ARM_PS_REGNUM, val | CPSR_T);
       else
 	regcache_cooked_write_unsigned (regcache, ARM_PS_REGNUM,
-					val & ~(ULONGEST) 0x20);
+					val & ~(ULONGEST) CPSR_T);
     }
 }
 
@@ -3036,6 +3185,8 @@ arm_gdbarch_init (struct gdbarch_info in
   /* Hook in the ABI-specific overrides, if they have been registered.  */
   gdbarch_init_osabi (info, gdbarch);
 
+  dwarf2_frame_set_init_reg (gdbarch, arm_dwarf2_frame_init_reg);
+
   /* Add some default predicates.  */
   frame_unwind_append_sniffer (gdbarch, arm_stub_unwind_sniffer);
   frame_unwind_append_sniffer (gdbarch, dwarf2_frame_sniffer);
Index: arm-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.h,v
retrieving revision 1.28
diff -u -p -r1.28 arm-tdep.h
--- arm-tdep.h	10 Oct 2007 14:04:53 -0000	1.28
+++ arm-tdep.h	11 Oct 2007 14:16:49 -0000
@@ -38,6 +38,7 @@ enum gdb_regnum {
   ARM_F7_REGNUM = 23, 		/* last floating point register */
   ARM_FPS_REGNUM = 24,		/* floating point status register */
   ARM_PS_REGNUM = 25,		/* Contains processor status */
+  ARM_CPSR_REGNUM = ARM_PS_REGNUM,
   ARM_WR0_REGNUM,		/* WMMX data registers.  */
   ARM_WR15_REGNUM = ARM_WR0_REGNUM + 15,
   ARM_WC0_REGNUM,		/* WMMX control registers.  */
@@ -107,6 +108,8 @@ enum gdb_regnum {
 #define FLAG_C		0x20000000
 #define FLAG_V		0x10000000
 
+#define CPSR_T		0x20
+
 /* Type of floating-point code in use by inferior.  There are really 3 models
    that are traditionally supported (plus the endianness issue), but gcc can
    only generate 2 of those.  The third is APCS_FLOAT, where arguments to
Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.77
diff -u -p -r1.77 dwarf2-frame.c
--- dwarf2-frame.c	8 Oct 2007 12:46:09 -0000	1.77
+++ dwarf2-frame.c	11 Oct 2007 14:16:49 -0000
@@ -1022,7 +1022,7 @@ dwarf2_signal_frame_this_id (struct fram
 			       frame_func_unwind (next_frame, SIGTRAMP_FRAME));
 }
 
-static void
+void
 dwarf2_frame_prev_register (struct frame_info *next_frame, void **this_cache,
 			    int regnum, int *optimizedp,
 			    enum lval_type *lvalp, CORE_ADDR *addrp,
@@ -1168,6 +1168,11 @@ dwarf2_frame_prev_register (struct frame
         }
       break;
 
+    case DWARF2_FRAME_REG_FN:
+      cache->reg[regnum].loc.fn (next_frame, this_cache, regnum, optimizedp,
+				 lvalp, addrp, realnump, valuep);
+      break;
+
     default:
       internal_error (__FILE__, __LINE__, _("Unknown register rule."));
     }
Index: dwarf2-frame.h
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.h,v
retrieving revision 1.16
diff -u -p -r1.16 dwarf2-frame.h
--- dwarf2-frame.h	23 Aug 2007 18:08:28 -0000	1.16
+++ dwarf2-frame.h	11 Oct 2007 14:16:49 -0000
@@ -55,6 +55,7 @@ enum dwarf2_frame_reg_rule
 
   /* These aren't defined by the DWARF2 CFI specification, but are
      used internally by GDB.  */
+  DWARF2_FRAME_REG_FN,		/* Call a registered function.  */
   DWARF2_FRAME_REG_RA,		/* Return Address.  */
   DWARF2_FRAME_REG_RA_OFFSET,	/* Return Address with offset.  */
   DWARF2_FRAME_REG_CFA,		/* Call Frame Address.  */
@@ -71,6 +72,10 @@ struct dwarf2_frame_state_reg
     LONGEST offset;
     ULONGEST reg;
     unsigned char *exp;
+    void (*fn) (struct frame_info *next_frame, void **this_cache,
+		int regnum, int *optimizedp,
+		enum lval_type *lvalp, CORE_ADDR *addrp, int *realnump,
+		gdb_byte *valuep);
   } loc;
   ULONGEST exp_len;
   enum dwarf2_frame_reg_rule how;
@@ -116,4 +121,10 @@ extern const struct frame_base *
 
 void dwarf2_frame_build_info (struct objfile *objfile);
 
+void dwarf2_frame_prev_register (struct frame_info *next_frame, void
+				 **this_cache,
+				 int regnum, int *optimizedp,
+				 enum lval_type *lvalp, CORE_ADDR *addrp,
+				 int *realnump, gdb_byte *valuep);
+
 #endif /* dwarf2-frame.h */
Index: testsuite/gdb.arch/thumb-prologue.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/thumb-prologue.exp,v
retrieving revision 1.5
diff -u -p -r1.5 thumb-prologue.exp
--- testsuite/gdb.arch/thumb-prologue.exp	23 Aug 2007 18:14:16 -0000	1.5
+++ testsuite/gdb.arch/thumb-prologue.exp	11 Oct 2007 14:16:50 -0000
@@ -57,5 +57,5 @@ gdb_test "backtrace 10" \
 	"backtrace in TPCS"
 
 gdb_test "info frame" \
-	".*Saved registers:.*r7 at.*r10 at.*r11 at.*lr at.*pc at .*" \
+	".*Saved registers:.*r7 at.*r10 at.*r11 at.*lr at.*" \
 	"saved registers in TPCS"

Re: [rfc] Unwind the ARM CPSR

[Daniel Jacobowitz]

On Thu, Oct 11, 2007 at 10:51:37AM -0400, Daniel Jacobowitz wrote:
> So why shouldn't the argument be the CURRENT (i.e. THIS) frame
> instead?  Then we can call frame_register (CURRENT) instead of
> frame_unwind_register (NEXT) to get the same result, plus we'll
> have the option of calling frame_unwind_register (CURRENT) when
> we need it.
> 
> Of course this would be a pain to change all at once since there are
> so many unwinders.  I'd introduce a new method instead
> (unwind->prev_register_this?).  What do you think?  Is there some
> reason I didn't think of why it would be naughty to use the current
> frame instead of the next one?

By the way, if I'm going to be changing the prev_register interface,
the other thing I've thought of doing is making it return a struct
value instead of filling in a pile of value-like fields (register
number, address, lval, et cetera).  I am a little worried about how
this would affect watchpoints on local variables though.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Unwind the ARM CPSR

[Joel Brobecker]

> So why shouldn't the argument be the CURRENT (i.e. THIS) frame
> instead?  Then we can call frame_register (CURRENT) instead of
> frame_unwind_register (NEXT) to get the same result, plus we'll
> have the option of calling frame_unwind_register (CURRENT) when
> we need it.

I can see why you'd like some feedback. This (amazing piece of) code
has always been a bit difficult for me to grasp in its entirety...
I hope others will take some time to think about it too, because
I don't feel sufficiently proficient to provide a confident answer.

FWIW, I don't see how we could end up breaking something with your
approach. As it turns out, there is a comment that says about
the prev_register method:

   Why not pass in THIS_FRAME?  By passing in NEXT frame and THIS
   cache, the supplied parameters are consistent with the sibling
   function THIS_ID.

So it sounds like the author wasn't seeing any issue with that
either.

I personally think that passing the current frame will make
this function a little easier to understand too, no? (I find
the twist of computing the caller's registers of this frame
using the next frame a constant mental exercise)

> Of course this would be a pain to change all at once since there are
> so many unwinders.  I'd introduce a new method instead
> (unwind->prev_register_this?).  What do you think?

Sounds like a possible plan to me.

-- 
Joel

Re: [rfc] Unwind the ARM CPSR

[Mark Kettenis]

> Date: Thu, 11 Oct 2007 20:53:36 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > So why shouldn't the argument be the CURRENT (i.e. THIS) frame
> > instead?  Then we can call frame_register (CURRENT) instead of
> > frame_unwind_register (NEXT) to get the same result, plus we'll
> > have the option of calling frame_unwind_register (CURRENT) when
> > we need it.

And risking infinite recursion?  I'm fairly sure it is a mistake to
call frame_register(CURRENT) at this place.

> I can see why you'd like some feedback. This (amazing piece of) code
> has always been a bit difficult for me to grasp in its entirety...
> I hope others will take some time to think about it too, because
> I don't feel sufficiently proficient to provide a confident answer.
> 
> FWIW, I don't see how we could end up breaking something with your
> approach. As it turns out, there is a comment that says about
> the prev_register method:
> 
>    Why not pass in THIS_FRAME?  By passing in NEXT frame and THIS
>    cache, the supplied parameters are consistent with the sibling
>    function THIS_ID.
> 
> So it sounds like the author wasn't seeing any issue with that
> either.

I wouldn't be so sure.  Andrew Cagney put quite a bit of thought in
the interface, and while it sometimes appears to be strange on first
sight, it has always proven to be the right choice.

> I personally think that passing the current frame will make
> this function a little easier to understand too, no? (I find
> the twist of computing the caller's registers of this frame
> using the next frame a constant mental exercise)

I agree it is a mental excercise, but it prevents you from doing
stupid things.

> > Of course this would be a pain to change all at once since there are
> > so many unwinders.  I'd introduce a new method instead
> > (unwind->prev_register_this?).  What do you think?
> 
> Sounds like a possible plan to me.

I think it is a very bad idea.  It will take ages before all targets
are converted, and having two functions to do the same thing will be
even more confusing.

Re: [rfc] Unwind the ARM CPSR

[Mark Kettenis]

> Date: Thu, 11 Oct 2007 10:51:37 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> Most of CPSR is not saved by normal function calls.  But the nicest
> way by far to handle Thumb-ness checking for frames is to unwind that
> frame's CPSR and check its T (Thumb state) bit, which requires the
> unwinders to synthesize a saved CPSR from the next frame's CPSR
> and the saved return address.  Pretty straightforward for the prologue
> unwinder, but harder for the DWARF-2 frame unwinder.

Yeah, DWARF-2 doesn't really have the concept of bit-fields.

> To handle that, I added a new DWARF2_FRAME_REG_FN which supplies a
> default unwinder.  For the PC, I originally added
> DWARF2_FRAME_REG_RA_MASK instead to mask out the low bit, but
> that wasn't enough for the CPSR.  And I discovered that
> there is no easy way, given the arguments to an unwinder's
> prev_register method, to request the unwound value of another
> register from the same frame!  That's why the patch below had
> to make dwarf2_frame_prev_register global, and pass its opaque
> THIS_CACHE to the function supplied by DWARF2_FRAME_REG_FN.

Two remarks:

1. Isn't this function always going to return a plain value?  That
   means we can probably simplify the register.

2. Perhaps the function should get passed the unwinder struct, such
   that it can use the function pointer.

> During a prev_register method there are three frames of interest.  The
> NEXT frame is passed; the CURRENT frame's cache is passed and some
> knowledge of the CURRENT frame is implicit in the called function; and
> the PREVIOUS frame's register value is calculated.  Given a frame
> there are functions to get its register values or the values of the
> previous frame; since we start with NEXT that means we can use generic
> frame interfaces to get only NEXT or CURRENT registers.  But to
> calculate PREVIOUS's CPSR we need PREVIOUS's LR so we have to call
> dwarf2_frame_prev_register directly.

I think this means that making the unwinder reconstruct CPSR may not
be the right approach.  Isn't it better to determine thumbness based
on the unwound LR?  That should work for all frames, except the
topmost.  But for the topmost frame we should be able to trust CPSR.

I also see that you treat unwinding the PC specially in the unwinders,
to strip off bits.  But isn't that why we have the unwind_pc gdbarch
method?

Re: [rfc] Unwind the ARM CPSR

[Daniel Jacobowitz]

Hi Mark, and thanks for the comments.  I've been working on
this again (all day...), so it's fresh in my mind.

On Wed, Oct 17, 2007 at 12:01:03AM +0200, Mark Kettenis wrote:
> > I can see why you'd like some feedback. This (amazing piece of) code
> > has always been a bit difficult for me to grasp in its entirety...
> > I hope others will take some time to think about it too, because
> > I don't feel sufficiently proficient to provide a confident answer.
> > 
> > FWIW, I don't see how we could end up breaking something with your
> > approach. As it turns out, there is a comment that says about
> > the prev_register method:
> > 
> >    Why not pass in THIS_FRAME?  By passing in NEXT frame and THIS
> >    cache, the supplied parameters are consistent with the sibling
> >    function THIS_ID.
> > 
> > So it sounds like the author wasn't seeing any issue with that
> > either.
> 
> I wouldn't be so sure.  Andrew Cagney put quite a bit of thought in
> the interface, and while it sometimes appears to be strange on first
> sight, it has always proven to be the right choice.

I agree that he put a lot of thought into it, but some of his
decisions were forced by the path to get GDB from where it was then
(ew) to where it is now (much nicer).  I looked into the history of
this today.

The original version of frame-unwind.h (excluding development
branches) was on 2003-01-18, and passed the current frame.  It was
changed to pass the next frame on 2003-03-17 as the result
of Andrew's offbyone branch.  Here's the patch:

  http://sourceware.org/ml/gdb-patches/2003-03/msg00339.html

The problem was a mismatch between which cache the this_id function
was passed and which cache the prev_register function for the same
frame was passed.  I rediscovered this problem today and I can
see why it was such a pain to fix at the time - it rather
ruined my evening.

But the most interesting thing I found in the archives was this:

  http://sourceware.org/ml/gdb-patches/2003-03/msg00365.html

That, a couple of days later, was the first step in not calculating
the frame ID until we needed it.  Before that happened, Andrew's fix
for the off-by-one bug was the only workable solution: the previous
frame wasn't usable until after its ID had been calculated.  After the
frame ID was calculated lazily, though, getting the previous frame
became a cheap operation and we could postpone ID checks until we
needed them.

So the problem that Andrew originally solved by passing the NEXT frame
to the unwinder is gone now.

Just for kicks I took a look at Frysk, whose unwinder he's working on
as we speak.  A Frame object has a getFrameIdentifier method
(equivalent to ->this_id taking this_frame), and a getRegister method.
I can't tell if that's prev_register taking this_frame or my_register
taking this_frame.

> I think it is a very bad idea.  It will take ages before all targets
> are converted, and having two functions to do the same thing will be
> even more confusing.

Yes, that's true; it will be confusing.  I've taken the opportunity to
work on a related project at the same time though and so far I like
the shape of the result:

typedef struct value * (frame_prev_register_value_ftype)
  (struct frame_info *this_frame, void **this_prologue_cache,
   int prev_regnum);

The mental exercise of having only the NEXT frame prevents some stupid
things.  But it also prevents some smart things, and it makes
unwinders (in my personal opinion only, of course) much harder to
write.  This is probably the subtlest part of GDB; I like anything
that makes it more obvious how it works.

I'll have a concrete example finished in another day or so, and
then I'll post a proper RFC for the frame changes.

On Wed, Oct 17, 2007 at 12:20:49AM +0200, Mark Kettenis wrote:
> > To handle that, I added a new DWARF2_FRAME_REG_FN which supplies a
> > default unwinder.  For the PC, I originally added
> > DWARF2_FRAME_REG_RA_MASK instead to mask out the low bit, but
> > that wasn't enough for the CPSR.  And I discovered that
> > there is no easy way, given the arguments to an unwinder's
> > prev_register method, to request the unwound value of another
> > register from the same frame!  That's why the patch below had
> > to make dwarf2_frame_prev_register global, and pass its opaque
> > THIS_CACHE to the function supplied by DWARF2_FRAME_REG_FN.
> 
> Two remarks:
> 
> 1. Isn't this function always going to return a plain value?  That
>    means we can probably simplify the register.

Not pass all those extra arguments, you mean?  Yes, maybe we can.  It
did always return a plain value in my case and we could change it
later if more generality was needed.

> 2. Perhaps the function should get passed the unwinder struct, such
>    that it can use the function pointer.

Yeah, that's a nice alternative to making dwarf2_frame_prev_register
global.  It means we need to pass the frame, the unwinder, and the
cache, though - that still suggests to me that there is a simpler and
righter answer.

> > During a prev_register method there are three frames of interest.  The
> > NEXT frame is passed; the CURRENT frame's cache is passed and some
> > knowledge of the CURRENT frame is implicit in the called function; and
> > the PREVIOUS frame's register value is calculated.  Given a frame
> > there are functions to get its register values or the values of the
> > previous frame; since we start with NEXT that means we can use generic
> > frame interfaces to get only NEXT or CURRENT registers.  But to
> > calculate PREVIOUS's CPSR we need PREVIOUS's LR so we have to call
> > dwarf2_frame_prev_register directly.
> 
> I think this means that making the unwinder reconstruct CPSR may not
> be the right approach.  Isn't it better to determine thumbness based
> on the unwound LR?  That should work for all frames, except the
> topmost.  But for the topmost frame we should be able to trust CPSR.

I implemented that first, actually.  In that case I need to check the
types (get_frame_type) of various frames, since the frame before a
signal frame or dummy frame are also "topmost".  And I ended up with
some assumptions that violated the abstraction boundaries of the
unwinder, e.g., that signal and dummy frames would always have a saved
CPSR.

It seemed very inelegant to me; the right "unwound" value of the CPSR
depends on how we unwound, so why not have the unwinder supply it?

> I also see that you treat unwinding the PC specially in the unwinders,
> to strip off bits.  But isn't that why we have the unwind_pc gdbarch
> method?

The failing testcase that started me down this rabbit hole was
return.exp, which uses frame_pop.  The gdbarch unwind_pc method is
always used when core GDB wants a PC or resume address, but it's not
used when we want to restore the value of a register (which happens to
be register number PC_REGNUM).  And something needs to clean up the
saved CPSR value when popping, for the same reason - you need
interworking (mixed ARM and Thumb) for that to cause a failure
though so the current testsuite never triggers it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Unwind the ARM CPSR

[Daniel Jacobowitz]

On Tue, Oct 16, 2007 at 08:30:13PM -0400, Daniel Jacobowitz wrote:
> > I think it is a very bad idea.  It will take ages before all targets
> > are converted, and having two functions to do the same thing will be
> > even more confusing.
> 
> Yes, that's true; it will be confusing.  I've taken the opportunity to
> work on a related project at the same time though and so far I like
> the shape of the result:

By the way: if we do make this switch, I will personally commit to
fixing every single unwinder in GDB before the next release and
eliminating all traces of the current prev_register method.

I sure hope someone appreciates that offer enough to help.  I think
I'm out of my mind.  There are 68 of them in the source tree now and
they have tendrils all over the place.  But I did my share of shouting
about how much of GDB was deprecated and then left to rot; I do not
want to contribute to the problem.

I'm not volunteering to test all of them, but I'll test the ones
I can and make mechanical fixes to the rest.  

Fortunately about half of those use trad-frame.c.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Unwind the ARM CPSR

[Joel Brobecker]

> By the way: if we do make this switch, I will personally commit to
> fixing every single unwinder in GDB before the next release and
> eliminating all traces of the current prev_register method.
> I sure hope someone appreciates that offer enough to help.

If we make the switch, I can help with the transition. The changes
are going to be relatively mechanical and I have access to some
exotic platforms that I can test on.

-- 
Joel

Re: [rfc] Unwind the ARM CPSR

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> I sure hope someone appreciates that offer enough to help.  I think
> I'm out of my mind.  There are 68 of them in the source tree now and
> they have tendrils all over the place.  But I did my share of shouting
> about how much of GDB was deprecated and then left to rot; I do not
> want to contribute to the problem.

I also would much prefer to see the conversion done in a single step,
so thank you very much for the offer :-) I would be happy to help with
either/all of s390, spu, ppc, i386/amd64, ia64 ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Unwind the ARM CPSR

[Daniel Jacobowitz]

On Thu, Oct 11, 2007 at 10:51:37AM -0400, Daniel Jacobowitz wrote:
> This patch is based on a suggestion that Mark made around March 2006,
> and another related problem that Paul Brook and Vladimir Prus
> discovered.  I would love comments on it; it works, but there is
> one place that I'm unhappy with (in frame unwinding and the DWARF-2
> unwinder).

That place is now gone.  I've tested this version on arm-none-eabi
(where it fixes one failure with -mthumb, gdb.base/ending-run.exp:
step to end of run) and on arm-none-symbianelf.

Checked in.

-- 
Daniel Jacobowitz
CodeSourcery

2008-05-01  Daniel Jacobowitz  <dan@codesourcery.com>

	* arm-linux-tdep.h (ARM_CPSR_REGNUM): Delete definition.
	* arm-tdep.c (arm_frame_is_thumb): New.
	(arm_pc_is_thumb): Clarify comment.
	(thumb_analyze_prologue): Remove PC special case.
	(thumb_scan_prologue): Take a block_addr argument.  Use it for
	find_pc_partial_function.  Remove unused variables.
	(arm_scan_prologue): Use arm_frame_is_thumb.  Use the block address
	for find_pc_partial_function.  Remove PC special case.
	(arm_prologue_prev_register): Add special handling for PC and CPSR.
	(arm_dwarf2_prev_register, arm_dwarf2_frame_init_reg): New.
	(arm_get_next_pc): Use arm_frame_is_thumb.
	(arm_write_pc): Use CPSR_T instead of 0x20.
	(arm_gdbarch_init): Call dwarf2_frame_set_init_reg.
	* arm-tdep.h (enum gdb_regnum): Add ARM_CPSR_REGNUM.
	(CPSR_T): Define.
	* dwarf2-frame.c (dwarf2_frame_prev_register): Handle
	DWARF2_FRAME_REG_FN.
	* dwarf2-frame.h (enum dwarf2_frame_reg_rule): Add
	DWARF2_FRAME_REG_FN.
	(struct dwarf2_frame_state_reg): Add FN to loc union.

2007-10-11  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.arch/thumb-prologue.exp: Do not expect a saved PC.

---
 gdb/arm-linux-tdep.h                      |    2 
 gdb/arm-tdep.c                            |  147 +++++++++++++++++++++++-------
 gdb/arm-tdep.h                            |    3 
 gdb/dwarf2-frame.c                        |    3 
 gdb/dwarf2-frame.h                        |    3 
 gdb/testsuite/gdb.arch/thumb-prologue.exp |    2 
 6 files changed, 126 insertions(+), 34 deletions(-)

Index: src/gdb/arm-linux-tdep.h
===================================================================
--- src.orig/gdb/arm-linux-tdep.h	2008-01-01 17:10:01.000000000 -0500
+++ src/gdb/arm-linux-tdep.h	2008-05-01 10:00:54.000000000 -0400
@@ -20,8 +20,6 @@
 struct regset;
 struct regcache;
 
-#define		ARM_CPSR_REGNUM		16
-
 #define ARM_LINUX_SIZEOF_NWFPE (8 * FP_REGISTER_SIZE \
 				+ 2 * INT_REGISTER_SIZE \
 				+ 8 + INT_REGISTER_SIZE)
Index: src/gdb/arm-tdep.c
===================================================================
--- src.orig/gdb/arm-tdep.c	2008-04-30 17:23:09.000000000 -0400
+++ src/gdb/arm-tdep.c	2008-05-01 13:56:01.000000000 -0400
@@ -211,8 +211,25 @@ struct arm_prologue_cache
 
 int arm_apcs_32 = 1;
 
+/* Determine if FRAME is executing in Thumb mode.  */
+
+static int
+arm_frame_is_thumb (struct frame_info *frame)
+{
+  CORE_ADDR cpsr;
+
+  /* Every ARM frame unwinder can unwind the T bit of the CPSR, either
+     directly (from a signal frame or dummy frame) or by interpreting
+     the saved LR (from a prologue or DWARF frame).  So consult it and
+     trust the unwinders.  */
+  cpsr = get_frame_register_unsigned (frame, ARM_PS_REGNUM);
+
+  return (cpsr & CPSR_T) != 0;
+}
+
 /* Determine if the program counter specified in MEMADDR is in a Thumb
-   function.  */
+   function.  This function should be called for addresses unrelated to
+   any executing frame; otherwise, prefer arm_frame_is_thumb.  */
 
 static int
 arm_pc_is_thumb (CORE_ADDR memaddr)
@@ -273,14 +290,6 @@ thumb_analyze_prologue (struct gdbarch *
   stack = make_pv_area (ARM_SP_REGNUM);
   back_to = make_cleanup_free_pv_area (stack);
 
-  /* The call instruction saved PC in LR, and the current PC is not
-     interesting.  Due to this file's conventions, we want the value
-     of LR at this function's entry, not at the call site, so we do
-     not record the save of the PC - when the ARM prologue analyzer
-     has also been converted to the pv mechanism, we could record the
-     save here and remove the hack in prev_register.  */
-  regs[ARM_PC_REGNUM] = pv_unknown ();
-
   while (start < limit)
     {
       unsigned short insn;
@@ -535,22 +544,14 @@ arm_skip_prologue (struct gdbarch *gdbar
 
 static void
 thumb_scan_prologue (struct gdbarch *gdbarch, CORE_ADDR prev_pc,
-		     struct arm_prologue_cache *cache)
+		     CORE_ADDR block_addr, struct arm_prologue_cache *cache)
 {
   CORE_ADDR prologue_start;
   CORE_ADDR prologue_end;
   CORE_ADDR current_pc;
-  /* Which register has been copied to register n?  */
-  int saved_reg[16];
-  /* findmask:
-     bit 0 - push { rlist }
-     bit 1 - mov r7, sp  OR  add r7, sp, #imm  (setting of r7)
-     bit 2 - sub sp, #simm  OR  add sp, #simm  (adjusting of sp)
-  */
-  int findmask = 0;
-  int i;
 
-  if (find_pc_partial_function (prev_pc, NULL, &prologue_start, &prologue_end))
+  if (find_pc_partial_function (block_addr, NULL, &prologue_start,
+				&prologue_end))
     {
       struct symtab_and_line sal = find_pc_line (prologue_start, 0);
 
@@ -644,6 +645,7 @@ arm_scan_prologue (struct frame_info *th
   int regno;
   CORE_ADDR prologue_start, prologue_end, current_pc;
   CORE_ADDR prev_pc = get_frame_pc (this_frame);
+  CORE_ADDR block_addr = get_frame_address_in_block (this_frame);
   pv_t regs[ARM_FPS_REGNUM];
   struct pv_area *stack;
   struct cleanup *back_to;
@@ -654,15 +656,16 @@ arm_scan_prologue (struct frame_info *th
   cache->framesize = 0;
 
   /* Check for Thumb prologue.  */
-  if (arm_pc_is_thumb (prev_pc))
+  if (arm_frame_is_thumb (this_frame))
     {
-      thumb_scan_prologue (gdbarch, prev_pc, cache);
+      thumb_scan_prologue (gdbarch, prev_pc, block_addr, cache);
       return;
     }
 
   /* Find the function prologue.  If we can't find the function in
      the symbol table, peek in the stack frame to find the PC.  */
-  if (find_pc_partial_function (prev_pc, NULL, &prologue_start, &prologue_end))
+  if (find_pc_partial_function (block_addr, NULL, &prologue_start,
+				&prologue_end))
     {
       /* One way to find the end of the prologue (which works well
          for unoptimized code) is to do the following:
@@ -751,8 +754,6 @@ arm_scan_prologue (struct frame_info *th
   stack = make_pv_area (ARM_SP_REGNUM);
   back_to = make_cleanup_free_pv_area (stack);
 
-  regs[ARM_PC_REGNUM] = pv_unknown ();
-
   for (current_pc = prologue_start;
        current_pc < prologue_end;
        current_pc += 4)
@@ -985,10 +986,18 @@ arm_prologue_prev_register (struct frame
   cache = *this_cache;
 
   /* If we are asked to unwind the PC, then we need to return the LR
-     instead.  The saved value of PC points into this frame's
-     prologue, not the next frame's resume location.  */
+     instead.  The prologue may save PC, but it will point into this
+     frame's prologue, not the next frame's resume location.  Also
+     strip the saved T bit.  A valid LR may have the low bit set, but
+     a valid PC never does.  */
   if (prev_regnum == ARM_PC_REGNUM)
-    prev_regnum = ARM_LR_REGNUM;
+    {
+      CORE_ADDR lr;
+
+      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
+      return frame_unwind_got_constant (this_frame, prev_regnum,
+					arm_addr_bits_remove (lr));
+    }
 
   /* SP is generally not saved to the stack, but this frame is
      identified by the next frame's stack pointer at the time of the call.
@@ -996,6 +1005,27 @@ arm_prologue_prev_register (struct frame
   if (prev_regnum == ARM_SP_REGNUM)
     return frame_unwind_got_constant (this_frame, prev_regnum, cache->prev_sp);
 
+  /* The CPSR may have been changed by the call instruction and by the
+     called function.  The only bit we can reconstruct is the T bit,
+     by checking the low bit of LR as of the call.  This is a reliable
+     indicator of Thumb-ness except for some ARM v4T pre-interworking
+     Thumb code, which could get away with a clear low bit as long as
+     the called function did not use bx.  Guess that all other
+     bits are unchanged; the condition flags are presumably lost,
+     but the processor status is likely valid.  */
+  if (prev_regnum == ARM_PS_REGNUM)
+    {
+      CORE_ADDR lr, cpsr;
+
+      cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
+      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
+      if (IS_THUMB_ADDR (lr))
+	cpsr |= CPSR_T;
+      else
+	cpsr &= ~CPSR_T;
+      return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
+    }
+
   return trad_frame_get_prev_register (this_frame, cache->saved_regs,
 				       prev_regnum);
 }
@@ -1113,6 +1143,59 @@ arm_unwind_sp (struct gdbarch *gdbarch, 
   return frame_unwind_register_unsigned (this_frame, ARM_SP_REGNUM);
 }
 
+static struct value *
+arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
+			  int regnum)
+{
+  CORE_ADDR lr, cpsr;
+
+  switch (regnum)
+    {
+    case ARM_PC_REGNUM:
+      /* The PC is normally copied from the return column, which
+	 describes saves of LR.  However, that version may have an
+	 extra bit set to indicate Thumb state.  The bit is not
+	 part of the PC.  */
+      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
+      return frame_unwind_got_constant (this_frame, regnum,
+					arm_addr_bits_remove (lr));
+
+    case ARM_PS_REGNUM:
+      /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
+      CORE_ADDR lr, cpsr;
+
+      cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
+      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
+      if (IS_THUMB_ADDR (lr))
+	cpsr |= CPSR_T;
+      else
+	cpsr &= ~CPSR_T;
+      return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
+
+    default:
+      internal_error (__FILE__, __LINE__,
+		      _("Unexpected register %d"), regnum);
+    }
+}
+
+static void
+arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
+			   struct dwarf2_frame_state_reg *reg,
+			   struct frame_info *this_frame)
+{
+  switch (regnum)
+    {
+    case ARM_PC_REGNUM:
+    case ARM_PS_REGNUM:
+      reg->how = DWARF2_FRAME_REG_FN;
+      reg->loc.fn = arm_dwarf2_prev_register;
+      break;
+    case ARM_SP_REGNUM:
+      reg->how = DWARF2_FRAME_REG_CFA;
+      break;
+    }
+}
+
 /* When arguments must be pushed onto the stack, they go on in reverse
    order.  The code below implements a FILO (stack) to do this.  */
 
@@ -1694,7 +1777,7 @@ arm_get_next_pc (struct frame_info *fram
   unsigned long status;
   CORE_ADDR nextpc;
 
-  if (arm_pc_is_thumb (pc))
+  if (arm_frame_is_thumb (frame))
     return thumb_get_next_pc (frame, pc);
 
   pc_val = (unsigned long) pc;
@@ -2667,10 +2750,10 @@ arm_write_pc (struct regcache *regcache,
       ULONGEST val;
       regcache_cooked_read_unsigned (regcache, ARM_PS_REGNUM, &val);
       if (arm_pc_is_thumb (pc))
-	regcache_cooked_write_unsigned (regcache, ARM_PS_REGNUM, val | 0x20);
+	regcache_cooked_write_unsigned (regcache, ARM_PS_REGNUM, val | CPSR_T);
       else
 	regcache_cooked_write_unsigned (regcache, ARM_PS_REGNUM,
-					val & ~(ULONGEST) 0x20);
+					val & ~(ULONGEST) CPSR_T);
     }
 }
 
@@ -3034,6 +3117,8 @@ arm_gdbarch_init (struct gdbarch_info in
   /* Hook in the ABI-specific overrides, if they have been registered.  */
   gdbarch_init_osabi (info, gdbarch);
 
+  dwarf2_frame_set_init_reg (gdbarch, arm_dwarf2_frame_init_reg);
+
   /* Add some default predicates.  */
   frame_unwind_append_unwinder (gdbarch, &arm_stub_unwind);
   dwarf2_append_unwinders (gdbarch);
Index: src/gdb/arm-tdep.h
===================================================================
--- src.orig/gdb/arm-tdep.h	2008-01-01 17:09:44.000000000 -0500
+++ src/gdb/arm-tdep.h	2008-05-01 10:00:54.000000000 -0400
@@ -38,6 +38,7 @@ enum gdb_regnum {
   ARM_F7_REGNUM = 23, 		/* last floating point register */
   ARM_FPS_REGNUM = 24,		/* floating point status register */
   ARM_PS_REGNUM = 25,		/* Contains processor status */
+  ARM_CPSR_REGNUM = ARM_PS_REGNUM,
   ARM_WR0_REGNUM,		/* WMMX data registers.  */
   ARM_WR15_REGNUM = ARM_WR0_REGNUM + 15,
   ARM_WC0_REGNUM,		/* WMMX control registers.  */
@@ -107,6 +108,8 @@ enum gdb_regnum {
 #define FLAG_C		0x20000000
 #define FLAG_V		0x10000000
 
+#define CPSR_T		0x20
+
 /* Type of floating-point code in use by inferior.  There are really 3 models
    that are traditionally supported (plus the endianness issue), but gcc can
    only generate 2 of those.  The third is APCS_FLOAT, where arguments to
Index: src/gdb/dwarf2-frame.c
===================================================================
--- src.orig/gdb/dwarf2-frame.c	2008-04-30 17:17:53.000000000 -0400
+++ src/gdb/dwarf2-frame.c	2008-05-01 10:22:48.000000000 -0400
@@ -1139,6 +1139,9 @@ dwarf2_frame_prev_register (struct frame
       addr += get_frame_register_unsigned (this_frame, regnum);
       return frame_unwind_got_address (this_frame, regnum, addr);
 
+    case DWARF2_FRAME_REG_FN:
+      return cache->reg[regnum].loc.fn (this_frame, this_cache, regnum);
+
     default:
       internal_error (__FILE__, __LINE__, _("Unknown register rule."));
     }
Index: src/gdb/dwarf2-frame.h
===================================================================
--- src.orig/gdb/dwarf2-frame.h	2008-04-30 17:17:53.000000000 -0400
+++ src/gdb/dwarf2-frame.h	2008-05-01 10:22:05.000000000 -0400
@@ -55,6 +55,7 @@ enum dwarf2_frame_reg_rule
 
   /* These aren't defined by the DWARF2 CFI specification, but are
      used internally by GDB.  */
+  DWARF2_FRAME_REG_FN,		/* Call a registered function.  */
   DWARF2_FRAME_REG_RA,		/* Return Address.  */
   DWARF2_FRAME_REG_RA_OFFSET,	/* Return Address with offset.  */
   DWARF2_FRAME_REG_CFA,		/* Call Frame Address.  */
@@ -71,6 +72,8 @@ struct dwarf2_frame_state_reg
     LONGEST offset;
     ULONGEST reg;
     unsigned char *exp;
+    struct value *(*fn) (struct frame_info *this_frame, void **this_cache,
+			 int regnum);
   } loc;
   ULONGEST exp_len;
   enum dwarf2_frame_reg_rule how;
Index: src/gdb/testsuite/gdb.arch/thumb-prologue.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.arch/thumb-prologue.exp	2008-01-01 17:10:10.000000000 -0500
+++ src/gdb/testsuite/gdb.arch/thumb-prologue.exp	2008-05-01 10:00:54.000000000 -0400
@@ -57,5 +57,5 @@ gdb_test "backtrace 10" \
 	"backtrace in TPCS"
 
 gdb_test "info frame" \
-	".*Saved registers:.*r7 at.*r10 at.*r11 at.*lr at.*pc at .*" \
+	".*Saved registers:.*r7 at.*r10 at.*r11 at.*lr at.*" \
 	"saved registers in TPCS"

Re: [rfc] Unwind the ARM CPSR

[Daniel Jacobowitz]

On Thu, May 01, 2008 at 02:30:48PM -0400, Daniel Jacobowitz wrote:
> That place is now gone.  I've tested this version on arm-none-eabi
> (where it fixes one failure with -mthumb, gdb.base/ending-run.exp:
> step to end of run) and on arm-none-symbianelf.
> 
> Checked in.

Correction to that patch.  Sorry, this got tangled up in the other
patch I'm testing.

-- 
Daniel Jacobowitz
CodeSourcery

Index: src/gdb/arm-tdep.c
===================================================================
--- src.orig/gdb/arm-tdep.c	2008-05-01 13:56:01.000000000 -0400
+++ src/gdb/arm-tdep.c	2008-05-01 13:56:33.000000000 -0400
@@ -1162,15 +1190,13 @@ arm_dwarf2_prev_register (struct frame_i
 
     case ARM_PS_REGNUM:
       /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
-      CORE_ADDR lr, cpsr;
-
-      cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
+      cpsr = get_frame_register_unsigned (this_frame, regnum);
       lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
       if (IS_THUMB_ADDR (lr))
 	cpsr |= CPSR_T;
       else
 	cpsr &= ~CPSR_T;
-      return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
+      return frame_unwind_got_constant (this_frame, regnum, cpsr);
 
     default:
       internal_error (__FILE__, __LINE__,