Re: [patch] Change trace buffer size

Yao Qi
2013-02-19T02:53:13Z

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 (é½å°§)