PATCH: Circular trace buffers

PATCH: Circular trace buffers

[Stan Shebs]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

This patch adds a flag that requests the target agent to make the trace 
buffer circular, so that instead of filling it up and then stopping, the 
agent discards the oldest trace frames as necessary to accommodate new 
ones.  Any hairy memory management code is going to be on the target 
side; GDB just has to transmit the setting (and now always via target 
vector), and report back status, which may now include a total number of 
frames that were created.  This also adds complete documentation of the 
qTStatus reply, per request.  Any comments before I commit?

Stan

2010-03-16 Stan Shebs  <stan@codesourcery.com>
           Pedro Alves  <pedro@codesourcery.com>

        * target.h (struct target_ops): New method
        to_set_circular_trace_buffer.
        (target_set_circular_trace_buffer): New macro.
        * target.c (update_current_target): Add
        to_set_circular_trace_buffer, fix to_set_disconnected_tracing
        default behavior.
        * remote.c (remote_set_circular_trace_buffer): New function.
        (init_remote_ops): Add it to vector.
        * tracepoint.h (struct trace_status): New field traceframes_created,
        change buffer_size and buffer_free to int.
        * tracepoint.c (circular_trace_buffer): New global.
        (start_tracing): Send values of disconnected tracing and circular
        trace buffer settings.
        (set_circular_trace_buffer): New function.
        (parse_trace_state): Handle total space and frames created.
        (trace_status_command): Display total space and total frames
        created.
        (trace_save): Write out new status values.
        (parse_trace_status): Set traceframe_count, traceframes_created,
        buffer_free and buffer_size to -1 by default.
        (_initialize_tracepoint): New setshow for circular-trace-buffer.
        * NEWS: Mention the circular trace buffer option.

        [gdb/doc]
        * gdb.texinfo (Starting and Stopping Trace Experiments): Describe
        circular-trace-buffer.
        (Tracepoint Packets): Describe QTBuffer, and details of the
        qTStatus reply.


[-- Attachment #2: circ-patch-1 --]
[-- Type: text/plain, Size: 17540 bytes --]

Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.362
diff -p -r1.362 NEWS
*** NEWS	8 Mar 2010 19:20:38 -0000	1.362
--- NEWS	16 Mar 2010 21:27:00 -0000
*************** Renesas RX			rx
*** 114,120 ****
    tracing run at the moment that it was saved.  To create a trace
    file, use "tsave <filename>", and to use it, do "target tfile
    <name>".
!   
  * Changed commands
  
  disassemble
--- 114,127 ----
    tracing run at the moment that it was saved.  To create a trace
    file, use "tsave <filename>", and to use it, do "target tfile
    <name>".
! 
!   ** Circular trace buffer
! 
!   You can ask the target agent to handle the trace buffer as a
!   circular buffer, discarding the oldest trace frames to make room for
!   newer ones, by setting circular-trace-buffer to 1.  This feature may
!   not be available for all target agents.
! 
  * Changed commands
  
  disassemble
*************** show disconnected-tracing
*** 215,220 ****
--- 222,234 ----
     loses its connection to GDB.  If 0, the target is to stop tracing
     upon disconnection.
  
+ set circular-trace-buffer
+ show circular-trace-buffer
+    If set to 1, the target is instructed to use a circular trace buffer
+    and discard the oldest trace frames instead of stopping the trace due
+    to a full trace buffer.  If set to 0, the trace stops when the buffer
+    fills up.
+ 
  set script-extension off|soft|strict
  show script-extension
     If set to "off", the debugger does not perform any script language
*************** qTV
*** 258,263 ****
--- 272,280 ----
  QTDisconnected
     Set desired tracing behavior upon disconnection.
  
+ QTBuffer:circular
+    Set the trace buffer to be linear or circular.
+ 
  qTfP, qTsP
     Get data about the tracepoints currently in use.
  
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.389
diff -p -r1.389 remote.c
*** remote.c	7 Mar 2010 14:36:44 -0000	1.389
--- remote.c	16 Mar 2010 21:27:00 -0000
*************** remote_core_of_thread (struct target_ops
*** 9659,9664 ****
--- 9659,9676 ----
  }
  
  static void
+ remote_set_circular_trace_buffer (int val)
+ {
+   struct remote_state *rs = get_remote_state ();
+ 
+   sprintf (rs->buf, "QTBuffer:circular:%x", val);
+   putpkt (rs->buf);
+   remote_get_noisy_reply (&target_buf, &target_buf_size);
+   if (strcmp (target_buf, "OK"))
+     error (_("Target does not support this command."));
+ }
+ 
+ static void
  init_remote_ops (void)
  {
    remote_ops.to_shortname = "remote";
*************** Specify the serial device it is connecte
*** 9736,9741 ****
--- 9748,9754 ----
    remote_ops.to_upload_trace_state_variables = remote_upload_trace_state_variables;
    remote_ops.to_get_raw_trace_data = remote_get_raw_trace_data;
    remote_ops.to_set_disconnected_tracing = remote_set_disconnected_tracing;
+   remote_ops.to_set_circular_trace_buffer = remote_set_circular_trace_buffer;
    remote_ops.to_core_of_thread = remote_core_of_thread;
  }
  
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.242
diff -p -r1.242 target.c
*** target.c	12 Mar 2010 03:54:45 -0000	1.242
--- target.c	16 Mar 2010 21:27:00 -0000
*************** update_current_target (void)
*** 659,664 ****
--- 659,665 ----
        INHERIT (to_upload_trace_state_variables, t);
        INHERIT (to_get_raw_trace_data, t);
        INHERIT (to_set_disconnected_tracing, t);
+       INHERIT (to_set_circular_trace_buffer, t);
        INHERIT (to_magic, t);
        /* Do not inherit to_memory_map.  */
        /* Do not inherit to_flash_erase.  */
*************** update_current_target (void)
*** 848,854 ****
  	    tcomplain);
    de_fault (to_set_disconnected_tracing,
  	    (void (*) (int))
! 	    tcomplain);
  #undef de_fault
  
    /* Finally, position the target-stack beneath the squashed
--- 849,858 ----
  	    tcomplain);
    de_fault (to_set_disconnected_tracing,
  	    (void (*) (int))
! 	    target_ignore);
!   de_fault (to_set_circular_trace_buffer,
! 	    (void (*) (int))
! 	    target_ignore);
  #undef de_fault
  
    /* Finally, position the target-stack beneath the squashed
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.173
diff -p -r1.173 target.h
*** target.h	22 Feb 2010 23:35:16 -0000	1.173
--- target.h	16 Mar 2010 21:27:00 -0000
*************** struct target_ops
*** 664,669 ****
--- 664,670 ----
      /* Set the target's tracing behavior in response to unexpected
         disconnection - set VAL to 1 to keep tracing, 0 to stop.  */
      void (*to_set_disconnected_tracing) (int val);
