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

[Bruno Larsen <blarsen@redhat.com>]

Hi Andrew,

thanks for the review!

On 5/27/22 13:19, Andrew Burgess wrote:
> 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.
> 

Good point. I've used your version of the comment.

>> +
>> +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.

That's a good reason. I didn't have any for my choice other than it's what I was comfortable with. Using your version again

> 
>> +    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.

I do like this version more too, more clear that it is a global variable for new contributors.

At this point, I wonder if I should add a co-authored tag to the patch.

> 
> 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
> 
Cheers!
Bruno Larsen