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

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

[Simon Marchi]

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"

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.

Change-Id: Ib1733737feab7dde0f7095866e089081a891054e
---
 gdb/testsuite/lib/gdb.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index bdb8da9daf98..abc67fb2b9e2 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9138,7 +9138,7 @@ proc get_set_option_choices {set_cmd} {
 
     with_set max-completions unlimited {
 	gdb_test_multiple $cmd $test {
-	    -re "\r\n$set_cmd (\[^\r\n\]+)" {
+	    -re "\r\n$set_cmd (\[^\r\n\]+)\r\n" {
 		lappend values $expect_out(1,string)
 		exp_continue
 	    }

base-commit: a5b6e43669b78729d2ef78d668e19bac2b11197d
-- 
2.38.1

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

[Tom de Vries]

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.

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)" {
...
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.

Thanks,
- Tom

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

[Simon Marchi]

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

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

[Tom de Vries]

On 11/15/22 15:59, Simon Marchi wrote:
> 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
>               }
>           }
>       }

Yep, more strict matching is ok.

LGTM.

Thanks,
- Tom

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

[Simon Marchi]

> Yep, more strict matching is ok.
> 
> LGTM.
Thanks.  I pushed it, but actually I went with this, since we're not
using -wrap anyway.  I think it's a bit less cryptic than the lookahead
version:

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

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

	    -re "^$::gdb_prompt $" {
		pass $gdb_test_name
	    }
	}
    }

Simon