Re: [patch] Change trace buffer size

[Pedro Alves <palves at redhat dot com>]

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