[patch] Change trace buffer size(v3)

[Abid, Hafiz]

Hi Pedro,
Thanks for review. Here is an updated patch which tries to addresses  
your comments.

On 01/03/13 20:01:58, Pedro Alves wrote:
> but, OTOH, new packets should really be using packet_ok.
> I think you missed this note.
Fixed.
> 
> 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.
Done.
> 
> Please model from previous examples:
> 
> * New remote packets
> ...
> QTBuffer:circular
>    Set the trace buffer to be linear or circular.
Done.
> 
> > +@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.
Fixed.
> 

> > +  /* 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/
Fixed.
> 
> > +  /* 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/
Fixed.
> 
> > +  if (tracing)
> > +    {
> > +       write_enn (own_buf);
> > +       return;
> > +    }
> 
> Better do this check before anything else, avoid
> unnecessary special cases.
Moved to start of function.
> 
> 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":
I have now added a default size. On -1, that deafult size is used.
> 
>  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.
> 
> 
> > +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.
> 
> 
> > +
> > +  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.
> 
> > +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?
Fixed

> > +
> > +# 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.
Fixed
> 
> > +gdb_test "tstart"     ".*" ""
> 
> gdb_test_no_output "tstart"
Done
> 
> > +gdb_test "continue"   ".*" ""
> Better to match something rather than ".*".
> Look for "run trace experiment" on other tests.
Fixed.
> 
> > +gdb_test "tstatus"    ".*Trace stopped because the buffer was  
> full.*" \
> > +"Buffer full check"
> 
> Column overflow again, I think.
Fixed.
> 
> gdb_test_test_no_output.
Removed this command as it was not really needed.
> 
> 
> 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?

Regards,
Abid

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.

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

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

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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: trace-buffer-size_v3.patch
Type: text/x-patch
Size: 16393 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20130304/ee91a3b2/attachment.bin>