Re: Cortex-M CPSR thumb bit fix

[Jonathan Larmour <jifl@eCosCentric.com>]

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

On 08/07/10 22:37, Daniel Jacobowitz wrote:
> On Thu, Jul 08, 2010 at 08:26:15PM +0100, Jonathan Larmour wrote:
>> I posted a patch in 2008 here:
>> http://sourceware.org/ml/gdb-patches/2008-10/msg00462.html but
>> unfortunately it wasn't applied. Without it, GDB cannot debug Cortex-M
>> processors as the thumb bit is in a different place in the CPSR register.
>>
>> I have updated it for GDB 7.1, as per the attached mail and updated
>> ChangeLog below.
> 
> Please compare with:
> 
>   http://sourceware.org/ml/gdb-patches/2010-06/msg00229.html
> 
> Kazu has been busy with another project, but I think he'll be back to
> that patch pretty soon.  What I'd like, ideally, is a combination of his
> patch with yours; you have the bits to auto-probe from the binary, and
> he has the ones that let the target specify.  Both are useful.

Hmm, that leads to the question: what if one says it's M and the other
says it's something else? Personally I'd always go with what the actual
binary being debugged says. Or at least that should override the XML
definition. Although in that case the only benefit I can think of for the
XML at all is connecting to a target when you don't have a binary to
debug, which is rather a limited benefit.

> I see that since I wrote the first version of this patch, we've
> started testing CPSR_T in more places :-(  Thanks for finding the
> extras.

Changing it to a macro function has the benefit of giving obvious errors
on compilation :).

> I would prefer not to call it the "M-profile CPSR"; it's not called
> CPSR in the architecture documentation.  It's the XPSR. [snip]
> ARM v7-R has a CPSR, just like ARM v7-A and other non-microcontroller
> profile devices. [snip]
> I didn't look too closely at the code, except to observe that you
> didn't follow the GNU coding standards [snip]

I've attached an updated patch based on your comments.

> Jonathan, Kazu, anyone have a preference on producing a combined patch?

I haven't mucked around with the XML stuff before, so if you do want a
combined patch, perhaps Kazu could look, particularly as there were also
unresolved comments on his patch, and none of the XML side was posted.
Hopefully with my patch fixed with your comments, then I'm reducing the
effort to combine at least.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

[-- Attachment #2: arm.patch --]
[-- Type: text/plain, Size: 7365 bytes --]

--- gdb-7.1.clean/gdb/arm-tdep.c	2010-02-26 20:45:33.000000000 +0000
+++ gdb-7.1/gdb/arm-tdep.c	2010-07-08 23:18:49.000000000 +0100
@@ -256,6 +256,7 @@ static int
 arm_frame_is_thumb (struct frame_info *frame)
 {
   CORE_ADDR cpsr;
+  struct gdbarch_tdep *tdep = gdbarch_tdep (get_frame_arch (frame));
 
   /* Every ARM frame unwinder can unwind the T bit of the CPSR, either
      directly (from a signal frame or dummy frame) or by interpreting
@@ -263,7 +264,7 @@ arm_frame_is_thumb (struct frame_info *f
      trust the unwinders.  */
   cpsr = get_frame_register_unsigned (frame, ARM_PS_REGNUM);
 
-  return (cpsr & CPSR_T) != 0;
+  return (cpsr & CPSR_T (tdep)) != 0;
 }
 
 /* Callback for VEC_lower_bound.  */
@@ -1113,6 +1114,7 @@ arm_prologue_prev_register (struct frame
 			    int prev_regnum)
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   struct arm_prologue_cache *cache;
 
   if (*this_cache == NULL)
@@ -1154,9 +1156,9 @@ arm_prologue_prev_register (struct frame
       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;
+	cpsr |= CPSR_T (tdep);
       else
-	cpsr &= ~CPSR_T;
+	cpsr &= ~CPSR_T (tdep);
       return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
     }
 
@@ -1283,6 +1285,7 @@ arm_dwarf2_prev_register (struct frame_i
 {
   struct gdbarch * gdbarch = get_frame_arch (this_frame);
   CORE_ADDR lr, cpsr;
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
   switch (regnum)
     {
@@ -1300,9 +1303,9 @@ arm_dwarf2_prev_register (struct frame_i
       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;
+	cpsr |= CPSR_T (tdep);
       else
-	cpsr &= ~CPSR_T;
+	cpsr &= ~CPSR_T (tdep);
       return frame_unwind_got_constant (this_frame, regnum, cpsr);
 
     default:
@@ -3185,11 +3188,12 @@ displaced_read_reg (struct regcache *reg
 static int
 displaced_in_arm_mode (struct regcache *regs)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (get_regcache_arch (regs));
   ULONGEST ps;
 
   regcache_cooked_read_unsigned (regs, ARM_PS_REGNUM, &ps);
 
-  return (ps & CPSR_T) == 0;
+  return (ps & CPSR_T (tdep)) == 0;
 }
 
 /* Write to the PC as from a branch instruction.  */
@@ -3210,19 +3214,20 @@ branch_write_pc (struct regcache *regs, 
 static void
 bx_write_pc (struct regcache *regs, ULONGEST val)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (get_regcache_arch (regs));
   ULONGEST ps;
 
   regcache_cooked_read_unsigned (regs, ARM_PS_REGNUM, &ps);
 
   if ((val & 1) == 1)
     {
-      regcache_cooked_write_unsigned (regs, ARM_PS_REGNUM, ps | CPSR_T);
+      regcache_cooked_write_unsigned (regs, ARM_PS_REGNUM, ps | CPSR_T (tdep));
       regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM, val & 0xfffffffe);
     }
   else if ((val & 2) == 0)
     {
       regcache_cooked_write_unsigned (regs, ARM_PS_REGNUM,
-				      ps & ~(ULONGEST) CPSR_T);
+				      ps & ~(ULONGEST) CPSR_T (tdep));
       regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM, val);
     }
   else
@@ -3231,7 +3236,7 @@ bx_write_pc (struct regcache *regs, ULON
 	  mode, align dest to 4 bytes).  */
       warning (_("Single-stepping BX to non-word-aligned ARM instruction."));
       regcache_cooked_write_unsigned (regs, ARM_PS_REGNUM,
-				      ps & ~(ULONGEST) CPSR_T);
+				      ps & ~(ULONGEST) CPSR_T (tdep));
       regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM, val & 0xfffffffc);
     }
 }
