Re: [patch] Change trace buffer size(v2)

[Pedro Alves <palves@redhat.com>]

On 03/01/2013 05:07 PM, Abid, Hafiz wrote:

>>   if (*rs->buf == '\0')
>>
>> but, OTOH, new packets should really be using packet_ok.

I think you missed this note.

>> This test doesn't seem to really test that much.  I wonder if
>> you have ideas on expanding it?
> I have tried to expand the test by setting the buffer size to be very small and then testing the buffer to be full.

Good idea.

I think it'd be even better to run the test twice, once with
a larger buffer that fits the same trace frames, and make sure
the buffer doesn't end up full, and then another time with the
small buffer.  That'd cover the case of the target's buffer being
small by default, and the buffer size packet failing to work.

> 


> +* New remote packets
> +  A new packet has been defined to set the trace buffer size.
> +

Please model from previous examples:

* New remote packets
...
QTBuffer:circular
   Set the trace buffer to be linear or circular.

It's good to actually show the packet here, so the user
can quite find it in the manual, and, makes it easier to
answer the question in a few years "when was packet FOO added?".

> +@item QTBuffer:size:@var{size}
> +This packet directs the target to make the trace buffer be of size
> +@var{size} if possible. A value of @code{-1} tells the target to
> +use whatever size it likes.

Double space after period.  I think "prefers" is more natural in the
manual.

>  static void
> +cmd_bigqtbuffer_size (char *own_buf)
> +{
> +  ULONGEST val;
> +  LONGEST sval;
> +  char *packet = own_buf;
> +
> +  packet += strlen ("QTBuffer:size:");
> +
> +  /* -1 is sent as literal "-1".  */
> +  if (strcmp (packet, "-1") == 0)
> +    sval = -1;
> +  else
> +    {
> +      unpack_varlen_hex (packet, &val);
> +      sval = (LONGEST)val;
> +    }
> +  /* If we don't care about the size, or the size is unchanged,
> +     all is happy and nothing to do.  */

     s/all is happy and/there's/

> +  if (sval < 0 || sval == trace_buffer_size)
> +    {
> +      write_ok (own_buf);
> +      return;
> +    }
> +  /* Right now we can't change the size during a tracing run.  */

Let's drop distracting speculation:

  s/Right now we can't/Can't/

> +  if (tracing)
> +    {
> +       write_enn (own_buf);
> +       return;
> +    }

Better do this check before anything else, avoid
unnecessary special cases.

Also, if sval is < 0, this is not checking for "tracing" (fixed
by doing the check earlier), but also,

> +  init_trace_buffer (sval);

... nor does init_trace_buffer have any logic to
set the trace buffer size to whatever the "target likes":

 static void
-init_trace_buffer (unsigned char *buf, int bufsize)
+init_trace_buffer (LONGEST bufsize)
 {
-  trace_buffer_lo = buf;
+  trace_buffer_size = bufsize;
+
+  /* If we already have a trace buffer, try realloc'ing.  */
+  trace_buffer_lo = xrealloc (trace_buffer_lo, bufsize);
+
   trace_buffer_hi = trace_buffer_lo + bufsize;

   clear_trace_buffer ();


Please make sure to test that at least manually, though
it'd be better if the testsuite also exercised this.


> +static void
> +remote_set_trace_buffer_size (LONGEST val)
> +{
> +  struct remote_state *rs = get_remote_state ();
> +  char *reply;
> +  char *buf = rs->buf;
> +  char *endbuf = rs->buf + get_remote_packet_size ();
> +
> +  buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
> +  /* Send -1 as literal "-1" to avoid host size dependency.  */
> +  if (val == -1)
> +    buf += xsnprintf (buf, endbuf - buf, "-1");
> +  else
> +    buf += hexnumstr (buf, (ULONGEST) val);

This would mishandle other negative numbers (though
something else should be preventing those.
var_zuinteger_unlimited would be one way, though it'd
probably be a good idea to check for val<0 && val != -1 here too.

FYI, there's no need to special case -1 actually:

  if (val < 0)
    buf += hexnumstr (buf, (ULONGEST) -val);
  else
    buf += hexnumstr (buf, (ULONGEST) val);


> +
> +  putpkt (rs->buf);
> +  reply = remote_get_noisy_reply (&rs->buf, &rs->buf_size);
> +  if (*rs->buf == '\0')

Did you try packet_ok?  Really new commands/packets should be
using it.

> +    warning (_("Target does not support setting trace buffer size."));
> +  if (strcmp (reply, "OK") != 0)
> +    warning (_("Bogus reply from target: %s"), reply);
> +}
> +


> +
> +set BUFFER_SIZE 4
> +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"

I think this overflows 80 columns.  Since you're breaking the line, could
you break it so it doesn't overflow 80 columns, please?

I'd write as:

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"

> +
> +# We set trace buffer to very small size. Then after running trace, we check
> +# if it is full. This will show if setting trace buffer size really worked.

Double space after period.

> +gdb_breakpoint ${srcfile}:[gdb_get_line_number "breakpoint1"]
> +gdb_test "trace test_function" \
> +    "Tracepoint \[0-9\]+ at .*" \
> +    "set tracepoint at test_function"
> +gdb_trace_setactions "1. Set action for trace point" "" \
> +    "collect var" "^$"
> +gdb_test "tstart"     ".*" ""

gdb_test_no_output "tstart"

> +gdb_test "continue"   ".*" ""

Better to match something rather than ".*".

Look for "run trace experiment" on other tests.

> +gdb_test "tstatus"    ".*Trace stopped because the buffer was full.*" \
> +"Buffer full check"

Column overflow again, I think.

> +gdb_test "tstop"    ".*" ""

gdb_test_test_no_output.


>  
> +  add_setshow_zinteger_cmd ("trace-buffer-size", no_class,
> +			    &trace_buffer_size, _("\
> +Set requested size of trace buffer."), _("\
> +Show requested size of trace buffer."), _("\
> +Use this to choose a size for the trace buffer.  Some targets\n\
> +may have fixed or limited buffer sizes.  A value of -1 disables\n\
> +any attempt to set the buffer size and lets the target choose."),
> +			    set_trace_buffer_size, NULL,
> +			    &setlist, &showlist);
> +

As I mentioned elsewhere, I think it'd be better to use the
new var_zuinteger_unlimited.

We should really fix the broken -1 handling too, before 7.6.

>    add_setshow_string_cmd ("trace-user", class_trace,
>  			  &trace_user, _("\
>  Set the user name to use for current and future trace runs"), _("\

-- 
Pedro Alves