mips-tdep.c: Fix sw watchpoint-out-of-scope events

mips-tdep.c: Fix sw watchpoint-out-of-scope events

[Maciej W. Rozycki]

Hello,

 This is a fix for software watchpoint-out-of-scope events for MIPS 
platforms.  It implements a in_function_epilogue_p() hook which is used to 
single-step a function's epilogue once a local watchpoint has been 
destroyed (because of the restoration of the frame pointer).  As a result 
the event is only reported once the caller has been reached back.

 This change has been tested natively for mips-linux-gnu and remotely for 
mipsisa32-sde-elf, with mips-sim-sde32/-EB, mips-sim-sde32/-EB/-mips16, 
mips-sim-sde32/-EL and mips-sim-sde32/-EL/-mips16.  It fixes sw watchpoint 
regressions in gdb.mi/mi*-watch.exp:

FAIL: gdb.mi/mi-watch.exp: wp out of scope (2)

for the remote targets.  It does not fix them for Linux, because the o32 
calling convention makes the restoration of the GOT pointer in the caller 
("lw gp,..." after "jalr") a part of the function call.  Therefore 
execution stops in the right function, but one instruction too early, 
still within the source line of the call.  It is still a step forward 
though and less of nuisance for the user.

2007-09-11  Maciej W. Rozycki  <macro@mips.com>

	* mips-tdep.c (mips32_in_function_epilogue_p): New function.
	(mips16_in_function_epilogue_p): Likewise.
	(mips_in_function_epilogue_p): Likewise.
	(mips_gdbarch_init): Register mips_in_function_epilogue_p().

 OK to apply?

  Maciej

14677.diff
Index: binutils-quilt/src/gdb/mips-tdep.c
===================================================================
--- binutils-quilt.orig/src/gdb/mips-tdep.c	2007-09-11 14:16:22.000000000 +0100
+++ binutils-quilt/src/gdb/mips-tdep.c	2007-09-11 14:17:42.000000000 +0100
@@ -4883,6 +4883,95 @@
     return mips32_scan_prologue (pc, limit_pc, NULL, NULL);
 }
 
+/* Check whether the PC is in a function epilogue (32-bit version).
+   This is a helper function for mips_in_function_epilogue_p.  */
+static int
+mips32_in_function_epilogue_p (CORE_ADDR pc)
+{
+  CORE_ADDR func_addr = 0, func_end = 0;
+
+  if (find_pc_partial_function (pc, NULL, &func_addr, &func_end))
+    {
+      /* The MIPS epilogue is max. 12 bytes long.  */
+      CORE_ADDR addr = func_end - 12;
+
+      if (addr < func_addr + 4)
+        addr = func_addr + 4;
+      if (pc < addr)
+        return 0;
+
+      for (; pc < func_end; pc += MIPS_INSN32_SIZE)
+	{
+	  unsigned long high_word;
+	  unsigned long inst;
+
+	  inst = mips_fetch_instruction (pc);
+	  high_word = (inst >> 16) & 0xffff;
+
+	  if (high_word != 0x27bd	/* addiu $sp,$sp,offset */
+	      && high_word != 0x67bd	/* daddiu $sp,$sp,offset */
+	      && inst != 0x03e00008	/* jr $ra */
+	      && inst != 0x00000000)	/* nop */
+	    return 0;
+	}
+
+      return 1;
+    }
+
+  return 0;
+}
+
+/* Check whether the PC is in a function epilogue (16-bit version).
+   This is a helper function for mips_in_function_epilogue_p.  */
+static int
+mips16_in_function_epilogue_p (CORE_ADDR pc)
+{
+  CORE_ADDR func_addr = 0, func_end = 0;
+
+  if (find_pc_partial_function (pc, NULL, &func_addr, &func_end))
+    {
+      /* The MIPS epilogue is max. 12 bytes long.  */
+      CORE_ADDR addr = func_end - 12;
+
+      if (addr < func_addr + 4)
+        addr = func_addr + 4;
+      if (pc < addr)
+        return 0;
+
+      for (; pc < func_end; pc += MIPS_INSN16_SIZE)
+	{
+	  unsigned short inst;
+
+	  inst = mips_fetch_instruction (pc);
+
+	  if ((inst & 0xf800) == 0xf000)	/* extend */
+	    continue;
+
+	  if (inst != 0x6300		/* addiu $sp,offset */
+	      && inst != 0xfb00		/* daddiu $sp,$sp,offset */
+	      && inst != 0xe820		/* jr $ra */
+	      && inst != 0xe8a0		/* jrc $ra */
+	      && inst != 0x6500)	/* nop */
+	    return 0;
+	}
+
+      return 1;
+    }
+
+  return 0;
+}
+
+/* The epilogue is defined here as the area at the end of a function,
+   after an instruction which destroys the function's stack frame.  */
+static int
+mips_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  if (mips_pc_is_mips16 (pc))
+    return mips16_in_function_epilogue_p (pc);
+  else
+    return mips32_in_function_epilogue_p (pc);
+}
+
 /* Root of all "set mips "/"show mips " commands. This will eventually be
    used for all MIPS-specific commands.  */
 
