Re: [PATCH v3 01/14] gdb/testsuite: introduce gdb_step_until_regexp

[Andrew Burgess <aburgess@redhat.com>]

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

> Currently, GDB's testsuite uses a set amount of step commands to exit
> functions. This is a problem if a compiler emits different epilogue
> information from gcc, or emits no epilogue information at all. It was
> most noticeable if Clang was used to test GDB.
>
> To fix this unreliability, this commit introduces a new proc that will
> single step the inferior until it is stopped at a line that matches the
> given regexp, or until it steps too many times - defined as an optional
> argument. If the line is found, it shows up as a single PASS in the
> test, and if the line is not found, a single FAIL is emitted.
>
> This patch only introduces this proc, but does not add it to any
> existing tests, these will be introduced in the following commit.
> ---
>
> No change in v3
>
> New patch in v2
>
> ---
>  gdb/testsuite/lib/gdb.exp | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index b04fbb89e4e..c0ca1d04cc2 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -8648,5 +8648,35 @@ proc get_set_option_choices {set_cmd} {
>      return $values
>  }
>  
> +# This proc is used mainly to exit function in a compiler agnostic way
> +# It makes gdb single step and evaluate the output at every step, to see
> +# if the regexp is present.
> +#
> +# The proc takes 2 optional arguments, the first being the name of the
> +# test and the second the maximum amount of iterations until we expect to
> +# see the function. The default is 10 steps, since this is meant as the
> +# last step by default, and we don't expect any compiler generated epilogue
> +# longer than 10 steps.

I feel like you are being overly prescriptive in how this function
should be used.

I would rewrite this to just describe what the function does, and let
folk use it as they see fit.  Sure, initially it will only be used as
you imagine - that's why you're adding it.  But once it's there, who
knows what uses it might be put too.

I'd go with something like:

  # Single step until the pattern REGEXP is found.  Step at most
  # MAX_STEPS times, but stop stepping once REGEXP is found.
  #
  # If REGEXP is found then a single pass is emitted, otherwise, after
  # MAX_STEPS steps, a single fail is emitted.
  #
  # TEST_NAME is the name used in the pass/fail calls.

> +
> +proc gdb_step_until_regexp { regexp {test_name "single stepping until regexp"} {max_steps 10} } {

You should keep this line under 80 characters.  You can wrap the
arguments I believe, like:

  proc gdb_step_until_regexp { regexp
                               {test_name "single stepping until regexp"}
                               {max_steps 10} } {

However, I'd be tempted to take a different approach, like this:

  proc gdb_step_until_regexp { regexp {test_name ""} {max_steps 10} } {

    if { $test_name == "" } {
      set test_name "single stepping until regexp"
    }

The benefit I see in this approach is that if a user wants to adjust
max_steps, but doesn't care about the test name, they can do this:

  gdb_step_until_regexp "SOME_PATTERN" "" 50

And still get the default test name.

> +    global gdb_prompt

I think this is OK, there's certainly lots of precedent for this
approach, but I think more often these days, we just refer to the global
directly as:

  $::gdb_prompt

As this removes the need for the 'global gdb_prompt' line.

But I don't think this is a hard requirement if you prefer what you
have.

Thanks,
Andrew

> +
> +    set count 0
> +    gdb_test_multiple "step" "$test_name" {
> +	-re "$regexp\r\n$gdb_prompt $" {
> +	    pass $test_name
> +	}
> +	-re ".*$gdb_prompt $" {
> +	    if {$count < $max_steps} {
> +		incr count
> +		send_gdb "step\n"
> +		exp_continue
> +	    } else {
> +		fail $test_name
> +	    }
> +	}
> +    }
> +}
> +
>  # Always load compatibility stuff.
>  load_lib future.exp
> -- 
> 2.31.1