[rfa] ARM prologue parsing support for Thumb-2 instructions

[rfa] ARM prologue parsing support for Thumb-2 instructions

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Mon, Jul 19, 2010 at 12:59:05PM +0200, Ulrich Weigand wrote:
> > > Trunk thumb_analyze_prologue does have support for Thumb-2.
> > 
> > Well, all the support for Thumb-2 I can see is in this block:
> > 
> >       else if ((insn & 0xe000) == 0xe000 && cache == NULL)
> >         {
> >           /* Only recognize 32-bit instructions for prologue skipping.  */
> > 
> > which, as the comment says, is active *only* if this routine is
> > called from arm_skip_prologue (with cache == NULL), but not if the
> > routine is called from arm_scan_prologue (with cache != NULL),
> > which is what is used during unwinding.
> 
> IIRC, it would not be hard to fill in the missing pieces; I just
> didn't need them at the time, and could not easily test them.  So
> rather than risk them being wrong, I left them for later.

I've finally gotten around to finish my implementation of this.

The main part of this patch is support in thumb_analyze_prologue for
various Thumb-2 instructions.  In addition, the patch improves support
for optimized code by skipping unrecognized instructions instead of
aborting the prologue scan, and by not relying on line number data to
determine an upper bound for the end of the prologue.  (Both these
changes are already in place for the ARM decoder; the patch simply
brings them over to the Thumb decode as well.)

Finally, the patch adds support for a couple of more function calls
that may happen within the prologue: __tls_get_addr and __aeabi_read_tp.

Tested on armv7l-linux-gnueabi with no regressions.

In addition, I've run a test with the following line disabled:
  dwarf2_append_unwinders (gdbarch);
This causes GDB to ignore DWARF CFI even if present, and always
fall back to prologue parsing.  With GDB before this patch, that
test introduces a large number of extra regressions.  *With* this
patch, nearly all of those regressions are fixed again.  The
following failures still remain:

FAIL: gdb.base/call-signal-resume.exp: backtrace
FAIL: gdb.base/call-signal-resume.exp: dummy stack frame number
FAIL: gdb.base/call-signal-resume.exp: continue to breakpoint at stop_two
FAIL: gdb.base/call-signal-resume.exp: continue to receipt of signal
FAIL: gdb.base/call-signal-resume.exp: continue to breakpoint at handle_signal
FAIL: gdb.base/call-signal-resume.exp: continue to program exit

FAIL: gdb.base/foll-vfork.exp: (timeout) vfork parent follow, finish after tcatch vfork
FAIL: gdb.base/foll-vfork.exp: vfork child follow, finish after tcatch vfork

FAIL: gdb.threads/attach-stopped.exp: threaded: attach2 to stopped bt
FAIL: gdb.threads/attachstop-mt.exp: attach0 to sleeping bt
FAIL: gdb.threads/attachstop-mt.exp: attach3 to stopped bt
FAIL: gdb.threads/attachstop-mt.exp: attach4 to stopped bt
FAIL: gdb.threads/pthreads.exp: check backtrace from thread 1
FAIL: gdb.threads/pthreads.exp: check backtrace from thread 2
FAIL: gdb.threads/pthreads.exp: apply backtrace command to all three threads

All these are caused by failures to parse the prologues of hand-written
*ARM* assembler routines in glibc (system call handlers like nanosleep).
These simply deviate too far from the usual rules (e.g. by intermixing
stack saving/restoring of registers with conditional branches, or by
temporarily saving registers into other registers instead of the stack)
for the ARM prologue parser to be able to handle them.

It seems to me that there is not much sense in attempting to support
even this type of code.  I guess we should strongly recommend to have
(at least) glibc debuginfo files installed if you want to debug.

OK for mainline?

Bye,
Ulrich

ChangeLog:

	* arm-tdep.c (thumb_expand_immediate): New function.
	(thumb_instruction_changes_pc): Likewise.
	(thumb2_instruction_changes_pc): Likewise.
	(thumb_analyze_prologue): Handle 32-bit Thumb instructions during
	prologue parsing.  Improved support for optimized code.
	(thumb_scan_prologue): Do not reply on line-number information,
	use same heuristics as arm_scan_prologue insead.
	(skip_prologue_function): Accept functions
	"__tls_get_addr" and "__aeabi_read_tp".

Index: gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.307
diff -u -p -r1.307 arm-tdep.c
--- gdb/arm-tdep.c	30 Aug 2010 15:26:28 -0000	1.307
+++ gdb/arm-tdep.c	7 Oct 2010 22:58:27 -0000
@@ -476,6 +476,12 @@ skip_prologue_function (CORE_ADDR pc)
   if (strncmp (name, "__aeabi_d2f", strlen ("__aeabi_d2f")) == 0)
     return 1;
 
+  /* Internal functions related to thread-local storage.  */
+  if (strncmp (name, "__tls_get_addr", strlen ("__tls_get_addr")) == 0)
+    return 1;
+  if (strncmp (name, "__aeabi_read_tp", strlen ("__aeabi_read_tp")) == 0)
+    return 1;
+
   return 0;
 }
 
