From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 35CE138930CB for ; Wed, 29 Apr 2020 16:03:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 35CE138930CB Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-200-yd6e76owNFe7UIZE61LRBQ-1; Wed, 29 Apr 2020 12:03:40 -0400 X-MC-Unique: yd6e76owNFe7UIZE61LRBQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D74E380058A; Wed, 29 Apr 2020 16:03:38 +0000 (UTC) Received: from theo.uglyboxes.com (ovpn-112-87.phx2.redhat.com [10.3.112.87]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 753EE60C18; Wed, 29 Apr 2020 16:03:38 +0000 (UTC) Subject: Re: [PATCHv2 0/3] Automatic detection of test name problems To: Andrew Burgess Cc: Simon Marchi , gdb-patches@sourceware.org References: <290d0de3-264b-9a60-487f-ff6b484c35f1@redhat.com> <20200429090216.GI3522@embecosm.com> <20200429153852.GL3522@embecosm.com> From: Keith Seitz Message-ID: Date: Wed, 29 Apr 2020 09:03:37 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200429153852.GL3522@embecosm.com> Content-Language: en-US X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-25.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_BL, RCVD_IN_MSPIKE_L3, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Apr 2020 16:03:52 -0000 On 4/29/20 8:38 AM, Andrew Burgess wrote: > * Simon Marchi [2020-04-29 11:04:40 -0400]: > >> When you detect an offender, do you think you could print something? A bit like a "FAIL" >> is printed when a test fails. For example: >> >> DUPLICATE: gdb.base/break.exp: set convenience variable $foo to 81.5 >> >> That would make it easier to spot the problematic test(s). But even without that, your >> patchset looks good and useful to me. That is a great idea, Simon. [Again -- why didn't I think of that?] > However, I chose to print "DUPLICATE: ...." for _every_ duplicate, so > you would get this: > > PASS: foo > PASS: foo > DUPLICATE: foo > PASS: foo > DUPLICATE: foo > PASS: bar > PASS: bar > DUPLICATE: bar > PASS: bar > DUPLICATE: bar > > Obviously we don't get a DUPLICATE message for the first 'foo', or the > first 'bar', we can't know by that point that these are going to be > duplicated. > > But, it might be confusing that this test will report 2 duplicates, > but contain 4 DUPLICATE lines. Would it in fact be better to report a > count of 4 in this case I wonder? I don't really care one way or the other. This is an exceptional case, and is /far/ less offensive than, e.g., assertions aborting gdb. Seeing messages just reinforces the message IMO. YMMV. Just one or two Tcl comments. Of course. ;-) > diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp > index 15c5544c8e4..8b525897ba3 100644 > --- a/gdb/testsuite/lib/check-test-names.exp > +++ b/gdb/testsuite/lib/check-test-names.exp > @@ -43,24 +43,27 @@ namespace eval ::CheckTestNames { > incr counts($type,total) > } > > - # Check if MESSAGE contains either the source path or the build path. > - # This will result in test names that can't easily be compared between > - # different runs of GDB. > - # > - # Any offending paths in message cause PATHS_IN_TEST_NAMES to be > - # incremented. > - proc check { message } { > + # Check if MESSAGE contains a build or source path, if it does increment > + # the relevant counter and return 1, otherwise, return 0. > + proc _check_paths { message } { > global srcdir objdir > - variable all_test_names > > foreach path [list $srcdir $objdir] { > if { [ string first $path $message ] >= 0 } { > # Count each test just once. > inc_count paths > - return > + return 1 > } > } > > + return 0 > + } Tcl supports booleans. This could return true/false. > + > + # Check if MESSAGE is a duplicate, if it is then increment the > + # duplicates counter and return 1, otherwise, return 0. > + proc _check_duplicates { message } { > + variable all_test_names > + > # Initialise a count, or increment the count for this test name. > if {![info exists all_test_names($message)]} { > set all_test_names($message) 0 > @@ -69,6 +72,43 @@ namespace eval ::CheckTestNames { > inc_count duplicates > } > incr all_test_names($message) > + return 1 > + } > + > + return 0 > + } > + > + # Remove the leading Dejagnu status marker from MESSAGE, and return the > + # remainder of MESSAGE. A status marker is something like 'PASS: '. If > + # no status marker is found, then just return MESSAGE unmodified. > + proc _strip_status { message } { > + foreach status {PASS FAIL XFAIL KFAIL XPASS KPASS UNRESOLVED \ > + UNTESTED UNSUPPORTED} { > + set txt "${status}: " > + if { [string compare -length [string len $txt] \ > + $txt $message] == 0 } { > + return [string range $message [string len $txt] end] Maybe it doesn't matter, but we know /all/ test result messages from DejaGNU are going to start "STATUS: $result_message". Would it be worth simply looking for ": " from the start of the string? That would eliminate the loop here and greatly simplify this. It is, of course, less precise than the above. Keith > + } > + } > + > + return $message > + } > + > + # Check if MESSAGE contains either the source path or the build path. > + # This will result in test names that can't easily be compared between > + # different runs of GDB. > + # > + # Any offending paths in message cause PATHS_IN_TEST_NAMES to be > + # incremented. > + proc check { message } { > + set message [ _strip_status $message ] > + > + if [ _check_paths $message ] { > + clone_output "PATH: $message" > + } > + > + if [ _check_duplicates $message ] { > + clone_output "DUPLICATE: $message" > } > } > >