From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5191 invoked by alias); 1 Mar 2013 20:02:20 -0000 Received: (qmail 5181 invoked by uid 22791); 1 Mar 2013 20:02:19 -0000 X-SWARE-Spam-Status: No, hits=-8.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_XS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 01 Mar 2013 20:02:10 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r21K21Kt015164 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 1 Mar 2013 15:02:01 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r21K1w3j005318; Fri, 1 Mar 2013 15:01:59 -0500 Message-ID: <51310936.40406@redhat.com> Date: Fri, 01 Mar 2013 20:02:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: "Abid, Hafiz" CC: gdb-patches@sourceware.org, stan@codesourcery.com, yao@codesourcery.com, eliz@gnu.org Subject: Re: [patch] Change trace buffer size(v2) References: <1362157623.18212.0@abidh-ubunto1104> In-Reply-To: <1362157623.18212.0@abidh-ubunto1104> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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/msg00043.txt.bz2 On 03/01/2013 05:07 PM, Abid, Hafiz wrote: >> if (*rs->buf == '\0') >> >> but, OTOH, new packets should really be using packet_ok. I think you missed this note. >> 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. 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. > > +* New remote packets > + A new packet has been defined to set the trace buffer size. > + Please model from previous examples: * New remote packets ... QTBuffer:circular Set the trace buffer to be linear or circular. It's good to actually show the packet here, so the user can quite find it in the manual, and, makes it easier to answer the question in a few years "when was packet FOO added?". > +@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. > 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. */ s/all is happy and/there's/ > + if (sval < 0 || sval == trace_buffer_size) > + { > + write_ok (own_buf); > + return; > + } > + /* 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/ > + if (tracing) > + { > + write_enn (own_buf); > + return; > + } Better do this check before anything else, avoid unnecessary special cases. 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": 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. > +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. FYI, there's no need to special case -1 actually: if (val < 0) buf += hexnumstr (buf, (ULONGEST) -val); else buf += hexnumstr (buf, (ULONGEST) val); > + > + 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. > + warning (_("Target does not support setting trace buffer size.")); > + if (strcmp (reply, "OK") != 0) > + warning (_("Bogus reply from target: %s"), reply); > +} > + > + > +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? I'd write as: 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. Double space after period. > +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_no_output "tstart" > +gdb_test "continue" ".*" "" Better to match something rather than ".*". Look for "run trace experiment" on other tests. > +gdb_test "tstatus" ".*Trace stopped because the buffer was full.*" \ > +"Buffer full check" Column overflow again, I think. > +gdb_test "tstop" ".*" "" gdb_test_test_no_output. > > + 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); > + As I mentioned elsewhere, I think it'd be better to use the new var_zuinteger_unlimited. We should really fix the broken -1 handling too, before 7.6. > add_setshow_string_cmd ("trace-user", class_trace, > &trace_user, _("\ > Set the user name to use for current and future trace runs"), _("\ -- Pedro Alves