@@ -488,6 +494,149 @@ skip_prologue_function (CORE_ADDR pc)
 #define BranchDest(addr,instr) \
   ((CORE_ADDR) (((long) (addr)) + 8 + (sbits (instr, 0, 23) << 2)))
 
+/* Decode immediate value; implements ThumbExpandImmediate pseudo-op.  */
+
+static unsigned int
+thumb_expand_immediate (unsigned int imm)
+{
+  unsigned int count = imm >> 7;
+
+  if (count < 8)
+    switch (count / 2)
+      {
+      case 0:
+	return imm & 0xff;
+      case 1:
+	return (imm & 0xff) | ((imm & 0xff) << 16);
+      case 2:
+	return ((imm & 0xff) << 8) | ((imm & 0xff) << 24);
+      case 3:
+	return (imm & 0xff) | ((imm & 0xff) << 8)
+		| ((imm & 0xff) << 16) | ((imm & 0xff) << 24);
+      }
+
+  return (0x80 | (imm & 0x7f)) << (32 - count);
+}
+
+/* Return 1 if the 16-bit Thumb instruction INST might change
+   control flow, 0 otherwise.  */
+
+static int
+thumb_instruction_changes_pc (unsigned short inst)
+{
+  if ((inst & 0xff00) == 0xbd00)	/* pop {rlist, pc} */
+    return 1;
+
+  if ((inst & 0xf000) == 0xd000)	/* conditional branch */
+    return 1;
+
+  if ((inst & 0xf800) == 0xe000)	/* unconditional branch */
+    return 1;
+
+  if ((inst & 0xff00) == 0x4700)	/* bx REG, blx REG */
+    return 1;
+
+  if ((inst & 0xf500) == 0xb100)	/* CBNZ or CBZ.  */
+    return 1;
+
+  return 0;
+}
+
+/* Return 1 if the 32-bit Thumb instruction in INST1 and INST2
+   might change control flow, 0 otherwise.  */
+
+static int
+thumb2_instruction_changes_pc (unsigned short inst1, unsigned short inst2)
+{
+  if ((inst1 & 0xf800) == 0xf000 && (inst2 & 0x8000) == 0x8000)
+    {
+      /* Branches and miscellaneous control instructions.  */
+
+      if ((inst2 & 0x1000) != 0 || (inst2 & 0xd001) == 0xc000)
+	{
+	  /* B, BL, BLX.  */
+	  return 1;
+	}
+      else if (inst1 == 0xf3de && (inst2 & 0xff00) == 0x3f00)
+	{
+	  /* SUBS PC, LR, #imm8.  */
+	  return 1;
+	}
+      else if ((inst2 & 0xd000) == 0x8000 && (inst1 & 0x0380) != 0x0380)
+	{
+	  /* Conditional branch.  */
+	  return 1;
+	}
+
+      return 0;
+    }
+
+  if ((inst1 & 0xfe50) == 0xe810)
+    {
+      /* Load multiple or RFE.  */
+
+      if (bit (inst1, 7) && !bit (inst1, 8))
+	{
+	  /* LDMIA or POP */
+	  if (bit (inst2, 15))
+	    return 1;
+	}
+      else if (!bit (inst1, 7) && bit (inst1, 8))
+	{
+	  /* LDMDB */
+	  if (bit (inst2, 15))
+	    return 1;
+	}
+      else if (bit (inst1, 7) && bit (inst1, 8))
+	{
+	  /* RFEIA */
+	  return 1;
+	}
+      else if (!bit (inst1, 7) && !bit (inst1, 8))
+	{
+	  /* RFEDB */
+	  return 1;
+	}
+
+      return 0;
+    }
+
+  if ((inst1 & 0xffef) == 0xea4f && (inst2 & 0xfff0) == 0x0f00)
+    {
+      /* MOV PC or MOVS PC.  */
+      return 1;
+    }
+
+  if ((inst1 & 0xff70) == 0xf850 && (inst2 & 0xf000) == 0xf000)
+    {
+      /* LDR PC.  */
+      if (bits (inst1, 0, 3) == 15)
+	return 1;
+      if (bit (inst1, 7))
+	return 1;
+      if (bit (inst2, 11))
+	return 1;
+      if ((inst2 & 0x0fc0) == 0x0000)
+	return 1;	
+
+      return 0;
+    }
+
+  if ((inst1 & 0xfff0) == 0xe8d0 && (inst2 & 0xfff0) == 0xf000)
+    {
+      /* TBB.  */
+      return 1;
+    }
+
+  if ((inst1 & 0xfff0) == 0xe8d0 && (inst2 & 0xfff0) == 0xf010)
+    {
+      /* TBH.  */
+      return 1;
+    }
+
+  return 0;
+}
+
 /* Analyze a Thumb prologue, looking for a recognizable stack frame
    and frame pointer.  Scan until we encounter a store that could
    clobber the stack frame unexpectedly, or an unknown instruction.
@@ -506,6 +655,7 @@ thumb_analyze_prologue (struct gdbarch *
   struct pv_area *stack;
   struct cleanup *back_to;
   CORE_ADDR offset;
+  CORE_ADDR unrecognized_pc = 0;
 
   for (i = 0; i < 16; i++)
     regs[i] = pv_register (i, 0);
@@ -643,9 +793,8 @@ thumb_analyze_prologue (struct gdbarch *
 	  constant = read_memory_unsigned_integer (loc, 4, byte_order);
 	  regs[bits (insn, 8, 10)] = pv_constant (constant);
 	}
-      else if ((insn & 0xe000) == 0xe000 && cache == NULL)
+      else if ((insn & 0xe000) == 0xe000)
 	{
-	  /* Only recognize 32-bit instructions for prologue skipping.  */
 	  unsigned short inst2;
 
 	  inst2 = read_memory_unsigned_integer (start + 2, 2,
@@ -675,59 +824,257 @@ thumb_analyze_prologue (struct gdbarch *
 	      if (!skip_prologue_function (nextpc))
 		break;
 	    }
-	  else if ((insn & 0xfe50) == 0xe800	/* stm{db,ia} Rn[!], { registers } */
+
+	  else if ((insn & 0xffd0) == 0xe900    /* stmdb Rn{!}, { registers } */
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    {
+	      pv_t addr = regs[bits (insn, 0, 3)];
+	      int regno;
+
+	      if (pv_area_store_would_trash (stack, addr))
+		break;
+
+	      /* Calculate offsets of saved registers.  */
+	      for (regno = ARM_LR_REGNUM; regno >= 0; regno--)
+		if (inst2 & (1 << regno))
+		  {
+		    addr = pv_add_constant (addr, -4);
+		    pv_area_store (stack, addr, 4, regs[regno]);
+		  }
+
+	      if (insn & 0x0020)
+		regs[bits (insn, 0, 3)] = addr;
+	    }
+
+	  else if ((insn & 0xff50) == 0xe940	/* strd Rt, Rt2, [Rn, #+/-imm]{!} */
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    {
+	      int regno1 = bits (inst2, 12, 15);
+	      int regno2 = bits (inst2, 8, 11);
+	      pv_t addr = regs[bits (insn, 0, 3)];
+
+	      offset = inst2 & 0xff;
+	      if (insn & 0x0080)
+		addr = pv_add_constant (addr, offset);
+	      else
+		addr = pv_add_constant (addr, -offset);
+
+	      if (pv_area_store_would_trash (stack, addr))
+		break;
+
+	      pv_area_store (stack, addr, 4, regs[regno1]);
+	      pv_area_store (stack, pv_add_constant (addr, 4),
+			     4, regs[regno2]);
+
+	      if (insn & 0x0020)
+		regs[bits (insn, 0, 3)] = addr;
+	    }
+
+	  else if ((insn & 0xfff0) == 0xf8c0	/* str Rt,[Rn,+/-#imm]{!} */
+		   && (inst2 & 0x0c00) == 0x0c00
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    {
+	      int regno = bits (inst2, 12, 15);
+	      pv_t addr = regs[bits (insn, 0, 3)];
+
+	      offset = inst2 & 0xff;
+	      if (inst2 & 0x0200)
+		addr = pv_add_constant (addr, offset);
+	      else
+		addr = pv_add_constant (addr, -offset);
+
+	      if (pv_area_store_would_trash (stack, addr))
+		break;
+
+	      pv_area_store (stack, addr, 4, regs[regno]);
+
+	      if (inst2 & 0x0100)
+		regs[bits (insn, 0, 3)] = addr;
+	    }
+
+	  else if ((insn & 0xfff0) == 0xf8c0	/* str.w Rt,[Rn,#imm] */
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    {
+	      int regno = bits (inst2, 12, 15);
+	      pv_t addr;
+
+	      offset = inst2 & 0xfff;
+	      addr = pv_add_constant (regs[bits (insn, 0, 3)], offset);
+
+	      if (pv_area_store_would_trash (stack, addr))
+		break;
+
+	      pv_area_store (stack, addr, 4, regs[regno]);
+	    }
+
+	  else if ((insn & 0xffd0) == 0xf880	/* str{bh}.w Rt,[Rn,#imm] */
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    /* Ignore stores of argument registers to the stack.  */
 	    ;
-	  else if ((insn & 0xfe50) == 0xe840	/* strd Rt, Rt2, [Rn, #imm] */
+
+	  else if ((insn & 0xffd0) == 0xf800	/* str{bh} Rt,[Rn,#+/-imm] */
+		   && (inst2 & 0x0d00) == 0x0c00
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    /* Ignore stores of argument registers to the stack.  */
 	    ;
+
 	  else if ((insn & 0xffd0) == 0xe890	/* ldmia Rn[!], { registers } */
-	      && (inst2 & 0x8000) == 0x0000
-	      && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+		   && (inst2 & 0x8000) == 0x0000
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    /* Ignore block loads from the stack, potentially copying
+	       parameters from memory.  */
 	    ;
-	  else if ((insn & 0xfbf0) == 0xf100	/* add.w Rd, Rn, #imm */
-		   && (inst2 & 0x8000) == 0x0000)
-	    /* Since we only recognize this for prologue skipping, do not bother
-	       to compute the constant.  */
-	    regs[bits (inst2, 8, 11)] = regs[bits (insn, 0, 3)];
-	  else if ((insn & 0xfbf0) == 0xf1a0	/* sub.w Rd, Rn, #imm12 */
-		   && (inst2 & 0x8000) == 0x0000)
-	    /* Since we only recognize this for prologue skipping, do not bother
-	       to compute the constant.  */
-	    regs[bits (inst2, 8, 11)] = regs[bits (insn, 0, 3)];
-	  else if ((insn & 0xfbf0) == 0xf2a0	/* sub.w Rd, Rn, #imm8 */
-		   && (inst2 & 0x8000) == 0x0000)
-	    /* Since we only recognize this for prologue skipping, do not bother
-	       to compute the constant.  */
-	    regs[bits (inst2, 8, 11)] = regs[bits (insn, 0, 3)];
-	  else if ((insn & 0xff50) == 0xf850	/* ldr.w Rd, [Rn, #imm]{!} */
+
+	  else if ((insn & 0xffb0) == 0xe950	/* ldrd Rt, Rt2, [Rn, #+/-imm] */
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    /* Similarly ignore dual loads from the stack.  */
 	    ;
-	  else if ((insn & 0xff50) == 0xe950	/* ldrd Rt, Rt2, [Rn, #imm]{!} */
+
+	  else if ((insn & 0xfff0) == 0xf850	/* ldr Rt,[Rn,#+/-imm] */
+		   && (inst2 & 0x0d00) == 0x0c00
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    /* Similarly ignore single loads from the stack.  */
 	    ;
-	  else if ((insn & 0xff50) == 0xf800	/* strb.w or strh.w */
+
+	  else if ((insn & 0xfff0) == 0xf8d0	/* ldr.w Rt,[Rn,#imm] */
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    /* Similarly ignore single loads from the stack.  */
 	    ;
-	  else
+
+	  else if ((insn & 0xfbf0) == 0xf100	/* add.w Rd, Rn, #imm */
+		   && (inst2 & 0x8000) == 0x0000)
+	    {
+	      unsigned int imm = ((bits (insn, 10, 10) << 11)
+				  | (bits (inst2, 12, 14) << 8)
+				  | bits (inst2, 0, 7));
+
+	      regs[bits (inst2, 8, 11)]
+		= pv_add_constant (regs[bits (insn, 0, 3)],
+				   thumb_expand_immediate (imm));
+	    }
+
+	  else if ((insn & 0xfbf0) == 0xf200	/* addw Rd, Rn, #imm */
+		   && (inst2 & 0x8000) == 0x0000)
+	    {
+	      unsigned int imm = ((bits (insn, 10, 10) << 11)
+				  | (bits (inst2, 12, 14) << 8)
+				  | bits (inst2, 0, 7));
+
+	      regs[bits (inst2, 8, 11)]
+		= pv_add_constant (regs[bits (insn, 0, 3)], imm);
+	    }
+
+	  else if ((insn & 0xfbf0) == 0xf1a0	/* sub.w Rd, Rn, #imm */
+		   && (inst2 & 0x8000) == 0x0000)
+	    {
+	      unsigned int imm = ((bits (insn, 10, 10) << 11)
+				  | (bits (inst2, 12, 14) << 8)
+				  | bits (inst2, 0, 7));
+
+	      regs[bits (inst2, 8, 11)]
+		= pv_add_constant (regs[bits (insn, 0, 3)],
+				   - (CORE_ADDR) thumb_expand_immediate (imm));
+	    }
+
+	  else if ((insn & 0xfbf0) == 0xf2a0	/* subw Rd, Rn, #imm */
+		   && (inst2 & 0x8000) == 0x0000)
+	    {
+	      unsigned int imm = ((bits (insn, 10, 10) << 11)
+				  | (bits (inst2, 12, 14) << 8)
+				  | bits (inst2, 0, 7));
+
+	      regs[bits (inst2, 8, 11)]
+		= pv_add_constant (regs[bits (insn, 0, 3)], - (CORE_ADDR) imm);
+	    }
+
+	  else if ((insn & 0xfbff) == 0xf04f)	/* mov.w Rd, #const */
+	    {
+	      unsigned int imm = ((bits (insn, 10, 10) << 11)
+				  | (bits (inst2, 12, 14) << 8)
+				  | bits (inst2, 0, 7));
+
+	      regs[bits (inst2, 8, 11)]
+		= pv_constant (thumb_expand_immediate (imm));
+	    }
+
+	  else if ((insn & 0xfbf0) == 0xf240)	/* movw Rd, #const */
+	    {
+	      unsigned int imm = ((bits (insn, 0, 3) << 12)
+				  | (bits (insn, 10, 10) << 11)
+				  | (bits (inst2, 12, 14) << 8)
+				  | bits (inst2, 0, 7));
+
+	      regs[bits (inst2, 8, 11)] = pv_constant (imm);
+	    }
+
+	  else if (insn == 0xea5f		/* mov.w Rd,Rm */
+		   && (inst2 & 0xf0f0) == 0)
+	    {
+	      int dst_reg = (inst2 & 0x0f00) >> 8;
+	      int src_reg = inst2 & 0xf;
+	      regs[dst_reg] = regs[src_reg];
+	    }
+
+	  else if ((insn & 0xff7f) == 0xf85f)	/* ldr.w Rt,<label> */
+	    {
+	      /* Constant pool loads.  */
+	      unsigned int constant;
+	      CORE_ADDR loc;
+
+	      offset = bits (insn, 0, 11);
+	      if (insn & 0x0080)
+		loc = start + 4 + offset;
+	      else
+		loc = start + 4 - offset;
+
+	      constant = read_memory_unsigned_integer (loc, 4, byte_order);
+	      regs[bits (inst2, 12, 15)] = pv_constant (constant);
+	    }
+
+	  else if ((insn & 0xff7f) == 0xe95f)	/* ldrd Rt,Rt2,<label> */
+	    {
+	      /* Constant pool loads.  */
+	      unsigned int constant;
+	      CORE_ADDR loc;
+
+	      offset = bits (insn, 0, 7) << 2;
+	      if (insn & 0x0080)
+		loc = start + 4 + offset;
+	      else
+		loc = start + 4 - offset;
+
+	      constant = read_memory_unsigned_integer (loc, 4, byte_order);
+	      regs[bits (inst2, 12, 15)] = pv_constant (constant);
+
+	      constant = read_memory_unsigned_integer (loc + 4, 4, byte_order);
+	      regs[bits (inst2, 8, 11)] = pv_constant (constant);
+	    }
+
+	  else if (thumb2_instruction_changes_pc (insn, inst2))
 	    {
-	      /* We don't know what this instruction is.  We're finished
-		 scanning.  NOTE: Recognizing more safe-to-ignore
-		 instructions here will improve support for optimized
-		 code.  */
+	      /* Don't scan past anything that might change control flow.  */
 	      break;
 	    }
+	  else
+	    {
+	      /* The optimizer might shove anything into the prologue,
+		 so we just skip what we don't recognize.  */
+	      unrecognized_pc = start;
+	    }
 
 	  start += 2;
 	}
-      else
+      else if (thumb_instruction_changes_pc (insn))
 	{
-	  /* We don't know what this instruction is.  We're finished
-	     scanning.  NOTE: Recognizing more safe-to-ignore
-	     instructions here will improve support for optimized
-	     code.  */
+	  /* Don't scan past anything that might change control flow.  */
 	  break;
 	}
+      else
+	{
+	  /* The optimizer might shove anything into the prologue,
+	     so we just skip what we don't recognize.  */
+	  unrecognized_pc = start;
+	}
 
       start += 2;
     }
@@ -736,10 +1083,13 @@ thumb_analyze_prologue (struct gdbarch *
     fprintf_unfiltered (gdb_stdlog, "Prologue scan stopped at %s\n",
 			paddress (gdbarch, start));
 
+  if (unrecognized_pc == 0)
+    unrecognized_pc = start;
+
   if (cache == NULL)
     {
       do_cleanups (back_to);
-      return start;
+      return unrecognized_pc;
     }
 
   if (pv_is_register (regs[ARM_FP_REGNUM], ARM_SP_REGNUM))
@@ -772,7 +1122,7 @@ thumb_analyze_prologue (struct gdbarch *
       cache->saved_regs[i].addr = offset;
 
   do_cleanups (back_to);
-  return start;
+  return unrecognized_pc;
 }
 
 /* Advance the PC across any function entry prologue instructions to
@@ -956,12 +1306,12 @@ thumb_scan_prologue (struct gdbarch *gdb
   if (find_pc_partial_function (block_addr, NULL, &prologue_start,
 				&prologue_end))
     {
-      struct symtab_and_line sal = find_pc_line (prologue_start, 0);
-
-      if (sal.line == 0)		/* no line info, use current PC  */
-	prologue_end = prev_pc;
-      else if (sal.end < prologue_end)	/* next line begins after fn end */
-	prologue_end = sal.end;		/* (probably means no prologue)  */
+      /* See comment in arm_scan_prologue for an explanation of
+	 this heuristics.  */
+      if (prologue_end > prologue_start + 64)
+	{
+	  prologue_end = prologue_start + 64;
+	}
     }
   else
     /* We're in the boondocks: we have no idea where the start of the

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

Re: [rfa] ARM prologue parsing support for Thumb-2 instructions

[Richard Earnshaw]

On Fri, 2010-10-08 at 14:54 +0200, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> > On Mon, Jul 19, 2010 at 12:59:05PM +0200, Ulrich Weigand wrote:
> > > > Trunk thumb_analyze_prologue does have support for Thumb-2.
> > > 
> > > Well, all the support for Thumb-2 I can see is in this block:
> > > 
> > >       else if ((insn & 0xe000) == 0xe000 && cache == NULL)
> > >         {
> > >           /* Only recognize 32-bit instructions for prologue skipping.  */
> > > 
> > > which, as the comment says, is active *only* if this routine is
> > > called from arm_skip_prologue (with cache == NULL), but not if the
> > > routine is called from arm_scan_prologue (with cache != NULL),
> > > which is what is used during unwinding.
> > 
> > IIRC, it would not be hard to fill in the missing pieces; I just
> > didn't need them at the time, and could not easily test them.  So
> > rather than risk them being wrong, I left them for later.
> 
> I've finally gotten around to finish my implementation of this.
> 
> The main part of this patch is support in thumb_analyze_prologue for
> various Thumb-2 instructions.  In addition, the patch improves support
> for optimized code by skipping unrecognized instructions instead of
> aborting the prologue scan, and by not relying on line number data to
> determine an upper bound for the end of the prologue.  (Both these
> changes are already in place for the ARM decoder; the patch simply
> brings them over to the Thumb decode as well.)
> 
> Finally, the patch adds support for a couple of more function calls
> that may happen within the prologue: __tls_get_addr and __aeabi_read_tp.
> 
> Tested on armv7l-linux-gnueabi with no regressions.
> 
> In addition, I've run a test with the following line disabled:
>   dwarf2_append_unwinders (gdbarch);
> This causes GDB to ignore DWARF CFI even if present, and always
> fall back to prologue parsing.  With GDB before this patch, that
> test introduces a large number of extra regressions.  *With* this
> patch, nearly all of those regressions are fixed again.  The
> following failures still remain:
> 
> FAIL: gdb.base/call-signal-resume.exp: backtrace
> FAIL: gdb.base/call-signal-resume.exp: dummy stack frame number
> FAIL: gdb.base/call-signal-resume.exp: continue to breakpoint at stop_two
> FAIL: gdb.base/call-signal-resume.exp: continue to receipt of signal
> FAIL: gdb.base/call-signal-resume.exp: continue to breakpoint at handle_signal
> FAIL: gdb.base/call-signal-resume.exp: continue to program exit
> 
> FAIL: gdb.base/foll-vfork.exp: (timeout) vfork parent follow, finish after tcatch vfork
> FAIL: gdb.base/foll-vfork.exp: vfork child follow, finish after tcatch vfork
> 
> FAIL: gdb.threads/attach-stopped.exp: threaded: attach2 to stopped bt
> FAIL: gdb.threads/attachstop-mt.exp: attach0 to sleeping bt
> FAIL: gdb.threads/attachstop-mt.exp: attach3 to stopped bt
> FAIL: gdb.threads/attachstop-mt.exp: attach4 to stopped bt
> FAIL: gdb.threads/pthreads.exp: check backtrace from thread 1
> FAIL: gdb.threads/pthreads.exp: check backtrace from thread 2
> FAIL: gdb.threads/pthreads.exp: apply backtrace command to all three threads
> 
> All these are caused by failures to parse the prologues of hand-written
> *ARM* assembler routines in glibc (system call handlers like nanosleep).
> These simply deviate too far from the usual rules (e.g. by intermixing
> stack saving/restoring of registers with conditional branches, or by
> temporarily saving registers into other registers instead of the stack)
> for the ARM prologue parser to be able to handle them.
> 
> It seems to me that there is not much sense in attempting to support
> even this type of code.  I guess we should strongly recommend to have
> (at least) glibc debuginfo files installed if you want to debug.
> 
> OK for mainline?
> 
> Bye,
> Ulrich
> 
> ChangeLog:
> 
> 	* arm-tdep.c (thumb_expand_immediate): New function.
> 	(thumb_instruction_changes_pc): Likewise.
> 	(thumb2_instruction_changes_pc): Likewise.
> 	(thumb_analyze_prologue): Handle 32-bit Thumb instructions during
> 	prologue parsing.  Improved support for optimized code.
> 	(thumb_scan_prologue): Do not reply on line-number information,
> 	use same heuristics as arm_scan_prologue insead.
> 	(skip_prologue_function): Accept functions
> 	"__tls_get_addr" and "__aeabi_read_tp".

OK.

R.

Re: [rfa] ARM prologue parsing support for Thumb-2 instructions

[Ulrich Weigand]

Richard Earnshaw wrote:
> On Fri, 2010-10-08 at 14:54 +0200, Ulrich Weigand wrote:
> > ChangeLog:
> > 
> > 	* arm-tdep.c (thumb_expand_immediate): New function.
> > 	(thumb_instruction_changes_pc): Likewise.
> > 	(thumb2_instruction_changes_pc): Likewise.
> > 	(thumb_analyze_prologue): Handle 32-bit Thumb instructions during
> > 	prologue parsing.  Improved support for optimized code.
> > 	(thumb_scan_prologue): Do not reply on line-number information,
> > 	use same heuristics as arm_scan_prologue insead.
> > 	(skip_prologue_function): Accept functions
> > 	"__tls_get_addr" and "__aeabi_read_tp".
> 
> OK.

Thanks for the quick review!  I've checked this in as well.

Bye,
Ulrich

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

Re: [rfa] ARM prologue parsing support for Thumb-2 instructions

[Daniel Jacobowitz]

On Fri, Oct 08, 2010 at 02:54:59PM +0200, Ulrich Weigand wrote:
> I've finally gotten around to finish my implementation of this.

Thanks for doing this!

> All these are caused by failures to parse the prologues of hand-written
> *ARM* assembler routines in glibc (system call handlers like nanosleep).
> These simply deviate too far from the usual rules (e.g. by intermixing
> stack saving/restoring of registers with conditional branches, or by
> temporarily saving registers into other registers instead of the stack)
> for the ARM prologue parser to be able to handle them.
> 
> It seems to me that there is not much sense in attempting to support
> even this type of code.  I guess we should strongly recommend to have
> (at least) glibc debuginfo files installed if you want to debug.

I agree with your conclusion.  It's just not worthwhile; either
install debuginfo files, or leave .debug_frame in the stripped
libraries.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfa] ARM prologue parsing support for Thumb-2 instructions

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Fri, Oct 08, 2010 at 02:54:59PM +0200, Ulrich Weigand wrote:
> > All these are caused by failures to parse the prologues of hand-written
> > *ARM* assembler routines in glibc (system call handlers like nanosleep).
> > These simply deviate too far from the usual rules (e.g. by intermixing
> > stack saving/restoring of registers with conditional branches, or by
> > temporarily saving registers into other registers instead of the stack)
> > for the ARM prologue parser to be able to handle them.
> > 
> > It seems to me that there is not much sense in attempting to support
> > even this type of code.  I guess we should strongly recommend to have
> > (at least) glibc debuginfo files installed if you want to debug.
> 
> I agree with your conclusion.  It's just not worthwhile; either
> install debuginfo files, or leave .debug_frame in the stripped
> libraries.

Hmm, I noticed one set of problems is due to the __libc_do_syscall
function you added in this patch:

http://sourceware.org/ml/gdb-patches/2010-10/msg00137.html

	.thumb
	.syntax unified
	.hidden __libc_do_syscall

ENTRY (__libc_do_syscall)
	.fnstart
	push	{r7, lr}
	.save	{r7, lr}
	cfi_adjust_cfa_offset (8)
	cfi_rel_offset (r7, 0)
	cfi_rel_offset (lr, 4)
	mov	r7, ip
	swi	0x0
	pop	{r7, pc}
	.fnend
END (__libc_do_syscall)


The GDB prologue parser would actually be easily capable of understanding
this function prologue.  However, it doesn't work because GDB does not even
find the start of the function in the first place, since the symbol is
present only in the regular symbol table (which is stripped), and not in
the dynamic symbol table, because of the ".hidden" directive.

Is there any particular reason why this function could not be at least
.protected instead?

[ Note that this shows up even with glibc debuginfo installed when running
the break-interp.exp test case, because that test uses a copy of libc.so
so that GDB doesn't find the install debuginfo any more ... ]

Bye,
Ulrich

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

Re: [rfa] ARM prologue parsing support for Thumb-2 instructions

[Daniel Jacobowitz]

On Tue, Oct 12, 2010 at 06:56:22PM +0200, Ulrich Weigand wrote:
> Hmm, I noticed one set of problems is due to the __libc_do_syscall
> function you added in this patch:
> 
> http://sourceware.org/ml/gdb-patches/2010-10/msg00137.html

Wrong link :-)

> The GDB prologue parser would actually be easily capable of understanding
> this function prologue.  However, it doesn't work because GDB does not even
> find the start of the function in the first place, since the symbol is
> present only in the regular symbol table (which is stripped), and not in
> the dynamic symbol table, because of the ".hidden" directive.
> 
> Is there any particular reason why this function could not be at least
> .protected instead?

It doesn't need to be exposed; that would waste space in the dynamic
symbol table.  Plus I don't understand the complete details, but
.protected is problematic.

I am not real sympathetic.  Either have unwind info, or if you have a
system where that is not practical, implement .ARM.exidx based
unwinding for GDB.  It can't be done in general but it can be done for
the standard table formats; there's code in readelf to dump them, for
instance.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfa] ARM prologue parsing support for Thumb-2 instructions

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> I am not real sympathetic.  Either have unwind info, or if you have a
> system where that is not practical, implement .ARM.exidx based
> unwinding for GDB.  It can't be done in general but it can be done for
> the standard table formats; there's code in readelf to dump them, for
> instance.

Good point.  I'll have a look and see if this is feasible ...

Bye,
Ulrich

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