[rfa] Fix software-watchpoint failures by adding epilogue detection

[rfa] Fix software-watchpoint failures by adding epilogue detection

[Ulrich Weigand]

Hello,

I've been seeing failures in software watchpoint tests on ARM (Thumb-2),
due to missing modeling of precise unwind states during function
epilogues.  This problem is well known from other architectures,
and is usually fixed by skipping epilogue during sofware watchpoint
single-stepping via the gdbarch_in_function_epilogue_p callback.

However, ARM currently does not define this routine.  The following
patch adds an implementation that detects common Thumb epilogue
sequences (ARM is missing, but could be added in an analogous way).

This fixes the following test case failures:
FAIL: gdb.base/recurse.exp: continue to second instance watchpoint, second time
FAIL: gdb.base/recurse.exp: second instance watchpoint deleted when leaving scope (the program exited)
FAIL: gdb.base/recurse.exp: continue to first instance watchpoint, second time (the program is no longer running)
FAIL: gdb.base/recurse.exp: first instance watchpoint deleted when leaving scope (the program is no longer running)
FAIL: gdb.base/watch-cond.exp: watchpoint with local expression, local condition evaluates in correct frame

and adds:
XPASS: gdb.mi/mi-watch.exp: sw: watchpoint trigger
XPASS: gdb.mi/mi2-watch.exp: sw: watchpoint trigger

Tested on armv7l-linux-gnueabi with no regressions.

OK for mainline?

Bye,
Ulrich


ChangeLog:

	* arm-tdep.c (arm_in_function_epilogue_p): New function.
	(arm_gdbarch_init): Install it.


=== modified file 'gdb/arm-tdep.c'
--- gdb/arm-tdep.c	2010-08-30 15:33:03 +0000
+++ gdb/arm-tdep.c	2010-09-22 17:07:18 +0000
@@ -1686,6 +1686,87 @@
     }
 }
 
+/* Return true if we are in the function's epilogue, i.e. after the
+   instruction that destroyed the function's stack frame.
+
+   We consider this to be the case if, starting from the current
+   instruction, we have a sequence of:
+
+   - [optional] setting SP from the frame pointer
+   - restoring registers from SP [may include PC]
+   - a return-type instruction [if PC wasn't already restored]
+
+  If we find anything else, we assume we're not in the epilogue.  */
+
+static int
+arm_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+
+  if (arm_pc_is_thumb (pc))
+    {
+      for (;;)
+	{
+	  unsigned int insn;
+	  gdb_byte buf[2];
+
+	  if (target_read_memory (pc, buf, 2))
+	    return 0;
+
+	  pc += 2;
+	  insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
+
+	  if (insn == 0x4770)  /* bx lr */
+	    return 1;
+	  else if (insn == 0x46f7)  /* mov pc, lr */
+	    return 1;
+	  else if (insn == 0x46bd)  /* mov sp, r7 */
+	    ;
+	  else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
+	    {
+	      if (insn & 0x0100)  /* <registers> include PC.  */
+		return 1;
+	    }
+	  else if ((insn & 0xe000) == 0xe000)  /* 32-bit Thumb-2 instruction */
+	    {
+	      unsigned int insn2;
+
+	      if (target_read_memory (pc, buf, 2))
+		return 0;
+
+	      pc += 2;
+	      insn2 = extract_unsigned_integer (buf, 2, byte_order_for_code);
+
+	      if ((insn & 0xffdf) == 0xe89d)  /* ldm.w sp{!}, <registers> */
+		{
+		  if (insn2 & 0x8000)  /* <registers> include PC.  */
+		    return 1;
+		}
+	      else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
+		       && (insn2 & 0x0fff) == 0x0d04)
+		{
+		  if ((insn2 & 0xf000) == 0xf000) /* <Rt> is PC.  */
+		    return 1;
+		}
+	      else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
+		       && (insn2 & 0x0e00) == 0x0a00)
+		;
+	      else
+		return 0;
+	    }
+	  else
+	    return 0;
+	}
+    }
+  else
+    {
+      /* FIXME: We don't handle ARM for now.  */
+    }
+
+  return 0;
+}
+
+
 /* When arguments must be pushed onto the stack, they go on in reverse
    order.  The code below implements a FILO (stack) to do this.  */
 
@@ -6818,6 +6899,9 @@
   /* Advance PC across function entry code.  */
   set_gdbarch_skip_prologue (gdbarch, arm_skip_prologue);
 
+  /* Detect whether PC is in function epilogue.  */
+  set_gdbarch_in_function_epilogue_p (gdbarch, arm_in_function_epilogue_p);
+
   /* Skip trampolines.  */
   set_gdbarch_skip_trampoline_code (gdbarch, arm_skip_stub);
 

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

Re: [rfa] Fix software-watchpoint failures by adding epilogue detection

[Daniel Jacobowitz]

On Wed, Sep 22, 2010 at 08:46:47PM +0200, Ulrich Weigand wrote:
> Hello,
> 
> I've been seeing failures in software watchpoint tests on ARM (Thumb-2),
> due to missing modeling of precise unwind states during function
> epilogues.  This problem is well known from other architectures,
> and is usually fixed by skipping epilogue during sofware watchpoint
> single-stepping via the gdbarch_in_function_epilogue_p callback.
> 
> However, ARM currently does not define this routine.  The following
> patch adds an implementation that detects common Thumb epilogue
> sequences (ARM is missing, but could be added in an analogous way).

Hi Ulrich,

One of the patches we haven't had a chance to submit took the same
approach to solve these failures.  I also did ARM mode, as well as
some additional sequences - probably those used by RealView.  I've
attached it.

We have at least one other patch related to ARM testsuite failures,
specifically an ERROR (probably infinite backtrace) in gdb1250.exp.
I can extract it for you if you'd like, but I'm not sure it's in
condition to be merged.

I haven't retested this patch against HEAD, but it's in all our
builds.

-- 
Daniel Jacobowitz
CodeSourcery

2010-07-02  Daniel Jacobowitz  <dan@codesourcery.com>

	* arm-tdep.c (thumb_in_function_epilogue_p)
	(arm_in_function_epilogue_p): New.
	(thumb_insn_size): Move higher.
	(arm_gdbarch_init): Install arm_in_function_epilogue_p as
	gdbarch_in_function_epilogue_p callback.

--- /net/henry4/scratch/gcc/src/gdb-mainline/gdb/arm-tdep.c	2010-08-30 10:47:04.000000000 -0700
+++ gdb/arm-tdep.c	2010-09-13 12:32:50.000000000 -0700
@@ -1706,6 +1815,168 @@ arm_dwarf2_frame_init_reg (struct gdbarc
     }
 }
 
