Re: [PATCH] Support targets that know how to step over breakpoints

[Luis Machado <lgustavo at codesourcery dot com>]

On 10/30/2012 08:57 AM, Luis Machado wrote:
> On 10/17/2012 08:42 AM, Luis Machado wrote:
> > Ping?
> >
> > On 10/04/2012 09:48 AM, Luis Machado wrote:
> > > Hi,
> > >
> > > Most of the targets we deal with need to be told to lift off breakpoints
> > > from the inferior prior to single-stepping. A few targets are able to
> > > just step over those breakpoints without being told so.
> > >
> > > In the latter case, GDB assumes the target knows how to manage
> > > breakpoints on its own.
> > >
> > > gdbserver does not know how to do this unless we force this mechanism
> > > into it, but, honestly, i don't see the point. Usually targets that know
> > > how to step over breakpoints do so via a more low level interface like
> > > the kernel. Let me know if you think otherwise.
>
> Ping? http://sourceware.org/ml/gdb-patches/2012-10/msg00067.html

Meanwhile i've updated this patch for the latest cvs head.

I'm wondering if the patch is too ugly for someone to take a look at it 
or if it is too odd a feature to add. I suppose not.

Hopefully i can get some traction with this new refreshed and shiny 
version! :-)

Luis

2012-11-27  Luis Machado  <lgustavo@codesourcery.com>
	    Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* 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/
	* gdb.texinfo (General Query Packets): Document
	StepOverBreakpoints under qSupported.

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

Index: gdb_head/gdb/doc/gdb.texinfo
===================================================================
--- gdb_head.orig/gdb/doc/gdb.texinfo	2012-11-27 09:24:30.955089370 -0200
+++ gdb_head/gdb/doc/gdb.texinfo	2012-11-27 09:33:05.411095450 -0200
@@ -37083,6 +37083,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:
@@ -37237,6 +37242,17 @@ See @ref{Bytecode Descriptions} for deta
 The remote stub supports running a breakpoint's command list itself,
 rather than reporting the hit to @value{GDBN}.
 
+@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: gdb_head/gdb/doc/gdbint.texinfo
===================================================================
--- gdb_head.orig/gdb/doc/gdbint.texinfo	2012-10-10 15:35:25.384415678 -0300
+++ gdb_head/gdb/doc/gdbint.texinfo	2012-11-27 09:33:05.415095451 -0200
@@ -593,6 +593,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: gdb_head/gdb/infrun.c
===================================================================
--- gdb_head.orig/gdb/infrun.c	2012-11-27 09:24:30.987089370 -0200
+++ gdb_head/gdb/infrun.c	2012-11-27 09:33:05.423095450 -0200
@@ -1786,7 +1786,8 @@ a command like `return' or `jump' to con
      We can't use displaced stepping when we are waiting for vfork_done
      event, displaced stepping breaks the vfork child similarly as single
      step software breakpoint.  */
-  if (use_displaced_stepping (gdbarch)
+  if (!target_can_step_over_breakpoints ()
+      && use_displaced_stepping (gdbarch)
       && (tp->control.trap_expected
 	  || (step && gdbarch_software_single_step_p (gdbarch)))
       && sig == GDB_SIGNAL_0
@@ -1912,16 +1913,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->control.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 (aspace, pc))
 	    step = 0;
 	}
 
