This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Target can step over breakpoints itself


On Tuesday 28 October 2008 14:17:37, Daniel Jacobowitz wrote:
> On Mon, Oct 27, 2008 at 02:19:00PM +0000, Pedro Alves wrote:
> > The attached patch adds a new "StepOverBreakpoints" feature to the
> > remote protocol to the remote stub can tell GDB that the debug api has
> > that feature, and exposes that knowledge to the rest of GDB by
> > adding a new target_ops method (target_can_step_over_breakpoints),
> > and teaching infrun.c to it doesn't need to do the traditional hold-and-step
> > dance (remove breakpoints, single-step, insert breakpoints) from the
> > inferior(s), if the target reports support for stepping over them
> > itself.
> > 
> > Any objections/thoughts on this?
> 
> Does DICOS hit breakpoints at the current PC if you continue, rather
> than step?  I'd hope so - and if so, let's document that expectation.
> It's details like that which confuse people trying to implement the
> remote protocol.

Yeah, thanks for the hint.  It doesn't hit the breakpoint if proceeding
at the PC where the thread stopped (an equivalent to stop_pc on the
GDB side), but will hit it if telling it to resume at some other PC
with a breakpoint there (e.g., jump *PC).  Since GDB normally single-steps
to move past a breakpoint at stop_pc, this hardly matters for DICOS
(other cases where GDB expects to hit a breakpoint at PC happen around
unix signals, but DICOS doesn't have a notion of unix signals).

What I think we should do, is document that the feature only
applies to single-stepping, as attached.

> 
> Does this need to be conditional on the type of breakpoint (hardware
> vs software, Z0 vs memory) or on the actual breakpoint?  For instance,
> platforms with per-thread hardware breakpoints can 'step over'
> hardware breakpoints by temporarily removing them from just one
> thread; and I believe setting IA64_PSR_DD lets you step or continue
> over a breakpoint.
> 

This feature is meant to apply to all breakpoints the stub side
supports.  Although it can be seen as an optimization in
all-stop mode, this is an alternative to displaced stepping
on the GDB side, for non-stop mode --- where we need to make sure
running threads wouldn't miss a breakpoint.
Memory breakpoints are off the chart.  Having a remote stub
support this feature with memory breakpoints on the GDB side
would be too much of a hack.

How about something like this?

@item StepOverBreakpoints
The remote stub knows how to step over breakpoints itself.  If this
feature is supported by the target, then @value{GDBN} does not need to
lift breakpoints off of the inferior to step over them.  This feature
only applies to single-step requests.  @value{GDBN} assumes that the
target hits breakpoints at the current PC if told to continue, rather
than single-step.  This feature is only defined when the remote stub
supports managing breakpoints itself (see @ref{insert breakpoint or
watchpoint packet}).  @value{GDBN} assumes that this feature is
applicable to all breakpoints types supported by the stub.

-- 
Pedro Alves
gdb/
2008-11-03  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (resume, proceed, keep_going, handle_inferior_event):
	Check if target can step over breakpoints.

	* remote.c (struct remote_state): New member
	can_step_over_breakpoints.
	(remote_step_over_breakpoints_feature): New.
	(remote_protocol_features): Add StepOverBreakpoints feature.
	(remote_open_1): Clear can_step_over_breakpoints.
	(remote_can_step_over_breakpoints): New.
	(init_remote_ops): Set remote_ops.to_can_step_over_breakpoints to
	remote_can_step_over_breakpoints.

	* target.c (update_current_target): Inherit
	to_can_step_over_breakpoints, and default it to return 0.

	* target.h (struct target_ops): Add new
	to_can_step_over_breakpoints member.
	(target_can_step_over_breakpoints): New.

gdb/doc/
2008-11-03  Pedro Alves  <pedro@codesourcery.com>

	* gdb.texinfo (General Query Packets): Document
	StepOverBreakpoints under qSupported.

	* gdbint.texinfo (Single Stepping): Document
	target_can_step_over_breakpoints.

---
 gdb/doc/gdb.texinfo    |   16 +++++++++++++
 gdb/doc/gdbint.texinfo |    9 +++++++
 gdb/infrun.c           |   59 ++++++++++++++++++++++++++++++-------------------
 gdb/remote.c           |   24 +++++++++++++++++++
 gdb/target.c           |    4 +++
 gdb/target.h           |   11 +++++++++
 6 files changed, 101 insertions(+), 22 deletions(-)

Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2008-11-03 13:24:04.000000000 +0000
+++ src/gdb/doc/gdb.texinfo	2008-11-03 15:09:52.000000000 +0000
@@ -26118,6 +26118,11 @@ These are the currently defined stub fea
 @tab @samp{-}
 @tab No
 
+@item @samp{StepOverBreakpoints}
+@tab No
+@tab @samp{-}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -26183,6 +26188,17 @@ debugging of more than one process at a 
 multiprocess extensions in packet replies unless @value{GDBN} has also
 indicated it supports them in its @samp{qSupported} request.
 
+@item StepOverBreakpoints
+The remote stub knows how to step over breakpoints itself.  If this
+feature is supported by the target, then @value{GDBN} does not need to
+lift breakpoints off of the inferior to step over them.  This feature
+only applies to single-step requests.  @value{GDBN} assumes that the
+target hits breakpoints at the current PC if told to continue, rather
+than single-step.  This feature is only defined when the remote stub
+supports managing breakpoints itself (see @ref{insert breakpoint or
+watchpoint packet}).  @value{GDBN} assumes that this feature is
+applicable to all breakpoints types supported by the stub.
+
 @end table
 
 @item qSymbol::
Index: src/gdb/doc/gdbint.texinfo
===================================================================
--- src.orig/gdb/doc/gdbint.texinfo	2008-11-03 13:24:04.000000000 +0000
+++ src/gdb/doc/gdbint.texinfo	2008-11-03 14:12:57.000000000 +0000
@@ -554,6 +554,15 @@ but @code{placed_size} may be.
 
 @section Single Stepping
 
+@table @code
+@cindex stepping over breakpoints
+@findex target_can_step_over_breakpoints
+@item target_can_step_over_breakpoints
+Returns true if the target handles stepping over breakpoints
+transparently, hence @value{GDBN} does not need to lift breakpoints
+off the inferior when single-stepping over a breakpoint.
+@end table
+
 @section Signal Handling
 
 @section Thread Handling
Index: src/gdb/target.h
===================================================================
--- src.orig/gdb/target.h	2008-11-03 13:24:04.000000000 +0000
+++ src/gdb/target.h	2008-11-03 14:12:57.000000000 +0000
@@ -540,6 +540,11 @@ struct target_ops
        simultaneously?  */
     int (*to_supports_multi_process) (void);
 
+    /* Return true if GDB does not need to lift breakpoints from the
+       the inferior while stepping over a breakpoint; the target
+       handles that transparently.  */
+    int (*to_can_step_over_breakpoints) (void);
+
     int to_magic;
     /* Need sub-structure for target machine related rather than comm related?
      */
@@ -657,6 +662,12 @@ extern void target_resume (ptid_t ptid, 
 #define	target_supports_multi_process()	\
      (*current_target.to_supports_multi_process) ()
 
+/* Return true if GDB does not need to lift breakpoints from the the
+   inferior while stepping over a breakpoint; the target handles that
+   transparently.  */
+#define target_can_step_over_breakpoints() \
+     (*current_target.to_can_step_over_breakpoints) ()
+
 extern DCACHE *target_dcache;
 
 extern int target_read_string (CORE_ADDR, char **, int, int *);
Index: src/gdb/target.c
===================================================================
--- src.orig/gdb/target.c	2008-11-03 14:12:48.000000000 +0000
+++ src/gdb/target.c	2008-11-03 14:12:57.000000000 +0000
@@ -472,6 +472,7 @@ update_current_target (void)
       INHERIT (to_get_ada_task_ptid, t);
       /* Do not inherit to_search_memory.  */
       INHERIT (to_supports_multi_process, t);
+      INHERIT (to_can_step_over_breakpoints, t);
       INHERIT (to_magic, t);
       /* Do not inherit to_memory_map.  */
       /* Do not inherit to_flash_erase.  */
@@ -642,6 +643,9 @@ update_current_target (void)
   de_fault (to_supports_multi_process,
 	    (int (*) (void))
 	    return_zero);
+  de_fault (to_can_step_over_breakpoints,
+	    (int (*) (void))
+	    return_zero);
 #undef de_fault
 
   /* Finally, position the target-stack beneath the squashed
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-11-03 13:27:20.000000000 +0000
+++ src/gdb/infrun.c	2008-11-03 14:12:57.000000000 +0000
@@ -1009,7 +1009,8 @@ a command like `return' or `jump' to con
      the comments for displaced_step_prepare explain why.  The
      comments in the handle_inferior event for dealing with 'random
      signals' explain what we do instead.  */
-  if (use_displaced_stepping (gdbarch)
+  if (!target_can_step_over_breakpoints ()
+      && use_displaced_stepping (gdbarch)
       && tp->trap_expected
       && sig == TARGET_SIGNAL_0)
     {
@@ -1123,16 +1124,23 @@ a command like `return' or `jump' to con
 	  resume_ptid = inferior_ptid;
 	}
 
-      if (gdbarch_cannot_step_breakpoint (gdbarch))
+      /* Most targets can step a breakpoint instruction, thus
+	 executing it normally (hitting the breakpoint).  But if this
+	 one cannot, just continue and we will hit it anyway.  */
+      /* A target that executes the instruction under a breakpoint
+	 automatically when told to single-step, by definition also
+	 must be told to continue, otherwise the breakpoint won't be
+	 hit.  */
+      if (gdbarch_cannot_step_breakpoint (gdbarch)
+	  || (target_can_step_over_breakpoints ()
+	      && !tp->trap_expected))
 	{
-	  /* Most targets can step a breakpoint instruction, thus
-	     executing it normally.  But if this one cannot, just
-	     continue and we will hit it anyway.  */
 	  if (step && breakpoint_inserted_here_p (pc))
 	    step = 0;
 	}
 
-      if (debug_displaced
+      if (!target_can_step_over_breakpoints ()
+	  && debug_displaced
           && use_displaced_stepping (gdbarch)
           && tp->trap_expected)
         {
@@ -1329,19 +1337,22 @@ proceed (CORE_ADDR addr, enum target_sig
   if (oneproc)
     {
       tp->trap_expected = 1;
-      /* If displaced stepping is enabled, we can step over the
-	 breakpoint without hitting it, so leave all breakpoints
-	 inserted.  Otherwise we need to disable all breakpoints, step
-	 one instruction, and then re-add them when that step is
-	 finished.  */
-      if (!use_displaced_stepping (gdbarch))
+      /* If displaced stepping is enabled or the target can step over
+	 breakpoints, we can step over the breakpoint without hitting
+	 it, so leave all breakpoints inserted.  Otherwise we need to
+	 disable all breakpoints, step one instruction, and then
+	 re-add them when that step is finished.  */
+      if (!target_can_step_over_breakpoints ()
+	  && !use_displaced_stepping (gdbarch))
 	remove_breakpoints ();
     }
 
   /* We can insert breakpoints if we're not trying to step over one,
-     or if we are stepping over one but we're using displaced stepping
-     to do so.  */
-  if (! tp->trap_expected || use_displaced_stepping (gdbarch))
+     or if we are stepping over one but the target can step over them
+     transparently or we're using displaced stepping to do so.  */
+  if (! tp->trap_expected
+      || target_can_step_over_breakpoints ()
+      || use_displaced_stepping (gdbarch))
     insert_breakpoints ();
 
   if (!non_stop)
@@ -2615,9 +2626,11 @@ targets should add new threads to the th
 	      singlestep_breakpoints_inserted_p = 0;
 	    }
 
-	  /* If the arch can displace step, don't remove the
-	     breakpoints.  */
-	  if (!use_displaced_stepping (current_gdbarch))
+	  /* If the target can step over breakpoints transparently, or
+	     we can use displace stepping with this arch, don't remove
+	     the breakpoints.  */
+	  if (!target_can_step_over_breakpoints ()
+	      && !use_displaced_stepping (current_gdbarch))
 	    remove_status = remove_breakpoints ();
 
 	  /* Did we fail to remove breakpoints?  If so, try
@@ -3900,10 +3913,12 @@ keep_going (struct execution_control_sta
       
       if (ecs->event_thread->stepping_over_breakpoint)
 	{
-	  if (! use_displaced_stepping (current_gdbarch))
-	    /* Since we can't do a displaced step, we have to remove
-	       the breakpoint while we step it.  To keep things
-	       simple, we remove them all.  */
+	  if (!target_can_step_over_breakpoints ()
+	      && !use_displaced_stepping (current_gdbarch))
+	    /* Since neither the target can step over breakpoints
+	       transparently, neither can we do a displaced step, we
+	       have to remove the breakpoint while we step it.  To
+	       keep things simple, we remove them all.  */
 	    remove_breakpoints ();
 	}
       else
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-11-03 14:12:48.000000000 +0000
+++ src/gdb/remote.c	2008-11-03 14:12:57.000000000 +0000
@@ -301,6 +301,10 @@ struct remote_state
 
   /* True if the stub reports support for vCont;t.  */
   int support_vCont_t;
+
+  /* True if the stub reports support for stepping over breakpoints
+     transparently.  */
+  int can_step_over_breakpoints;
 };
 
 /* Returns true if the multi-process extensions are in effect.  */
@@ -2932,6 +2936,15 @@ remote_non_stop_feature (const struct pr
   rs->non_stop_aware = (support == PACKET_ENABLE);
 }
 
+static void
+remote_step_over_breakpoints_feature (const struct protocol_feature *feature,
+				      enum packet_support support,
+				      const char *value)
+{
+  struct remote_state *rs = get_remote_state ();
+  rs->can_step_over_breakpoints = (support == PACKET_ENABLE);
+}
+
 static struct protocol_feature remote_protocol_features[] = {
   { "PacketSize", PACKET_DISABLE, remote_packet_size, -1 },
   { "qXfer:auxv:read", PACKET_DISABLE, remote_supported_packet,
@@ -2952,6 +2965,8 @@ static struct protocol_feature remote_pr
     PACKET_QStartNoAckMode },
   { "multiprocess", PACKET_DISABLE, remote_multi_process_feature, -1 },
   { "QNonStop", PACKET_DISABLE, remote_non_stop_feature, -1 },
+  { "StepOverBreakpoints", PACKET_DISABLE,
+    remote_step_over_breakpoints_feature, -1 },
 };
 
 static void
@@ -3185,6 +3200,7 @@ remote_open_1 (char *name, int from_tty,
   rs->extended = extended_p;
   rs->non_stop_aware = 0;
   rs->waiting_for_stop_reply = 0;
+  rs->can_step_over_breakpoints = 0;
 
   general_thread = not_sent_ptid;
   continue_thread = not_sent_ptid;
@@ -8566,6 +8582,13 @@ remote_supports_multi_process (void)
   return remote_multi_process_p (rs);
 }
 
+static int
+remote_can_step_over_breakpoints (void)
+{
+  struct remote_state *rs = get_remote_state ();
+  return rs->can_step_over_breakpoints;
+}
+
 static void
 init_remote_ops (void)
 {
@@ -8629,6 +8652,7 @@ Specify the serial device it is connecte
   remote_ops.to_terminal_ours = remote_terminal_ours;
   remote_ops.to_supports_non_stop = remote_supports_non_stop;
   remote_ops.to_supports_multi_process = remote_supports_multi_process;
+  remote_ops.to_can_step_over_breakpoints = remote_can_step_over_breakpoints;
 }
 
 /* Set up the extended remote vector by making a copy of the standard

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]