+/* Return the size in bytes of the complete Thumb instruction whose
+   first halfword is INST1.  */
+
+static int
+thumb_insn_size (unsigned short inst1)
+{
+  if ((inst1 & 0xe000) == 0xe000 && (inst1 & 0x1800) != 0)
+    return 4;
+  else
+    return 2;
+}
+
+/* Return 1 if the stack frame has already been destroyed by the
+   function epilogue.  */
+
+static int
+thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+  unsigned int inst1, inst2;
+  int found_return, found_stack_adjust;
+  CORE_ADDR func_start, func_end;
+
+  if (!find_pc_partial_function (pc, NULL, &func_start, &func_end))
+    return 0;
+
+  /* We are in the epilogue if the previous instruction was a stack
+     adjustment and the next instruction is a possible return (bx, mov
+     pc, or pop).  We could have to scan backwards to find the stack
+     adjustment, or forwards to find the return, but this is a decent
+     approximation.  First scan forwards.  */
+
+  found_return = 0;
+  inst1 = read_memory_unsigned_integer (pc, 2, byte_order_for_code);
+
+  if ((inst1 & 0xff00) == 0xbc00 && pc + 2 < func_end)
+    /* POP (without PC).  Potentially loading the return address for BX.  */
+    inst1 = read_memory_unsigned_integer (pc + 2, 2, byte_order_for_code);
+
+  if ((inst1 & 0xff80) == 0x4700)
+    /* BX lr is typically used for returns.  */
+    found_return = 1;
+  else if ((inst1 & 0xff00) == 0xbd00)
+    /* POP, including PC.  */
+    found_return = 1;
+  else if (thumb_insn_size (inst1) == 4)
+    {
+      inst2 = read_memory_unsigned_integer (pc, 2, byte_order_for_code);
+
+      if (inst1 == 0xe8bd && (inst2 & 0x8000) == 0x8000)
+	/* POP.W, including PC.  */
+	found_return = 1;
+      else if (inst1 == 0xf85d && inst2 == 0xfb04)
+	/* POP.W, only PC.  */
+	found_return = 1;
+    }
+
+  if (!found_return)
+    return 0;
+
+  /* Scan backwards.  This is just a heuristic, so do not worry about
+     false positives from mode changes or 16-bit vs 32-bit instructions.  */
+
+  if (pc < func_start + 4)
+    return 0;
+
+  found_stack_adjust = 0;
+  inst1 = read_memory_unsigned_integer (pc - 4, 2, byte_order_for_code);
+  inst2 = read_memory_unsigned_integer (pc - 2, 2, byte_order_for_code);
+  if ((inst2 & 0xff00) == 0xbc00)
+    /* POP (without PC).  */
+    found_stack_adjust = 1;
+  else if (inst1 == 0xe8bd)
+    /* POP.W (register list).  */
+    found_stack_adjust = 1;
+  else if (inst1 == 0xf85d && (inst2 & 0x0fff) == 0x0b04)
+    /* POP.W (one register).  */
+    found_stack_adjust = 1;
+  else if ((inst2 & 0xff00) == 0xb000)
+    /* ADD SP, imm or SUB SP, imm.  */
+    found_stack_adjust = 1;
+
+  /* There are many other stack adjusting instructions which could be
+     recognized here.  */
+
+  if (found_stack_adjust)
+    return 1;
+
+  return 0;
+}
+
+/* Return 1 if the stack frame has already been destroyed by the
+   function epilogue.  */
+
+static int
+arm_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+  unsigned int insn;
+  int found_return, found_stack_adjust;
+  CORE_ADDR func_start, func_end;
+
+  if (arm_pc_is_thumb (gdbarch, pc))
+    return thumb_in_function_epilogue_p (gdbarch, pc);
+
+  if (!find_pc_partial_function (pc, NULL, &func_start, &func_end))
+    return 0;
+
+  /* We are in the epilogue if the previous instruction was a stack
+     adjustment and the next instruction is a possible return (bx, mov
+     pc, or pop).  We could have to scan backwards to find the stack
+     adjustment, or forwards to find the return, but this is a decent
+     approximation.  First scan forwards.  */
+
+  found_return = 0;
+  insn = read_memory_unsigned_integer (pc, 4, byte_order_for_code);
+  if (bits (insn, 28, 31) != INST_NV)
+    {
+      if ((insn & 0x0ffffff0) == 0x012fff10)
+	/* BX.  */
+	found_return = 1;
+      else if ((insn & 0x0ffffff0) == 0x01a0f000)
+	/* MOV PC.  */
+	found_return = 1;
+      else if ((insn & 0x0fff0000) == 0x08bd0000
+	  && (insn & 0x0000c000) != 0)
+	/* POP (LDMIA), including PC or LR.  */
+	found_return = 1;
+    }
+
+  if (!found_return)
+    return 0;
+
+  /* Scan backwards.  This is just a heuristic, so do not worry about
+     false positives from mode changes.  */
+
+  if (pc < func_start + 4)
+    return 0;
+
+  insn = read_memory_unsigned_integer (pc - 4, 4, byte_order_for_code);
+  if (bits (insn, 28, 31) != INST_NV)
+    {
+      if ((insn & 0x0df0f000) == 0x0080d000)
+	/* ADD SP (register or immediate).  */
+	found_stack_adjust = 1;
+      else if ((insn & 0x0df0f000) == 0x0040d000)
+	/* SUB SP (register or immediate).  */
+	found_stack_adjust = 1;
+      else if ((insn & 0x0ffffff0) == 0x01a0d000)
+	/* MOV SP.  */
+	found_return = 1;
+      else if ((insn & 0x0fff0000) == 0x08bd0000)
+	/* POP (LDMIA).  */
+	found_stack_adjust = 1;
+    }
+
+  if (found_stack_adjust)
+    return 1;
+
+  return 0;
+}
+
 /* When arguments must be pushed onto the stack, they go on in reverse
    order.  The code below implements a FILO (stack) to do this.  */
 
@@ -2647,18 +2918,6 @@ bitcount (unsigned long val)
   return nbits;
 }
 