-      if (debug_displaced
+      if (!target_can_step_over_breakpoints ()
+	  && debug_displaced
           && use_displaced_stepping (gdbarch)
           && tp->control.trap_expected)
         {
@@ -2219,19 +2227,22 @@ proceed (CORE_ADDR addr, enum gdb_signal
   if (oneproc)
     {
       tp->control.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->control.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->control.trap_expected
+      || target_can_step_over_breakpoints ()
+      || use_displaced_stepping (gdbarch))
     insert_breakpoints ();
 
   if (!non_stop)
@@ -3941,10 +3952,12 @@ handle_inferior_event (struct execution_
 	      singlestep_breakpoints_inserted_p = 0;
 	    }
 
-	  /* If the arch can displace step, don't remove the
-	     breakpoints.  */
+	  /* If the target can step over breakpoints transparently, or
+	     we can use displace stepping with this arch, don't remove
+	     the breakpoints.  */
 	  thread_regcache = get_thread_regcache (ecs->ptid);
-	  if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
+	  if (!target_can_step_over_breakpoints ()
+	      && !use_displaced_stepping (get_regcache_arch (thread_regcache)))
 	    remove_status = remove_breakpoints ();
 
 	  /* Did we fail to remove breakpoints?  If so, try
@@ -5715,10 +5728,12 @@ keep_going (struct execution_control_sta
 	{
 	  struct regcache *thread_regcache = get_thread_regcache (ecs->ptid);
 
-	  if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
-	    /* 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 (get_regcache_arch (thread_regcache)))
+	    /* 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: gdb_head/gdb/remote.c
===================================================================
--- gdb_head.orig/gdb/remote.c	2012-11-27 09:24:31.007089371 -0200
+++ gdb_head/gdb/remote.c	2012-11-27 09:33:05.423095450 -0200
@@ -319,6 +319,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;
+
   /* True if the stub reports support for conditional tracepoints.  */
   int cond_tracepoints;
 
@@ -3879,6 +3883,15 @@ remote_string_tracing_feature (const str
   rs->string_tracing = (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,
@@ -3909,6 +3922,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 },
   { "qXfer:siginfo:read", PACKET_DISABLE, remote_supported_packet,
     PACKET_qXfer_siginfo_read },
   { "qXfer:siginfo:write", PACKET_DISABLE, remote_supported_packet,
@@ -4236,6 +4251,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;
   rs->ctrlc_pending_p = 0;
 
   general_thread = not_sent_ptid;
@@ -11001,6 +11017,13 @@ remote_can_use_agent (void)
   return (remote_protocol_packets[PACKET_QAgent].support != PACKET_DISABLE);
 }
 
+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)
 {
@@ -11070,6 +11093,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;
   remote_ops.to_supports_disable_randomization
     = remote_supports_disable_randomization;
   remote_ops.to_fileio_open = remote_hostio_open;
Index: gdb_head/gdb/target.c
===================================================================
--- gdb_head.orig/gdb/target.c	2012-11-27 09:24:31.023089369 -0200
+++ gdb_head/gdb/target.c	2012-11-27 09:33:05.427095452 -0200
@@ -673,6 +673,7 @@ update_current_target (void)
       INHERIT (to_supports_multi_process, t);
       INHERIT (to_supports_enable_disable_tracepoint, t);
       INHERIT (to_supports_string_tracing, t);
+      INHERIT (to_can_step_over_breakpoints, t);
       INHERIT (to_trace_init, t);
       INHERIT (to_download_tracepoint, t);
       INHERIT (to_can_download_tracepoint, t);
@@ -930,6 +931,9 @@ update_current_target (void)
   de_fault (to_traceframe_info,
 	    (struct traceframe_info * (*) (void))
 	    tcomplain);
+  de_fault (to_can_step_over_breakpoints,
+	    (int (*) (void))
+	    return_zero);
   de_fault (to_supports_evaluation_of_breakpoint_conditions,
 	    (int (*) (void))
 	    return_zero);
Index: gdb_head/gdb/target.h
===================================================================
--- gdb_head.orig/gdb/target.h	2012-11-27 09:24:31.023089369 -0200
+++ gdb_head/gdb/target.h	2012-11-27 09:33:05.431095451 -0200
@@ -857,6 +857,11 @@ struct target_ops
     /* Is the target able to use agent in current state?  */
     int (*to_can_use_agent) (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?
      */
@@ -1011,6 +1016,12 @@ int target_supports_disable_randomizatio
 #define target_can_run_breakpoint_commands() \
   (*current_target.to_can_run_breakpoint_commands) ()
 
+/* Return true if GDB does not need to lift breakpoints from 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) ()
+
 /* Invalidate all target dcaches.  */
 extern void target_dcache_invalidate (void);