+     void (*to_set_circular_trace_buffer) (int val);
  
      /* Return the processor core that thread PTID was last seen on.
         This information is updated only when:
*************** extern int target_search_memory (CORE_AD
*** 1359,1364 ****
--- 1360,1368 ----
  #define target_set_disconnected_tracing(val) \
    (*current_target.to_set_disconnected_tracing) (val)
  
+ #define	target_set_circular_trace_buffer(val)	\
+   (*current_target.to_set_circular_trace_buffer) (val)
+ 
  /* Command logging facility.  */
  
  #define target_log_command(p)						\
Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.146
diff -p -r1.146 tracepoint.c
*** tracepoint.c	12 Mar 2010 03:54:45 -0000	1.146
--- tracepoint.c	16 Mar 2010 21:27:00 -0000
*************** char *default_collect = "";
*** 153,158 ****
--- 153,163 ----
  
  static int disconnected_tracing;
  
+ /* This variable controls whether we ask the target for a linear or
+    circular trace buffer.  */
+ 
+ static int circular_trace_buffer;
+ 
  /* ======= Important command functions: ======= */
  static void trace_actions_command (char *, int);
  static void trace_start_command (char *, int);
*************** trace_start_command (char *args, int fro
*** 1560,1565 ****
--- 1565,1573 ----
    
    /* Tell target to treat text-like sections as transparent.  */
    target_trace_set_readonly_regions ();
+   /* Set some mode flags.  */
+   target_set_disconnected_tracing (disconnected_tracing);
+   target_set_circular_trace_buffer (circular_trace_buffer);
  
    /* Now insert traps and begin collecting data.  */
    target_trace_start ();
*************** trace_status_command (char *args, int fr
*** 1649,1664 ****
  	}
      }
  
!   if (ts->traceframe_count >= 0)
      {
        printf_filtered (_("Collected %d trace frames.\n"),
  		       ts->traceframe_count);
      }
  
!   if (ts->buffer_free)
      {
!       printf_filtered (_("Trace buffer has %llu bytes free.\n"),
! 		       ts->buffer_free);
      }
  
    /* Now report on what we're doing with tfind.  */
--- 1657,1689 ----
  	}
      }
  
!   if (ts->traceframes_created >= 0)
!     {
!       printf_filtered (_("Buffer contains %d trace frames (of %d created total).\n"),
! 		       ts->traceframe_count, ts->traceframes_created);
!     }
!   else if (ts->traceframe_count >= 0)
      {
        printf_filtered (_("Collected %d trace frames.\n"),
  		       ts->traceframe_count);
      }
  
!   if (ts->buffer_free >= 0)
      {
!       if (ts->buffer_size >= 0)
! 	{
! 	  printf_filtered (_("Trace buffer has %d bytes of %d bytes free"),
! 			   ts->buffer_free, ts->buffer_size);
! 	  if (ts->buffer_size > 0)
! 	    printf_filtered (_(" (%d%% full)"),
! 			     ((int) ((((long long) (ts->buffer_size
! 						    - ts->buffer_free)) * 100)
! 				     / ts->buffer_size)));
! 	  printf_filtered (_(".\n"));
! 	}
!       else
! 	printf_filtered (_("Trace buffer has %d bytes free.\n"),
! 			 ts->buffer_free);
      }
  
    /* Now report on what we're doing with tfind.  */
