From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6824 invoked by alias); 1 Mar 2013 17:07:34 -0000 Received: (qmail 6811 invoked by uid 22791); 1 Mar 2013 17:07:32 -0000 X-SWARE-Spam-Status: No, hits=-4.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_QT,TW_XF,TW_XS X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 01 Mar 2013 17:07:10 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1UBTQK-0005mt-Qe from Hafiz_Abid@mentor.com ; Fri, 01 Mar 2013 09:07:08 -0800 Received: from SVR-IES-FEM-02.mgc.mentorg.com ([137.202.0.106]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 1 Mar 2013 09:07:08 -0800 Received: from abidh-ubunto1104 (137.202.0.76) by SVR-IES-FEM-02.mgc.mentorg.com (137.202.0.106) with Microsoft SMTP Server (TLS) id 14.1.289.1; Fri, 1 Mar 2013 17:07:04 +0000 Date: Fri, 01 Mar 2013 17:07:00 -0000 From: "Abid, Hafiz" Subject: Re: [patch] Change trace buffer size(v2) To: Pedro Alves CC: , , , In-Reply-To: <512E36AB.90200@redhat.com> (from palves@redhat.com on Wed Feb 27 16:39:07 2013) Message-ID: <1362157623.18212.0@abidh-ubunto1104> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-vI4LJcP6BVo0T5C1Lmu6" Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-03/txt/msg00030.txt.bz2 --=-vI4LJcP6BVo0T5C1Lmu6 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 5984 Hi All, Thanks for the review. I am attaching a new version with the changes.=20=20 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=20=20 > them. >=20 > Yeah, http://sourceware.org/ml/gdb-patches/2012-02/msg00303.html >=20 > 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=20=20 modify circ.exp a little to send literal '-1'. Also I observed that=20=20 most of the tracing tests in circ.exp were being skipped. I had to add=20=20 a runto_main before gdb_target_supports_trace to execute those tests. I=20= =20 will send another patch that will take care of circ.exp. >=20 > 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. >=20 > This misses explaining the "-1" magic. >=20 > > +static int trace_buffer_size; >=20 > Should be a ssize_t, or LONGEST. The latter is probably > better/simpler in this case. Fixed. >=20 > > +init_trace_buffer (int bufsize) >=20 > This "int" too. Fixed. >=20 > xrealloc(NULL, bufsize) is equivalent to xmalloc(bufsize), > so the if is unnecessary. Write only: >=20 > trace_buffer_lo =3D xrealloc (trace_buffer_lo, bufsize); Done. > > +static void > > +remote_set_trace_buffer_size (LONGEST val) > > +{ > > + struct remote_state *rs =3D get_remote_state (); > > + char *p, *reply; > > + > > + sprintf (rs->buf, "QTBuffer:size:"); > > + p =3D rs->buf + strlen (rs->buf); > > + p +=3D hexnumstr (p, (ULONGEST) val); > > + putpkt (rs->buf); >=20 > 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. >=20 > > + reply =3D remote_get_noisy_reply (&target_buf, &target_buf_size); >=20 > We shouldn't be adding more uses of target_buf, target_buf_size. > Use: > remote_get_noisy_reply (&rs->buf, &rs->buf_size); Done. >=20 > and: >=20 > if (*rs->buf =3D=3D '\0') >=20 > but, OTOH, new packets should really be using packet_ok. >=20 > This: > looks unrelated to trace buffer size. Please split it > into a separate patch. Removed from this patch. Will send in a separate patch. >=20 > > +set BUFFER_SIZE 1024 > > +gdb_test_no_output "set trace-buffer-size $BUFFER_SIZE" "Set Trace=20= =20 > Buffer Size" > > +gdb_test "show trace-buffer-size $BUFFER_SIZE" "Requested size of=20=20 > trace buffer is $BUFFER_SIZE.*" \ > > +"Show Trace Buffer Size" > > + >=20 > 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=20=20 small and then testing the buffer to be full. >=20 > > +/* This variable is the requested trace buffer size, or -1 to=20=20 > indicate > > + that we don't care and leave it up to the target to set a=20=20 > size. */ > > + > > +static int trace_buffer_size =3D -1; >=20 > Should probably be a LONGEST too. There's to reason to > actually limit to 31-bits, right? >=20 > > + add_setshow_zinteger_cmd ("trace-buffer-size", no_class, > > + &trace_buffer_size, _("\ >=20 > Oh, I see... add_setshow_zinteger_cmd works with int... Observed that commands added with add_setshow_zinteger_cmd have problem=20= =20 with -1. For var_zinteger, do_set_command uses unsigned int and it ends=20= =20 up comparing 0xffffffff to INT_MAX. If it sounds like an oversight to=20=20 you then I will send a patch. Here are those lines for a quick look. unsigned int val; ... val =3D parse_and_eval_long (arg); ... else if (val >=3D INT_MAX) error (_("integer %u out of range"), val); >=20 > -- > Pedro Alves >=20 >=20 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):=20=20 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. --=-vI4LJcP6BVo0T5C1Lmu6 Content-Type: text/x-patch; charset="us-ascii"; name="trace-buffer-size_v2.patch" Content-Disposition: attachment; filename="trace-buffer-size_v2.patch" Content-Transfer-Encoding: quoted-printable Content-length: 13853 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. =20 +set trace-buffer-size +show trace-buffer-size + Request target to change the size of trace buffer. + * Removed commands =20 ** For the Renesas Super-H architecture, the "regs" command has been rem= oved @@ -159,6 +163,9 @@ show filename-display feature to be enabled. For more information, see: http://fedoraproject.org/wiki/Features/MiniDebugInfo =20 +* New remote packets + A new packet has been defined to set the trace buffer size. + *** Changes in GDB 7.5 =20 * GDB now supports x32 ABI. Visit 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 t= race file. @end table =20 @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 =20 @@ -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. =20 +@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 =3D -1; static int circular_trace_buffer; #endif =20 +static LONGEST trace_buffer_size; + /* Pointer to the block of memory that traceframes all go into. */ =20 static unsigned char *trace_buffer_lo; @@ -1478,9 +1480,13 @@ clear_inferior_trace_buffer (void) #endif =20 static void -init_trace_buffer (unsigned char *buf, int bufsize) +init_trace_buffer (LONGEST bufsize) { - trace_buffer_lo =3D buf; + trace_buffer_size =3D bufsize; + + /* If we already have a trace buffer, try realloc'ing. */ + trace_buffer_lo =3D xrealloc (trace_buffer_lo, bufsize); + trace_buffer_hi =3D trace_buffer_lo + bufsize; =20 clear_trace_buffer (); @@ -4020,6 +4026,42 @@ cmd_bigqtbuffer_circular (char *own_buf) } =20 static void +cmd_bigqtbuffer_size (char *own_buf) +{ + ULONGEST val; + LONGEST sval; + char *packet =3D own_buf; + + packet +=3D strlen ("QTBuffer:size:"); + + /* -1 is sent as literal "-1". */ + if (strcmp (packet, "-1") =3D=3D 0) + sval =3D -1; + else + { + unpack_varlen_hex (packet, &val); + sval =3D (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 =3D=3D 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:")) = =3D=3D 0) + { + cmd_bigqtbuffer_size (packet); + return 1; + } else if (strncmp ("QTNotes:", packet, strlen ("QTNotes:")) =3D=3D 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 =3D 5 * 1024 * 1024; - unsigned char *buf =3D xmalloc (sizeOfBuffer); - init_trace_buffer (buf, sizeOfBuffer); + /* Start with a size that's not too large. */ + const int sizeOfBuffer =3D 1024 * 1024; + init_trace_buffer (sizeOfBuffer); =20 /* 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) } } =20 +static void +remote_set_trace_buffer_size (LONGEST val) +{ + struct remote_state *rs =3D get_remote_state (); + char *reply; + char *buf =3D rs->buf; + char *endbuf =3D rs->buf + get_remote_packet_size (); + + buf +=3D xsnprintf (buf, endbuf - buf, "QTBuffer:size:"); + /* Send -1 as literal "-1" to avoid host size dependency. */ + if (val =3D=3D -1) + buf +=3D xsnprintf (buf, endbuf - buf, "-1"); + else + buf +=3D hexnumstr (buf, (ULONGEST) val); + + putpkt (rs->buf); + reply =3D remote_get_noisy_reply (&rs->buf, &rs->buf_size); + if (*rs->buf =3D=3D '\0') + warning (_("Target does not support setting trace buffer size.")); + if (strcmp (reply, "OK") !=3D 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 =3D remote_get_min_fast_t= racepoint_insn_len; remote_ops.to_set_disconnected_tracing =3D remote_set_disconnected_traci= ng; remote_ops.to_set_circular_trace_buffer =3D remote_set_circular_trace_bu= ffer; + remote_ops.to_set_trace_buffer_size =3D remote_set_trace_buffer_size; remote_ops.to_set_trace_notes =3D remote_set_trace_notes; remote_ops.to_core_of_thread =3D remote_core_of_thread; remote_ops.to_verify_memory =3D 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); =20 /* 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) =20 +#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)) =20 diff --git a/gdb/testsuite/gdb.trace/trace-buffer-size.c b/gdb/testsuite/gd= b.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 . = */ + +int var =3D 10; + +int +test_function () +{ + return 0; +} + +int +main () +{ + test_function ();=20 + 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 . + +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 bu= ffer is $BUFFER_SIZE.*" \ +"Show Trace Buffer Size" + +# We set trace buffer to very small size. Then after running trace, we che= ck +# 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; =20 static int circular_trace_buffer; =20 +/* 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 =3D -1; + /* Textual notes applying to the current and/or future trace runs. */ =20 char *trace_user =3D 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); =20 if (!notes) notes =3D trace_notes; @@ -3231,6 +3237,13 @@ set_circular_trace_buffer (char *args, int from_tty, } =20 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); =20 + 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"), _("\ --=-vI4LJcP6BVo0T5C1Lmu6--