Re: [PATCH] gdb/testsuite: get_set_option_choices: expect \r\n after each item

[Simon Marchi <simon.marchi@polymtl.ca>]

On 11/15/22 02:01, Tom de Vries wrote:
> On 11/14/22 16:14, Simon Marchi via Gdb-patches wrote:
>> I get some random failures since commit 8d45c3a82a0e ("[gdb/testsuite]
>> Set completions to unlimited in get_set_option_choices"), which can be
>> reproduced with:
>>
>>      $ make check-read1 TESTS="gdb.base/parse_number.exp"
>>
> 
> Hi,
> 
> sorry, that's my fault.  I can reproduce this.
> 
>> For instance:
>>
>>      set architecture A^M
>>      Ambiguous item "A".^M
>>      (gdb) FAIL: gdb.base/parse_number.exp: arch=A: set architecture A
>>
>> The problem is the regexp in get_set_option_choices, it can match only
>> part of a completion result.  With check-read1, that is alwasy one
>> letter.  Fix this by expecting the \r\n at the end of the line, so we
>> only match entire results.
>>
> 
> Now, with --enable-targets=all I have 364 architectures:
> ...
> $ gdb -q -batch -ex "set max-completions unlimited" -ex "complete set architecture " 2>&1 | grep -v -c "set architecture"
> 0
> $ gdb -q -batch -ex "set max-completions unlimited" -ex "complete set architecture " 2>&1 | grep -c "set architecture"
> 364
> ...
> 
> And using this:
> ...
> @@ -9156,6 +9156,7 @@ proc get_set_option_choices {set_cmd} {
>         }
>      }
> 
> +    verbose -log "Found [llength $values] choices for $set_cmd"
>      return $values
>  }
> 
> ...
> we can see how many choices get_set_option_choices finds.
> 
> Without this patch I get:
> ...
> (gdb) Found 364 choices for set architecture
> ...
> but with the patch:
> ...
> (gdb) Found 182 choices for set architecture
> ...
> 
> The problem is that by consuming \r\n both at the start and the end of the line, we drop about half of the choices.

Ah, noob mistake, thanks for catching, it would have been bad.

> 
> I usually solve this by using positive lookahead:
> ...
> -           -re "\r\n$set_cmd (\[^\r\n\]+)\r\n" {
> +           -re "\r\n$set_cmd (\[^\r\n\]+)(?=\r\n)" {

I had to look up what ?= does (positive lookahead), but that looks good
to me.

> ...
> This fixes the number of choices for me, and still passes with check-read1.
> 
> Note that in the original patch I did:
> ...
> -       -re "$set_cmd (\[^\r\n\]+)\r\n" {
> +       -re "\r\n$set_cmd (\[^\r\n\]+)" {
> ...
> in order to be able to use -wrap (which requires a leading \r\n) instead of gdb_prompt. I remember typing that up in the commit message, but it seems to have gone missing.

Since this let me make a mistake, I'm thinking that we could be a bit
more paranoid in the match patterns, ensuring that we match at the
beginning of the expect buffer, and that no output is left behind.
Something like:


     with_set max-completions unlimited {
         gdb_test_multiple $cmd $test {
             -re "^[string_to_regexp $cmd]" {
                 exp_continue
             }

             -re "^\r\n$set_cmd (\[^\r\n\]+)(?=\r\n)" {
                 lappend values $expect_out(1,string)
                 exp_continue
             }

             -re "^\r\n$::gdb_prompt $" {
                 pass $gdb_test_name
             }
         }
     }

Simon