*************** trace_save_command (char *args, int from
*** 2419,2428 ****
    fprintf (fp, "R %x\n", trace_regblock_size);
  
    /* Write out status of the tracing run (aka "tstatus" info).  */
!   fprintf (fp, "status %c;%s:%x;tframes:%x;tfree:%llx\n",
  	   (ts->running ? '1' : '0'),
! 	   stop_reason_names[ts->stop_reason], ts->stopping_tracepoint,
! 	   ts->traceframe_count, ts->buffer_free);
  
    /* Note that we want to upload tracepoints and save those, rather
       than simply writing out the local ones, because the user may have
--- 2444,2461 ----
    fprintf (fp, "R %x\n", trace_regblock_size);
  
    /* Write out status of the tracing run (aka "tstatus" info).  */
!   fprintf (fp, "status %c;%s:%x",
  	   (ts->running ? '1' : '0'),
!  	   stop_reason_names[ts->stop_reason], ts->stopping_tracepoint);
!   if (ts->traceframe_count >= 0)
!     fprintf (fp, ";tframes:%x", ts->traceframe_count);
!   if (ts->traceframes_created >= 0)
!     fprintf (fp, ";tcreated:%x", ts->traceframes_created);
!   if (ts->buffer_free >= 0)
!     fprintf (fp, ";tfree:%x", ts->buffer_free);
!   if (ts->buffer_size >= 0)
!     fprintf (fp, ";tsize:%x", ts->buffer_size);
!   fprintf (fp, "\n");
  
    /* Note that we want to upload tracepoints and save those, rather
       than simply writing out the local ones, because the user may have
*************** set_disconnected_tracing (char *args, in
*** 2527,2532 ****
--- 2560,2572 ----
    send_disconnected_tracing_value (disconnected_tracing);
  }
  
+ static void
+ set_circular_trace_buffer (char *args, int from_tty,
+ 			   struct cmd_list_element *c)
+ {
+   target_set_circular_trace_buffer (circular_trace_buffer);
+ }
+ 
  /* Convert the memory pointed to by mem into hex, placing result in buf.
   * Return a pointer to the last char put in buf (null)
   * "stolen" from sparc-stub.c
*************** parse_trace_status (char *line, struct t
*** 3040,3045 ****
--- 3080,3090 ----
    ts->running_known = 1;
    ts->running = (*p++ == '1');
    ts->stop_reason = trace_stop_reason_unknown;
+   ts->traceframe_count = -1;
+   ts->traceframes_created = -1;
+   ts->buffer_free = -1;
+   ts->buffer_size = -1;
+ 
    while (*p++)
      {
        p1 = strchr (p, ':');
*************** Status line: '%s'\n"), p, line);
*** 3067,3082 ****
  	  p = unpack_varlen_hex (++p1, &val);
  	  ts->stop_reason = tstop_command;
  	}
!       if (strncmp (p, "tframes", p1 - p) == 0)
  	{
  	  p = unpack_varlen_hex (++p1, &val);
  	  ts->traceframe_count = val;
  	}
!       if (strncmp (p, "tfree", p1 - p) == 0)
  	{
  	  p = unpack_varlen_hex (++p1, &val);
  	  ts->buffer_free = val;
  	}
        else
  	{
  	  /* Silently skip unknown optional info.  */
--- 3112,3137 ----
  	  p = unpack_varlen_hex (++p1, &val);
  	  ts->stop_reason = tstop_command;
  	}
!       else if (strncmp (p, "tframes", p1 - p) == 0)
  	{
  	  p = unpack_varlen_hex (++p1, &val);
  	  ts->traceframe_count = val;
  	}
!       else if (strncmp (p, "tcreated", p1 - p) == 0)
! 	{
! 	  p = unpack_varlen_hex (++p1, &val);
! 	  ts->traceframes_created = val;
! 	}
!       else if (strncmp (p, "tfree", p1 - p) == 0)
  	{
  	  p = unpack_varlen_hex (++p1, &val);
  	  ts->buffer_free = val;
  	}
+       else if (strncmp (p, "tsize", p1 - p) == 0)
+ 	{
+ 	  p = unpack_varlen_hex (++p1, &val);
+ 	  ts->buffer_size = val;
+ 	}
        else
  	{
  	  /* Silently skip unknown optional info.  */
*************** trace data collected in the meantime."),
*** 3800,3805 ****
--- 3855,3872 ----
  			   &setlist,
  			   &showlist);
  
+   add_setshow_boolean_cmd ("circular-trace-buffer", no_class,
+ 			   &circular_trace_buffer, _("\
+ Set target's use of circular trace buffer."), _("\
+ Show target's use of circular trace buffer."), _("\
+ Use this to make the trace buffer into a circular buffer,\n\
+ which will discard traceframes (oldest first) instead of filling\n\
+ up and stopping the trace run."),
+ 			   set_circular_trace_buffer,
+ 			   NULL,
+ 			   &setlist,
+ 			   &showlist);
+ 
    init_tfile_ops ();
  
    add_target (&tfile_ops);
Index: tracepoint.h
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.h,v
retrieving revision 1.22
diff -p -r1.22 tracepoint.h
*** tracepoint.h	24 Feb 2010 01:06:28 -0000	1.22
--- tracepoint.h	16 Mar 2010 21:27:00 -0000
*************** struct trace_status
*** 95,105 ****
  
    int stopping_tracepoint;
  
    int traceframe_count;
  
!   unsigned long long buffer_size;
  
!   unsigned long long buffer_free;
  };
  
  struct trace_status *current_trace_status (void);
--- 95,115 ----
  
    int stopping_tracepoint;
  
+   /* Number of traceframes currently in the buffer.  */
+ 
    int traceframe_count;
  
!   /* Number of traceframes created since start of run.  */
! 
!   int traceframes_created;
! 
!   /* Total size of the target's trace buffer.  */
! 
!   int buffer_size;
! 
!   /* Unused bytes left in the target's trace buffer.  */
  
!   int buffer_free;
  };
  
  struct trace_status *current_trace_status (void);
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.680
diff -p -r1.680 gdb.texinfo
*** doc/gdb.texinfo	12 Mar 2010 19:15:52 -0000	1.680
--- doc/gdb.texinfo	16 Mar 2010 21:27:01 -0000
*************** which to specify that tracepoint.  This 
*** 9836,9841 ****
--- 9836,9868 ----
  necessarily heuristic, and it may result in useless tracepoints being
  created; you may simply delete them if they are of no use.
  
