[patch] Change trace buffer size
Pedro Alves
palves@redhat.com
Wed Feb 27 16:39:00 GMT 2013
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
More information about the Gdb-patches
mailing list