-/* Return the size in bytes of the complete Thumb instruction whose
-   first halfword is INST1.  */
-
-static int
-thumb_insn_size (unsigned short inst1)
-{
-  if ((inst1 & 0xe000) == 0xe000 && (inst1 & 0x1800) != 0)
-    return 4;
-  else
-    return 2;
-}
-
 static int
 thumb_advance_itstate (unsigned int itstate)
 {
@@ -6881,6 +7160,7 @@ arm_gdbarch_init (struct gdbarch_info in
   /* Advance PC across function entry code.  */
   set_gdbarch_skip_prologue (gdbarch, arm_skip_prologue);
 
+  set_gdbarch_in_function_epilogue_p (gdbarch, arm_in_function_epilogue_p);
   /* Skip trampolines.  */
   set_gdbarch_skip_trampoline_code (gdbarch, arm_skip_stub);
 

Re: [rfa] Fix software-watchpoint failures by adding epilogue detection

[Ulrich Weigand]

Dan Jacobowitz wrote:

> One of the patches we haven't had a chance to submit took the same
> approach to solve these failures.  I also did ARM mode, as well as
> some additional sequences - probably those used by RealView.  I've
> attached it.

Thanks!  Unfortunately it doesn't fix the problem with our current
ARM compiler, because your heuristics doesn't recognize the sequence
we get as epilogue:

    83c8:       f107 0710       add.w   r7, r7, #16
    83cc:       46bd            mov     sp, r7
    83ce:       bd80            pop     {r7, pc}

On the other hand, your patch does indeed accept a couple of additonal
instructions compared to mine.  I'll come up with a merged approach ...

> We have at least one other patch related to ARM testsuite failures,
> specifically an ERROR (probably infinite backtrace) in gdb1250.exp.
> I can extract it for you if you'd like, but I'm not sure it's in
> condition to be merged.

Hmm, this test is passing now.  We did have problems with backtraces
out of abort, but this was due to a compiler problem -- it simply
would not save the return address at all for no-return functions.
This is now fixed in the compiler ...

Bye,
Ulrich

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

Re: [rfa] Fix software-watchpoint failures by adding epilogue detection

[Ulrich Weigand]

I wrote:
> Dan Jacobowitz wrote:
> 
> > One of the patches we haven't had a chance to submit took the same
> > approach to solve these failures.  I also did ARM mode, as well as
> > some additional sequences - probably those used by RealView.  I've
> > attached it.
> 
> Thanks!  Unfortunately it doesn't fix the problem with our current
> ARM compiler, because your heuristics doesn't recognize the sequence
> we get as epilogue:
> 
>     83c8:       f107 0710       add.w   r7, r7, #16
>     83cc:       46bd            mov     sp, r7
>     83ce:       bd80            pop     {r7, pc}
> 
> On the other hand, your patch does indeed accept a couple of additonal
> instructions compared to mine.  I'll come up with a merged approach ...

I've compared the Thumb cases of both patches with the following results:

1. Some epilogue sequences accepted by your patch were not accepted by mine:
- My patch only accepted "bx lr" as return, while yours accepts any "bx".
- I had a typo in one of the instruction opcodes.

2. Some epilogue sequences accepted by my patch were not accepted by yours:
- I'm allowing "mov sp, r7" and "vldm" instructions, as well as certain
  additional cases of "ldm.w".
- I'm accepting more diverse sequences due to forward-scanning for multiple
  instructions, and not requiring backward-scanning.

I've now merged the two by basically keeping with the main structure of my
patch for Thumb, and fixing the two issues identified under 1.

For ARM, I'm simply using your patch as-is; I've confirmed that it fixes
all the same testsuite failures when running with -marm as the Thumb patch
fixes with -mthumb.   The more limited scanning doesn't appear to hurt for
ARM as the epilogues tend to be very short anyway.


Retested with no regressions on armv7l-linux-gnueabi both with -marm and
with (implied) -mthumb.

Does this look good to you?

Bye,
Ulrich


2010-09-24  Ulrich Weigand  <uweigand@de.ibm.com>
            Daniel Jacobowitz  <dan@codesourcery.com>

	* arm-tdep.c (thumb_in_function_epilogue_p)
	(arm_in_function_epilogue_p): New.
	(arm_gdbarch_init): Install arm_in_function_epilogue_p as
	gdbarch_in_function_epilogue_p callback.


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	23 Sep 2010 18:43:29 -0000
@@ -1706,6 +1706,150 @@ arm_dwarf2_frame_init_reg (struct gdbarc
     }
 }
 
+/* Return 1 if the stack frame has already been destroyed by the
+   function epilogue.  */
+
+static int
+thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+
+  /* The epilogue is a sequence of instructions along the following lines:
+
+    - [optionally] set SP from the frame pointer
+    - restore registers from SP [may include PC]
+    - a return-type instruction [if PC wasn't already restored]
+
+    If, scanning forward from the current PC, the instructions we find
+    are compatible with this sequence (ending in a return), we consider
+    the current PC to be part of the epilogue.  */
+
+  for (;;)
+    {
+      unsigned int insn;
+      gdb_byte buf[2];
+
+      if (target_read_memory (pc, buf, 2))
+	return 0;
+
+      pc += 2;
+      insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
+
+      if ((insn & 0xff80) == 0x4700)  /* bx <Rm> */
+	return 1;
+      else if (insn == 0x46f7)  /* mov pc, lr */
+	return 1;
+      else if (insn == 0x46bd)  /* mov sp, r7 */
+	;
+      else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
+	{
+	  if (insn & 0x0100)  /* <registers> include PC.  */
+	    return 1;
+	}
+      else if ((insn & 0xe000) == 0xe000)  /* 32-bit Thumb-2 instruction */
+	{
+	  unsigned int insn2;
+
+	  if (target_read_memory (pc, buf, 2))
+	    return 0;
+
+	  pc += 2;
+	  insn2 = extract_unsigned_integer (buf, 2, byte_order_for_code);
+
+	  if ((insn & 0xffdf) == 0xe89d)  /* ldm.w sp{!}, <registers> */
+	    {
+	      if (insn2 & 0x8000)  /* <registers> include PC.  */
+		return 1;
+	    }
+	  else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
+		   && (insn2 & 0x0fff) == 0x0b04)
+	    {
+	      if ((insn2 & 0xf000) == 0xf000) /* <Rt> is PC.  */
+		return 1;
+	    }
+	  else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
+		   && (insn2 & 0x0e00) == 0x0a00)
+	    ;
+	  else
+	    return 0;
+	}
+      else
+	return 0;
+    }
+}
+
+/* Return 1 if the stack frame has already been destroyed by the
+   function epilogue.  */
+
+static int
+arm_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+  unsigned int insn;
+  int found_return, found_stack_adjust;
+  CORE_ADDR func_start, func_end;
+
+  if (arm_pc_is_thumb (gdbarch, pc))
+    return thumb_in_function_epilogue_p (gdbarch, pc);
+
+  if (!find_pc_partial_function (pc, NULL, &func_start, &func_end))
+    return 0;
+
+  /* We are in the epilogue if the previous instruction was a stack
+     adjustment and the next instruction is a possible return (bx, mov
+     pc, or pop).  We could have to scan backwards to find the stack
+     adjustment, or forwards to find the return, but this is a decent
+     approximation.  First scan forwards.  */
+
+  found_return = 0;
+  insn = read_memory_unsigned_integer (pc, 4, byte_order_for_code);
+  if (bits (insn, 28, 31) != INST_NV)
+    {
+      if ((insn & 0x0ffffff0) == 0x012fff10)
+	/* BX.  */
+	found_return = 1;
+      else if ((insn & 0x0ffffff0) == 0x01a0f000)
+	/* MOV PC.  */
+	found_return = 1;
+      else if ((insn & 0x0fff0000) == 0x08bd0000
+	  && (insn & 0x0000c000) != 0)
+	/* POP (LDMIA), including PC or LR.  */
+	found_return = 1;
+    }
+
+  if (!found_return)
+    return 0;
+
+  /* Scan backwards.  This is just a heuristic, so do not worry about
+     false positives from mode changes.  */
+
+  if (pc < func_start + 4)
+    return 0;
+
+  insn = read_memory_unsigned_integer (pc - 4, 4, byte_order_for_code);
+  if (bits (insn, 28, 31) != INST_NV)
+    {
+      if ((insn & 0x0df0f000) == 0x0080d000)
+	/* ADD SP (register or immediate).  */
+	found_stack_adjust = 1;
+      else if ((insn & 0x0df0f000) == 0x0040d000)
+	/* SUB SP (register or immediate).  */
+	found_stack_adjust = 1;
+      else if ((insn & 0x0ffffff0) == 0x01a0d000)
+	/* MOV SP.  */
+	found_return = 1;
+      else if ((insn & 0x0fff0000) == 0x08bd0000)
+	/* POP (LDMIA).  */
+	found_stack_adjust = 1;
+    }
+
+  if (found_stack_adjust)
+    return 1;
+
+  return 0;
+}
+
+
 /* When arguments must be pushed onto the stack, they go on in reverse
    order.  The code below implements a FILO (stack) to do this.  */
 
