Re: [PATCH v4 03/15] Change gdb.base/skip-solib.exp deal with lack of epilogue information

[Andrew Burgess <aburgess@redhat.com>]

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

> When running gdb.base/skip-solib.exp, the backtrace tests could fail with
> compilers that associated epilogue instructions with the last statement
> line of the function, instead of associating it with the closing brace,
> despite the feature being fully functional.  As an example, when testing
> skipping the function square, the testsuite would show
>
> Breakpoint 1, main () at (...)/binutils-gdb/gdb/testsuite/gdb.base/skip-solib-main.c:5
> 5         return square(0);
> (gdb) step
> 0x00007ffff7cef560 in __libc_start_call_main () from /lib64/libc.so.6
> (gdb) PASS: gdb.base/skip-solib.exp: ignoring solib file: step
> bt
>  #0  0x00007ffff7cef560 in __libc_start_call_main () from /lib64/libc.so.6
>  #1  0x00007ffff7cef60c in __libc_start_main_impl () from /lib64/libc.so.6
>  #2  0x0000000000401065 in _start ()
> (gdb) FAIL: gdb.base/skip-solib.exp: ignoring solib file: bt
>
> Which means that the feature is working, the testsuite is just
> mis-identifying it.  To avoid this problem, the skipped function calls
> have been sent to a line before `return`, so epilogues won't factor in.
>
> This commit has also changed a few hardcoded steps to leave functions to
> the newly introduced gdb_step_until to leave those functions.

I think I would like to see the skip-inline.exp change moved into a
separate commit given it's a completely different type of fix.

> ---
>  gdb/testsuite/gdb.base/skip-inline.exp   | 23 ++++++++++++++++-------
>  gdb/testsuite/gdb.base/skip-solib-lib.c  |  3 ++-
>  gdb/testsuite/gdb.base/skip-solib-main.c |  3 ++-
>  gdb/testsuite/gdb.base/skip-solib.exp    | 12 ++++++++++--
>  4 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/skip-inline.exp b/gdb/testsuite/gdb.base/skip-inline.exp
> index f6e9926b66c..3fbaa5469dd 100644
> --- a/gdb/testsuite/gdb.base/skip-inline.exp
> +++ b/gdb/testsuite/gdb.base/skip-inline.exp
> @@ -15,6 +15,11 @@
>  
>  standard_testfile
>  
> +set epilogue 1
> +if {![have_epilogue_line_info]} {
> +    set epilogue 0
> +}

I think 'set epilogue [have_epilogue_line_info]' would be better.

> +
>  if { [prepare_for_testing "failed to prepare" "skip-inline" \
>  			  {skip-inline.c skip1.c } \
>  			  {debug nowarnings}] } {
> @@ -35,16 +40,20 @@ gdb_test "skip function foo" "Function foo will be skipped when stepping\."
>  gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
>  gdb_test "step" ".*" "step into baz, since foo will be skipped"
>  gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped"
> -gdb_test "step" ".*" "step in the baz"
> -gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
> -gdb_test "step" ".*" "step back to main"
> +gdb_step_until ".*x = 0; x = baz \\(foo \\(\\)\\).*"
>  gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
>  gdb_test "step" ".*" "step again into baz, since foo will be skipped"
>  gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
> -gdb_test "step" ".*" "step in the baz, again"
> -gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again"
> -gdb_test "step" ".*" "step back to main, again"
> -gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
> +gdb_step_until "main \\(\\) at .*" "step back to main, again"
> +gdb_test "bt" "\\s*\\#0.*main.*" "again back to main"
> +
> +# Because clang doesn't add epilogue information, having a set number of
> +# steps puts clang more and more out of sync with gcc.  It is unlikely that
> +# the effort of keeping both outputs will be useful.
> +if {$epilogue == 0} {

Just 'if { !$epilogue } {' would be better.

> +    untested "Multiple steps tests are not supported with this compiler"
> +    return
> +}

I notice that there's actually another test at the end of this file that
doesn't rely on multiple steps, which we now end up skipping due to this
early return.

I wonder if this test file would be better structured something like:

  proc_with_prefix single_step { } {
    # The first block of tests that just does 'step'.
  }
  
  proc_with_prefix double_step { } {
    # The second block of tests that do 'step 2'.
  }
  
  proc_with_prefix triple_step { } {
    # The third block of tests that do 'step 3'.
  }
  
  proc_with_prefix skip_current_frame { } {
    # The final bit of test that sets up a skip of foo.
  }
  
  single_step
  if { $epilogue } {
    double_step
    triple_step
  }
  skip_current_frame

>  
>  if ![runto_main] {
>      return
> diff --git a/gdb/testsuite/gdb.base/skip-solib-lib.c b/gdb/testsuite/gdb.base/skip-solib-lib.c
> index b2c4d86d703..341f1440a3b 100644
> --- a/gdb/testsuite/gdb.base/skip-solib-lib.c
> +++ b/gdb/testsuite/gdb.base/skip-solib-lib.c
> @@ -7,5 +7,6 @@ int multiply(int a, int b)
>  
>  int square(int num)
>  {
> -  return multiply(num, num);
> +  int res = multiply(num, num);
> +  return res;
>  }
> diff --git a/gdb/testsuite/gdb.base/skip-solib-main.c b/gdb/testsuite/gdb.base/skip-solib-main.c
> index 746bb5f36bb..a3b6d417935 100644
> --- a/gdb/testsuite/gdb.base/skip-solib-main.c
> +++ b/gdb/testsuite/gdb.base/skip-solib-main.c
> @@ -2,5 +2,6 @@ int square(int num);
>  
>  int main()
>  {
> -  return square(0);
> +  int s = square(0);
> +  return s;
>  }
> diff --git a/gdb/testsuite/gdb.base/skip-solib.exp b/gdb/testsuite/gdb.base/skip-solib.exp
> index 0f2ce7e1ad8..8e61725ad1b 100644
> --- a/gdb/testsuite/gdb.base/skip-solib.exp
> +++ b/gdb/testsuite/gdb.base/skip-solib.exp
> @@ -82,7 +82,7 @@ with_test_prefix "ignoring solib file" {
>      # We shouldn't step into square(), since we skipped skip-solib-lib.c.
>      #
>      gdb_test "step" ""
> -    gdb_test "bt" "#0\\s+main.*"
> +    gdb_test "bt 1" "#0\\s+main.*"
>  }
>  
>  #
> @@ -114,5 +114,13 @@ with_test_prefix "ignoring solib function" {
>      # the last line of square.
>      #
>      gdb_test "step" ""
> -    gdb_test "bt" "#0\\s+square.*"
> +    gdb_test "bt 1" "#0\\s+square.*" "skipped multiply"
> +#    gdb_test_multiple "bt 1" "skipped multiply" {
> +#	-re "#0\\s+square.*" {
> +#	    pass "skipped multiply"
> +#	}
> +#	-re "#0.*main.*" {
> +#	    pass "skipped multiply"
> +#	}
> +#    }

This commented out code should be removed.

In fact, I wonder if any of the changes in skip-solib.exp are actually
needed.  Sure, 'bt 1' is maybe a little more specific than 'bt', but
given the pattern we check for doesn't change, I don't think this change
should make any difference.

If this is just a preference/cleanup then this should probably move into
a sperate patch to avoid any confusion.  Or just drop this part?

Thanks,
Andrew
>  }
> -- 
> 2.31.1