Re: [PATCH v4 15/15] gdb/testsuite: Add test to step through function epilogue

[Andrew Burgess <aburgess@redhat.com>]

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

> The testsuite implicitly tests GDB's ability to step through epilogues
> in multiple tests, without doing it explicitly anywhere.  This is
> unfortunate, as clang does not emit epilogue information, so using clang
> on our testsuite makes many tests fail.  This patch adds a central,
> explicit test for walking through the epilogue so we can safely remove
> this from other tests and have them working with clang.

Sounds sensible.

>
> The test created attempts to step through a simple empilogue, an

s/empilogue/epilogue/

> epilogue that ends on another epilogue, and epilogues leading to other
> function calls.
> ---
>  .../gdb.base/step-through-epilogue.c          | 15 ++++
>  .../gdb.base/step-through-epilogue.exp        | 86 +++++++++++++++++++
>  2 files changed, 101 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/step-through-epilogue.c
>  create mode 100644 gdb/testsuite/gdb.base/step-through-epilogue.exp
>
> diff --git a/gdb/testsuite/gdb.base/step-through-epilogue.c b/gdb/testsuite/gdb.base/step-through-epilogue.c
> new file mode 100644
> index 00000000000..3e0ea5a2522
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/step-through-epilogue.c

Source files should include a copyright notice at the top, checkout
gdb.base/break.c as an example.

> @@ -0,0 +1,15 @@
> +int multiply (int a, int b){
> +    return a*b;
> +}

Where possible, test source files should follow GNU convention, so:

  int
  multiply (int a, int b)
  {
    ...
  }

if you need to break convention for some reason, maybe having everything
on one line is required by the test, then this should be explained in a
comment.

Also, I notice everything is indented by 4 spaces instead of 2.

> +
> +int square (int x){
> +    return multiply (x, x);
> +}
> +
> +int main(){

Should be 'main (void)' I think.

> +    int x;
> +    x = multiply (1,2);
> +    x = square(2);

Space before '(2)'.

> +    x = multiply (square (1), square (2));
> +    return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/step-through-epilogue.exp b/gdb/testsuite/gdb.base/step-through-epilogue.exp
> new file mode 100644
> index 00000000000..762623cf72c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/step-through-epilogue.exp
> @@ -0,0 +1,86 @@
> +#   Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This test is used to confirm that GDB is able to step, stopping at an
> +# epilogue line, then step out of the function.
> +
> +standard_testfile
> +
> +if { [get_compiler_info "clang-*"] } {

This should be using 'test_compiler_info' here, the get call doesn't do
what you would expect given its name.

> +    untested "This test can't be used with clang"
> +    return
> +}
> +
> +if { [prepare_for_testing "failed to prepare" "skip" \
> +			  {step-through-epilogue.c}] } {

That's unexpected!  I suspect the use of "skip" here for the binary name
is the result of a cut & paste.  You probably want:

  if { [prepare_for_testing "failed to prepare" $testfile \
  			  {step-through-epilogue.c}] } {
      untested "failed to prepare"
      return
  }

which will result in the test binary being named after the test script.

> +    untested "failed to prepare"
> +    return
> +}
> +
> +if { ![runto_main] } {
> +    untested "couldn't run to main"
> +    return
> +}
> +
> +# Some gcc versions, at least 6.5.0 and 9.2.0, can mess this test by
> +# requiring extra steps in a few locations.  We test here and use
> +# this knowledge later to decide if the extra steps need to be taken.
> +set slow_gcc 0
> +if { [test_compiler_info "gcc-6-5-0"] || [test_compiler_info "gcc-9-2-0"]} {
> +    set slow_gcc 1
> +}
> +
> +proc step_till_epilogue_multiply {} {
> +    gdb_test "step" ".*return a.b;.*" "step into multiply"
> +    gdb_test "step" ".*3\\s+\\\}.*" "stop at the epilogue of multiply"

When you update the source file, you'll find this pattern breaks as the
line number changes.

One trick to avoid placing these line numbers is to add a comment to the
source file, like:

  } /* Epilogue line of multiply.  */

then the pattern can be:

  gdb_test "step" \
    ".*$::decimal\\s+\\\}\[^\r\n\]+Epilogue line of multiply.*" \
    "stop at the epilogue of multiply"

reducing the number of hard-coded line numbers is usually a good thing.

> +}
> +
> +proc step_till_epilogue_square {} {
> +    gdb_test "step" ".*return multiply.*" "step into square"
> +    step_till_epilogue_multiply
> +    gdb_test "step" ".*7\\s+\\\}.*" "stop at epilogue of square"
> +}
> +
> +with_test_prefix "multiply" {
> +    step_till_epilogue_multiply
> +    gdb_test "step" ".*x = square\\(2\\);" "leave function"
> +}
> +
> +with_test_prefix "square" {
> +    step_till_epilogue_square
> +    gdb_test "step" ".*x = multiply \\(square \\(1\\), square \\(2\\)\\);.*"\
> +	"leave function"
> +}
> +
> +with_test_prefix "square, first argument" {
> +    step_till_epilogue_square
> +    if {$slow_gcc} {
> +	gdb_test "step"\
> +	    ".*x = multiply \\(square \\(1\\), square \\(2\\)\\);.*"\
> +	    "step back to main"
> +    }

I'm kind-of bothered by the $slow_gcc test.  It feels pretty random that
we only check for those two specific gcc versions.  Plus returning back
to main for that one line doesn't feel that wrong to me, so it seems
like this is less working around a bug in specific compiler versions,
and more just variance in how the debug is emitted.

I wonder if we could make use of gdb_test_multiple to avoid the need for
$slow_gcc?  Something like this completely untested code:

  gcc_test_multiple "step" "step into square" {
    -re -wrap ".*x = multiply \\(square \\(1\\), square \\(2\\)\\);.*" {
      gdb_send "step\n"
      exp_continue
    }
  
    -re -wrap ".*return multiply.*" {
      pass $gdb_test_name
    }
  }

Of course, that would mean you couldn't then use
step_till_epilogue_square in its current form, as you do next, but maybe
with some refactoring, the above might be made to work?

> +}
> +with_test_prefix "square, second argument" {
> +    step_till_epilogue_square
> +    if {$slow_gcc} {
> +	gdb_test "step"\
> +	    ".*x = multiply \\(square \\(1\\), square \\(2\\)\\);.*"\
> +	    "step back to main"
> +    }
> +}
> +with_test_prefix "multiply with function args" {
> +    step_till_epilogue_multiply
> +    gdb_test "step" ".*return 0;.*" "leave last function"
> +}
> -- 
> 2.31.1