@@ -5995,6 +6084,8 @@
 
   set_gdbarch_skip_prologue (gdbarch, mips_skip_prologue);
 
+  set_gdbarch_in_function_epilogue_p (gdbarch, mips_in_function_epilogue_p);
+
   set_gdbarch_pointer_to_address (gdbarch, signed_pointer_to_address);
   set_gdbarch_address_to_pointer (gdbarch, address_to_signed_pointer);
   set_gdbarch_integer_to_address (gdbarch, mips_integer_to_address);

Re: mips-tdep.c: Fix sw watchpoint-out-of-scope events

[Daniel Jacobowitz]

On Tue, Sep 11, 2007 at 05:23:54PM +0100, Maciej W. Rozycki wrote:
>  This is a fix for software watchpoint-out-of-scope events for MIPS 
> platforms.  It implements a in_function_epilogue_p() hook which is used to 
> single-step a function's epilogue once a local watchpoint has been 
> destroyed (because of the restoration of the frame pointer).  As a result 
> the event is only reported once the caller has been reached back.

This patch is fine.  I wish it wasn't necessary, but for now it is.

> for the remote targets.  It does not fix them for Linux, because the o32 
> calling convention makes the restoration of the GOT pointer in the caller 
> ("lw gp,..." after "jalr") a part of the function call.  Therefore 
> execution stops in the right function, but one instruction too early, 
> still within the source line of the call.  It is still a step forward 
> though and less of nuisance for the user.

Hmm, do you think we should accept that line as a pass then?  The same
will happen on other architectures too.

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips-tdep.c: Fix sw watchpoint-out-of-scope events

[Maciej W. Rozycki]

On Tue, 18 Sep 2007, Daniel Jacobowitz wrote:

> This patch is fine.  I wish it wasn't necessary, but for now it is.

 Well, actually then, I have thought, rather than trying to fix the 
unfixable, why don't we mimic the behaviour of hardware watchpoints?  As 
they do not single-step (the whole point of them, at least for the write 
watchpoints) a scope software breakpoint is placed at the return target of 
the scope covered.  We could do the same for software watchpoints -- 
insert a scope software breakpoint, single-step till the watchpoints goes 
out of scope and then resume at the full speed till the scope breakpoint 
is hit.  Was there any particular reason for not doing so in the first 
place?

 I'll go forward with the patch regardless as it would become obsolete 
automatically if we accepted the hw watchpoint model for sw ones too as 
in_function_epilogue_p() would become irrelevant then as far as I can 
tell.

> Hmm, do you think we should accept that line as a pass then?  The same
> will happen on other architectures too.

 Well, it depends on the policy we have (if we have any in this area) -- 
the patch fixes non-PIC o32 and presumably new ABIs (I'll try to see 
whether I have a way to test it), but does not quite so for PIC o32 
(beacuse of the load to $gp after the return).  There may be other 
architectures with a similar problem, but if MIPS o32 was to be the only 
one affected, then a PASS would be reasonable, as stated originally.  
Though I have no idea how to take care of the load to $gp at the moment -- 
perhaps some logic from the scope breakpoint calculation could be reused.

  Maciej

Re: mips-tdep.c: Fix sw watchpoint-out-of-scope events

[Daniel Jacobowitz]

