From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23079 invoked by alias); 27 Feb 2013 16:39:22 -0000 Received: (qmail 22929 invoked by uid 22791); 27 Feb 2013 16:39: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; Wed, 27 Feb 2013 16:39:13 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1RGd9dF004478 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 27 Feb 2013 11:39:10 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r1RGd7Tb001130; Wed, 27 Feb 2013 11:39:08 -0500 Message-ID: <512E36AB.90200@redhat.com> Date: Wed, 27 Feb 2013 16:39: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 Subject: Re: [patch] Change trace buffer size References: <1361211216.2217.2@abidh-ubunto1104> In-Reply-To: <1361211216.2217.2@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-02/txt/msg00687.txt.bz2 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? 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. > + 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. > +init_trace_buffer (int bufsize) This "int" too. > { > - trace_buffer_lo = buf; > + trace_buffer_size = bufsize; > + > + /* If we already have a trace buffer, try realloc'ing. */ > + if (trace_buffer_lo) > + trace_buffer_lo = xrealloc (trace_buffer_lo, bufsize); > + else > + trace_buffer_lo = xmalloc (bufsize); xrealloc(NULL, bufsize) is equivalent to xmalloc(bufsize), so the if is unnecessary. Write only: trace_buffer_lo = xrealloc (trace_buffer_lo, bufsize); > +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. > + 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); and: if (*rs->buf == '\0') but, OTOH, new packets should really be using packet_ok. This: > static int > remote_set_trace_notes (char *user, char *notes, char *stop_notes) > { > @@ -11050,6 +11067,7 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes) > char *buf = rs->buf; > char *endbuf = rs->buf + get_remote_packet_size (); > int nbytes; > + int any = 0; > > buf += xsnprintf (buf, endbuf - buf, "QTNotes:"); > if (user) > @@ -11058,6 +11076,7 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes) > nbytes = bin2hex (user, buf, 0); > buf += 2 * nbytes; > *buf++ = ';'; > + any = 1; > } > if (notes) > { > @@ -11065,6 +11084,7 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes) > nbytes = bin2hex (notes, buf, 0); > buf += 2 * nbytes; > *buf++ = ';'; > + any = 1; > } > if (stop_notes) > { > @@ -11072,7 +11092,10 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes) > nbytes = bin2hex (stop_notes, buf, 0); > buf += 2 * nbytes; > *buf++ = ';'; > + any = 1; > } > + if (any == 0) > + return 0; > /* Ensure the buffer is terminated. */ > *buf = '\0'; looks unrelated to trace buffer size. Please split it into 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? > +/* 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... -- Pedro Alves