Hi Pedro,
Thanks for review. Here is an updated patch.

On 04/03/13 20:43:20, Pedro Alves wrote:
> One idea to make the testsuite check this would be to leverage  
> "tstatus",
> which prints the trace buffer size:
Done.

> The minus sign was missing:
> if (val < 0)
>     {
>       *buf++ = '-';
>       buf += hexnumstr (buf, (ULONGEST) -val);
>     }​
>   else
>     buf += hexnumstr (buf, (ULONGEST) val);
Done.

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

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

> 
> s/Added/Add/
Fixed. Sorry for making this mistake twice.

> 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?
Fixed.

> s/Added/Add/
Fixed.

> 
> >     (Tracepoint Packets): Document QTBuffer:size .
> Spurious space after that last '.'.
Fixed.

> 
> This new feature needs to be documented as well.
Done.

> 
> -  /* 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.
Changed it back to 5*1024*1024;

> > +static LONGEST trace_buffer_size;
> Misses leading comment.
Fixed.
> > +  init_trace_buffer (sval);
> 
> 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?

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

> 
> > +      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?
Yes. Only -1 in negative number should come here. So added an assertion.

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

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

> > +  "Buffer full check 1"
> s/Buffer/buffer/
Fixed.
> 
> > +
> > +# 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.
Fixed

> 
> > +  ".*Trace is running on the target.*" \
> > +  "Buffer full check 2"
> 
> s/Buffer/buffer/
Fixed.

> > +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.
I see that Yao has already sent a patch for it. So probably I don't  
need any change
here.

Thanks,
Abid

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.
	(PACKET_QTBuffer_size): New enum.
	(remote_protocol_features): Add an entry for
	PACKET_QTBuffer_size.

	gdb/gdbserver/
	* tracepoint.c (trace_buffer_size): New global.
	(DEFAULT_TRACE_BUFFER_SIZE): New define.
	(init_trace_buffer): Change to one-argument function. Allocate
	trace buffer memory.
	(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): Add QTBuffer:size in the
	supported packets.

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

	gdb/testsuite/
	* gdb.trace/trace-buffer-size.exp: New file.
	* gdb.trace/trace-buffer-size.c: New file.