[PATCH 01/11] change gdb.base/skip.exp to use finish instead of step

[Andrew Burgess]

Thanks for looking at improving clang compatibility.

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

> skip.exp was making use of a fixed amount of step commands to exit
> some functions.  This caused some problems when testing GDB with clang,
> as it doesn't add epilogue information for functions.  Since the step
> commands weren't testing important features, they were changed to finish
> commands.
> ---
>  gdb/testsuite/gdb.base/skip.exp | 56 ++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 33 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/skip.exp b/gdb/testsuite/gdb.base/skip.exp
> index 7c71bb07a84..a961fbbdf41 100644
> --- a/gdb/testsuite/gdb.base/skip.exp
> +++ b/gdb/testsuite/gdb.base/skip.exp
> @@ -117,9 +117,8 @@ 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"
> +    gdb_test "step" "foo \\(\\) at.*" "step"
> +    gdb_test "finish" ".*" "finish" ; # Return from foo()

The comment before this "with_test_prefix" block specifically talks
about two skips, which is no longer the case.  Can you ensure that is
updated please.

Additionally, the original third step did check that we ended up back in
main, which your finish doesn't.  I'd ideally like to see the same check
in the finish so that the test isn't getting easier.

Finally, there's really no reason to give this test a separate name if
you're just going to use the command again, so

  gdb_test "finish" "main \\(\\) at .*"	# Return from foo()

should hopefully do the job (completely untested).

I think this feedback applies throughout this patch, so I've not
repeated myself below.

>  }
>  
>  # Now disable the skiplist entry for  skip1.c.  We should now
> @@ -137,13 +136,11 @@ with_test_prefix "step after disabling 3" {
>      }
>  
>      gdb_test "step" "bar \\(\\) at.*" "step 1"
> -    gdb_test "step" ".*" "step 2"; # Return from bar()
> -    # With gcc 9.2.0 we jump once back to main before entering foo here.
> -    # If that happens try to step a second time.
> -    gdb_test "step" "foo \\(\\) at.*" "step 3" \
> -	"main \\(\\) at .*\r\n$gdb_prompt " "step"
> -    gdb_test "step" ".*" "step 4"; # Return from foo()
> -    gdb_test "step" "main \\(\\) at.*" "step 5"
> +    # guarantee that we jump once back to main before entering foo here.

Start comments with a capital letter.

> +    gdb_test "finish" ".*" "finish 1"; # Return to main()
> +    gdb_test "step" "foo \\(\\) at.*" "step 2" \
> +	"main \\(\\) at .*\r\n$gdb_prompt " "step" # enter foo()

This line with the possibility of sending an extra 'step' used to have a
comment attached, which has been removed.

I do wonder if the problem was actually with the preceeding 'step',
which you've now switched for a finish command?

Handily, the old comment had a GCC version number atteched, so it might
be possible to try and recreate the old problem.

I think that if the extra 'step' is still needed then we should be
keeping the comment about GCC 9.2.0 in some form, or, ideally, we'd show
that switching to finish removes the need for that extra 'step', and
remove it.

> +    gdb_test "finish" ".*" "finish 2"; # Return from foo()

Again, would be nicer to have a better pattern than ".*" here.

I think for the rest of the patch it's just the same feedback repeated.

Thank,
Andrew


>  }
>  
>  # Enable skiplist entry 3 and make sure we step over it like before.
> @@ -159,9 +156,8 @@ 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"
> +    gdb_test "step" "foo \\(\\) at.*" "step"
> +    gdb_test "finish" ".*" "finish"; # Return from foo()
>  }
>  
>  # Admin tests (disable,enable,delete).
> @@ -230,9 +226,8 @@ 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"
> +    gdb_test "step" "foo \\(\\) at.*" "step"
> +    gdb_test "finish" ".*" "finish"; # Return from foo()
>  }
>  
>  with_test_prefix "step using -gfi" {
> @@ -242,9 +237,8 @@ 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"
> +    gdb_test "step" "foo \\(\\) at.*" "step"
> +    gdb_test "finish" ".*" "finish"; # Return from foo()
>  }
>  
>  with_test_prefix "step using -fu for baz" {
> @@ -255,13 +249,11 @@ with_test_prefix "step using -fu for baz" {
>      gdb_test_no_output "skip disable"
>      gdb_test_no_output "skip enable 7"
>      gdb_test "step" "bar \\(\\) at.*" "step 1"
> -    gdb_test "step" ".*" "step 2"; # Return from bar()
> -    # With gcc 9.2.0 we jump once back to main before entering foo here.
> -    # If that happens try to step a second time.
> -    gdb_test "step" "foo \\(\\) at.*" "step 3" \
> -	"main \\(\\) at .*\r\n$gdb_prompt " "step"
> -    gdb_test "step" ".*" "step 4"; # Return from foo()
> -    gdb_test "step" "main \\(\\) at.*" "step 5"
> +    # guarantee that we jump once back to main before entering foo here.
> +    gdb_test "finish" ".*" "finish 1"; # Return to main()
> +    gdb_test "step" "foo \\(\\) at.*" "step 2" \
> +	"main \\(\\) at .*\r\n$gdb_prompt " "step" # enter foo()
> +    gdb_test "finish" ".*" "finish 2"; # Return from foo()
>  }
>  
>  with_test_prefix "step using -rfu for baz" {
> @@ -272,13 +264,11 @@ with_test_prefix "step using -rfu for baz" {
>      gdb_test_no_output "skip disable"
>      gdb_test_no_output "skip enable 8"
>      gdb_test "step" "bar \\(\\) at.*" "step 1"
> -    gdb_test "step" ".*" "step 2"; # Return from bar()
> -    # With gcc 9.2.0 we jump once back to main before entering foo here.
> -    # If that happens try to step a second time.
> -    gdb_test "step" "foo \\(\\) at.*" "step 3" \
> -	"main \\(\\) at .*\r\n$gdb_prompt " "step"
> -    gdb_test "step" ".*" "step 4"; # Return from foo()
> -    gdb_test "step" "main \\(\\) at.*" "step 5"
> +    # guarantee that we jump once back to main before entering foo here.
> +    gdb_test "finish" ".*" "finish 1"; # Return to main()
> +    gdb_test "step" "foo \\(\\) at.*" "step 2" \
> +	"main \\(\\) at .*\r\n$gdb_prompt " "step" # enter foo()
> +    gdb_test "finish" ".*" "finish 2"; # Return from foo()
>  }
>  
>  # Test -fi + -fu.
> -- 
> 2.31.1