@@ -6881,6 +7025,9 @@ arm_gdbarch_init (struct gdbarch_info in
   /* Advance PC across function entry code.  */
   set_gdbarch_skip_prologue (gdbarch, arm_skip_prologue);
 
+  /* Detect whether PC is in function epilogue.  */
+  set_gdbarch_in_function_epilogue_p (gdbarch, arm_in_function_epilogue_p);
+
   /* Skip trampolines.  */
   set_gdbarch_skip_trampoline_code (gdbarch, arm_skip_stub);
 

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

Re: [rfa] Fix software-watchpoint failures by adding epilogue detection

[Daniel Jacobowitz]

On Fri, Sep 24, 2010 at 02:39:05PM +0200, Ulrich Weigand wrote:
> 1. Some epilogue sequences accepted by your patch were not accepted by mine:
> - My patch only accepted "bx lr" as return, while yours accepts any "bx".
> - I had a typo in one of the instruction opcodes.
> 
> 2. Some epilogue sequences accepted by my patch were not accepted by yours:
> - I'm allowing "mov sp, r7" and "vldm" instructions, as well as certain
>   additional cases of "ldm.w".

These all sound good.

> - I'm accepting more diverse sequences due to forward-scanning for multiple
>   instructions, and not requiring backward-scanning.

This I'm worried about.  From my patch:

+  /* We are in the epilogue if the previous instruction was a stack
+     adjustment and the next instruction is a possible return (bx, mov
+     pc, or pop).

This is definitely an epilogue:

  pop { r4, r5, r6, lr }
  bx lr

This could be an epilogue, but it could also be an indirect call:

  bx lr

If it's an indirect call there would be a mov lr, pc before it.
If it's an indirect tail call, then it's an epilogue, and the return
address won't be saved.

If there's no stack adjustment, then gdbarch_in_function_epilogue_p
does not need to return 1; the predicate really means "we can not
check for watchpoints because the frame might be in an inconsistent
state".

Is it safe for this predicate to return 1 around something that is not
an epilogue?

Given that definition of the predicate, the backwards scan is
appropriate; without a backwards scan, we can only answer "is there an
epilogue after this point", not "are we already inside an epilogue".

Of course, if it turns out harmless to return false positives... I'm
not sure.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfa] Fix software-watchpoint failures by adding epilogue detection

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> On Fri, Sep 24, 2010 at 02:39:05PM +0200, Ulrich Weigand wrote:
> > - I'm accepting more diverse sequences due to forward-scanning for multiple
> >   instructions, and not requiring backward-scanning.
> 
> This I'm worried about.  From my patch:
> 
> +  /* We are in the epilogue if the previous instruction was a stack
> +     adjustment and the next instruction is a possible return (bx, mov
> +     pc, or pop).
> 
> This is definitely an epilogue:
> 
>   pop { r4, r5, r6, lr }
>   bx lr
> 
> This could be an epilogue, but it could also be an indirect call:
> 
>   bx lr
> 
> If it's an indirect call there would be a mov lr, pc before it.
> If it's an indirect tail call, then it's an epilogue, and the return
> address won't be saved.

I'm wondering how "bx lr" could be an indirect call; for a call,
lr would have to point to the return address, so it couldn't also
contain the target address ...  Am I missing something here?

My original patch accepted only specifically "bx lr"; yours also
accepts different registers for bx.  If we have a bx with a
different register, this may of course well be an indirect call.

As far as I can see, GCC never uses bx with any other register but
lr to implement a return instruction.  Do you know whether this is
also true for other compilers?  If so, maybe the easiest fix would
be to change this back to only accepting "bx lr".

> If there's no stack adjustment, then gdbarch_in_function_epilogue_p
> does not need to return 1; the predicate really means "we can not
> check for watchpoints because the frame might be in an inconsistent
> state".
> 
> Is it safe for this predicate to return 1 around something that is not
> an epilogue?
> 
> Given that definition of the predicate, the backwards scan is
> appropriate; without a backwards scan, we can only answer "is there an
> epilogue after this point", not "are we already inside an epilogue".
> 
> Of course, if it turns out harmless to return false positives... I'm
> not sure.

It seems to me that it is relatively harmless to return a false positive;
the only thing that happens is that the check for watchpoint hits is
delayed until the next instruction.  In particular, returning true in
the epilogue of a frameless functions should definitely be harmless.
(Returning true on a bx that implements a function call might in rare
cases lead to a watchpoint hit being detected on the first instruction
of the called function instead ...)

Bye,
Ulrich

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

Re: [rfa] Fix software-watchpoint failures by adding epilogue detection

[Daniel Jacobowitz]

On Tue, Sep 28, 2010 at 06:04:14PM +0200, Ulrich Weigand wrote:
> I'm wondering how "bx lr" could be an indirect call; for a call,
> lr would have to point to the return address, so it couldn't also
> contain the target address ...  Am I missing something here?

Bah, you are correct.  Poor choice of example.  bx ip is a better
example; that can be an indirect call, a return, or a tail call.

> As far as I can see, GCC never uses bx with any other register but
> lr to implement a return instruction.  Do you know whether this is
> also true for other compilers?  If so, maybe the easiest fix would
> be to change this back to only accepting "bx lr".

Sorry, I don't know :-(  Does GCC also only use lr for an indirect
tail call?  I can't tell - I couldn't get GCC to issue an indirect
tail call.  But I did get this out of RealView:

void (*foo)();
void bar()
{
  foo();
}

bar PROC
        LDR      r0,|L1.12|
        LDR      r0,[r0,#0]  ; foo
        BX       r0
        ENDP

> It seems to me that it is relatively harmless to return a false positive;
> the only thing that happens is that the check for watchpoint hits is
> delayed until the next instruction.  In particular, returning true in
> the epilogue of a frameless functions should definitely be harmless.
> (Returning true on a bx that implements a function call might in rare
> cases lead to a watchpoint hit being detected on the first instruction
> of the called function instead ...)

Yes, that sounds like the case I was worried about.  Of course, it's
relatively harmless either way; nothing in GDB absolutely relies on
this hook.  So I won't object to the patch as-is.  This would be a
nice thing to clean up some day.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfa] Fix software-watchpoint failures by adding epilogue detection

[Yao Qi]

On Tue, Sep 28, 2010 at 06:04:14PM +0200, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> 
> > On Fri, Sep 24, 2010 at 02:39:05PM +0200, Ulrich Weigand wrote:
> > > - I'm accepting more diverse sequences due to forward-scanning for multiple
> > >   instructions, and not requiring backward-scanning.
> > 
> > This I'm worried about.  From my patch:
> > 
> > +  /* We are in the epilogue if the previous instruction was a stack
> > +     adjustment and the next instruction is a possible return (bx, mov
> > +     pc, or pop).
> > 
> > This is definitely an epilogue:
> > 
> >   pop { r4, r5, r6, lr }
> >   bx lr
> > 
> > This could be an epilogue, but it could also be an indirect call:
> > 
> >   bx lr
> > 
> > If it's an indirect call there would be a mov lr, pc before it.
> > If it's an indirect tail call, then it's an epilogue, and the return
> > address won't be saved.
> 
> I'm wondering how "bx lr" could be an indirect call; for a call,
> lr would have to point to the return address, so it couldn't also
> contain the target address ...  Am I missing something here?
> 
> My original patch accepted only specifically "bx lr"; yours also
> accepts different registers for bx.  If we have a bx with a
> different register, this may of course well be an indirect call.
> 
> As far as I can see, GCC never uses bx with any other register but
> lr to implement a return instruction.  Do you know whether this is
> also true for other compilers?  If so, maybe the easiest fix would
> be to change this back to only accepting "bx lr".
> 

I haven't seen any examples GCC uses bx with other registers so far,
but I noticed that some one is thinking of use 'bx r3' for
optimization purpose.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40887
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19599
The patch for PR19599 hasn't been committed yet, so I still can't find a
real example that using 'bx r3' to return.

In short, there *might* be some cases now or in the future, that
registers other than lr are used with bx for return.

-- 
Yao Qi
CodeSourcery
yao@codesourcery.com
(650) 331-3385 x739

Re: [rfa] Fix software-watchpoint failures by adding epilogue detection

[Richard Earnshaw]

On Tue, 2010-09-28 at 12:32 -0400, Daniel Jacobowitz wrote:
> On Tue, Sep 28, 2010 at 06:04:14PM +0200, Ulrich Weigand wrote:
> > I'm wondering how "bx lr" could be an indirect call; for a call,
> > lr would have to point to the return address, so it couldn't also
> > contain the target address ...  Am I missing something here?
> 
> Bah, you are correct.  Poor choice of example.  bx ip is a better
> example; that can be an indirect call, a return, or a tail call.
> 
> > As far as I can see, GCC never uses bx with any other register but
> > lr to implement a return instruction.  Do you know whether this is
> > also true for other compilers?  If so, maybe the easiest fix would
> > be to change this back to only accepting "bx lr".
> 
> Sorry, I don't know :-(  Does GCC also only use lr for an indirect
> tail call?  I can't tell - I couldn't get GCC to issue an indirect
> tail call.  But I did get this out of RealView:
> 

You can never use LR for an indirect tailcall as it must contain the
return address from the outer function (at least, in any ABI compliant
code).  The mcount code does something a bit funky here, but that's a
special case.

[rfa, v3] Fix software-watchpoint failures on ARM by adding epilogue detection

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Tue, Sep 28, 2010 at 06:04:14PM +0200, Ulrich Weigand wrote:
> > I'm wondering how "bx lr" could be an indirect call; for a call,
> > lr would have to point to the return address, so it couldn't also
> > contain the target address ...  Am I missing something here?
> 
> Bah, you are correct.  Poor choice of example.  bx ip is a better
> example; that can be an indirect call, a return, or a tail call.
> 
> > As far as I can see, GCC never uses bx with any other register but
> > lr to implement a return instruction.  Do you know whether this is
> > also true for other compilers?  If so, maybe the easiest fix would
> > be to change this back to only accepting "bx lr".
> 
> Sorry, I don't know :-(  Does GCC also only use lr for an indirect
> tail call?  I can't tell - I couldn't get GCC to issue an indirect
> tail call.

OK, so it looks like we do have to add a backward scanning pass in order
to reliably distinguish whether a BX is an indirect call or an indirect
tail call (which GCC doesn't issue today, but probably could in the
future, and RealView already does).

Fortunately, this is relatively straightforward, since we at most need
to scan back one instruction.  *Every* instruction in the default
epilogue sequence modifies SP, except possibly the return itself.
Therefore, we either found some instruction(s) modifying SP during
the forward-scanning pass before hitting the return, or else we
check back one instruction.

The following patch implements this approach for Thumb (the ARM part
remains unchanged from your version).

Re-tested (with the same results) on armv7l-linux-gnueabi.

Richard, would this be OK with you?

Bye,
Ulrich


2010-10-07  Ulrich Weigand  <uweigand@de.ibm.com>
            Daniel Jacobowitz  <dan@codesourcery.com>

	* arm-tdep.c (thumb_in_function_epilogue_p)
	(arm_in_function_epilogue_p): New.
	(arm_gdbarch_init): Install arm_in_function_epilogue_p as
	gdbarch_in_function_epilogue_p callback.


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 11:55:02 -0000
@@ -1706,6 +1706,203 @@ arm_dwarf2_frame_init_reg (struct gdbarc
     }
 }
 
+/* Return true if we are in the function's epilogue, i.e. after the
+   instruction that destroyed the function's stack frame.  */
+
+static int
+thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+  unsigned int insn, insn2;
+  int found_return = 0, found_stack_adjust = 0;
+  CORE_ADDR func_start, func_end;
+  CORE_ADDR scan_pc;
+  gdb_byte buf[4];
+
+  if (!find_pc_partial_function (pc, NULL, &func_start, &func_end))
+    return 0;
+
+  /* The epilogue is a sequence of instructions along the following lines:
+
+    - add stack frame size to SP or FP
+    - [if frame pointer used] restore SP from FP
+    - restore registers from SP [may include PC]
+    - a return-type instruction [if PC wasn't already restored]
+
+    In a first pass, we scan forward from the current PC and verify the
+    instructions we find as compatible with this sequence, ending in a
+    return instruction.
+
+    However, this is not sufficient to distinguish indirect function calls
+    within a function from indirect tail calls in the epilogue in some cases.
+    Therefore, if we didn't already find any SP-changing instruction during
+    forward scan, we add a backward scanning heuristic to ensure we actually
+    are in the epilogue.  */
+
+  scan_pc = pc;
+  while (scan_pc < func_end && !found_return)
+    {
+      if (target_read_memory (scan_pc, buf, 2))
+	break;
+
+      scan_pc += 2;
+      insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
+
+      if ((insn & 0xff80) == 0x4700)  /* bx <Rm> */
+	found_return = 1;
+      else if (insn == 0x46f7)  /* mov pc, lr */
+	found_return = 1;
+      else if (insn == 0x46bd)  /* mov sp, r7 */
+	found_stack_adjust = 1;
+      else if ((insn & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
+	found_stack_adjust = 1;
+      else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
+	{
+	  found_stack_adjust = 1;
+	  if (insn & 0x0100)  /* <registers> include PC.  */
+	    found_return = 1;
+	}
+      else if ((insn & 0xe000) == 0xe000)  /* 32-bit Thumb-2 instruction */
+	{
+	  if (target_read_memory (scan_pc, buf, 2))
+	    break;
+
+	  scan_pc += 2;
+	  insn2 = extract_unsigned_integer (buf, 2, byte_order_for_code);
+
+	  if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
+	    {
+	      found_stack_adjust = 1;
+	      if (insn2 & 0x8000)  /* <registers> include PC.  */
+		found_return = 1;
+	    }
+	  else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
+		   && (insn2 & 0x0fff) == 0x0b04)
+	    {
+	      found_stack_adjust = 1;
+	      if ((insn2 & 0xf000) == 0xf000) /* <Rt> is PC.  */
+		found_return = 1;
+	    }
+	  else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
+		   && (insn2 & 0x0e00) == 0x0a00)
+	    found_stack_adjust = 1;
+	  else
+	    break;
+	}
+      else
+	break;
+    }
+
+  if (!found_return)
+    return 0;
+
+  /* Since any instruction in the epilogue sequence, with the possible
+     exception of return itself, updates the stack pointer, we need to
+     scan backwards for at most one instruction.  Try either a 16-bit or
+     a 32-bit instruction.  This is just a heuristic, so we do not worry
+     too much about false positives.*/
+
+  if (!found_stack_adjust)
+    {
+      if (pc - 4 < func_start)
+	return 0;
+      if (target_read_memory (pc - 4, buf, 4))
+	return 0;
+
+      insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
+      insn2 = extract_unsigned_integer (buf + 2, 2, byte_order_for_code);
+
+      if (insn2 == 0x46bd)  /* mov sp, r7 */
+	found_stack_adjust = 1;
+      else if ((insn2 & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
+	found_stack_adjust = 1;
+      else if ((insn2 & 0xff00) == 0xbc00)  /* pop <registers> without PC */
+	found_stack_adjust = 1;
+      else if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
+	found_stack_adjust = 1;
+      else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
+	       && (insn2 & 0x0fff) == 0x0b04)
+	found_stack_adjust = 1;
+      else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
+	       && (insn2 & 0x0e00) == 0x0a00)
+	found_stack_adjust = 1;
+    }
+
+  return found_stack_adjust;
+}
+
+/* Return true if we are in the function's epilogue, i.e. after the
+   instruction that destroyed the function's stack frame.  */
+
+static int
+arm_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+  unsigned int insn;
+  int found_return, found_stack_adjust;
+  CORE_ADDR func_start, func_end;
+
+  if (arm_pc_is_thumb (gdbarch, pc))
+    return thumb_in_function_epilogue_p (gdbarch, pc);
+
+  if (!find_pc_partial_function (pc, NULL, &func_start, &func_end))
+    return 0;
+
+  /* We are in the epilogue if the previous instruction was a stack
+     adjustment and the next instruction is a possible return (bx, mov
+     pc, or pop).  We could have to scan backwards to find the stack
+     adjustment, or forwards to find the return, but this is a decent
+     approximation.  First scan forwards.  */
+
+  found_return = 0;
+  insn = read_memory_unsigned_integer (pc, 4, byte_order_for_code);
+  if (bits (insn, 28, 31) != INST_NV)
+    {
+      if ((insn & 0x0ffffff0) == 0x012fff10)
+	/* BX.  */
+	found_return = 1;
+      else if ((insn & 0x0ffffff0) == 0x01a0f000)
+	/* MOV PC.  */
+	found_return = 1;
+      else if ((insn & 0x0fff0000) == 0x08bd0000
+	  && (insn & 0x0000c000) != 0)
+	/* POP (LDMIA), including PC or LR.  */
+	found_return = 1;
+    }
+
+  if (!found_return)
+    return 0;
+
+  /* Scan backwards.  This is just a heuristic, so do not worry about
+     false positives from mode changes.  */
+
+  if (pc < func_start + 4)
+    return 0;
+
+  insn = read_memory_unsigned_integer (pc - 4, 4, byte_order_for_code);
+  if (bits (insn, 28, 31) != INST_NV)
+    {
+      if ((insn & 0x0df0f000) == 0x0080d000)
+	/* ADD SP (register or immediate).  */
+	found_stack_adjust = 1;
+      else if ((insn & 0x0df0f000) == 0x0040d000)
+	/* SUB SP (register or immediate).  */
+	found_stack_adjust = 1;
+      else if ((insn & 0x0ffffff0) == 0x01a0d000)
+	/* MOV SP.  */
+	found_return = 1;
+      else if ((insn & 0x0fff0000) == 0x08bd0000)
+	/* POP (LDMIA).  */
+	found_stack_adjust = 1;
+    }
+
+  if (found_stack_adjust)
+    return 1;
+
+  return 0;
+}
+
+
 /* When arguments must be pushed onto the stack, they go on in reverse
    order.  The code below implements a FILO (stack) to do this.  */
 
