[patch] Change trace buffer size(v3)

[Pedro Alves]

On 03/04/2013 07:02 PM, Abid, Hafiz wrote:

>>  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.
> I tested this manualy by running trace experiment to fill the trace buffer.
> Then changed the size to a larger value and run trace again. It worked ok.

One idea to make the testsuite check this would be to leverage "tstatus",
which prints the trace buffer size:

 (gdb) tstatus
 No trace has been run on the target.
 Collected 0 trace frames.
 Trace buffer has 1048576 bytes of 1048576 bytes free (0% full).
                                   ^^^^^^^
 Trace will stop if GDB disconnects.
 Not looking at any trace frame.

So, you'd read the default buffer size, change the trace buffer
size to something not the default, and then change it back
to the default with -1, and check that really worked.

>>
>>
>> > +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.
> Added check for val<0 && val != -1.
>>
>> FYI, there's no need to special case -1 actually:
>>
>>   if (val < 0)
>>     buf += hexnumstr (buf, (ULONGEST) -val);
>>   else
>>     buf += hexnumstr (buf, (ULONGEST) val);
> I did not clearly understand how it works. Because -val will end up sending 1. So I kept the
> literal -1 as you suggested in previous email. Although I modified the check from == -1 to < 0.

The minus sign was missing:

   if (val < 0)
     {
       *buf++ = '-';
       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.
> I have used packet_ok and related machinery so that gdbserver informs about the support of QTBuffer:size packet now.

Thanks.  The qSupported feature need to be documented as well.

>>
>> As I mentioned elsewhere, I think it'd be better to use the
>> new var_zuinteger_unlimited.
> Done.
>>
>> We should really fix the broken -1 handling too, before 7.6.
>>
> How do you suggest we do that?

Dunno, haven't looked in detail but I'm hoping it to
be obvious to whoever debugs it.  ;-)

> 2012-03-04  Stan Shebs  <stan@codesourcery.com>
>         Hafiz Abid Qadeer  <abidh@codesourcery.com>
> 
>     gdb/
>     * NEWS: Mention set and show trace-buffer-size commands.
>     Mention new packet.
>     * target.h (struct target_ops): New method
>     to_set_trace_buffer_size.
>     (target_set_trace_buffer_size): New macro.
>     * target.c (update_current_target): Set up new method.
>     * tracepoint.c (trace_buffer_size): New global.
>     (start_tracing): Send it to the target.
>     (set_trace_buffer_size): New function.
>     (_initialize_tracepoint): Add new setshow for trace-buffer-size.
>     * remote.c (remote_set_trace_buffer_size): New function.
>     (_initialize_remote): Use it.
>     (PACKET_QTBuffer_size): New enum.
>     (remote_protocol_features): Added an entry for
>     PACKET_QTBuffer_size.

s/Added/Add/

> 
>     gdb/gdbserver/
>     * tracepoint.c (trace_buffer_size): New global.
>     (DEFAULT_TRACE_BUFFER_SIZE): New define.
>     (init_trace_buffer): Change to one-argument function, add
>     realloc option.

Use '.', not ',' to separate changes.  "add realloc option" doesn't
really make sense.  I can guess it referred to the "if" in a previous
revision?

>     (handle_tracepoint_general_set): Call cmd_bigqtbuffer_size to
>     handle QTBuffer:size packet.
>     (cmd_bigqtbuffer_size): New function.
>     (initialize_tracepoint): Call init_trace_buffer with
>     DEFAULT_TRACE_BUFFER_SIZE.
>     * server.c (handle_query): Added QTBuffer:size in the
>     supported packets.

s/Added/Add/

> 
>     gdb/doc/
>     * gdb.texinfo (Starting and Stopping Trace Experiments): Document
>     trace-buffer-size set and show commands.
>     (Tracepoint Packets): Document QTBuffer:size .

Spurious space after that last '.'.

> index 768eae7..922a5bf 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -1637,6 +1637,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>  	  strcat (own_buf, ";qXfer:statictrace:read+");
>  	  strcat (own_buf, ";qXfer:traceframe-info:read+");
>  	  strcat (own_buf, ";EnableDisableTracepoints+");
> +	  strcat (own_buf, ";QTBuffer:size+");

This new feature needs to be documented as well.

>  	  strcat (own_buf, ";tracenz+");
>  	}
>  
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 0f83ae6..61d09c0 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -30,6 +30,8 @@
>  
>  #include "ax.h"
>  
> +#define DEFAULT_TRACE_BUFFER_SIZE 1048576 /* 1024*1024 */

This actually changes the default:

-  /* There currently no way to change the buffer size.  */
-  const int sizeOfBuffer = 5 * 1024 * 1024;

Not sure if that was intended.  If it was, please do that
as a separate change.

