Re: [PATCH v4 14/15] gdb.base/skip.exp: Use finish to exit functions

[Andrew Burgess <aburgess@redhat.com>]

Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

> gdb.base/skip.exp was making use of a fixed amount of step commands to

I think s/amount/number/ - amount is for uncountable things, while the
number of steps used can be counted.

> exit some functions.  This caused some problems when using clang to test
> GDB, as GDB would need fewer steps to reach the desired spots.  For
> instance, when testing in the section "step after disabling 3", the log
> looks like this:
>
>     Breakpoint 4, main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:32
>     32        x = baz ((bar (), foo ()));
>     (gdb) step
>     bar () at binutils-gdb/gdb/testsuite/gdb.base/skip1.c:21
>     21        return 1;
>     (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step 1
>     step
>     foo () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:42
>     42        return 0;
>     (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step 2
>     step
>     main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:34
>     34        test_skip_file_and_function ();
>     (gdb) step
>     test_skip_file_and_function () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:59
>     59        test_skip ();
>     (gdb) FAIL: gdb.base/skip.exp: step after disabling 3: step 3
>     step
>     test_skip () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:48
>     48      }
>     (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step 4
>     step
>     test_skip_file_and_function () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:60
>     60        skip1_test_skip_file_and_function ();
>     (gdb) FAIL: gdb.base/skip.exp: step after disabling 3: step 5
>
> This shows that the feature is working, but it is not easy to use steps
> to test this feature without analyzing all possible outputs, such as
> using gdb_step_until_regexp.  Instead, skip.exp now uses finish to leave

You mention gdb_step_until_regexp here, which I think is now named just
gdb_step_until.  But more importantly, you mention it, but then don't
actually use this, and its not clear why (from this commit message).

I think it's worth pointing out here that we can't use gdb_step_until
because it would potentially step over the very thing we are trying to
ensure that GDB doesn't do, i.e. call into the skipped function.

> functions, synchronizing through compilers and compiler versions.  Some
> test names were also changed to be a bit more descriptive.  The new log
> looks like this, independently of compiler used:
>
>     Breakpoint 4, main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:32
>     32        x = baz ((bar (), foo ()));
>     (gdb) step
>     bar () at binutils-gdb/gdb/testsuite/gdb.base/skip1.c:21
>     21        return 1;
>     (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step into bar
>     finish
>     Run till exit from #0  bar () at binutils-gdb/gdb/testsuite/gdb.base/skip1.c:21
>     main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:32
>     32        x = baz ((bar (), foo ()));
>     Value returned is $2 = 1
>     (gdb) PASS: gdb.base/skip.exp: step after disabling 3: return from bar
>     step
>     foo () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:42
>     42        return 0;
>     (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step into foo
>     finish
>     Run till exit from #0  foo () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:42
>     main () at binutils-gdb/gdb/testsuite/gdb.base/skip.c:32
>     32        x = baz ((bar (), foo ()));
>     Value returned is $3 = 0
>     (gdb) PASS: gdb.base/skip.exp: step after disabling 3: Return from foo
>     step
>     34        test_skip_file_and_function ();
>     (gdb) PASS: gdb.base/skip.exp: step after disabling 3: step and skip baz
> ---
>  gdb/testsuite/gdb.base/skip.exp | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/skip.exp b/gdb/testsuite/gdb.base/skip.exp
> index e6b660004d9..5b08cde93b7 100644
> --- a/gdb/testsuite/gdb.base/skip.exp
> +++ b/gdb/testsuite/gdb.base/skip.exp
> @@ -100,6 +100,12 @@ if ![runto_main] {
>  gdb_test "step" ".*" "step in the main"
>  gdb_test "bt" "\\s*\\#0\\s+main.*" "step after all ignored"
>  
> +proc step_foo_skip_bar_baz {} {

I think this proc should have a comment describing what it's doing.

> +    gdb_test "step" "foo \\(\\) at.*" "step and skip bar"
> +    gdb_test "finish" ".*" "return from bar"
> +    gdb_test "step" ".*test_skip_file_and_function.*" "step and skip baz"
> +}
> +
>  # Now remove skip.c from the skiplist.  Our first step should take us
>  # into foo(), and our second step should take us to the next line in main().
>  
> @@ -117,21 +123,19 @@ with_test_prefix "step after deleting 1" {
>  	return
>      }
>  
> -    gdb_test "step" "foo \\(\\) at.*" "step 1"
> -    gdb_test "step" ".*" "step 2" ; # Return from foo()
> -    gdb_test "step" "main \\(\\) at.*" "step 3"
> +    step_foo_skip_bar_baz
>  }
>  
>  # Test that we step into foo(), then into bar(), but not into baz().
>  proc step_bar_foo_skip_baz {} {
> -    gdb_test "step" "bar \\(\\) at.*" "step 1"
> -    gdb_test "step" ".*" "step 2"; # Return from bar()
> +    gdb_test "step" "bar \\(\\) at.*" "step into bar"
> +    gdb_test "finish" ".*" "return from bar"
>  
>      # With at least gcc 6.5.0 and 9.2.0, we jump once back to main
>      # before entering foo here.  If that happens try to step a second
>      # time.
>      set stepped_again 0
> -    gdb_test_multiple "step" "step 3" {
> +    gdb_test_multiple "step" "step into foo" {
>  	-re -wrap "foo \\(\\) at.*" {
>  	    pass $gdb_test_name
>  	}
> @@ -144,8 +148,8 @@ proc step_bar_foo_skip_baz {} {
>  	}
>      }
>  
> -    gdb_test "step" ".*" "step 4"; # Return from foo()
> -    gdb_test "step" "main \\(\\) at.*" "step 5"
> +    gdb_test "finish" ".*" "Return from foo"
> +    gdb_test "step" ".*test_skip_file_and_function.*" "step and skip baz"
>  }
>  
>  # Now disable the skiplist entry for  skip1.c.  We should now
> @@ -178,9 +182,7 @@ with_test_prefix "step after enable 3" {
>  	return
>      }
>  
> -    gdb_test "step" "foo \\(\\) at.*" "step 1"
> -    gdb_test "step" ".*" "step 2"; # Return from foo()
> -    gdb_test "step" "main \\(\\) at.*" "step 3"
> +    step_foo_skip_bar_baz
>  }
>  
>  # Admin tests (disable,enable,delete).
> @@ -249,9 +251,7 @@ with_test_prefix "step using -fi" {
>  
>      gdb_test_no_output "skip disable"
>      gdb_test_no_output "skip enable 5"
> -    gdb_test "step" "foo \\(\\) at.*" "step 1"
> -    gdb_test "step" ".*" "step 2"; # Return from foo()
> -    gdb_test "step" "main \\(\\) at.*" "step 3"
> +    step_foo_skip_bar_baz
>  }
>  
>  with_test_prefix "step using -gfi" {
> @@ -261,9 +261,7 @@ with_test_prefix "step using -gfi" {
>  
>      gdb_test_no_output "skip disable"
>      gdb_test_no_output "skip enable 6"
> -    gdb_test "step" "foo \\(\\) at.*" "step 1"
> -    gdb_test "step" ".*" "step 2"; # Return from foo()
> -    gdb_test "step" "main \\(\\) at.*" "step 3"
> +    step_foo_skip_bar_baz
>  }

In an earlier version of this patch Pedro raised the concern that if we
remove all of the "step out of a function" tests, then we may remove
some important testing.

With the patch #15 in this series (once that's merged), I think it will
be OK to change this test as you propose.

Thanks,
Andrew


>  
>  with_test_prefix "step using -fu for baz" {
> -- 
> 2.31.1