@@ -6881,6 +7078,9 @@ arm_gdbarch_init (struct gdbarch_info in
   /* Advance PC across function entry code.  */
   set_gdbarch_skip_prologue (gdbarch, arm_skip_prologue);
 
+  /* Detect whether PC is in function epilogue.  */
+  set_gdbarch_in_function_epilogue_p (gdbarch, arm_in_function_epilogue_p);
+
   /* Skip trampolines.  */
   set_gdbarch_skip_trampoline_code (gdbarch, arm_skip_stub);
 

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

Re: [rfa, v3] Fix software-watchpoint failures on ARM by adding epilogue detection

[Richard Earnshaw]

On Thu, 2010-10-07 at 18:12 +0200, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> > On Tue, Sep 28, 2010 at 06:04:14PM +0200, Ulrich Weigand wrote:
> > > I'm wondering how "bx lr" could be an indirect call; for a call,
> > > lr would have to point to the return address, so it couldn't also
> > > contain the target address ...  Am I missing something here?
> > 
> > Bah, you are correct.  Poor choice of example.  bx ip is a better
> > example; that can be an indirect call, a return, or a tail call.
> > 
> > > As far as I can see, GCC never uses bx with any other register but
> > > lr to implement a return instruction.  Do you know whether this is
> > > also true for other compilers?  If so, maybe the easiest fix would
> > > be to change this back to only accepting "bx lr".
> > 
> > Sorry, I don't know :-(  Does GCC also only use lr for an indirect
> > tail call?  I can't tell - I couldn't get GCC to issue an indirect
> > tail call.
> 
> OK, so it looks like we do have to add a backward scanning pass in order
> to reliably distinguish whether a BX is an indirect call or an indirect
> tail call (which GCC doesn't issue today, but probably could in the
> future, and RealView already does).
> 
> Fortunately, this is relatively straightforward, since we at most need
> to scan back one instruction.  *Every* instruction in the default
> epilogue sequence modifies SP, except possibly the return itself.
> Therefore, we either found some instruction(s) modifying SP during
> the forward-scanning pass before hitting the return, or else we
> check back one instruction.
> 
> The following patch implements this approach for Thumb (the ARM part
> remains unchanged from your version).
> 
> Re-tested (with the same results) on armv7l-linux-gnueabi.
> 
> Richard, would this be OK with you?
> 
> Bye,
> Ulrich
> 
> 
> 2010-10-07  Ulrich Weigand  <uweigand@de.ibm.com>
>             Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* arm-tdep.c (thumb_in_function_epilogue_p)
> 	(arm_in_function_epilogue_p): New.
> 	(arm_gdbarch_init): Install arm_in_function_epilogue_p as
> 	gdbarch_in_function_epilogue_p callback.
> 

This is OK.  There are some sequences that could be generated that this
can't handle, and I don't think we'd ever get a stack decrement in an
epilogue sequence.

An example I don't think your code can handle is stack adjustments that
are not valid immediates for ADD SP, #n.  So very large stack adjusts my
fail.  These sequences will load or create an immediate in a register
(particularly on Thumb1) and then add that to SP.

R.

> 
> 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 11:55:02 -0000
> @@ -1706,6 +1706,203 @@ arm_dwarf2_frame_init_reg (struct gdbarc
>      }
>  }
>  
> +/* Return true if we are in the function's epilogue, i.e. after the
> +   instruction that destroyed the function's stack frame.  */
> +
> +static int
> +thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> +  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
> +  unsigned int insn, insn2;
> +  int found_return = 0, found_stack_adjust = 0;
> +  CORE_ADDR func_start, func_end;
> +  CORE_ADDR scan_pc;
> +  gdb_byte buf[4];
> +
> +  if (!find_pc_partial_function (pc, NULL, &func_start, &func_end))
> +    return 0;
> +
> +  /* The epilogue is a sequence of instructions along the following lines:
> +
> +    - add stack frame size to SP or FP
> +    - [if frame pointer used] restore SP from FP
> +    - restore registers from SP [may include PC]
> +    - a return-type instruction [if PC wasn't already restored]
> +
> +    In a first pass, we scan forward from the current PC and verify the
> +    instructions we find as compatible with this sequence, ending in a
> +    return instruction.
> +
> +    However, this is not sufficient to distinguish indirect function calls
> +    within a function from indirect tail calls in the epilogue in some cases.
> +    Therefore, if we didn't already find any SP-changing instruction during
> +    forward scan, we add a backward scanning heuristic to ensure we actually
> +    are in the epilogue.  */
> +
> +  scan_pc = pc;
> +  while (scan_pc < func_end && !found_return)
> +    {
> +      if (target_read_memory (scan_pc, buf, 2))
> +	break;
> +
> +      scan_pc += 2;
> +      insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
> +
> +      if ((insn & 0xff80) == 0x4700)  /* bx <Rm> */
> +	found_return = 1;
> +      else if (insn == 0x46f7)  /* mov pc, lr */
> +	found_return = 1;
> +      else if (insn == 0x46bd)  /* mov sp, r7 */
> +	found_stack_adjust = 1;
> +      else if ((insn & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
> +	found_stack_adjust = 1;
> +      else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
> +	{
> +	  found_stack_adjust = 1;
> +	  if (insn & 0x0100)  /* <registers> include PC.  */
> +	    found_return = 1;
> +	}
> +      else if ((insn & 0xe000) == 0xe000)  /* 32-bit Thumb-2 instruction */
> +	{
> +	  if (target_read_memory (scan_pc, buf, 2))
> +	    break;
> +
> +	  scan_pc += 2;
> +	  insn2 = extract_unsigned_integer (buf, 2, byte_order_for_code);
> +
> +	  if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
> +	    {
> +	      found_stack_adjust = 1;
> +	      if (insn2 & 0x8000)  /* <registers> include PC.  */
> +		found_return = 1;
> +	    }
> +	  else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
> +		   && (insn2 & 0x0fff) == 0x0b04)
> +	    {
> +	      found_stack_adjust = 1;
> +	      if ((insn2 & 0xf000) == 0xf000) /* <Rt> is PC.  */
> +		found_return = 1;
> +	    }
> +	  else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
> +		   && (insn2 & 0x0e00) == 0x0a00)
> +	    found_stack_adjust = 1;
> +	  else
> +	    break;
> +	}
> +      else
> +	break;
> +    }
> +
> +  if (!found_return)
> +    return 0;
> +
> +  /* Since any instruction in the epilogue sequence, with the possible
> +     exception of return itself, updates the stack pointer, we need to
> +     scan backwards for at most one instruction.  Try either a 16-bit or
> +     a 32-bit instruction.  This is just a heuristic, so we do not worry
> +     too much about false positives.*/
> +
> +  if (!found_stack_adjust)
> +    {
> +      if (pc - 4 < func_start)
> +	return 0;
> +      if (target_read_memory (pc - 4, buf, 4))
> +	return 0;
> +
> +      insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
> +      insn2 = extract_unsigned_integer (buf + 2, 2, byte_order_for_code);
> +
> +      if (insn2 == 0x46bd)  /* mov sp, r7 */
> +	found_stack_adjust = 1;
> +      else if ((insn2 & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
> +	found_stack_adjust = 1;
> +      else if ((insn2 & 0xff00) == 0xbc00)  /* pop <registers> without PC */
> +	found_stack_adjust = 1;
> +      else if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
> +	found_stack_adjust = 1;
> +      else if (insn == 0xf85d  /* ldr.w <Rt>, [sp], #4 */
> +	       && (insn2 & 0x0fff) == 0x0b04)
> +	found_stack_adjust = 1;
> +      else if ((insn & 0xffbf) == 0xecbd  /* vldm sp!, <list> */
> +	       && (insn2 & 0x0e00) == 0x0a00)
> +	found_stack_adjust = 1;
> +    }
> +
> +  return found_stack_adjust;
> +}
> +
> +/* Return true if we are in the function's epilogue, i.e. after the
> +   instruction that destroyed the function's stack frame.  */
> +
> +static int
> +arm_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> +  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
> +  unsigned int insn;
> +  int found_return, found_stack_adjust;
> +  CORE_ADDR func_start, func_end;
> +
> +  if (arm_pc_is_thumb (gdbarch, pc))
> +    return thumb_in_function_epilogue_p (gdbarch, pc);
> +
> +  if (!find_pc_partial_function (pc, NULL, &func_start, &func_end))
> +    return 0;
> +
> +  /* We are in the epilogue if the previous instruction was a stack
> +     adjustment and the next instruction is a possible return (bx, mov
> +     pc, or pop).  We could have to scan backwards to find the stack
> +     adjustment, or forwards to find the return, but this is a decent
> +     approximation.  First scan forwards.  */
> +
> +  found_return = 0;
> +  insn = read_memory_unsigned_integer (pc, 4, byte_order_for_code);
> +  if (bits (insn, 28, 31) != INST_NV)
> +    {
> +      if ((insn & 0x0ffffff0) == 0x012fff10)
> +	/* BX.  */
> +	found_return = 1;
> +      else if ((insn & 0x0ffffff0) == 0x01a0f000)
> +	/* MOV PC.  */
> +	found_return = 1;
> +      else if ((insn & 0x0fff0000) == 0x08bd0000
> +	  && (insn & 0x0000c000) != 0)
> +	/* POP (LDMIA), including PC or LR.  */
> +	found_return = 1;
> +    }
> +
> +  if (!found_return)
> +    return 0;
> +
> +  /* Scan backwards.  This is just a heuristic, so do not worry about
> +     false positives from mode changes.  */
> +
> +  if (pc < func_start + 4)
> +    return 0;
> +
> +  insn = read_memory_unsigned_integer (pc - 4, 4, byte_order_for_code);
> +  if (bits (insn, 28, 31) != INST_NV)
> +    {
> +      if ((insn & 0x0df0f000) == 0x0080d000)
> +	/* ADD SP (register or immediate).  */
> +	found_stack_adjust = 1;
> +      else if ((insn & 0x0df0f000) == 0x0040d000)
> +	/* SUB SP (register or immediate).  */
> +	found_stack_adjust = 1;
> +      else if ((insn & 0x0ffffff0) == 0x01a0d000)
> +	/* MOV SP.  */
> +	found_return = 1;
> +      else if ((insn & 0x0fff0000) == 0x08bd0000)
> +	/* POP (LDMIA).  */
> +	found_stack_adjust = 1;
> +    }
> +
> +  if (found_stack_adjust)
> +    return 1;
> +
> +  return 0;
> +}
> +
> +
>  /* When arguments must be pushed onto the stack, they go on in reverse
>     order.  The code below implements a FILO (stack) to do this.  */
>  
> @@ -6881,6 +7078,9 @@ arm_gdbarch_init (struct gdbarch_info in
>    /* Advance PC across function entry code.  */
>    set_gdbarch_skip_prologue (gdbarch, arm_skip_prologue);
>  
> +  /* Detect whether PC is in function epilogue.  */
> +  set_gdbarch_in_function_epilogue_p (gdbarch, arm_in_function_epilogue_p);
> +
>    /* Skip trampolines.  */
>    set_gdbarch_skip_trampoline_code (gdbarch, arm_skip_stub);
>  
> 

Re: [rfa, v3] Fix software-watchpoint failures on ARM by adding epilogue detection

[Ulrich Weigand]

Richard Earnshaw wrote:
> On Thu, 2010-10-07 at 18:12 +0200, Ulrich Weigand wrote:
> > 2010-10-07  Ulrich Weigand  <uweigand@de.ibm.com>
> >             Daniel Jacobowitz  <dan@codesourcery.com>
> > 
> > 	* arm-tdep.c (thumb_in_function_epilogue_p)
> > 	(arm_in_function_epilogue_p): New.
> > 	(arm_gdbarch_init): Install arm_in_function_epilogue_p as
> > 	gdbarch_in_function_epilogue_p callback.
> > 
> 
> This is OK.  There are some sequences that could be generated that this
> can't handle, and I don't think we'd ever get a stack decrement in an
> epilogue sequence.
> 
> An example I don't think your code can handle is stack adjustments that
> are not valid immediates for ADD SP, #n.  So very large stack adjusts my
> fail.  These sequences will load or create an immediate in a register
> (particularly on Thumb1) and then add that to SP.

Thanks for the review; I've checked the patch in now.  We can always
go back and add more instructions later ...

Bye,
Ulrich

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