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

[Pedro Alves <palves@redhat.com>]

On 03/05/2013 02:12 PM, Abid, Hafiz wrote:

>> Oh, I just realized this does nothing to update
>> the trace buffer of the in-process agent.  :-/
> Changing IPA trace buffer was not supported in this patch.
> Do you think that it must be done before this patch can go in?

It's always good for the submission to be explicit in these
things, and good to consider these issues upfront.  I'm interested
in hearing your thoughts on the subject upfront, and the plans
you had, if any.

I gave it some thought, and looked at the code to refresh a bit,
and I think things will still work correctly if the IPA buffer is
either smaller or larger than gdbserver's.

Running the whole set of gdb.trace tests (--directory=gdb.trace)
with a gdbserver and IPA hacked to default to different
buffer sizes once would provide some assurance, though
not that much.

>> > +  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);
> I think you mean to say that there is no harm in calling init_trace_buffer
> when sval == trace_buffer_size, as xrealloc will not do anything. So I
> removed the check.

I had actually meant as I wrote it, to preserve your original
intention.  It's was functionally the same as the original, but
with only one write_ok call, and one return point, with the same
number of ifs, hence, more concise.  But removing the "if" is of
course fine too, and perhaps really better for being simpler
and for always displaying the debug message.

>> > +      putpkt (rs->buf);
>> > +      getpkt (&rs->buf, &rs->buf_size, 0);
>>
>> Any reason this version lost remote_get_noisy_reply?
> I noticed that packet_ok () is used with getpkt thoughout the file. So I
> removed remote_get_noisy_reply.

OTOH, tracepoint packets call remote_get_noisy_reply, for an
ancient reason, but nowadays to so that we assist the target
with relocation if necessary (qRelocInsn; gdbserver uses it).

> 
>>
>> > +      result = packet_ok (rs->buf,
>> > +          &remote_protocol_packets[PACKET_QTBuffer_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.
> Fixed. Sorry again on this. I should have been more careful.

Not a problem.  After a while these things start sticking
out.  It'll come to you too.  ;-)

> 2012-03-05  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.

Missed mentioning the new remote command.

> +@item QTBuffer:size
> +The remote stub supports the @samp{QTBuffer:size} (@pxref{QTBuffer:size})
> +packet that allows to change the size of trace buffer.
> +

"of the trace buffer" I think.

> +# Save default trace buffer size in 'default_size'.
> +gdb_test_multiple "tstatus" "tstatus check 1" {
> +    -re ".*Trace buffer has ($decimal) bytes of ($decimal) bytes free.*" {
> +        set default_size $expect_out(2,string)
> +    }
> +}

If this fails, then $default_size is left uninitialized,
and then further below, we'll get tcl errors for using
an unknown variable.  The pattern we usually follow around
such gdb_test_multiple cases, is to preinitialize the variable:

   set default_size -1
   gdb_test_multiple "tstatus" "tstatus check 1" {
      -re ".*Trace buffer has ($decimal) bytes of ($decimal) bytes free.*" {
          set default_size $expect_out(2,string)
      }
   }

and then something like,

 if { $default_size < 0 } {
   ...
 }

> +* New remote packets
> +
> +QTBuffer:size
> +   Set the size of trace buffer.

Add "The remote stub reports support for this packet
to gdb's qSupported query."

This looks to be almost ready now.  Thanks,
-- 
Pedro Alves