[v2,1/5] Extend test gdb.python/py-recurse-unwind.exp

  On Wed, 9 Nov 2016 13:59:13 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 11/02/2016 10:14 PM, Kevin Buettner wrote:
> > diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.c b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> > index 02a835a..bd0330a 100644
> > --- a/gdb/testsuite/gdb.python/py-recurse-unwind.c
> > +++ b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> > @@ -18,14 +18,26 @@
> >  /* This is the test program loaded into GDB by the py-recurse-unwind test.  */
> >  
> >  void
> > -ccc (int arg)
> > +ccc0 (int arg)
> > +{
> > +}
> > +
> > +void
> > +ccc1 (int arg)
> > +{
> > +}
> > +
> > +void
> > +ccc2 (int arg)
> >  {
> >  }
> >  
> >  void
> >  bbb (int arg)
> >  {
> > -  ccc (789);
> > +  ccc0 (789);
> > +  ccc1 (789);
> > +  ccc2 (789);
> >  }
> >    
> 
> Do we need separate functions?  You could do that with a
> single function by making main call the same function more
> than once, in a loop or unrolled, so that you don't need to
> keep adding functions.  Or do with without continuing the
> inferior, even, by using gdb's "flushregs" command.
> Or was that to make sure test messages are unique below?

I don't remember why I added separate functions, though it might have
had something to do with uniqueness.  I don't see much value in it
now, so I added a loop in main() instead.

> > +    # Test that the python-based unwinder / sniffer was actually called
> > +    # during generation of the backtrace.
> > +    gdb_test "python print(TestUnwinder.count > 0)" "True" \
> > +             "python unwinder called for $tst"
> >  }  
> 
> I would suggest using "with_test_prefix $tst" instead of manually
> adding $tst.  The gdb_breakpoint / gdb_continue_to_breakpoint
> calls don't include $tst, and while currently you'll end up with
> unique test messages due the unique function names, that seems
> like fragility easily avoided.

I've done this too.

> Otherwise, LGTM.

Thanks again for the review.

This is what I've pushed...

commit 1a2f3d7ff1d79b1290704e48c71e905b987393a6
Author: Kevin Buettner <kevinb@redhat.com>
Date:   Mon Sep 26 15:00:37 2016 -0700

    Extend test gdb.python/py-recurse-unwind.exp
    
    This patch modifies the unwinder (sniffer) defined in
    py-recurse-unwind.py so that, depending upon the value of one of its
    class variables, it will take different paths through the code,
    testing different functionality.
    
    The original test attempted to obtain the value of an undefined
    symbol.
    
    This somewhat expanded test checks to see if 'pc' can be read via
    gdb.PendingFrame.read_register() and also via gdb.parse_and_eval().
    
    gdb/testsuite/ChangeLog:
    
    	* gdb.python/py-recurse-unwind.c (main): Add loop.
    	* gdb.python/py-recurse-unwind.py (TestUnwinder): Add calls
    	to read_register() and gdb.parse_and_eval().  Make each code
    	call a separate case that can be individually tested.
    	* gdb.python/py-recurse-unwind.exp (cont_and_backtrace): New
    	proc. Call cont_and_backtrace for each of the code paths that
    	we want to test in the unwinder.
---
 gdb/testsuite/ChangeLog                        | 10 ++++
 gdb/testsuite/gdb.python/py-recurse-unwind.c   |  6 ++-
 gdb/testsuite/gdb.python/py-recurse-unwind.exp | 63 ++++++++++++++++----------
 gdb/testsuite/gdb.python/py-recurse-unwind.py  | 29 ++++++++++--
 4 files changed, 79 insertions(+), 29 deletions(-)
  

On Wednesday, November 16 2016, Kevin Buettner wrote:

>> Otherwise, LGTM.
>
> Thanks again for the review.
>
> This is what I've pushed...

Heya,

While checking some BuildBot logs, I noticed a few new FAILures
introduced by this commit.  You can see them at:

  <https://sourceware.org/ml/gdb-testers/2016-q4/msg03062.html>

Cheers,

