Re: [patch] Change trace buffer size

Pedro Alves
2013-02-27T16:39:22Z

On 02/18/2013 06:13 PM, Abid, Hafiz wrote:
>
> Hi All, This patch adds support for QTBuffer:size in gdbserver
> and adds commands in gdb to use this packet to change the
> trace buffer size. The default value of the buffer size in
> gdb is -1 which means that target will use whatever size it likes.
>
> I notice that there are some tests that are using "maint packet"
> to send QTBuffer:size. I was not sure what to do with them so
> I have left them alone. If reviewers suggest, I can update/remove them.

Yeah, http://sourceware.org/ml/gdb-patches/2012-02/msg00303.html

So, previously, because gdbserver didn't implement this, some
tests were skipped. What about after the patch?

On 02/18/2013 06:13 PM, Abid, Hafiz wrote:
> +@item QTBuffer:size:@var{size}
> +This packet directs the target to make the trace buffer be of size
> +@var{size} if possible.
> +

This misses explaining the "-1" magic.

> +static int trace_buffer_size;

Should be a ssize_t, or LONGEST. The latter is probably
better/simpler in this case.

> +init_trace_buffer (int bufsize)

This "int" too.

> {
> - trace_buffer_lo = buf;
> + trace_buffer_size = bufsize;
> +
> + /* If we already have a trace buffer, try realloc'ing. */
> + if (trace_buffer_lo)
> + trace_buffer_lo = xrealloc (trace_buffer_lo, bufsize);
> + else
> + trace_buffer_lo = xmalloc (bufsize);

xrealloc(NULL, bufsize) is equivalent to xmalloc(bufsize),
so the if is unnecessary. Write only:

trace_buffer_lo = xrealloc (trace_buffer_lo, bufsize);

> +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);

This should send -1 really as literal "-1",
instead of ffff..ff, which depends on the size
of ULONGEST, which may not be the same as the target's.
IOW, it's host dependent.

> + reply = remote_get_noisy_reply (&target_buf, &target_buf_size);

We shouldn't be adding more uses of target_buf, target_buf_size.
Use:
remote_get_noisy_reply (&rs->buf, &rs->buf_size);

and:

if (*rs->buf == '\0')

but, OTOH, new packets should really be using packet_ok.

This:

> 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';

looks unrelated to trace buffer size. Please split it
into a separate patch.

> +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 test doesn't seem to really test that much. I wonder if
you have ideas on expanding it?

> +/* This variable is the requested trace buffer size, or -1 to indicate
> + that we don't care and leave it up to the target to set a size. */
> +
> +static int trace_buffer_size = -1;

Should probably be a LONGEST too. There's to reason to
actually limit to 31-bits, right?

> + add_setshow_zinteger_cmd ("trace-buffer-size", no_class,
> + &trace_buffer_size, _("\

Oh, I see... add_setshow_zinteger_cmd works with int...

--
Pedro Alves