Hi All, Thanks for the review. I am attaching a new version with the changes. How does it look? On 27/02/13 16:39:07, Pedro Alves wrote: > On 02/18/2013 06:13 PM, Abid, Hafiz wrote: > > > > Hi All, This patch adds support for QTBuffer:size in gdbserver > > and adds commands in gdb to use this packet to change the > > trace buffer size. The default value of the buffer size in > > gdb is -1 which means that target will use whatever size it likes. > > > > I notice that there are some tests that are using "maint packet" > > to send QTBuffer:size. I was not sure what to do with them so > > I have left them alone. If reviewers suggest, I can update/remove > them. > > Yeah, http://sourceware.org/ml/gdb-patches/2012-02/msg00303.html > > So, previously, because gdbserver didn't implement this, some > tests were skipped. What about after the patch? I run the circ.exp with it. The qtBuffer:size test work ok. I have to modify circ.exp a little to send literal '-1'. Also I observed that most of the tracing tests in circ.exp were being skipped. I had to add a runto_main before gdb_target_supports_trace to execute those tests. I will send another patch that will take care of circ.exp. > > On 02/18/2013 06:13 PM, Abid, Hafiz wrote: > > +@item QTBuffer:size:@var{size} > > +This packet directs the target to make the trace buffer be of size > > +@var{size} if possible. > > + Fixed. > > This misses explaining the "-1" magic. > > > +static int trace_buffer_size; > > Should be a ssize_t, or LONGEST. The latter is probably > better/simpler in this case. Fixed. > > > +init_trace_buffer (int bufsize) > > This "int" too. Fixed. > > xrealloc(NULL, bufsize) is equivalent to xmalloc(bufsize), > so the if is unnecessary. Write only: > > trace_buffer_lo = xrealloc (trace_buffer_lo, bufsize); Done. > > +static void > > +remote_set_trace_buffer_size (LONGEST val) > > +{ > > + struct remote_state *rs = get_remote_state (); > > + char *p, *reply; > > + > > + sprintf (rs->buf, "QTBuffer:size:"); > > + p = rs->buf + strlen (rs->buf); > > + p += hexnumstr (p, (ULONGEST) val); > > + putpkt (rs->buf); > > This should send -1 really as literal "-1", > instead of ffff..ff, which depends on the size > of ULONGEST, which may not be the same as the target's. > IOW, it's host dependent. Fixed. > > > + reply = remote_get_noisy_reply (&target_buf, &target_buf_size); > > We shouldn't be adding more uses of target_buf, target_buf_size. > Use: > remote_get_noisy_reply (&rs->buf, &rs->buf_size); Done. > > and: > > if (*rs->buf == '\0') > > but, OTOH, new packets should really be using packet_ok. > > This: > looks unrelated to trace buffer size. Please split it > into a separate patch. Removed from this patch. Will send in a separate patch. > > > +set BUFFER_SIZE 1024 > > +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" > > + > > 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. > > > +/* 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; > > Should probably be a LONGEST too. There's to reason to > actually limit to 31-bits, right? > > > + add_setshow_zinteger_cmd ("trace-buffer-size", no_class, > > + &trace_buffer_size, _("\ > > Oh, I see... add_setshow_zinteger_cmd works with int... Observed that commands added with add_setshow_zinteger_cmd have problem with -1. For var_zinteger, do_set_command uses unsigned int and it ends up comparing 0xffffffff to INT_MAX. If it sounds like an oversight to you then I will send a patch. Here are those lines for a quick look. unsigned int val; ... val = parse_and_eval_long (arg); ... else if (val >= INT_MAX) error (_("integer %u out of range"), val); > > -- > Pedro Alves > > Following changes were suggested by Yao. > We may mention the new remote packet in NEWS. Done. >> + error (_("Target does not support setting trace buffer size.")); > We may need a warning instead of an error here. Fixed. >> + void (*to_set_trace_buffer_size) (LONGEST val); > Comments on this field are needed, IMO. Done. Also fixed the documentation as suggested by Eli. Regards, Abid 2012-03-01 Stan Shebs Hafiz Abid Qadeer 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. gdb/gdbserver/ * tracepoint.c (trace_buffer_size): New global. (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): Default to a smaller buffer. 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.