Re: [patch] Change trace buffer size

[Yao Qi <yao@codesourcery.com>]

On 02/19/2013 02:13 AM, Abid, Hafiz wrote:

Hi Abid,

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 0877aa2..ea565ea 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -104,6 +104,10 @@ set debug notification
>   show debug notification
>     Control display of debugging info for async remote notification.
>
> +set trace-buffer-size
> +show trace-buffer-size
> +  Request target to change the size of trace buffer.
> +
>   * Removed commands

We may mention the new remote packet in NEWS.

> diff --git a/gdb/remote.c b/gdb/remote.c
> index 88a57c8..e0ddaf9 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -11042,6 +11042,23 @@ remote_get_min_fast_tracepoint_insn_len (void)
>       }
>   }
>
> +static void
> +remote_set_trace_buffer_size (LONGEST val)
> +{
> +  struct remote_state *rs = get_remote_state ();
> +  char *p, *reply;
> +
> +  sprintf (rs->buf, "QTBuffer:size:");
> +  p = rs->buf + strlen (rs->buf);
> +  p += hexnumstr (p, (ULONGEST) val);
> +  putpkt (rs->buf);
> +  reply = remote_get_noisy_reply (&target_buf, &target_buf_size);
> +  if (*reply == '\0')
> +    error (_("Target does not support setting trace buffer size."));

We may need a warning instead of an error here.

> +  if (strcmp (reply, "OK") != 0)
> +    error (_("Bogus reply from target: %s"), reply);
> +}
> +
>   static int
>   remote_set_trace_notes (char *user, char *notes, char *stop_notes)
>   {
> @@ -11050,6 +11067,7 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes)
>     char *buf = rs->buf;
>     char *endbuf = rs->buf + get_remote_packet_size ();
>     int nbytes;
> +  int any = 0;
>
>     buf += xsnprintf (buf, endbuf - buf, "QTNotes:");
>     if (user)
> @@ -11058,6 +11076,7 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes)
>         nbytes = bin2hex (user, buf, 0);
>         buf += 2 * nbytes;
>         *buf++ = ';';
> +      any = 1;
>       }
>     if (notes)
>       {
> @@ -11065,6 +11084,7 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes)
>         nbytes = bin2hex (notes, buf, 0);
>         buf += 2 * nbytes;
>         *buf++ = ';';
> +      any = 1;
>       }
>     if (stop_notes)
>       {
> @@ -11072,7 +11092,10 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes)
>         nbytes = bin2hex (stop_notes, buf, 0);
>         buf += 2 * nbytes;
>         *buf++ = ';';
> +      any = 1;
>       }
> +    if (any == 0)
> +      return 0;
>     /* Ensure the buffer is terminated.  */
>     *buf = '\0';
>

I think this chunk is correct, but is it really related to this patch?

> diff --git a/gdb/target.c b/gdb/target.c
> index 9d8bf6e..2e507e0 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -693,6 +693,7 @@ update_current_target (void)
>         INHERIT (to_get_min_fast_tracepoint_insn_len, t);
>         INHERIT (to_set_disconnected_tracing, t);
>         INHERIT (to_set_circular_trace_buffer, t);
> +      INHERIT (to_set_trace_buffer_size, t);
>         INHERIT (to_set_trace_notes, t);
>         INHERIT (to_get_tib_address, t);
>         INHERIT (to_set_permissions, t);
> @@ -912,6 +913,9 @@ update_current_target (void)
>     de_fault (to_set_circular_trace_buffer,
>   	    (void (*) (int))
>   	    target_ignore);
> +  de_fault (to_set_trace_buffer_size,
> +	    (void (*) (LONGEST))
> +	    target_ignore);

I am wondering 'tcomplain' may be better than 'target_ignore'.

>     de_fault (to_set_trace_notes,
>   	    (int (*) (char *, char *, char *))
>   	    return_zero);
> diff --git a/gdb/target.h b/gdb/target.h
> index 1971265..03b3d78 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -806,6 +806,7 @@ struct target_ops
>          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);
> +    void (*to_set_trace_buffer_size) (LONGEST val);

Comments on this field are needed, IMO.

> diff --git a/gdb/testsuite/gdb.trace/trace-buffer-size.c b/gdb/testsuite/gdb.trace/trace-buffer-size.c
> new file mode 100644
> index 0000000..79a9ed1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/trace-buffer-size.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2011-2013 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see<http://www.gnu.org/licenses/>.  */
> +
> +int
> +main ()
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.trace/trace-buffer-size.exp b/gdb/testsuite/gdb.trace/trace-buffer-size.exp
> new file mode 100644
> index 0000000..5c6c989
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/trace-buffer-size.exp
> @@ -0,0 +1,40 @@
> +# Copyright 1998-2013 Free Software Foundation, Inc.
                ^^^^ 2011? to align with trace-buffer-size.c
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see<http://www.gnu.org/licenses/>.
> +
> +load_lib "trace-support.exp"
> +
> +standard_testfile
> +
> +if [prepare_for_testing ${testfile}.exp $testfile $srcfile \
> +        {debug nowarnings}] {
> +    untested "failed to prepare for trace tests"
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    fail "can't run to main to check for trace support"
> +    return -1
> +}
> +
> +if ![gdb_target_supports_trace] {
> +    unsupported "target does not support trace"
> +    return -1;
> +}
> +
> +set BUFFER_SIZE 1024
> +gdb_test_no_output "set trace-buffer-size $BUFFER_SIZE" "Set Trace Buffer Size"
> +gdb_test "show trace-buffer-size $BUFFER_SIZE" "Requested size of trace buffer is $BUFFER_SIZE.*" \
> +"Show Trace Buffer Size"
> +

This only tests the commands on setting and showing the trace buffer 
size.  Do you think we need also test the affected behaviour of tracing? 
  I mean, setting the trace buffer size to a really small number, and 
see tracing is stopped quickly as trace buffer becomes full?

-- 
Yao (齐尧)