Fix instruction skipping when using software single step in GDBServer

Without this patch, when doing a software single step, with for example
a conditional breakpoint, gdbserver would wrongly avance the pc of
breakpoint_len and skips an instruction.

This is due to gdbserver assuming that it's hardware single stepping.
When it resumes from the breakpoint address it expects the trap to be
caused by ptrace and if it's rather caused by a software breakpoint
it assumes this is a permanent breakpoint and that it needs to skip
over it.

However when software single stepping, this breakpoint is legitimate as
it's the reinsert breakpoint gdbserver has put in place to break at
the next instruction. Thus gdbserver wrongly advances the pc and skips
an instruction.

This patch fixes this behavior so that gdbserver checks if it is a
reinsert breakpoint from software single stepping. If it is it won't
advance the pc. And if there's no reinsert breakpoint there we assume
then that it's a permanent breakpoint and advance the pc.

Here's a commented log of what would happen before and after the fix on
gdbserver :

/* Here there is a conditional breakpoint at 0x10428 that needs to be
stepped over. */

Need step over [LWP 11204]? yes, found breakpoint at 0x10428
...
/* e7f001f0 is a breakpoint instruction on arm
   Here gdbserver writes the software breakpoint we would like to hit
*/
Writing e7f001f0 to 0x0001042c in process 11204
...
Resuming lwp 11220 (continue, signal 0, stop not expected)
  pending reinsert at 0x10428
stop pc is 00010428
  continue from pc 0x10428
...

/* Here gdbserver hit the software breakpoint that was in place
   for the step over */

stop pc is 0001042c
pc is 0x1042c
step-over for LWP 11220.11220 executed software breakpoint
Finished step over.
Could not find fast tracepoint jump at 0x10428 in list (reinserting).

/* Here gdbserver writes back the original instruction */
Writing e50b3008 to 0x0001042c in process 11220
Step-over finished.
Need step over [LWP 11220]? No

/* Here because gdbserver assumes this is a permenant breakpoint it advances
the pc of breakpoint_len, in this case 4 bytes, so we have just skipped
the instruction that was written back here :
Writing e50b3008 to 0x0001042c in process 11220
*/

stop pc is 00010430
pc is 0x10430
Need step over [LWP 11220]? No, no breakpoint found at 0x10430
Proceeding, no step-over needed
proceed_one_lwp: lwp 11220
stop pc is 00010430

This patch fixes this situation and we get the right behavior :

Writing e50b3008 to 0x0001042c in process 11245
Hit a gdbserver breakpoint.
Hit a gdbserver breakpoint.
Step-over finished.
proceeding all threads.
Need step over [LWP 11245]? No
stop pc is 0001042c
pc is 0x1042c
Need step over [LWP 11245]? No, no breakpoint found at 0x1042c
Proceeding, no step-over needed
proceed_one_lwp: lwp 11245
stop pc is 0001042c
pc is 0x1042c
Resuming lwp 11245 (continue, signal 0, stop not expected)
stop pc is 0001042c
  continue from pc 0x1042c

It also works if the value at 0x0001042c is a permanent breakpoint.
If so gdbserver will finish the step over, remove the reinserted breakpoint,
resume at that location and on the next SIGTRAP gdbserver will trigger
the advance PC condition as reinsert_breakpoint_inserted_here will be false.

I also tested this against bp-permanent.exp on arm (with a work in progress
software single step patchset) without any regressions.

It's also tested against x86 bp-permanent.exp without any regression.

So both software and hardware single step are tested.

No regressions on Ubuntu 14.04 on ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/gdbserver/ChangeLog:

	* linux-low.c (linux_wait_1): Fix pc advance condition.
	* mem-break.c (reinsert_breakpoint_inserted_here): New function.
	* mem-break.h (reinsert_breakpoint_inserted_here): New declaration.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 87ce9e7..f48d7f2 100644 (file)
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,11 @@
 2015-11-30  Antoine Tremblay  <antoine.tremblay@ericsson.com>
 
+       * linux-low.c (linux_wait_1): Fix pc advance condition.
+       * mem-break.c (reinsert_breakpoint_inserted_here): New function.
+       * mem-break.h (reinsert_breakpoint_inserted_here): New declaration.
+
+2015-11-30  Antoine Tremblay  <antoine.tremblay@ericsson.com>
+
        * linux-arm-low.c (arm_is_thumb_mode): New function.
        (arm_breakpoint_at): Use arm_is_thumb_mode.
        (arm_breakpoint_kind_from_current_state): New function.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 207a5ba..b29f54e 100644 (file)
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3071,14 +3071,21 @@ linux_wait_1 (ptid_t ptid,
       return ptid_of (current_thread);
     }
 
-  /* If step-over executes a breakpoint instruction, it means a
-     gdb/gdbserver breakpoint had been planted on top of a permanent
-     breakpoint.  The PC has been adjusted by
-     check_stopped_by_breakpoint to point at the breakpoint address.
-     Advance the PC manually past the breakpoint, otherwise the
-     program would keep trapping the permanent breakpoint forever.  */
+  /* If step-over executes a breakpoint instruction, in the case of a
+     hardware single step it means a gdb/gdbserver breakpoint had been
+     planted on top of a permanent breakpoint, in the case of a software
+     single step it may just mean that gdbserver hit the reinsert breakpoint.
+     The PC has been adjusted by check_stopped_by_breakpoint to point at
+     the breakpoint address.
+     So in the case of the hardware single step advance the PC manually
+     past the breakpoint and in the case of software single step advance only
+     if it's not the reinsert_breakpoint we are hitting.
+     This avoids that a program would keep trapping a permanent breakpoint
+     forever.  */
   if (!ptid_equal (step_over_bkpt, null_ptid)
-      && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
+      && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT
+      && (event_child->stepping
+         || !reinsert_breakpoint_inserted_here (event_child->stop_pc)))
     {
       int increment_pc = 0;
       int breakpoint_kind = 0;

diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 11c21db..ea1140a 100644 (file)
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -1662,6 +1662,23 @@ hardware_breakpoint_inserted_here (CORE_ADDR addr)
   return 0;
 }
 
+/* See mem-break.h.  */
+
+int
+reinsert_breakpoint_inserted_here (CORE_ADDR addr)
+{
+  struct process_info *proc = current_process ();
+  struct breakpoint *bp;
+
+  for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
+    if (bp->type == reinsert_breakpoint
+       && bp->raw->pc == addr
+       && bp->raw->inserted)
+      return 1;
+
+  return 0;
+}
+
 static int
 validate_inserted_breakpoint (struct raw_breakpoint *bp)
 {

diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index d199cc4..40b66a7 100644 (file)
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -100,6 +100,10 @@ int software_breakpoint_inserted_here (CORE_ADDR addr);
 
 int hardware_breakpoint_inserted_here (CORE_ADDR addr);
 
+/* Returns TRUE if there's any reinsert breakpoint at ADDR.  */
+
+int reinsert_breakpoint_inserted_here (CORE_ADDR addr);
+
 /* Clear all breakpoint conditions and commands associated with a
    breakpoint.  */