On Wed, Sep 19, 2007 at 01:31:29PM +0100, Maciej W. Rozycki wrote:
>  Well, actually then, I have thought, rather than trying to fix the 
> unfixable, why don't we mimic the behaviour of hardware watchpoints?  As 
> they do not single-step (the whole point of them, at least for the write 
> watchpoints) a scope software breakpoint is placed at the return target of 
> the scope covered.  We could do the same for software watchpoints -- 
> insert a scope software breakpoint, single-step till the watchpoints goes 
> out of scope and then resume at the full speed till the scope breakpoint 
> is hit.  Was there any particular reason for not doing so in the first 
> place?

Could you give me an example?  The watchpoint shouldn't go out of
scope before the scope breakpoint is hit.  The problem is, instead,
that it appears to change value.  There's two cases.  In the epilogue
of the function containing the watched variable, the stack frame gets
destroyed, so the watchpoint is no longer valid... but we don't have
any marker to indicate that.  In the epilogue of other called
functions, the watchpoint is still valid but we fail to backtrace
correctly so we don't find the original function on the stack.

Both of these can happen for hardware watchpoints too, e.g., if the
user is single stepping or has other breakpoints set.  In practice
they rarely do.

> > Hmm, do you think we should accept that line as a pass then?  The same
> > will happen on other architectures too.
> 
>  Well, it depends on the policy we have (if we have any in this area) -- 
> the patch fixes non-PIC o32 and presumably new ABIs (I'll try to see 
> whether I have a way to test it), but does not quite so for PIC o32 
> (beacuse of the load to $gp after the return).  There may be other 
> architectures with a similar problem, but if MIPS o32 was to be the only 
> one affected, then a PASS would be reasonable, as stated originally.  
> Though I have no idea how to take care of the load to $gp at the moment -- 
> perhaps some logic from the scope breakpoint calculation could be reused.

I think I wasn't clear.  I'm asking if returning to the original call
line, the way MIPS o32 PIC does, should be a PASS.  Other platforms
that have teardown after function calls will show the same behavior.
I think there's at least one other test which allows it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips-tdep.c: Fix sw watchpoint-out-of-scope events

[Maciej W. Rozycki]

On Wed, 19 Sep 2007, Daniel Jacobowitz wrote:

> Could you give me an example?  The watchpoint shouldn't go out of
> scope before the scope breakpoint is hit.  The problem is, instead,

 Well, the scope breakpoint is set up in the caller of the function 
providing the frame containing the variable being watched.  So by 
definition at the point it is hit the watchpoint has always gone away 
already.  This is how it works with hw watchpoints.

> that it appears to change value.  There's two cases.  In the epilogue
> of the function containing the watched variable, the stack frame gets
> destroyed, so the watchpoint is no longer valid... but we don't have
> any marker to indicate that.  In the epilogue of other called
> functions, the watchpoint is still valid but we fail to backtrace
> correctly so we don't find the original function on the stack.

 Correct.  There is actually a third case which is somewhere inbetween -- 
where the function containing the watched variable ends by jumping to a 
sibling call.  For a C program from the language's point of view the 
watchpoint is still in the scope during the execution of the sibling call, 
while in reality its stack frame has gone away already.

> Both of these can happen for hardware watchpoints too, e.g., if the
> user is single stepping or has other breakpoints set.  In practice
> they rarely do.

 True.

> I think I wasn't clear.  I'm asking if returning to the original call
> line, the way MIPS o32 PIC does, should be a PASS.  Other platforms
> that have teardown after function calls will show the same behavior.

 Well, that sounds reasonable given the complication around getting things 
perfect here.  Ideally the out-of-scope event should happen at the same 
place where "finish" from the function in question would stop; in my 
opinion at least.

  Maciej

Re: mips-tdep.c: Fix sw watchpoint-out-of-scope events

[Daniel Jacobowitz]

On Wed, Sep 19, 2007 at 02:05:02PM +0100, Maciej W. Rozycki wrote:
>  Well, that sounds reasonable given the complication around getting things 
> perfect here.  Ideally the out-of-scope event should happen at the same 
> place where "finish" from the function in question would stop; in my 
> opinion at least.

Honestly, I'm not sure we should even be stopping when watchpoints go
out of scope.  Anyway a patch to accept the call site as a PASS is pre-approved.

-- 
Daniel Jacobowitz
CodeSourcery