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

Abid, Hafiz
2013-03-01T17:07:34Z

[-- Attachment #1: Type: text/plain, Size: 6035 bytes --]

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

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.

[-- Attachment #2: trace-buffer-size_v2.patch --]
[-- Type: text/x-patch, Size: 14095 bytes --]

diff --git a/gdb/NEWS b/gdb/NEWS
index 0877aa2..8fc433b 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -104,6 +104,10 @@ set debug notification
show debug notification
Control display of debugging info for async remote notification.

+set trace-buffer-size
+show trace-buffer-size
+ Request target to change the size of trace buffer.
+
* Removed commands

** For the Renesas Super-H architecture, the "regs" command has been removed
@@ -159,6 +163,9 @@ show filename-display
feature to be enabled. For more information, see:
http://fedoraproject.org/wiki/Features/MiniDebugInfo

+* New remote packets
+ A new packet has been defined to set the trace buffer size.
+
*** Changes in GDB 7.5

* GDB now supports x32 ABI. Visit <http://sites.google.com/site/x32abi/>
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5f39d2e..3cb24d5 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -11728,6 +11728,25 @@ for instance if you are looking at frames from a trace file.
@end table

@table @code
+@item set trace-buffer-size @var{n}
+@kindex set trace-buffer-size
+Request that the target use a trace buffer of @var{n} bytes. Not all
+targets will honor the request; they may have a compiled-in size for
+the trace buffer, or some other limitation. Set to a value of
+@code{-1} to let the target use whatever size it likes. This is also
+the default.
+
+@item show trace-buffer-size
+@kindex show trace-buffer-size
+Show the current requested size for the trace buffer. Note that this
+will only match the actual size if the target supports size-setting,
+and was able to handle the requested size. For instance, if the
+target can only change buffer size between runs, this variable will
+not reflect the change until the next run starts. Use @code{tstatus}
+to get a report of the actual buffer size.
+@end table
+
+@table @code
@item set trace-user @var{text}
@kindex set trace-user

@@ -38450,6 +38469,11 @@ available.
This packet directs the target to use a circular trace buffer if
@var{value} is 1, or a linear buffer if the value is 0.

+@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.
+
@item QTNotes:@r{[}@var{type}:@var{text}@r{]}@r{[};@var{type}:@var{text}@r{]}@dots{}
@cindex @samp{QTNotes} packet
This packet adds optional textual notes to the trace run. Allowable
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 0f83ae6..c0b3bd7 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -992,6 +992,8 @@ int current_traceframe = -1;
static int circular_trace_buffer;
#endif

+static LONGEST trace_buffer_size;
+
/* Pointer to the block of memory that traceframes all go into. */

static unsigned char *trace_buffer_lo;
@@ -1478,9 +1480,13 @@ clear_inferior_trace_buffer (void)
#endif

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 ();
@@ -4020,6 +4026,42 @@ cmd_bigqtbuffer_circular (char *own_buf)
}

static void
+cmd_bigqtbuffer_size (char *own_buf)
+{
+ ULONGEST val;
+ LONGEST sval;
+ char *packet = own_buf;
+
+ packet += strlen ("QTBuffer:size:");
+
+ /* -1 is sent as literal "-1". */
+ if (strcmp (packet, "-1") == 0)
+ sval = -1;
+ else
+ {
+ unpack_varlen_hex (packet, &val);
+ sval = (LONGEST)val;
+ }
+ /* If we don't care about the size, or the size is unchanged,
+ all is happy and nothing to do. */
+ if (sval < 0 || sval == trace_buffer_size)
+ {
+ write_ok (own_buf);
+ return;
+ }
+ /* Right now we can't change the size during a tracing run. */
+ if (tracing)
+ {
+ write_enn (own_buf);
+ return;
+ }
+ init_trace_buffer (sval);
+ trace_debug ("Trace buffer is now %s bytes",
+ plongest (trace_buffer_size));
+ write_ok (own_buf);
+}
+
+static void
cmd_qtnotes (char *own_buf)
{
size_t nbytes;
@@ -4143,6 +4185,11 @@ handle_tracepoint_general_set (char *packet)
cmd_bigqtbuffer_circular (packet);
return 1;
}
+ else if (strncmp ("QTBuffer:size:", packet, strlen ("QTBuffer:size:")) == 0)
+ {
+ cmd_bigqtbuffer_size (packet);
+ return 1;
+ }
else if (strncmp ("QTNotes:", packet, strlen ("QTNotes:")) == 0)
{
cmd_qtnotes (packet);
@@ -7228,10 +7275,9 @@ get_timestamp (void)
void
initialize_tracepoint (void)
{
- /* There currently no way to change the buffer size. */
- const int sizeOfBuffer = 5 * 1024 * 1024;
- unsigned char *buf = xmalloc (sizeOfBuffer);
- init_trace_buffer (buf, sizeOfBuffer);
+ /* Start with a size that's not too large. */
+ const int sizeOfBuffer = 1024 * 1024;
+ init_trace_buffer (sizeOfBuffer);

/* Wire trace state variable 1 to be the timestamp. This will be
uploaded to GDB upon connection and become one of its trace state
diff --git a/gdb/remote.c b/gdb/remote.c
index 88a57c8..8352e80 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -11042,6 +11042,29 @@ remote_get_min_fast_tracepoint_insn_len (void)
}
}

+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);
+
+ putpkt (rs->buf);
+ reply = remote_get_noisy_reply (&rs->buf, &rs->buf_size);
+ if (*rs->buf == '\0')
+ warning (_("Target does not support setting trace buffer size."));
+ if (strcmp (reply, "OK") != 0)
+ warning (_("Bogus reply from target: %s"), reply);
+}
+
static int
remote_set_trace_notes (char *user, char *notes, char *stop_notes)
{
@@ -11219,6 +11242,7 @@ Specify the serial device it is connected to\n\
remote_ops.to_get_min_fast_tracepoint_insn_len = remote_get_min_fast_tracepoint_insn_len;
remote_ops.to_set_disconnected_tracing = remote_set_disconnected_tracing;
remote_ops.to_set_circular_trace_buffer = remote_set_circular_trace_buffer;
+ remote_ops.to_set_trace_buffer_size = remote_set_trace_buffer_size;
remote_ops.to_set_trace_notes = remote_set_trace_notes;
remote_ops.to_core_of_thread = remote_core_of_thread;
remote_ops.to_verify_memory = remote_verify_memory;
diff --git a/gdb/target.c b/gdb/target.c
index 9d8bf6e..2e507e0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -693,6 +693,7 @@ update_current_target (void)
INHERIT (to_get_min_fast_tracepoint_insn_len, t);
INHERIT (to_set_disconnected_tracing, t);
INHERIT (to_set_circular_trace_buffer, t);
+ INHERIT (to_set_trace_buffer_size, t);
INHERIT (to_set_trace_notes, t);
INHERIT (to_get_tib_address, t);
INHERIT (to_set_permissions, t);
@@ -912,6 +913,9 @@ update_current_target (void)
de_fault (to_set_circular_trace_buffer,
(void (*) (int))
target_ignore);
+ de_fault (to_set_trace_buffer_size,
+ (void (*) (LONGEST))
+ target_ignore);
de_fault (to_set_trace_notes,
(int (*) (char *, char *, char *))
return_zero);
diff --git a/gdb/target.h b/gdb/target.h
index 1971265..c99642d 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -806,6 +806,8 @@ struct target_ops
disconnection - set VAL to 1 to keep tracing, 0 to stop. */
void (*to_set_disconnected_tracing) (int val);
void (*to_set_circular_trace_buffer) (int val);
+ /* Set the size of trace buffer in the target. */
+ void (*to_set_trace_buffer_size) (LONGEST val);

/* Add/change textual notes about the trace run, returning 1 if
successful, 0 otherwise. */
@@ -1700,6 +1702,9 @@ extern char *target_fileio_read_stralloc (const char *filename);
#define target_set_circular_trace_buffer(val) \
(*current_target.to_set_circular_trace_buffer) (val)

+#define target_set_trace_buffer_size(val) \
+ (*current_target.to_set_trace_buffer_size) (val)
+
#define target_set_trace_notes(user,notes,stopnotes) \
(*current_target.to_set_trace_notes) ((user), (notes), (stopnotes))

diff --git a/gdb/testsuite/gdb.trace/trace-buffer-size.c b/gdb/testsuite/gdb.trace/trace-buffer-size.c
new file mode 100644
index 0000000..e32b756
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-buffer-size.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2011-2013 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+int var = 10;
+
+int
+test_function ()
+{
+ return 0;
+}
+
+int
+main ()
+{
+ test_function ();
+ return 0; /*breakpoint1*/
+}
diff --git a/gdb/testsuite/gdb.trace/trace-buffer-size.exp b/gdb/testsuite/gdb.trace/trace-buffer-size.exp
new file mode 100644
index 0000000..4533f77
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-buffer-size.exp
@@ -0,0 +1,54 @@
+# Copyright 1998-2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "trace-support.exp"
+
+standard_testfile
+
+if [prepare_for_testing ${testfile}.exp $testfile $srcfile \
+ {debug nowarnings}] {
+ untested "failed to prepare for trace tests"
+ return -1
+}
+
+if ![runto_main] {
+ fail "can't run to main to check for trace support"
+ return -1
+}
+
+if ![gdb_target_supports_trace] {
+ unsupported "target does not support trace"
+ return -1;
+}
+
+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"
+
+# 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 "1. Set action for trace point" "" \
+ "collect var" "^$"
+gdb_test "tstart" ".*" ""
+gdb_test "continue" ".*" ""
+gdb_test "tstatus" ".*Trace stopped because the buffer was full.*" \
+"Buffer full check"
+gdb_test "tstop" ".*" ""
+
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 9a80aa3..bd4f2c7 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;
+
/* 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,
}

static void
+set_trace_buffer_size (char *args, int from_tty,
+ struct cmd_list_element *c)
+{
+ target_set_trace_buffer_size (trace_buffer_size);
+}
+
+static void
set_trace_user (char *args, int from_tty,
struct cmd_list_element *c)
{
@@ -5416,6 +5429,16 @@ up and stopping the trace run."),
&setlist,
&showlist);

+ add_setshow_zinteger_cmd ("trace-buffer-size", no_class,
+ &trace_buffer_size, _("\
+Set requested size of trace buffer."), _("\
+Show requested size of trace buffer."), _("\
+Use this to choose a size for the trace buffer. Some targets\n\
+may have fixed or limited buffer sizes. A value of -1 disables\n\
+any attempt to set the buffer size and lets the target choose."),
+ set_trace_buffer_size, NULL,
+ &setlist, &showlist);
+
add_setshow_string_cmd ("trace-user", class_trace,
&trace_user, _("\
Set the user name to use for current and future trace runs"), _("\