@@ -5862,6 +5867,8 @@ arm_record_special_symbol (struct gdbarc
 static void
 arm_write_pc (struct regcache *regcache, CORE_ADDR pc)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (get_regcache_arch (regcache));
+
   regcache_cooked_write_unsigned (regcache, ARM_PC_REGNUM, pc);
 
   /* If necessary, set the T bit.  */
@@ -5870,10 +5877,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 | CPSR_T);
+	regcache_cooked_write_unsigned (regcache, ARM_PS_REGNUM, val | CPSR_T (tdep));
       else
 	regcache_cooked_write_unsigned (regcache, ARM_PS_REGNUM,
-					val & ~(ULONGEST) CPSR_T);
+					val & ~(ULONGEST) CPSR_T (tdep));
     }
 }
 
@@ -6058,6 +6065,8 @@ arm_gdbarch_init (struct gdbarch_info in
   int have_vfp_registers = 0, have_vfp_pseudos = 0, have_neon_pseudos = 0;
   int have_neon = 0;
   int have_fpa_registers = 1;
+  int arm_eabi_cpu_arch = -1;
+  int arm_eabi_cpu_arch_profile = -1;
 
   /* Check any target description for validity.  */
   if (tdesc_has_registers (info.target_desc))
@@ -6248,6 +6257,7 @@ arm_gdbarch_init (struct gdbarch_info in
 	  else if (ei_osabi == ELFOSABI_NONE)
 	    {
 	      int eabi_ver = EF_ARM_EABI_VERSION (e_flags);
+	      int attr;
 
 	      switch (eabi_ver)
 		{
@@ -6305,8 +6315,23 @@ arm_gdbarch_init (struct gdbarch_info in
 		  warning (_("unknown ARM EABI version 0x%x"), eabi_ver);
 		  break;
 		}
-	    }
 
+		/* Cortex-M uses the XPSR instead of the CPSR, which has a different
+		   layout. Use object tags to determine if this was generated for
+		   the Microcontroller (M) profile.
+		   Cortex-A and Cortex-R have a CPSR like previous ARM cores.  */
+
+		attr = bfd_elf_get_obj_attr_int(info.abfd, OBJ_ATTR_PROC, Tag_CPU_arch);
+		if (attr > 0)
+		  arm_eabi_cpu_arch = attr;
+		if (arm_eabi_cpu_arch >= TAG_CPU_ARCH_V7) /* Profile n/a before V7 */
+		  {
+		    attr = bfd_elf_get_obj_attr_int(info.abfd, OBJ_ATTR_PROC, Tag_CPU_arch_profile);
+		    if (attr > 0)
+		        arm_eabi_cpu_arch_profile = attr;
+		  }
+
+	    }
 	  if (fp_model == ARM_FLOAT_AUTO)
 	    {
 	      int e_flags = elf_elfheader (info.abfd)->e_flags;
@@ -6383,6 +6408,8 @@ arm_gdbarch_init (struct gdbarch_info in
   tdep->have_vfp_pseudos = have_vfp_pseudos;
   tdep->have_neon_pseudos = have_neon_pseudos;
   tdep->have_neon = have_neon;
+  tdep->arm_eabi_cpu_arch = arm_eabi_cpu_arch;
+  tdep->arm_eabi_cpu_arch_profile = arm_eabi_cpu_arch_profile;
 
   /* Breakpoints.  */
   switch (info.byte_order_for_code)
--- gdb-7.1.clean/gdb/arm-tdep.h	2010-02-01 16:13:15.000000000 +0000
+++ gdb-7.1/gdb/arm-tdep.h	2010-07-08 23:19:36.000000000 +0100
@@ -105,7 +105,10 @@ enum gdb_regnum {
 #define FLAG_C		0x20000000
 #define FLAG_V		0x10000000
 
-#define CPSR_T		0x20
+/* On most ARM architectures, the T-bit resides in the CPSR. But the ARM
+   Architecture v7+ Microcontroller Profile has the T-bit in a different
+   offset in the XPSR.  */
+#define CPSR_T(tdep)	((tdep)->arm_eabi_cpu_arch_profile==(int)'M' ? 0x01000000 : 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
@@ -192,6 +195,9 @@ struct gdbarch_tdep
   struct type *arm_ext_type;
   struct type *neon_double_type;
   struct type *neon_quad_type;
+
+  int arm_eabi_cpu_arch;         /* Arch version from EABI attribute (-1 == unknown) */
+  int arm_eabi_cpu_arch_profile; /* Arch profile from EABI attribute (-1 == unknown) */
 };
 
 /* Structures used for displaced stepping.  */