RE: [PATCH] gdb.btrace/*.exp: Make test names unique

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Simon Marchi [mailto:simon.marchi@ericsson.com]
> Sent: Thursday, October 6, 2016 4:32 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>; 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.
> 
> 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 aggressive
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 the initial
> > "next" and "stepi" that move to the relevant code and record the trace as
> > they are.
> 
> Ok.  Could we test_prefix them with maybe "setup", so it's clear what they're
> for?

Sounds good.  I was suggesting to leave them without but a "setup" test prefix
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.
> 
> 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 ...
> 
> 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.
> 
> 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:
> 
>   proc test_some_thing { } {
>     ...
>   }
> 
>   proc test_some_other_thing { } {
>     ...
>   }
> 
>   with_test_prefix "test some thing" { test_some_thing }
>   with_test_prefix "test some other thing" { test_some_other_thing }
> 
> 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 modify this
> test so it looks like that.

I agree in general.  In this case, we have two different recordings and a bunch
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 recording 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 well?
> > I.e. split the "continue" tests into sub-groups with separate prefixes.
> 
> Do you mean:
> 
> with_test_prefix "thread 1" {
>   with_test_prefix "continue forward" {
>     gdb_cont_to_no_history 1 "continue" 1
>     ...
>   }
> 
>   with_test_prefix "continue backward" {
>     gdb_cont_to_no_history 1 "reverse-continue" 1
>     ...
>   }
> }
> 
> ?

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