Re: [PATCH 4/5] range stepping: gdbserver (x86 GNU/Linux)

[Pedro Alves <palves at redhat dot com>]

On 05/23/2013 01:56 AM, Yao Qi wrote:
> On 05/15/2013 03:10 AM, Pedro Alves wrote:
>> @@ -2063,9 +2067,22 @@ handle_v_cont (char *own_buf)
>>           goto err;
>>         resume_info[i].sig = gdb_signal_to_host (sig);
>>       }
>> +      else if (p[0] == 'r')
>> +    {
>> +      char *p1;
>> +
>> +      p = p + 1;
>> +      p1 = strchr (p, ',');
>> +      decode_address (&resume_info[i].step_range_start, p, p1 - p);
>> +
>> +      p = p1 + 1;
>> +      p1 = strchr (p, ':');
>> +      decode_address (&resume_info[i].step_range_end, p, p1 - p);
>> +
>> +      p = p1;
>> +    }
>>         else
>>       {
>> -      resume_info[i].sig = 0;
> 
> What is the purpose to remove this line?

It's now unnecessary, given the new memset call added
further above:

@@ -2042,8 +2042,12 @@ handle_v_cont (char *own_buf)
     {
       p++;
 
+      memset (&resume_info[i], 0, sizeof resume_info[i]);

>>
> 
> The patch looks good to me.

Thanks.  Here's what I checked in.  I adjusted the step_range_start/end
comment in the same spirit it was adjusted in the documentation and on
the gdb side.  It also adds the missing word Tom pointed at.

----
range stepping: gdbserver (x86 GNU/Linux)

This patch adds support for range stepping to GDBserver, teaching it
about vCont;r.

It'd be easy to enable this for all hardware single-step targets
without needing the linux_target_ops hook, however, at least PPC needs
special care, due to the fact that PPC atomic sequences can't be
hardware single-stepped through, a thing which GDBserver doesn't know
about.  So this leaves the support limited to x86/x86_64.

gdb/
2013-05-23  Pedro Alves  <palves@redhat.com>

	* NEWS: Mention GDBserver range stepping support.

gdb/gdbserver/
2013-05-23  Yao Qi  <yao@codesourcery.com>
	    Pedro Alves  <palves@redhat.com>

	* linux-low.c (lwp_in_step_range): New function.
	(linux_wait_1): If the thread was range stepping and stopped
	outside the stepping range, report the stop to GDB.  Otherwise,
	continue stepping.  Add range stepping debug output.
	(linux_set_resume_request): Copy the step range from the resume
	request to the lwp.
	(linux_supports_range_stepping): New.
	(linux_target_ops) <supports_range_stepping>: Set to
	linux_supports_range_stepping.
	* linux-low.h (struct linux_target_ops)
	<supports_range_stepping>: New field.
	(struct lwp_info) <step_range_start, step_range_end>: New fields.
	* linux-x86-low.c (x86_supports_range_stepping): New.
	(the_low_target) <supports_range_stepping>: Set to
	x86_supports_range_stepping.
	* server.c (handle_v_cont): Handle 'r' action.
	(handle_v_requests): Append ";r" if the target supports range
	stepping.
	* target.h (struct thread_resume) <step_range_start,
	step_range_end>: New fields.
	(struct target_ops) <supports_range_stepping>:
	New field.
	(target_supports_range_stepping): New macro.
---
 gdb/NEWS                      |    5 +++
 gdb/gdbserver/linux-low.c     |   63 ++++++++++++++++++++++++++++++++++-------
 gdb/gdbserver/linux-low.h     |    8 +++++
 gdb/gdbserver/linux-x86-low.c |    7 +++++
 gdb/gdbserver/server.c        |   24 +++++++++++++++-
 gdb/gdbserver/target.h        |   15 ++++++++++
 6 files changed, 110 insertions(+), 12 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 1a4140d..a23e8e3 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -95,6 +95,11 @@ vCont;r
   stub to step through an address range itself, without GDB
   involvemement at each single-step.
 
+* New features in the GDB remote stub, GDBserver
+
+  ** GDBserver now supports target-assisted range stepping.  Currently
+     enabled on x86/x86_64 GNU/Linux targets.
+
 *** Changes in GDB 7.6
 
 * Target record has been renamed to record-full.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index b01b37c..7e18942 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -276,6 +276,16 @@ supports_fast_tracepoints (void)
   return the_low_target.install_fast_tracepoint_jump_pad != NULL;
 }
 
+/* True if LWP is stopped in its stepping range.  */
+
+static int
+lwp_in_step_range (struct lwp_info *lwp)
+{
+  CORE_ADDR pc = lwp->stop_pc;
+
+  return (pc >= lwp->step_range_start && pc < lwp->step_range_end);
+}
+
 struct pending_signals
 {
   int signal;
@@ -2337,6 +2347,7 @@ linux_wait_1 (ptid_t ptid,
   int maybe_internal_trap;
   int report_to_gdb;
   int trace_event;
+  int in_step_range;
 
   /* Translate generic target options into linux options.  */
   options = __WALL;
@@ -2346,6 +2357,7 @@ linux_wait_1 (ptid_t ptid,
 retry:
   bp_explains_trap = 0;
   trace_event = 0;
+  in_step_range = 0;
   ourstatus->kind = TARGET_WAITKIND_IGNORE;
 
   /* If we were only supposed to resume one thread, only wait for
@@ -2639,18 +2651,24 @@ Check if we're already there.\n",
       goto retry;
     }
 
-  /* If GDB wanted this thread to single step, we always want to
-     report the SIGTRAP, and let GDB handle it.  Watchpoints should
-     always be reported.  So should signals we can't explain.  A
-     SIGTRAP we can't explain could be a GDB breakpoint --- we may or
-     not support Z0 breakpoints.  If we do, we're be able to handle
-     GDB breakpoints on top of internal breakpoints, by handling the
-     internal breakpoint and still reporting the event to GDB.  If we
-     don't, we're out of luck, GDB won't see the breakpoint hit.  */
+  /* Note that all addresses are always "out of the step range" when
+     there's no range to begin with.  */
+  in_step_range = lwp_in_step_range (event_child);
+
+  /* If GDB wanted this thread to single step, and the thread is out
+     of the step range, we always want to report the SIGTRAP, and let
+     GDB handle it.  Watchpoints should always be reported.  So should
+     signals we can't explain.  A SIGTRAP we can't explain could be a
+     GDB breakpoint --- we may or not support Z0 breakpoints.  If we
+     do, we're be able to handle GDB breakpoints on top of internal
+     breakpoints, by handling the internal breakpoint and still
+     reporting the event to GDB.  If we don't, we're out of luck, GDB
+     won't see the breakpoint hit.  */
   report_to_gdb = (!maybe_internal_trap
-		   || current_inferior->last_resume_kind == resume_step
+		   || (current_inferior->last_resume_kind == resume_step
+		       && !in_step_range)
 		   || event_child->stopped_by_watchpoint
-		   || (!step_over_finished
+		   || (!step_over_finished && !in_step_range
 		       && !bp_explains_trap && !trace_event)
 		   || (gdb_breakpoint_here (event_child->stop_pc)
 		       && gdb_condition_true_at_breakpoint (event_child->stop_pc)
@@ -2671,6 +2689,11 @@ Check if we're already there.\n",
 	    fprintf (stderr, "Step-over finished.\n");
 	  if (trace_event)
 	    fprintf (stderr, "Tracepoint event.\n");
+	  if (lwp_in_step_range (event_child))
+	    fprintf (stderr, "Range stepping pc 0x%s [0x%s, 0x%s).\n",
+		     paddress (event_child->stop_pc),
+		     paddress (event_child->step_range_start),
+		     paddress (event_child->step_range_end));
 	}
 
       /* We're not reporting this breakpoint to GDB, so apply the
@@ -2702,7 +2725,12 @@ Check if we're already there.\n",
   if (debug_threads)
     {
       if (current_inferior->last_resume_kind == resume_step)
-	fprintf (stderr, "GDB wanted to single-step, reporting event.\n");
+	{
+	  if (event_child->step_range_start == event_child->step_range_end)
+	    fprintf (stderr, "GDB wanted to single-step, reporting event.\n");
+	  else if (!lwp_in_step_range (event_child))
+	    fprintf (stderr, "Out of step range, reporting event.\n");
+	}
       if (event_child->stopped_by_watchpoint)
 	fprintf (stderr, "Stopped by watchpoint.\n");
       if (gdb_breakpoint_here (event_child->stop_pc))
@@ -3401,6 +3429,9 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg)
 	  lwp->resume = &r->resume[ndx];
 	  thread->last_resume_kind = lwp->resume->kind;
 
+	  lwp->step_range_start = lwp->resume->step_range_start;
+	  lwp->step_range_end = lwp->resume->step_range_end;
+
 	  /* If we had a deferred signal to report, dequeue one now.
 	     This can happen if LWP gets more than one signal while
 	     trying to get out of a jump pad.  */
@@ -5094,6 +5125,15 @@ linux_supports_agent (void)
   return 1;
 }
 
+static int
+linux_supports_range_stepping (void)
+{
+  if (*the_low_target.supports_range_stepping == NULL)
+    return 0;
+
+  return (*the_low_target.supports_range_stepping) ();
+}
+
 /* Enumerate spufs IDs for process PID.  */
 static int
 spu_enumerate_spu_ids (long pid, unsigned char *buf, CORE_ADDR offset, int len)
@@ -5952,6 +5992,7 @@ static struct target_ops linux_target_ops = {
   NULL,
   NULL,
 #endif
+  linux_supports_range_stepping,
 };
 
 static void
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 205d803..4dd3c9c 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -166,6 +166,8 @@ struct linux_target_ops
      for use as a fast tracepoint.  */
   int (*get_min_fast_tracepoint_insn_len) (void);
 
+  /* Returns true if the low target supports range stepping.  */
+  int (*supports_range_stepping) (void);
 };
 
 extern struct linux_target_ops the_low_target;
@@ -235,6 +237,12 @@ struct lwp_info
      level on this process was a single-step.  */
   int stepping;
 
+  /* Range to single step within.  This is a copy of the step range
+     passed along the last resume request.  See 'struct
+     thread_resume'.  */
+  CORE_ADDR step_range_start;	/* Inclusive */
+  CORE_ADDR step_range_end;	/* Exclusive */
+
   /* If this flag is set, we need to set the event request flags the
      next time we see this LWP stop.  */
   int must_set_ptrace_flags;
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 31657d3..1d1df95 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -3175,6 +3175,12 @@ x86_emit_ops (void)
     return &i386_emit_ops;
 }
 
+static int
+x86_supports_range_stepping (void)
+{
+  return 1;
+}
+
 /* This is initialized assuming an amd64 target.
    x86_arch_setup will correct it for i386 or amd64 targets.  */
 
@@ -3214,4 +3220,5 @@ struct linux_target_ops the_low_target =
   x86_install_fast_tracepoint_jump_pad,
   x86_emit_ops,
   x86_get_min_fast_tracepoint_insn_len,
+  x86_supports_range_stepping,
 };
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 6bb36d8..1083aa9 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2042,8 +2042,12 @@ handle_v_cont (char *own_buf)
     {
       p++;
 
+      memset (&resume_info[i], 0, sizeof resume_info[i]);
+
       if (p[0] == 's' || p[0] == 'S')
 	resume_info[i].kind = resume_step;
+      else if (p[0] == 'r')
+	resume_info[i].kind = resume_step;
       else if (p[0] == 'c' || p[0] == 'C')
 	resume_info[i].kind = resume_continue;
       else if (p[0] == 't')
@@ -2063,9 +2067,22 @@ handle_v_cont (char *own_buf)
 	    goto err;
 	  resume_info[i].sig = gdb_signal_to_host (sig);
 	}
+      else if (p[0] == 'r')
+	{
+	  char *p1;
+
+	  p = p + 1;
+	  p1 = strchr (p, ',');
+	  decode_address (&resume_info[i].step_range_start, p, p1 - p);
+
+	  p = p1 + 1;
+	  p1 = strchr (p, ':');
+	  decode_address (&resume_info[i].step_range_end, p, p1 - p);
+
+	  p = p1;
+	}
       else
 	{
-	  resume_info[i].sig = 0;
 	  p = p + 1;
 	}
 
@@ -2311,6 +2328,11 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
       if (strncmp (own_buf, "vCont?", 6) == 0)
 	{
 	  strcpy (own_buf, "vCont;c;C;s;S;t");
+	  if (target_supports_range_stepping ())
+	    {
+	      own_buf = own_buf + strlen (own_buf);
+	      strcpy (own_buf, ";r");
+	    }
 	  return;
 	}
     }
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index f257459..c57cb40 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -57,6 +57,15 @@ struct thread_resume
      linux; SuspendThread on win32).  This is a host signal value (not
      enum gdb_signal).  */
   int sig;
+
+  /* Range to single step within.  Valid only iff KIND is resume_step.
+
+     Single-step once, and then continuing stepping as long as the
+     thread stops in this range.  (If the range is empty
+     [STEP_RANGE_START == STEP_RANGE_END], then this is a single-step
+     request.)  */
+  CORE_ADDR step_range_start;	/* Inclusive */
+  CORE_ADDR step_range_end;	/* Exclusive */
 };
 
 /* Generally, what has the program done?  */
@@ -414,6 +423,8 @@ struct target_ops
      to break a cyclic dependency.  */
   void (*read_btrace) (struct btrace_target_info *, struct buffer *, int type);
 
+  /* Return true if target supports range stepping.  */
+  int (*supports_range_stepping) (void);
 };
 
 extern struct target_ops *the_target;
@@ -549,6 +560,10 @@ int kill_inferior (int);
 #define target_read_btrace(tinfo, buffer, type)	\
   (*the_target->read_btrace) (tinfo, buffer, type)
 
+#define target_supports_range_stepping() \
+  (the_target->supports_range_stepping ? \
+   (*the_target->supports_range_stepping) () : 0)
+
 /* Start non-stop mode, returns 0 on success, -1 on failure.   */
 
 int start_non_stop (int nonstop);