[PATCH v2] gdb/testsuite: fix completion tests when using READ1

[PATCH v2] gdb/testsuite: fix completion tests when using READ1

[Bruno Larsen]

The commit a3da2e7e550c4fe79128b5e532dbb90df4d4f418 has introduced
regressions when testing using the READ1 mechanism. The reason for that
is the new failure path in proc test_gdb_complete_tab_unique, which
looks for GDB suggesting more than what the test inputted, but not the
correct answer, followed by a white space. Consider the following case:

int foo(int bar, int baz);

Sending the command "break foo<tab>" to GDB will return

break foo(int, int)

which easily fits the buffer in normal testing, so everything works, but
when reading one character at a time, the test will find the partial
"break foo(int, " and assume that there was a mistake, so we get a
spurious FAIL.

To fix this issue, the new code looks at the partial response, and the
test only fails if the received characters are not part of the expected
pattern.
---
 gdb/testsuite/lib/completion-support.exp | 33 ++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
index ea73c3bb367..bef4d7f1ed7 100644
--- a/gdb/testsuite/lib/completion-support.exp
+++ b/gdb/testsuite/lib/completion-support.exp
@@ -71,6 +71,16 @@ proc make_cmd_completion_list_re { cmd_prefix completion_list start_quote_char e
     return $completion_list_re
 }
 
+# Make a list of the characters in $PATTERN that come after $PREFIX.
+# this assumes that PATTERN does start with PREFIX and is at least
+# one character longer.
+
+proc make_optional_partial_complete { prefix pattern } {
+    set pattern [string range $pattern [string length $prefix]+1 end]
+
+    return [split $pattern ""]
+}
+
 # Clear the input line.
 
 proc clear_input_line { test } {
@@ -112,13 +122,32 @@ proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re }
     set test "tab complete \"$input_line\""
     send_gdb "$input_line\t"
     set partial_complete [string_to_regexp $input_line]