+ @cindex circular trace buffer
+ If your target agent supports a @dfn{circular trace buffer}, then you
+ can run a trace experiment indefinitely without filling the trace
+ buffer; when space runs out, the agent deletes already-collected trace
+ frames, oldest first, until there is enough room to continue
+ collecting.  This is especially useful if your tracepoints are being
+ hit too often and terminating tracing too soon.  To ask for a circular
+ trace buffer, simply set @samp{circular_trace_buffer} to 1.  You can
+ set this at any time, including during tracing; if the agent can do
+ it, it will change buffer handling on the fly, otherwise it will not
+ take effect until the next run.
+ 
+ @table @code
+ @item set circular-trace-buffer on
+ @itemx set circular-trace-buffer off
+ @kindex set circular-trace-buffer
+ Choose whether a tracing run should use a linear or circular buffer
+ for trace data.  A linear buffer will not lose any trace data, but may
+ fill up prematurely, while a circular buffer will discard old trace
+ data, but it will have always room for the latest tracepoint hits.
+ 
+ @item show circular-trace-buffer
+ @kindex show circular-trace-buffer
+ Show the current choice for the trace buffer.
+ 
+ @end table
+ 
  @node Tracepoint Restrictions
  @subsection Tracepoint Restrictions
  
*************** encoded).  @value{GDBN} will continue to
*** 30603,30608 ****
--- 30630,30636 ----
  @end table
  
  @item qTBuffer
+ @item QTBuffer
  @item QTDisconnected
  @itemx QTDP
  @itemx QTDV
*************** continue the tracing run, while 0 tells 
*** 31087,31098 ****
  @item qTStatus
  Ask the stub if there is a trace experiment running right now.
  
! Replies:
  @table @samp
! @item T0
! There is no trace experiment running.
! @item T1
! There is a trace experiment running.
  @end table
  
  @item qTV:@var{var}
--- 31115,31176 ----
  @item qTStatus
  Ask the stub if there is a trace experiment running right now.
  
! The reply has the form:
! 
! @table @samp
! 
! @item T@var{running}@r{[};@var{field}@r{]}@dots{}
! @var{running} is a single digit @code{1} if the trace is presently
! running, or @code{0} if not.  It is followed by optional fields that
! an agent may use to report additional status. 
! 
! @end table
! 
! If the trace is not running, the agent may report any of several
! explanations as one of the optional fields:
! 
! @table @samp
! 
! @item tnotrun:0
! No trace has been run yet.
! 
! @item tstop:0
! The trace was stopped by a user-originated stop command.
! 
! @item tfull:0
! The trace stopped because the trace buffer filled up.
! 
! @item tdisconnected:0
! The trace stopped because @value{GDBN} disconnected from the target.
! 
! @item tpasscount:@var{tpnum}
! The trace stopped because tracepoint @var{tpnum} exceeded its pass count.
! 
! @item tunknown:0
! The trace stopped for some other reason.
! 
! @end table
! 
! Additional optional fields supply statistical information.  Although
! not required, they are extremely useful for users monitoring the
! progress of a trace run.  If a trace has stopped, and these numbers
! are reported, they must reflect the state of the just-stopped trace.
! 
  @table @samp
! 
! @item tframes:@var{n}
! The number of trace frames in the buffer.
! 
! @item tcreated:@var{n}
! The total number of trace frames created during the run. This may
! be larger than the trace frame count, if the buffer is circular.
! 
! @item tsize:@var{n}
! The total size of the trace buffer.
! 
! @item tfree:@var{n}
! The number of bytes still unused in the buffer.
! 
  @end table
  
  @item qTV:@var{var}
*************** in a packet; it is not an error to retur
*** 31147,31152 ****
--- 31225,31234 ----
  A reply consisting of just @code{l} indicates that no bytes are
  available.
  
+ @item QTBuffer:circular:@var{value}
+ This packet directs the target to use a circular trace buffer if
+ @var{value} is 1, or a linear buffer if the value is 0.
+ 
  @end table
  
  @node Host I/O Packets

Re: PATCH: Circular trace buffers

[Eli Zaretskii]

> Date: Tue, 16 Mar 2010 14:42:02 -0700
> From: Stan Shebs <stan@codesourcery.com>
> 
> This patch adds a flag that requests the target agent to make the trace 
> buffer circular, so that instead of filling it up and then stopping, the 
> agent discards the oldest trace frames as necessary to accommodate new 
> ones.  Any hairy memory management code is going to be on the target 
> side; GDB just has to transmit the setting (and now always via target 
> vector), and report back status, which may now include a total number of 
> frames that were created.  This also adds complete documentation of the 
> qTStatus reply, per request.

Thanks.

> Any comments before I commit?

A few.

> + set circular-trace-buffer
> + show circular-trace-buffer
> +    If set to 1, the target is instructed to use a circular trace buffer
> +    and discard the oldest trace frames instead of stopping the trace due
> +    to a full trace buffer.  If set to 0, the trace stops when the buffer
> +    fills up.

The caveat about this feature being available only on some targets
should be present here as well, I think.

> + collecting.  This is especially useful if your tracepoints are being
> + hit too often and terminating tracing too soon.  To ask for a circular
                      ^^^^^^^^^^^
I think using "terminate" here makes the sentence more clear, if not
more grammatically correct.

