From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 95858 invoked by alias); 7 Oct 2016 07:43:11 -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 95836 invoked by uid 89); 7 Oct 2016 07:43:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.5 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=Managing, Tel, tel, commercial X-HELO: mga14.intel.com Received: from mga14.intel.com (HELO mga14.intel.com) (192.55.52.115) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Oct 2016 07:42:59 +0000 Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP; 07 Oct 2016 00:42:57 -0700 X-ExtLoop1: 1 Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by fmsmga002.fm.intel.com with ESMTP; 07 Oct 2016 00:42:50 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.19]) by IRSMSX101.ger.corp.intel.com ([163.33.3.153]) with mapi id 14.03.0248.002; Fri, 7 Oct 2016 08:42:49 +0100 From: "Metzger, Markus T" To: Simon Marchi , "gdb-patches@sourceware.org" Subject: RE: [PATCH] gdb.btrace/*.exp: Make test names unique Date: Fri, 07 Oct 2016 07:43:00 -0000 Message-ID: References: <20161003191729.1412-1-simon.marchi@ericsson.com> <1b86240b-3c1e-fdfe-6386-3f99f52e490c@ericsson.com> In-Reply-To: <1b86240b-3c1e-fdfe-6386-3f99f52e490c@ericsson.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-10/txt/msg00156.txt.bz2 > -----Original Message----- > From: Simon Marchi [mailto:simon.marchi@ericsson.com] > Sent: Thursday, October 6, 2016 4:32 PM > To: Metzger, Markus T ; gdb- > patches@sourceware.org > Subject: Re: [PATCH] gdb.btrace/*.exp: Make test names unique Hi Simon, > >> This patch makes the btrace test names unique. I was trying to > >> understand why rn-dl-bind.exp fails on my machine, and saw that it > >> failed at test "next". Given that there are multiple "next" in that > >> test, it's hard to know which one doesn't work. [...] > Here's the gdb.log, if it can help you. >=20 > http://paste.ubuntu.com/23284745/ Thanks for sharing the log. It does help. You don't have debug information installed, which makes things more difficult for GDB. The current tail-call detection heuristic is too aggres= sive if there are no symbols available. I posted a patch some time ago to make it less aggressive in this case: https://sourceware.org/ml/gdb-patches/2016-07/msg00277.html This fixes (or hides, if you prefer) the issue you're seeing. I don't want= to rule out that there might be other cases where we get the call stack wrong if we don't have symbols available. Let me ping again for this patch series. > >> diff --git a/gdb/testsuite/gdb.btrace/delta.exp > >> b/gdb/testsuite/gdb.btrace/delta.exp > >> index c9dbf38..c822400 100644 > >> --- a/gdb/testsuite/gdb.btrace/delta.exp > >> +++ b/gdb/testsuite/gdb.btrace/delta.exp > >> @@ -47,7 +47,7 @@ with_test_prefix "no trace" { > >> } > >> > >> # we record each single-step, even if we have not seen a branch, yet. > >> -gdb_test "stepi" > >> +gdb_test "stepi" "main\.4.*" "stepi #1" > > > > I wouldn't count on the stepi landing there. This is also not relevant= for the > > test. > > > > I'd rather add test prefixes for the below groups of tests and leave th= e initial > > "next" and "stepi" that move to the relevant code and record the trace = as > > they are. >=20 > Ok. Could we test_prefix them with maybe "setup", so it's clear what the= y're > for? Sounds good. I was suggesting to leave them without but a "setup" test pre= fix sounds even better. > > I.e. here... > > > >> # check that we can reverse-stepi that instruction > > > > ... and here ... > > > >> # and back > > > > We also wouldn't need to count the "info record" commands in this case. >=20 > Right, so maybe prefix those blocks with "backwards" and "forward"? OK. > >> diff --git a/gdb/testsuite/gdb.btrace/enable.exp > > > > Instead of adding a counter here ... > > > >> # enable btrace > >> -gdb_test_no_output "record btrace" "record btrace" > >> +gdb_test_no_output "record btrace" "record btrace #1" > > > > ... we might want to add a test prefix here ... >=20 > Perhaps "first record", "second record" and "record after re-run"? How about a test prefix "re-run" for the first block from the second runto_main until the second clean_restart (exclusive) and "re-run, enable" for the remainder of the file? > >> diff --git a/gdb/testsuite/gdb.btrace/function_call_history.exp > >> b/gdb/testsuite/gdb.btrace/function_call_history.exp > >> index 7d1e4049..53fd239 100644 > >> --- a/gdb/testsuite/gdb.btrace/function_call_history.exp > >> +++ b/gdb/testsuite/gdb.btrace/function_call_history.exp > >> @@ -30,7 +30,7 @@ if ![runto_main] { > >> } > >> > >> # start btrace > >> -gdb_test_no_output "record btrace" > >> +gdb_test_no_output "record btrace" "record btrace #1" > >> > >> # set bp after increment loop and continue > >> set bp_location [gdb_get_line_number "bp.1" $testfile.c] > >> @@ -236,7 +236,7 @@ gdb_continue_to_breakpoint "cont to fib.3" > >> gdb_continue_to_breakpoint "cont to fib.4" > >> > >> # start tracing > >> -gdb_test_no_output "record btrace" > >> +gdb_test_no_output "record btrace" "record btrace #2" > > > > The bottom of this file from the second "runto_main" is really an > > independent test. > > > > We might either prefix the entire test from the second runto_main until > > the end of the file or split the file into two. >=20 > If they test different aspects of the same feature, it's fine if they are= in the same > file. > I like to at least separate logically independent tests in procs: >=20 > proc test_some_thing { } { > ... > } >=20 > proc test_some_other_thing { } { > ... > } >=20 > with_test_prefix "test some thing" { test_some_thing } > with_test_prefix "test some other thing" { test_some_other_thing } >=20 > And as much as possible, have them be independent, meaning that one does = not > rely on the state left by the other, so you can comment out all but one, = reorder > them, and it would still work. If you agree with that I can try to modif= y this > test so it looks like that. I agree in general. In this case, we have two different recordings and a b= unch of tests for each. The tests must be different because the recordings are = different. Are you suggesting to add one test prefix around the first group of recordi= ng and tests and another test prefix around the second group? I'd be OK with that but it sounds like more effort than just splitting the = file. > >> diff --git a/gdb/testsuite/gdb.btrace/non-stop.exp [...] > >> @@ -175,29 +181,37 @@ with_test_prefix "continue" { > >> with_test_prefix "thread 1" { > >> gdb_cont_to_no_history 1 "continue" 1 > >> gdb_test "thread apply 1 info record" \ > >> - ".*Recorded \[0-9\]+ instructions \[^\\\r\\\n\]*" > >> + ".*Recorded \[0-9\]+ instructions \[^\\\r\\\n\]*" \ > >> + "thread apply 1 info record #1" > >> gdb_test "thread apply 2 info record" \ > >> - ".*Replay in progress\. At instruction 5\." > >> + ".*Replay in progress\. At instruction 5\." \ > >> + "thread apply 2 info record #1" > >> > >> gdb_cont_to_no_history 1 "reverse-continue" 1 > >> gdb_test "thread apply 1 info record" \ > >> - ".*Replay in progress\. At instruction 1\." > >> + ".*Replay in progress\. At instruction 1\." \ > >> + "thread apply 1 info record #2" > >> gdb_test "thread apply 2 info record" \ > >> - ".*Replay in progress\. At instruction 5\." > >> + ".*Replay in progress\. At instruction 5\." \ > >> + "thread apply 2 info record #2" > >> } > > > > Could we do the same trick you did with the "navigate" tests here, as w= ell? > > I.e. split the "continue" tests into sub-groups with separate prefixes. >=20 > Do you mean: >=20 > with_test_prefix "thread 1" { > with_test_prefix "continue forward" { > gdb_cont_to_no_history 1 "continue" 1 > ... > } >=20 > with_test_prefix "continue backward" { > gdb_cont_to_no_history 1 "reverse-continue" 1 > ... > } > } >=20 > ? Yes. There's already a test prefix "continue" around the group so you may just say "forward" and "backward". Thanks, Markus. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928