Re: PATCH: Circular trace buffers

[Eli Zaretskii <eliz at gnu dot org>]

> 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.