From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14210 invoked by alias); 8 May 2013 15:13:33 -0000 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 Received: (qmail 14198 invoked by uid 89); 8 May 2013 15:13:33 -0000 X-Spam-SWARE-Status: No, score=-8.4 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 08 May 2013 15:13:32 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r48FDUrD028099 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 8 May 2013 11:13:30 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r48FDRDB028473; Wed, 8 May 2013 11:13:29 -0400 Message-ID: <518A6B97.6070402@redhat.com> Date: Wed, 08 May 2013 15:13:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: "Abid, Hafiz" CC: gdb-patches@sourceware.org Subject: Re: [patch] circ.exp References: <1368007885.2238.4@abidh-ubunto1104> In-Reply-To: <1368007885.2238.4@abidh-ubunto1104> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00281.txt.bz2 On 05/08/2013 11:11 AM, Abid, Hafiz wrote: > I have used similar code sequence as you described above. But I was wondering if we can > issue an unsupported call in the 2nd '-re' and then return. That should eliminate the need > for 'circular_supported'. It also generates an extra pass while we then call unsupported below. It's really not a biggie either way. This, PASS: check whether circular buffer is supported FAIL: check whether circular buffer is supported UNSUPPORTED: check whether circular buffer is supported reads to me as if it's the checking itself that is not supported. (Very) Pedantically, that 2nd -re means the "check whether circular buffer is supported" test succeeded. And from that successful test, we can infer that the target in fact does not support a circular buffer. Contrast with an alternative message that focuses on what we want to outcome to be, and it'd make a little more sense to me to issue the unsupported in the 2nd -re: PASS: target buffer is set to circular FAIL: target buffer is set to circular UNSUPPORTED: target buffer is set to circular Here FAIL would be an unexpected outcome (caught by gdb_test_multiple internally, so adorned with "(timeout)" or something else). Note a separate a variable is still somewhat useful to bail in the case gdb_test_multiple catches a FAILs internally (timeout, internal error). We can also check those in the return of gdb_test_multiple, but a separate variable usually reads nicer, IMO. > -# return 0 for success, 1 for failure > -proc run_trace_experiment { pass } { > - gdb_run_cmd > +# Set a tracepoint on given func. The tracepoint is set at entry > +# address and not 'after prologue' address. I'd find extending the comment with something like: ", because we use 'tfind pc func' to find the corresponding trace frame afterwards, and that looks for entry address." to be useful. > +# Check if changing the trace buffer size is supported. This step is > +# repeated twice. This helps in case if trace buffer size is 100. s/This helps in case if/The helps in case the/ > + # Check that we get the total_size and free_size Missing period. > + if { $total_size < 0 } { > return 1 > } > > - return 0 > + if { $free_size < 0 } { > + return 1 > + } > } > > -# return 0 for success, 1 for failure > -proc gdb_trace_circular_tests { } { > - if { ![gdb_target_supports_trace] } then { > - unsupported "Current target does not support trace" > +# Calculate the size of a single frame Missing period. Might as well just give it a quick audit over all strings and add the missing periods. :-) > +# Shrink the trace buffer so that it will not hold > +# all ten trace frames. Verify that frame for func0 is still > +# collected, but frame for func9 is not. s/that frame for/that the frame for/ s/but frame for/but the the frame for/ > +# 1) the first frame will be overwritten and therefore unavailable Missing period here too. > + # Check that teh first frame is actually at func0. typo: "teh". The patch is okay with these fixed. Go ahead and apply it (but still post the final version). Thanks, -- Pedro Alves