> + trace buffer, simply set @samp{circular_trace_buffer} to 1.  You can
> + set this at any time, including during tracing; if the agent can do
> + it, it will change buffer handling on the fly, otherwise it will not
> + take effect until the next run.
> + 
> + @table @code
> + @item set circular-trace-buffer on
> + @itemx set circular-trace-buffer off

You use "on" and "off" here, but the text (and the NEWS entry) say 0
or 1.  I think you should use "on" and "off" throughout; the fact that
we also allow 0 and 1 is indeed a feature, but either it should be
explicitly mentioned as an add-on convenience, or not at all.

> ! The reply has the form:
> ! 
> ! @table @samp
> ! 
> ! @item T@var{running}@r{[};@var{field}@r{]}@dots{}
> ! @var{running} is a single digit @code{1} if the trace is presently
> ! running, or @code{0} if not.  It is followed by optional fields that
> ! an agent may use to report additional status. 

Is there indeed only one semi-colon, before the first FIELD?  If so,
how are the fields separated from one another?

> ! @item tcreated:@var{n}
> ! The total number of trace frames created during the run. This may
> ! be larger than the trace frame count, if the buffer is circular.

It would be reader-friendly to have an xref here to the description of
the circular buffer facility that you just added above.

> ! @item tsize:@var{n}
> ! The total size of the trace buffer.

In what units?  (I'm guessing bytes, but why should I guess?)

Okay with those changes.

Re: PATCH: Circular trace buffers

[Pedro Alves]

On Tuesday 16 March 2010 21:42:02, Stan Shebs wrote:
> This patch adds a flag that requests the target agent to make the trace 
> buffer circular, so that instead of filling it up and then stopping, the 
> agent discards the oldest trace frames as necessary to accommodate new 
> ones.  Any hairy memory management code is going to be on the target 
> side; GDB just has to transmit the setting (and now always via target 
> vector), and report back status, which may now include a total number of 
> frames that were created.  This also adds complete documentation of the 
> qTStatus reply, per request.  Any comments before I commit?

Playing devil's advogate here, I'm still not 100% convinced that
"set circular-trace-buffer" is 100% well defined and that
is isn't confusing in some cases; it applies on the fly
in some cases, does somewhat not-completly clear
things in other cases, and errors out in others.  I wonder
if we defined "set circular-trace-buffer" as another flag
that is respected at "tstart" time only, and made the
presently running trace run's circular-trace-buffer-ness reported
through "tstatus", and define "show circular-trace-buffer" as the
"circular-trace-buffer-ness" intent at next trace run start,
things would be more consistent and clear.  If not, vis.:

E.g.,

 - applies on the fly in non-stop mode.

 - all-stop/async + trace running + "set circular-trace-buffer"
   errors out because you can't talk to the target if it
   is running in all-stop.

 - E.g., what does "show circular-trace-buffer" mean when
   debugging a tfile?  "set circular-trace-buffer" changes
   the local GDB flag, and "show circular-trace-buffer"
   shows the according change, but, then we have no
   way of knowing when debugging a tfile had been
   in circular-trace-buffer mode or not when the tfile
   was created.


That said, I'm not opposed to the patch.  Just
dumping my thoughts.  :-)

-- 
Pedro Alves

Re: PATCH: Circular trace buffers

[Stan Shebs]

Pedro Alves wrote:
> On Tuesday 16 March 2010 21:42:02, Stan Shebs wrote:
>   
>> This patch adds a flag that requests the target agent to make the trace 
>> buffer circular, so that instead of filling it up and then stopping, the 
>> agent discards the oldest trace frames as necessary to accommodate new 
>> ones.  Any hairy memory management code is going to be on the target 
>> side; GDB just has to transmit the setting (and now always via target 
>> vector), and report back status, which may now include a total number of 
>> frames that were created.  This also adds complete documentation of the 
>> qTStatus reply, per request.  Any comments before I commit?
>>     
>
> Playing devil's advogate here, I'm still not 100% convinced that
> "set circular-trace-buffer" is 100% well defined and that
> is isn't confusing in some cases; it applies on the fly
> in some cases, does somewhat not-completly clear
> things in other cases, and errors out in others.
Hey Joel, pass me that aspirin bottle, willya? :-)  This is one of those 
places where I'd like more user feedback before getting too fancy.  It's 
the opposite situation of expression evaluation - we play fast and loose 
with language semantics, but we know from extensive experience that 
users don't want GDB to be too pedantic about visibility, scopes, etc.  
But for tracepoints we're still mostly guessing.
> I wonder
> if we defined "set circular-trace-buffer" as another flag
> that is respected at "tstart" time only, and made the
> presently running trace run's circular-trace-buffer-ness reported
> through "tstatus", and define "show circular-trace-buffer" as the
> "circular-trace-buffer-ness" intent at next trace run start,
> things would be more consistent and clear.
I thought about that, but it seemed like one of its uses would be as a 
hasty way to keep a trace run alive; you do a tstatus, say "oh sh*t" as 
you see the buffer at 80% full before you've reached the code of 
interest, and quickly switch to circular buffer.
>  - all-stop/async + trace running + "set circular-trace-buffer"
>    errors out because you can't talk to the target if it
>    is running in all-stop.
>   
I think the user would know to interrupt the program, because there's no 
prompt to type the command at?
>  - E.g., what does "show circular-trace-buffer" mean when
>    debugging a tfile?  "set circular-trace-buffer" changes
>    the local GDB flag, and "show circular-trace-buffer"
>    shows the according change, but, then we have no
>    way of knowing when debugging a tfile had been
>    in circular-trace-buffer mode or not when the tfile
>    was created.
>   
You would know if circularity had kicked in because tstatus on the file 
would show more frames created than were in the buffer.  If it hadn't 
kicked in, then the flag's value wouldn't be of much interest, right?

Stan

Re: PATCH: Circular trace buffers

[Pedro Alves]

On Wednesday 17 March 2010 16:55:38, Stan Shebs wrote:
> Pedro Alves wrote:
> > On Tuesday 16 March 2010 21:42:02, Stan Shebs wrote:
> >   
> >> This patch adds a flag that requests the target agent to make the trace 
> >> buffer circular, so that instead of filling it up and then stopping, the 
> >> agent discards the oldest trace frames as necessary to accommodate new 
> >> ones.  Any hairy memory management code is going to be on the target 
> >> side; GDB just has to transmit the setting (and now always via target 
> >> vector), and report back status, which may now include a total number of 
> >> frames that were created.  This also adds complete documentation of the 
> >> qTStatus reply, per request.  Any comments before I commit?
> >>     
> >
> > Playing devil's advogate here, I'm still not 100% convinced that
> > "set circular-trace-buffer" is 100% well defined and that
> > is isn't confusing in some cases; it applies on the fly
> > in some cases, does somewhat not-completly clear
> > things in other cases, and errors out in others.
> Hey Joel, pass me that aspirin bottle, willya? :-)  This is one of those 
> places where I'd like more user feedback before getting too fancy.  It's 
> the opposite situation of expression evaluation - we play fast and loose 
> with language semantics, but we know from extensive experience that 
> users don't want GDB to be too pedantic about visibility, scopes, etc.  
> But for tracepoints we're still mostly guessing.

But I'm not proposing anything fancy, on the contrary, something like:

 (gdb) help set circular-trace-buffer
 Set target's use of circular trace buffer in the next trace run.
 Use this to make the trace buffer into a circular buffer,
 which will discard traceframes (oldest first) instead of filling
 up and stopping the trace run.

Instead of:

 (gdb) help set circular-trace-buffer
 Set target's use of circular trace buffer.
 Use this to make the trace buffer into a circular buffer,
 which will discard traceframes (oldest first) instead of filling
 up and stopping the trace run.

Which doesn't say anything about the setting applying immediately,
or when a new trace run is started.

And:

 (gdb) help show circular-trace-buffer
 Show target's use of circular trace buffer in the next trace run.
 Use this to make the trace buffer into a circular buffer,
 which will discard traceframes (oldest first) instead of filling
 up and stopping the trace run.

Instead of:

 (gdb) help show circular-trace-buffer
 Show target's use of circular trace buffer.
 Use this to make the trace buffer into a circular buffer,
 which will discard traceframes (oldest first) instead of filling
 up and stopping the trace run.


As is "show circular-trace-buffer" is pretty much useless.
Answer this question:  What does this mean, in all supported
cases?

 (gdb) show circular-trace-buffer
 Target's use of circular trace buffer is on.

It looks as though these commands help strings should be
improved to clarify exactly what it means.

> > I wonder
> > if we defined "set circular-trace-buffer" as another flag
> > that is respected at "tstart" time only, and made the
> > presently running trace run's circular-trace-buffer-ness reported
> > through "tstatus", and define "show circular-trace-buffer" as the
> > "circular-trace-buffer-ness" intent at next trace run start,
> > things would be more consistent and clear.

> I thought about that, but it seemed like one of its uses would be as a 
> hasty way to keep a trace run alive; you do a tstatus, say "oh sh*t" as 
> you see the buffer at 80% full before you've reached the code of 
> interest, and quickly switch to circular buffer.

... oh sh*t, I forgot to disable that tracepoint!  Oh darn, you can't
do that when the trace is running.  Same thing, same general problem,
it seems.  This special casing in the circularity-ness adds
inconsistency (everything else is set at tstart time) which I
suspect will byte back.  But it's fine.  I'll just refuse to
address any such inconstencies myself and push the problem
back to you when it happens.  :-)

> >  - all-stop/async + trace running + "set circular-trace-buffer"
> >    errors out because you can't talk to the target if it
> >    is running in all-stop.
> >   

> I think the user would know to interrupt the program, because there's no 
> prompt to type the command at?

Note: "async".  Frontends are switching to use async mode by
default.  "-gdb-set circular-trace-buffer on" does not work
in that case, only in non-stop mode.

> >  - E.g., what does "show circular-trace-buffer" mean when
> >    debugging a tfile?  "set circular-trace-buffer" changes
> >    the local GDB flag, and "show circular-trace-buffer"
> >    shows the according change, but, then we have no
> >    way of knowing when debugging a tfile had been
> >    in circular-trace-buffer mode or not when the tfile
> >    was created.
> >   
> You would know if circularity had kicked in because tstatus on the file 
> would show more frames created than were in the buffer.  If it hadn't 
> kicked in, then the flag's value wouldn't be of much interest, right?

- this shows that "show circular-trace-buffer" is useless.
- this requires users know that fact.
- this doesn't sound user friendly.

-- 
Pedro Alves

Re: PATCH: Circular trace buffers

[Stan Shebs]

Pedro Alves wrote:
>> I thought about that, but it seemed like one of its uses would be as a 
>> hasty way to keep a trace run alive; you do a tstatus, say "oh sh*t" as 
>> you see the buffer at 80% full before you've reached the code of 
>> interest, and quickly switch to circular buffer.
>>     
>
> ... oh sh*t, I forgot to disable that tracepoint!  Oh darn, you can't
> do that when the trace is running.  Same thing, same general problem,
> it seems.
...and you may recall, we've been requested to add the ability to 
disable tracepoints during a run.
> This special casing in the circularity-ness adds
> inconsistency (everything else is set at tstart time) which I
> suspect will byte back.  But it's fine.  I'll just refuse to
> address any such inconstencies myself and push the problem
> back to you when it happens.  :-)
>   
If I had to guess, I'd say that the ultimate long-term model will tend 
toward on-the-fly change, rather than having it be a one-shot thing.  It 
is potentially messy to implement (tracepoint definitions that only 
apply to a subset of trace frames? ugh), but is more consistent with 
GDB's overall philosophy of letting users do whatever they can think of.
>   
>>>  - all-stop/async + trace running + "set circular-trace-buffer"
>>>    errors out because you can't talk to the target if it
>>>    is running in all-stop.
>>>   
>>>       
>
>   
>> I think the user would know to interrupt the program, because there's no 
>> prompt to type the command at?
>>     
>
> Note: "async".  Frontends are switching to use async mode by
> default.  "-gdb-set circular-trace-buffer on" does not work
> in that case, only in non-stop mode.
>   
Hmm, that doesn't sound good, guess I should investigate further.
>   
>>>  - E.g., what does "show circular-trace-buffer" mean when
>>>    debugging a tfile?  "set circular-trace-buffer" changes
>>>    the local GDB flag, and "show circular-trace-buffer"
>>>    shows the according change, but, then we have no
>>>    way of knowing when debugging a tfile had been
>>>    in circular-trace-buffer mode or not when the tfile
>>>    was created.
>>>   
>>>       
>> You would know if circularity had kicked in because tstatus on the file 
>> would show more frames created than were in the buffer.  If it hadn't 
>> kicked in, then the flag's value wouldn't be of much interest, right?
>>     
>
> - this shows that "show circular-trace-buffer" is useless.
> - this requires users know that fact.
> - this doesn't sound user friendly.
>   
I'm just not seeing a problem myself - it seems obvious that circularity 
of trace buffer only matters for future tracepoint hits, and doesn't 
matter for completed trace runs, trace files, etc.  But I can rephrase 
the docs to make that clearer.

Stan

Re: PATCH: Circular trace buffers

[Pedro Alves]

On Wednesday 17 March 2010 18:04:52, Stan Shebs wrote:
> > - this shows that "show circular-trace-buffer" is useless.
> > - this requires users know that fact.
> > - this doesn't sound user friendly.
> >   
> I'm just not seeing a problem myself - it seems obvious that circularity 
> of trace buffer only matters for future tracepoint hits, and doesn't 
> matter for completed trace runs, trace files, etc.  But I can rephrase 
> the docs to make that clearer.