> commit 1a2f3d7ff1d79b1290704e48c71e905b987393a6
> Author: Kevin Buettner <kevinb@redhat.com>
> Date:   Mon Sep 26 15:00:37 2016 -0700
>
>     Extend test gdb.python/py-recurse-unwind.exp
>     
>     This patch modifies the unwinder (sniffer) defined in
>     py-recurse-unwind.py so that, depending upon the value of one of its
>     class variables, it will take different paths through the code,
>     testing different functionality.
>     
>     The original test attempted to obtain the value of an undefined
>     symbol.
>     
>     This somewhat expanded test checks to see if 'pc' can be read via
>     gdb.PendingFrame.read_register() and also via gdb.parse_and_eval().
>     
>     gdb/testsuite/ChangeLog:
>     
>     	* gdb.python/py-recurse-unwind.c (main): Add loop.
>     	* gdb.python/py-recurse-unwind.py (TestUnwinder): Add calls
>     	to read_register() and gdb.parse_and_eval().  Make each code
>     	call a separate case that can be individually tested.
>     	* gdb.python/py-recurse-unwind.exp (cont_and_backtrace): New
>     	proc. Call cont_and_backtrace for each of the code paths that
>     	we want to test in the unwinder.
> ---
>  gdb/testsuite/ChangeLog                        | 10 ++++
>  gdb/testsuite/gdb.python/py-recurse-unwind.c   |  6 ++-
>  gdb/testsuite/gdb.python/py-recurse-unwind.exp | 63 ++++++++++++++++----------
>  gdb/testsuite/gdb.python/py-recurse-unwind.py  | 29 ++++++++++--
>  4 files changed, 79 insertions(+), 29 deletions(-)
>
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index d9e61f4..82126c0 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,13 @@
> +2016-11-16  Kevin Buettner  <kevinb@redhat.com>
> +
> +	* gdb.python/py-recurse-unwind.c (main): Add loop.
> +	* gdb.python/py-recurse-unwind.py (TestUnwinder): Add calls
> +	to read_register() and gdb.parse_and_eval().  Make each code
> +	call a separate case that can be individually tested.
> +	* gdb.python/py-recurse-unwind.exp (cont_and_backtrace): New
> +	proc. Call cont_and_backtrace for each of the code paths that
> +	we want to test in the unwinder.
> +
>  2016-11-15  Andreas Arnez  <arnez@linux.vnet.ibm.com>
>  
>  	* gdb.dwarf2/bitfield-parent-optimized-out.exp: Fix DWARF code for
> diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.c b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> index 02a835a..77acec1 100644
> --- a/gdb/testsuite/gdb.python/py-recurse-unwind.c
> +++ b/gdb/testsuite/gdb.python/py-recurse-unwind.c
> @@ -37,6 +37,10 @@ aaa (int arg)
>  int
>  main ()
>  {
> -  aaa (123);
> +  int i;
> +
> +  for (i = 0; i < 10; i++)
> +    aaa (123);
> +
>    return 0;
>  }
> diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.exp b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
> index 9629a97..97c69f7 100644
> --- a/gdb/testsuite/gdb.python/py-recurse-unwind.exp
> +++ b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
> @@ -45,29 +45,46 @@ if ![runto_main] then {
>      return 0
>  }
>  
> -gdb_breakpoint "ccc"
>  
> -gdb_continue_to_breakpoint "ccc"
> -
> -# If the unwinder is active, the usage count will increment while
> -# running to the breakpoint.  Reset it prior to doing the backtrace.
> -gdb_test_no_output "python TestUnwinder.reset_count()"
> -
> -# The python based unwinder should be called a number of times while
> -# generating the backtrace, but its sniffer always returns None.  So
> -# it doesn't really contribute to generating any of the frames below.
> -#
> -# But that's okay.  Our goal here is to make sure that GDB doesn't
> -# get hung up in potentially infinite recursion when invoking the
> -# Python-based unwinder.
> -
> -gdb_test_sequence "bt"  "backtrace" {
> -    "\\r\\n#0 .* ccc \\(arg=789\\) at "
> -    "\\r\\n#1 .* bbb \\(arg=456\\) at "
> -    "\\r\\n#2 .* aaa \\(arg=123\\) at "
> -    "\\r\\n#3 .* main \\(.*\\) at"
> +proc cont_and_backtrace { tst } {
> +
> +    with_test_prefix $tst {
> +	gdb_breakpoint "ccc"
> +
> +	# We're testing different code paths within the unwinder's sniffer.
> +	# Set the current path to be tested here.
> +	gdb_test_no_output "python TestUnwinder.set_test(\"$tst\")" \
> +			   "set code path within python unwinder to $tst"
> +
> +	# If the unwinder is active, the usage count will increment while
> +	# running to the breakpoint.  Reset it prior to doing the backtrace.
> +	gdb_test_no_output "python TestUnwinder.reset_count()" \
> +			   "reset count"
> +
> +	gdb_continue_to_breakpoint "ccc"
> +
> +	# The python based unwinder should be called a number of times while
> +	# generating the backtrace, but its sniffer always returns None.  So
> +	# it doesn't really contribute to generating any of the frames below.
> +	#
> +	# But that's okay.  Our goal here is to make sure that GDB doesn't
> +	# get hung up in potentially infinite recursion when invoking the
> +	# Python-based unwinder.
> +
> +	gdb_test_sequence "bt"  "backtrace" {
> +	    "\\r\\n#0 .* ccc \\(arg=789\\) at "
> +	    "\\r\\n#1 .* bbb \\(arg=456\\) at "
> +	    "\\r\\n#2 .* aaa \\(arg=123\\) at "
> +	    "\\r\\n#3 .* main \\(.*\\) at"
> +	}
> +
> +	# Test that the python-based unwinder / sniffer was actually called
> +	# during generation of the backtrace.
> +	gdb_test "python print(TestUnwinder.count > 0)" "True" \
> +		 "python unwinder called"
> +    }
>  }
>  
> -# Test that the python-based unwinder / sniffer was actually called
> -# during generation of the backtrace.
> -gdb_test "python print(TestUnwinder.count > 0)" "True"
> +cont_and_backtrace "check_undefined_symbol"
> +cont_and_backtrace "check_user_reg_pc"
> +cont_and_backtrace "check_pae_pc"
> diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.py b/gdb/testsuite/gdb.python/py-recurse-unwind.py
> index 1da7aca..5eb87bb 100644
> --- a/gdb/testsuite/gdb.python/py-recurse-unwind.py
> +++ b/gdb/testsuite/gdb.python/py-recurse-unwind.py
> @@ -40,13 +40,18 @@ class TestUnwinder(Unwinder):
>      def inc_count (cls):
>          cls.count += 1
>  
> +    test = 'check_undefined_symbol'
> +
> +    @classmethod
> +    def set_test (cls, test) :
> +       cls.test = test
> +
>      def __init__(self):
>          Unwinder.__init__(self, "test unwinder")
>          self.recurse_level = 0
>  
>      def __call__(self, pending_frame):
>  
> -
>          if self.recurse_level > 0:
>              gdb.write("TestUnwinder: Recursion detected - returning early.\n")
>              return None
> @@ -54,11 +59,25 @@ class TestUnwinder(Unwinder):
>          self.recurse_level += 1
>          TestUnwinder.inc_count()
>  
> -        try:
> -            val = gdb.parse_and_eval("undefined_symbol")
> +        if TestUnwinder.test == 'check_user_reg_pc' :
> +
> +            pc = pending_frame.read_register('pc')
> +            pc_as_int = int(pc.cast(gdb.lookup_type('int')))
> +            # gdb.write("In unwinder: pc=%x\n" % pc_as_int)
> +
> +        elif TestUnwinder.test == 'check_pae_pc' :
> +
> +            pc = gdb.parse_and_eval('$pc')
> +            pc_as_int = int(pc.cast(gdb.lookup_type('int')))
> +            # gdb.write("In unwinder: pc=%x\n" % pc_as_int)
> +
> +        elif TestUnwinder.test == 'check_undefined_symbol' :
> +
> +            try:
> +                val = gdb.parse_and_eval("undefined_symbol")
>  
> -        except Exception as arg:
> -            pass
> +            except Exception as arg:
> +                pass
>  
>          self.recurse_level -= 1
  

On Wed, 16 Nov 2016 17:46:18 -0500
Sergio Durigan Junior <sergiodj@redhat.com> wrote:

> While checking some BuildBot logs, I noticed a few new FAILures
> introduced by this commit.  You can see them at:
> 
>   <https://sourceware.org/ml/gdb-testers/2016-q4/msg03062.html>
> 
> Cheers,
> 
> > commit 1a2f3d7ff1d79b1290704e48c71e905b987393a6
> > Author: Kevin Buettner <kevinb@redhat.com>
> > Date:   Mon Sep 26 15:00:37 2016 -0700

I wrote and committed the test (which introduces some new FAILures)
prior to committing the work which fixes those failures.  I didn't
think it would be a problem, since I pushed them all at the same
time.

However, I see now that I should have reordered these commits prior to
pushing them.  That's what I'll do in the future.

My apologies for introducing temporary regressions into the testing...

Kevin