+    set partial_expected [make_optional_partial_complete $input_line $complete_line_re]
+    set partial_size [array size $partial_expected]
     set res 1
     gdb_test_multiple "" "$test" {
 	-re "^$complete_line_re$append_char_re$" {
 	    pass "$test"
 	}
-	-re "$partial_complete\[^ \]+ $" {
-	    fail "$test"
+	-re "${partial_complete}(\[^ \]+) $" {
+	    set captured [split $expect_out(1,string) ""]
+	    set captured_size [array size $captured]
+	    if { $captured_size >= $partial_size } {
+		set unexpected 0
+		for { set i 0 } { $i < $captured_size } { incr i } {
+		    if { $captured($i) != $partial_expected($i) } {
+			set unexpected 1
+			break
+		    }
+		}
+		if { $unexpected } {
+		    fail "$test"
+		    set res -1
+		}
+	    } else {
+		fail "$test"
+		set res -1
+	    }
 	}
 	timeout {
 	    fail "$test (timeout)"
-- 
2.41.0

Re: [PATCH v2] gdb/testsuite: fix completion tests when using READ1

[Guinevere Larsen]

Ping!

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

On 04/08/2023 18:09, Bruno Larsen wrote:
> The commit a3da2e7e550c4fe79128b5e532dbb90df4d4f418 has introduced
> regressions when testing using the READ1 mechanism. The reason for that
> is the new failure path in proc test_gdb_complete_tab_unique, which
> looks for GDB suggesting more than what the test inputted, but not the
> correct answer, followed by a white space. Consider the following case:
>
> int foo(int bar, int baz);
>
> Sending the command "break foo<tab>" to GDB will return
>
> break foo(int, int)
>
> which easily fits the buffer in normal testing, so everything works, but
> when reading one character at a time, the test will find the partial
> "break foo(int, " and assume that there was a mistake, so we get a
> spurious FAIL.
>
> To fix this issue, the new code looks at the partial response, and the
> test only fails if the received characters are not part of the expected
> pattern.
> ---
>   gdb/testsuite/lib/completion-support.exp | 33 ++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index ea73c3bb367..bef4d7f1ed7 100644
> --- a/gdb/testsuite/lib/completion-support.exp
> +++ b/gdb/testsuite/lib/completion-support.exp
> @@ -71,6 +71,16 @@ proc make_cmd_completion_list_re { cmd_prefix completion_list start_quote_char e
>       return $completion_list_re
>   }
>   
> +# Make a list of the characters in $PATTERN that come after $PREFIX.
> +# this assumes that PATTERN does start with PREFIX and is at least
> +# one character longer.
> +
> +proc make_optional_partial_complete { prefix pattern } {
> +    set pattern [string range $pattern [string length $prefix]+1 end]
> +
> +    return [split $pattern ""]
> +}
> +
>   # Clear the input line.
>   
>   proc clear_input_line { test } {
> @@ -112,13 +122,32 @@ proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re }
>       set test "tab complete \"$input_line\""
>       send_gdb "$input_line\t"
>       set partial_complete [string_to_regexp $input_line]
> +    set partial_expected [make_optional_partial_complete $input_line $complete_line_re]
> +    set partial_size [array size $partial_expected]
>       set res 1
>       gdb_test_multiple "" "$test" {
>   	-re "^$complete_line_re$append_char_re$" {
>   	    pass "$test"
>   	}
> -	-re "$partial_complete\[^ \]+ $" {
> -	    fail "$test"
> +	-re "${partial_complete}(\[^ \]+) $" {
> +	    set captured [split $expect_out(1,string) ""]
> +	    set captured_size [array size $captured]
> +	    if { $captured_size >= $partial_size } {
> +		set unexpected 0
> +		for { set i 0 } { $i < $captured_size } { incr i } {
> +		    if { $captured($i) != $partial_expected($i) } {
> +			set unexpected 1
> +			break
> +		    }
> +		}
> +		if { $unexpected } {
> +		    fail "$test"
> +		    set res -1
> +		}
> +	    } else {
> +		fail "$test"
> +		set res -1
> +	    }
>   	}
>   	timeout {
>   	    fail "$test (timeout)"

Re: [PATCH v2] gdb/testsuite: fix completion tests when using READ1

[Andrew Burgess]

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

> The commit a3da2e7e550c4fe79128b5e532dbb90df4d4f418 has introduced
> regressions when testing using the READ1 mechanism. The reason for that

It would be nice to have a list of at lest _some_ of the test scripts
that are fixed by this commit -- it makes it so much easier for a
reviewer (or someone looking at this test in the 'git log') to know what
to run to test the change.

> is the new failure path in proc test_gdb_complete_tab_unique, which
> looks for GDB suggesting more than what the test inputted, but not the
> correct answer, followed by a white space. Consider the following case:
>
> int foo(int bar, int baz);
>
> Sending the command "break foo<tab>" to GDB will return
>
> break foo(int, int)
>
> which easily fits the buffer in normal testing, so everything works, but
> when reading one character at a time, the test will find the partial
> "break foo(int, " and assume that there was a mistake, so we get a
> spurious FAIL.
>
> To fix this issue, the new code looks at the partial response, and the
> test only fails if the received characters are not part of the expected
> pattern.

Weirdly, when I run the script gdb.cp/cpcompletion.exp _without_ READ1
set, I see 20 passes including this one:

  PASS: gdb.cp/cpcompletion.exp: tab complete "break baz(int"

But when I set READ1 I only see 19 passes, and the pass line above
appears to be missing.  I've not looked into this at all yet, but I
wanted to share my thoughts before I stop for the night.

Thanks,
Andrew


> ---
>  gdb/testsuite/lib/completion-support.exp | 33 ++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index ea73c3bb367..bef4d7f1ed7 100644
> --- a/gdb/testsuite/lib/completion-support.exp
> +++ b/gdb/testsuite/lib/completion-support.exp
> @@ -71,6 +71,16 @@ proc make_cmd_completion_list_re { cmd_prefix completion_list start_quote_char e
>      return $completion_list_re
>  }
>  
> +# Make a list of the characters in $PATTERN that come after $PREFIX.
> +# this assumes that PATTERN does start with PREFIX and is at least
> +# one character longer.
> +
> +proc make_optional_partial_complete { prefix pattern } {
> +    set pattern [string range $pattern [string length $prefix]+1 end]
> +
> +    return [split $pattern ""]
> +}
> +
>  # Clear the input line.
>  
>  proc clear_input_line { test } {
> @@ -112,13 +122,32 @@ proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re }
>      set test "tab complete \"$input_line\""
>      send_gdb "$input_line\t"
>      set partial_complete [string_to_regexp $input_line]
> +    set partial_expected [make_optional_partial_complete $input_line $complete_line_re]
> +    set partial_size [array size $partial_expected]
>      set res 1
>      gdb_test_multiple "" "$test" {
>  	-re "^$complete_line_re$append_char_re$" {
>  	    pass "$test"
>  	}
> -	-re "$partial_complete\[^ \]+ $" {
> -	    fail "$test"
> +	-re "${partial_complete}(\[^ \]+) $" {
> +	    set captured [split $expect_out(1,string) ""]
> +	    set captured_size [array size $captured]
> +	    if { $captured_size >= $partial_size } {
> +		set unexpected 0
> +		for { set i 0 } { $i < $captured_size } { incr i } {
> +		    if { $captured($i) != $partial_expected($i) } {
> +			set unexpected 1
> +			break
> +		    }
> +		}
> +		if { $unexpected } {
> +		    fail "$test"
> +		    set res -1
> +		}
> +	    } else {
> +		fail "$test"
> +		set res -1
> +	    }
>  	}
>  	timeout {
>  	    fail "$test (timeout)"
> -- 
> 2.41.0

Re: [PATCH v2] gdb/testsuite: fix completion tests when using READ1

[Andrew Burgess]

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

> The commit a3da2e7e550c4fe79128b5e532dbb90df4d4f418 has introduced
> regressions when testing using the READ1 mechanism. The reason for that
> is the new failure path in proc test_gdb_complete_tab_unique, which
> looks for GDB suggesting more than what the test inputted, but not the
> correct answer, followed by a white space. Consider the following case:
>
> int foo(int bar, int baz);
>
> Sending the command "break foo<tab>" to GDB will return
>
> break foo(int, int)
>
> which easily fits the buffer in normal testing, so everything works, but
> when reading one character at a time, the test will find the partial
> "break foo(int, " and assume that there was a mistake, so we get a
> spurious FAIL.
>
> To fix this issue, the new code looks at the partial response, and the
> test only fails if the received characters are not part of the expected
> pattern.
> ---
>  gdb/testsuite/lib/completion-support.exp | 33 ++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-support.exp
> index ea73c3bb367..bef4d7f1ed7 100644
> --- a/gdb/testsuite/lib/completion-support.exp
> +++ b/gdb/testsuite/lib/completion-support.exp
> @@ -71,6 +71,16 @@ proc make_cmd_completion_list_re { cmd_prefix completion_list start_quote_char e
>      return $completion_list_re
>  }
>  
> +# Make a list of the characters in $PATTERN that come after $PREFIX.
> +# this assumes that PATTERN does start with PREFIX and is at least
> +# one character longer.
> +
> +proc make_optional_partial_complete { prefix pattern } {
> +    set pattern [string range $pattern [string length $prefix]+1 end]
> +
> +    return [split $pattern ""]
> +}
> +
>  # Clear the input line.
>  
>  proc clear_input_line { test } {
> @@ -112,13 +122,32 @@ proc test_gdb_complete_tab_unique { input_line complete_line_re append_char_re }
>      set test "tab complete \"$input_line\""
>      send_gdb "$input_line\t"
>      set partial_complete [string_to_regexp $input_line]
> +    set partial_expected [make_optional_partial_complete $input_line $complete_line_re]
> +    set partial_size [array size $partial_expected]
>      set res 1
>      gdb_test_multiple "" "$test" {
>  	-re "^$complete_line_re$append_char_re$" {
>  	    pass "$test"
>  	}
> -	-re "$partial_complete\[^ \]+ $" {
> -	    fail "$test"
> +	-re "${partial_complete}(\[^ \]+) $" {
> +	    set captured [split $expect_out(1,string) ""]
> +	    set captured_size [array size $captured]
> +	    if { $captured_size >= $partial_size } {
> +		set unexpected 0
> +		for { set i 0 } { $i < $captured_size } { incr i } {
> +		    if { $captured($i) != $partial_expected($i) } {
> +			set unexpected 1
> +			break
> +		    }
> +		}
> +		if { $unexpected } {
> +		    fail "$test"
> +		    set res -1
> +		}
> +	    } else {
> +		fail "$test"
> +		set res -1
> +	    }

The problem here is that if we don't happen to hit any of the `fail`
calls, then we're going to drop out of the gdb_test_multiple without
calling `pass`.

We can't just immediately call `pass` as CAPTURED might not have
captured the full contents of the line, in the READ1 case, which this is
about, this is definitely the case, CAPTURED will contain only the first
additional character.

We couple potentially call exp_continue, but then you need to ensure
that this `-re` block doesn't actually consume the input -- usually,
once a pattern matches, the matching input is discarded, but you can add
the -notransfer flag to prevent this.

However, I think the strategy you're taking here isn't going to work.
You can't split a regexp and then compare it character by character --
not unless you know that it isn't "really" a regexp, but the exact
string that we plan to match against.  If you want to move in that
direction then you'll need to update the comments (and variable names)
throughout in order to make it clear that COMPLETE_LINE_RE is not a
regexp, but is an exact string -- only then can you do character by
character matching.

Before thinking about how to fix this, I'd like to step back a moment
and understand the original commit (a3da2e7e550c4) a little more.
Adding patterns to avoid timeouts is not wrong, but, I worry that we
might have been trying a little too hard in the original commit.  A
timeout is a valid failure mode, not something we need to avoid at all
costs -- and the cost (complexity) of avoiding it here seems to be
growing.

Plus, the completion tests are pretty stable -- it's not like sometimes
a working GDB will randomly fail a completion test with a timeout and we
want to make this quicker.  So, I'm wondering: is it really worth the
cost (complexity) of trying to solve this case?  Would we be better off
backing out this one corner of the a3da2e7e550c4 patch?  The 'complete'
command changes are, of course, fine, and should stay.

If we really wanted to keep the current code, then the best solution
I've got would be implementing some kind of micro-timeout within
test_gdb_complete_tab_unique, where you use your PARTIAL_COMPLETE
regexp along with -notransfer to spot that new content is being emitted
from GDB.  You'd run the whole gdb_test_multiple under a really short
timeout, but, within the timeout block, instead of failing, you would
check to see if any new output had arrived from GDB or not, plus you'd
check the overall time taken, and possibly apply the normal, larger,
timeout limit too.

The question then becomes, how do you figure out how long to allow GDB
to emit some output?  Maybe you could do something clever where you time
how long it takes for any output to arrive, and you use some multiple of
this time as a limit....

.... I don't think that was very clear at all.  But, honestly, it all
sounds far too complicated just to avoid some timeouts.

Maybe a better solution would be to have the completion scripts just do
an early exit the first time one of the tests fails?  That way you only
ever see a single timeout, which should reduce the time taken to run the
tests.  For example, in gdb.cp/cpcompletion.exp, you could define a proc
something like:

  proc test_gdb_complete_unique_or_return { args } {
    if {![eval test_gdb_complete_unique $args]} {
      return -level 2
    }
  }

then update all callers of test_gdb_complete_unique in that script to
call the new proc instead.  Then all you need to do is updated
test_gdb_complete_unique to return 0 on failure and 1 on success, and
the test script will return immediately at the first timeout...

These are just my thoughts, but I hope some of this is helpful.

Thanks,
Andrew


>  	}
>  	timeout {
>  	    fail "$test (timeout)"
> -- 
> 2.41.0

Re: [PATCH v2] gdb/testsuite: fix completion tests when using READ1

[Guinevere Larsen]

On 24/08/2023 12:29, Andrew Burgess wrote:
> Maybe a better solution would be to have the completion scripts just do
> an early exit the first time one of the tests fails?  That way you only
> ever see a single timeout, which should reduce the time taken to run the
> tests.  For example, in gdb.cp/cpcompletion.exp, you could define a proc
> something like:
>
>    proc test_gdb_complete_unique_or_return { args } {
>      if {![eval test_gdb_complete_unique $args]} {
>        return -level 2
>      }
>    }
>
> then update all callers of test_gdb_complete_unique in that script to
> call the new proc instead.  Then all you need to do is updated
> test_gdb_complete_unique to return 0 on failure and 1 on success, and
> the test script will return immediately at the first timeout...
>
> These are just my thoughts, but I hope some of this is helpful.
>
> Thanks,
> Andrew

Just sending this to document what we spoke off-list (lest I forget 
before implementing), but there is a better way to do this. 
test_gdb_complete_unique already has an early exit mechanism, but it 
calls the tab completion first, then the command completion. If we 
switch the orders, we wont have this issue, because command completion 
has a well-defined end of output - the gdb prompt. With this, I can 
safely remove the faster fall method and we won't need to wait for timeouts.

I'll send a v3 with this as soon as I can.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers