From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19098 invoked by alias); 5 Mar 2013 20:13:24 -0000 Received: (qmail 19070 invoked by uid 22791); 5 Mar 2013 20:13:20 -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 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; Tue, 05 Mar 2013 20:13:10 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r25KD5Zu028979 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 5 Mar 2013 15:13:05 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r25KD3lH002289; Tue, 5 Mar 2013 15:13:04 -0500 Message-ID: <513651CF.6090504@redhat.com> Date: Tue, 05 Mar 2013 20:13:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130219 Thunderbird/17.0.3 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(v4) References: <1362492757.20677.0@abidh-ubunto1104> In-Reply-To: <1362492757.20677.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/msg00183.txt.bz2 On 03/05/2013 02:12 PM, Abid, Hafiz wrote: >> Oh, I just realized this does nothing to update >> the trace buffer of the in-process agent. :-/ > Changing IPA trace buffer was not supported in this patch. > Do you think that it must be done before this patch can go in? It's always good for the submission to be explicit in these things, and good to consider these issues upfront. I'm interested in hearing your thoughts on the subject upfront, and the plans you had, if any. I gave it some thought, and looked at the code to refresh a bit, and I think things will still work correctly if the IPA buffer is either smaller or larger than gdbserver's. Running the whole set of gdb.trace tests (--directory=gdb.trace) with a gdbserver and IPA hacked to default to different buffer sizes once would provide some assurance, though not that much. >> > + trace_debug ("Trace buffer is now %s bytes", >> > + plongest (trace_buffer_size)); >> > + write_ok (own_buf); >> >> Might as well write the more concise: >> >> if (sval == trace_buffer_size) >> { >> init_trace_buffer (sval); >> trace_debug ("Trace buffer is now %s bytes", >> plongest (trace_buffer_size)); >> } >> write_ok (own_buf); > I think you mean to say that there is no harm in calling init_trace_buffer > when sval == trace_buffer_size, as xrealloc will not do anything. So I > removed the check. I had actually meant as I wrote it, to preserve your original intention. It's was functionally the same as the original, but with only one write_ok call, and one return point, with the same number of ifs, hence, more concise. But removing the "if" is of course fine too, and perhaps really better for being simpler and for always displaying the debug message. >> > + putpkt (rs->buf); >> > + getpkt (&rs->buf, &rs->buf_size, 0); >> >> Any reason this version lost remote_get_noisy_reply? > I noticed that packet_ok () is used with getpkt thoughout the file. So I > removed remote_get_noisy_reply. OTOH, tracepoint packets call remote_get_noisy_reply, for an ancient reason, but nowadays to so that we assist the target with relocation if necessary (qRelocInsn; gdbserver uses it). > >> >> > + result = packet_ok (rs->buf, >> > + &remote_protocol_packets[PACKET_QTBuffer_size]); > >> > +gdb_test "show trace-buffer-size $BUFFER_SIZE" \ >> > +"Requested size of trace buffer is $BUFFER_SIZE.*" \ >> > +"Show Trace Buffer Size" >> Please indent the continuation lines, like you do below. > Fixed. Sorry again on this. I should have been more careful. Not a problem. After a while these things start sticking out. It'll come to you too. ;-) > 2012-03-05 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. Missed mentioning the new remote command. > +@item QTBuffer:size > +The remote stub supports the @samp{QTBuffer:size} (@pxref{QTBuffer:size}) > +packet that allows to change the size of trace buffer. > + "of the trace buffer" I think. > +# Save default trace buffer size in 'default_size'. > +gdb_test_multiple "tstatus" "tstatus check 1" { > + -re ".*Trace buffer has ($decimal) bytes of ($decimal) bytes free.*" { > + set default_size $expect_out(2,string) > + } > +} If this fails, then $default_size is left uninitialized, and then further below, we'll get tcl errors for using an unknown variable. The pattern we usually follow around such gdb_test_multiple cases, is to preinitialize the variable: set default_size -1 gdb_test_multiple "tstatus" "tstatus check 1" { -re ".*Trace buffer has ($decimal) bytes of ($decimal) bytes free.*" { set default_size $expect_out(2,string) } } and then something like, if { $default_size < 0 } { ... } > +* New remote packets > + > +QTBuffer:size > + Set the size of trace buffer. Add "The remote stub reports support for this packet to gdb's qSupported query." This looks to be almost ready now. Thanks, -- Pedro Alves