(Yes, please.  Okay, let's go with that then.)

Let me show you examples: hopefully it is easy to see with
these how "show circular-trace-buffer" is broken as is.


Please can we have the following inconsistencies resolved?:

Target supports circular:

 (gdb) tar rem :9999
 (gdb) set circular-trace-buffer on
 (gdb) show circular-trace-buffer
 ... on.

Fine.


Again, a target that supports circular, target wasn't
tracing on initial connection (disconnect-tracing off):

 <not connected yet>
 (gdb) set circular-trace-buffer on
 (gdb) show circular-trace-buffer
 ... on.
 (gdb) tar rem :9999
 (gdb) show circular-trace-buffer
 ... on.
 (gdb) set disconnected-tracing on
 <set tracepoints>
 (gdb) tstart
 (gdb) detach
 <end remote debug session>
 (gdb) set circular-trace-buffer off
 (gdb) tar rem :9999
 (gdb) show circular-trace-buffer
  ... off

Really "off"?  Ouch!  See?  QTstatus doesn't report the
circularity-ness, but the circularity is logically part of
the status of the current run.

Now against a target that _doesn't_ support QTBuffer (in circular mode):

 <not connected yet>
 (gdb) set circular-trace-buffer on
 (gdb) show circular-trace-buffer
 ... on.
 (gdb) tar rem :9999
 <note, no complain, no warning>
 (gdb) show circular-trace-buffer
 Target's use of circular trace buffer is on.

Really?

 (gdb) set circular-trace-buffer off
 Target does not support this command.
 (gdb) set circular-trace-buffer on
 Target does not support this command.
 (gdb)

Ouch!

You can't easily try the latter case, because a new
"set remote circular...-packet command is missing.

-- 
Pedro Alves

Re: PATCH: Circular trace buffers

[Pedro Alves]

I see the patch went in, but how were the issues raised below addressed?
I don't understand why we'd want to have a "show circular-trace-buffer"
command that doesn't work correctly half the times, and hence can't
be trusted.  This means a frontend can not rely
on "-gdb-show circular-trace-buffer" to draw a toggle button, for
example (this is what I mean by calling it useless...).  I find
the disconnected-tracing example below quite alarming.

On Wednesday 17 March 2010 18:51:37, Pedro Alves wrote:
> On Wednesday 17 March 2010 18:04:52, Stan Shebs wrote:
> > > - this shows that "show circular-trace-buffer" is useless.
> > > - this requires users know that fact.
> > > - this doesn't sound user friendly.
> > >   
> > I'm just not seeing a problem myself - it seems obvious that circularity 
> > of trace buffer only matters for future tracepoint hits, and doesn't 
> > matter for completed trace runs, trace files, etc.  But I can rephrase 
> > the docs to make that clearer.
> 
> (Yes, please.  Okay, let's go with that then.)
> 
> Let me show you examples: hopefully it is easy to see with
> these how "show circular-trace-buffer" is broken as is.
> 
> 
> Please can we have the following inconsistencies resolved?:
> 
> Target supports circular:
> 
>  (gdb) tar rem :9999
>  (gdb) set circular-trace-buffer on
>  (gdb) show circular-trace-buffer
>  ... on.
> 
> Fine.
> 
> 
> Again, a target that supports circular, target wasn't
> tracing on initial connection (disconnect-tracing off):
> 
>  <not connected yet>
>  (gdb) set circular-trace-buffer on
>  (gdb) show circular-trace-buffer
>  ... on.
>  (gdb) tar rem :9999
>  (gdb) show circular-trace-buffer
>  ... on.
>  (gdb) set disconnected-tracing on
>  <set tracepoints>
>  (gdb) tstart
>  (gdb) detach
>  <end remote debug session>
>  (gdb) set circular-trace-buffer off
>  (gdb) tar rem :9999
>  (gdb) show circular-trace-buffer
>   ... off
> 
> Really "off"?  Ouch!  See?  QTstatus doesn't report the
> circularity-ness, but the circularity is logically part of
> the status of the current run.
> 
> Now against a target that _doesn't_ support QTBuffer (in circular mode):
> 
>  <not connected yet>
>  (gdb) set circular-trace-buffer on
>  (gdb) show circular-trace-buffer
>  ... on.
>  (gdb) tar rem :9999
>  <note, no complain, no warning>
>  (gdb) show circular-trace-buffer
>  Target's use of circular trace buffer is on.
> 
> Really?
> 
>  (gdb) set circular-trace-buffer off
>  Target does not support this command.
>  (gdb) set circular-trace-buffer on
>  Target does not support this command.
>  (gdb)
> 
> Ouch!
> 
> You can't easily try the latter case, because a new
> "set remote circular...-packet command is missing.

-- 
Pedro Alves

Re: PATCH: Circular trace buffers

[Stan Shebs]

Pedro Alves wrote:
> I see the patch went in, but how were the issues raised below addressed?
> I don't understand why we'd want to have a "show circular-trace-buffer"
> command that doesn't work correctly half the times, and hence can't
> be trusted.  This means a frontend can not rely
> on "-gdb-show circular-trace-buffer" to draw a toggle button, for
> example (this is what I mean by calling it useless...).  I find
> the disconnected-tracing example below quite alarming.
>   
I did pump up the doc details a bit... I'm also thinking that an 
additional tstatus field would inform the user nicely in both the tfile 
and the disconnected tracing cases, plus frontends can use it, while 
leaving the setshow itself under the control of the user.  I didn't want 
to put that in this checkin though, need to think about it a little more 
first - the "target does not support" messages should probably be 
handled differently.

Stan
> On Wednesday 17 March 2010 18:51:37, Pedro Alves wrote:
>   
>> On Wednesday 17 March 2010 18:04:52, Stan Shebs wrote:
>>     
>>>> - this shows that "show circular-trace-buffer" is useless.
>>>> - this requires users know that fact.
>>>> - this doesn't sound user friendly.
>>>>   
>>>>         
>>> I'm just not seeing a problem myself - it seems obvious that circularity 
>>> of trace buffer only matters for future tracepoint hits, and doesn't 
>>> matter for completed trace runs, trace files, etc.  But I can rephrase 
>>> the docs to make that clearer.
>>>       
>> (Yes, please.  Okay, let's go with that then.)
>>
>> Let me show you examples: hopefully it is easy to see with
>> these how "show circular-trace-buffer" is broken as is.
>>
>>
>> Please can we have the following inconsistencies resolved?:
>>
>> Target supports circular:
>>
>>  (gdb) tar rem :9999
>>  (gdb) set circular-trace-buffer on
>>  (gdb) show circular-trace-buffer
>>  ... on.
>>
>> Fine.
>>
>>
>> Again, a target that supports circular, target wasn't
>> tracing on initial connection (disconnect-tracing off):
>>
>>  <not connected yet>
>>  (gdb) set circular-trace-buffer on
>>  (gdb) show circular-trace-buffer
>>  ... on.
>>  (gdb) tar rem :9999
>>  (gdb) show circular-trace-buffer
>>  ... on.
>>  (gdb) set disconnected-tracing on
>>  <set tracepoints>
>>  (gdb) tstart
>>  (gdb) detach
>>  <end remote debug session>
>>  (gdb) set circular-trace-buffer off
>>  (gdb) tar rem :9999
>>  (gdb) show circular-trace-buffer
>>   ... off
>>
>> Really "off"?  Ouch!  See?  QTstatus doesn't report the
>> circularity-ness, but the circularity is logically part of
>> the status of the current run.
>>
>> Now against a target that _doesn't_ support QTBuffer (in circular mode):
>>
>>  <not connected yet>
>>  (gdb) set circular-trace-buffer on
>>  (gdb) show circular-trace-buffer
>>  ... on.
>>  (gdb) tar rem :9999
>>  <note, no complain, no warning>
>>  (gdb) show circular-trace-buffer
>>  Target's use of circular trace buffer is on.
>>
>> Really?
>>
>>  (gdb) set circular-trace-buffer off
>>  Target does not support this command.
>>  (gdb) set circular-trace-buffer on
>>  Target does not support this command.
>>  (gdb)
>>
>> Ouch!
>>
>> You can't easily try the latter case, because a new
>> "set remote circular...-packet command is missing.
>>     
>
>