> +
>  /* This file is built for both GDBserver, and the in-process
>     agent (IPA), a shared library that includes a tracing agent that is
>     loaded by the inferior to support fast tracepoints.  Fast
> @@ -992,6 +994,8 @@ int current_traceframe = -1;
>  static int circular_trace_buffer;
>  #endif
>  
> +static LONGEST trace_buffer_size;
> +

Misses leading comment.

>  static void
> +cmd_bigqtbuffer_size (char *own_buf)
> +{
> +  ULONGEST val;
> +  LONGEST sval;
> +  char *packet = own_buf;
> +
> +  /* Can't change the size during a tracing run.  */
> +  if (tracing)
> +    {
> +      write_enn (own_buf);
> +      return;
> +    }
> +
> +  packet += strlen ("QTBuffer:size:");
> +
> +  /* -1 is sent as literal "-1".  */
> +  if (strcmp (packet, "-1") == 0)
> +    sval = DEFAULT_TRACE_BUFFER_SIZE;
> +  else
> +    {
> +      unpack_varlen_hex (packet, &val);
> +      sval = (LONGEST)val;
> +    }
> +  /* If the size is unchanged, there's nothing to do.  */
> +  if (sval == trace_buffer_size)
> +    {
> +      write_ok (own_buf);
> +      return;
> +    }
> +
> +  init_trace_buffer (sval);

Oh, I just realized this does nothing to update
the trace buffer of the in-process agent.  :-/

> +  trace_debug ("Trace buffer is now %s bytes",
> +	       plongest (trace_buffer_size));
> +  write_ok (own_buf);

Might as well write the more concise:

    if (sval == trace_buffer_size)
      {
        init_trace_buffer (sval);
        trace_debug ("Trace buffer is now %s bytes",
	       plongest (trace_buffer_size));
      }
    write_ok (own_buf);

> +}



> +static void
> +remote_set_trace_buffer_size (LONGEST val)
> +{
> +  if (remote_protocol_packets[PACKET_QTBuffer_size].support !=
> +      PACKET_DISABLE)
> +    {
> +      struct remote_state *rs = get_remote_state ();
> +      char *buf = rs->buf;
> +      char *endbuf = rs->buf + get_remote_packet_size ();
> +      enum packet_result result;
> +
> +      if (val < 0 && val != -1)
> +	{
> +	  warning (_("Invalid value %s for the size of trace buffer."),
> +		   plongest (val) );

Should really be an assertion.  There should be no way to
trigger this, unless there's a bug elsewhere, right?

> +	  return;
> +	}
> +
> +      buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
> +      /* Send -1 as literal "-1" to avoid host size dependency.  */
> +      if (val < 0)
> +	buf += xsnprintf (buf, endbuf - buf, "-1");
> +      else
> +	buf += hexnumstr (buf, (ULONGEST) val);
> +
> +      putpkt (rs->buf);
> +      getpkt (&rs->buf, &rs->buf_size, 0);

Any reason this version lost remote_get_noisy_reply?

> +      result = packet_ok (rs->buf,
> +		  &remote_protocol_packets[PACKET_QTBuffer_size]);
> +
> +      if (result != PACKET_OK)
> +	warning (_("Bogus reply from target: %s"), rs->buf);
> +    }
> +}


> diff --git a/gdb/testsuite/gdb.trace/trace-buffer-size.exp b/gdb/testsuite/gdb.trace/trace-buffer-size.exp

> +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"

Please indent the continuation lines, like you do below.

> +
> +# 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.
> +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 "Set action for trace point 1" "" \
> +  "collect var" "^$"
> +gdb_test_no_output "tstart" 
> +gdb_test "continue" \
> +  "Continuing.*Breakpoint $decimal.*" \
> +  "run trace experiment 1"
> +gdb_test "tstatus" \
> +  ".*Trace stopped because the buffer was full.*" \
> +  "Buffer full check 1"

s/Buffer/buffer/

> +
> +# Use the default size  and trace buffer should not be
> +# full this time.

Spurious double space after size.  I think connecting
the independent clauses with "and" reads a bit confusingly.
I suggest:

  # Use the default size -- the trace buffer should not end up
  # full this time.

> +clean_restart ${testfile}
> +runto_main
> +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 "Set action for trace point 2" "" \
> +  "collect var" "^$"
> +gdb_test_no_output "tstart" 
> +gdb_test "continue" \
> +  "Continuing.*Breakpoint $decimal.*" \
> +  "run trace experiment 2"
> +gdb_test "tstatus" \
> +  ".*Trace is running on the target.*" \
> +  "Buffer full check 2"

s/Buffer/buffer/

> +gdb_test_no_output "tstop"
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 9a80aa3..b8945bb 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -173,6 +173,11 @@ static int disconnected_tracing;
>  
>  static int circular_trace_buffer;
>  
> +/* 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;
> +

This is used with:

extern void
  add_setshow_zuinteger_unlimited_cmd (char *name,
				       enum command_class class,
				       unsigned int *var,
                                       ^^^^^^^^^^^^

So, ... we were discussing this in the other thread.
Not sure what to tell you to do here.  :-)  My inclination
is to change the function to take a signed integer, and
use -1 instead of UINT_MAX.

>  /* Textual notes applying to the current and/or future trace runs.  */
>  
>  char *trace_user = NULL;
> @@ -1829,6 +1834,7 @@ start_tracing (char *notes)
>    /* Set some mode flags.  */
>    target_set_disconnected_tracing (disconnected_tracing);
>    target_set_circular_trace_buffer (circular_trace_buffer);
> +  target_set_trace_buffer_size (trace_buffer_size);
>  
>    if (!notes)
>      notes = trace_notes;
> @@ -3231,6 +3237,13 @@ set_circular_trace_buffer (char *args, int from_tty,
>  }

-- 
Pedro Alves