[PATCH] gdb/testsuite: Fix race in gdb.dwarf2/calling-convention.exp

[PATCH] gdb/testsuite: Fix race in gdb.dwarf2/calling-convention.exp

[Lancelot SIX]

Pedro Alves warned me that there is a race in
gdb.dwarf2/calling-convention.exp making the test sometimes fail on his
setup.  This can be reliably reproduced using :

    make check-read1 TESTS="gdb.dwarf2/calling-convention.exp"

The relevant part of the gdb.log file is:

    return 35
    Function 'foo' does not follow the target calling convention.
    If you continue, setting the return value will probably lead to unpredictable behaviors.
    Make foo return now? (y or n) PASS: gdb.dwarf2/calling-convention.exp: return 35
    n
    Not confirmed
    (gdb) FAIL: gdb.dwarf2/calling-convention.exp: finish

The issue is that when doing the test for "return 35", the DejaGnu test
sends "n" (to tell GDB not to perform the return action) but never
consumes the "Not confirmed" acknowledgment sent by GDB.  Later, when
trying to do the next test, DejaGnu tries to match the leftover output
from the "return" test. As this output is not expected, the test fails.

Fix this by consuming the "Not confirmed\r\n$gdb_prompt " output.

While at cleaning up this test, also improve the regex for the return
test:
- Remove the leading ".*" pattern.
- Replace the "$" (end of line marker) with "\$" because it does not
  identify a TCL variable.

Tested on x86_64-gnu-linux, using

- make check TESTS="gdb.dwarf2/calling-convention.exp"
- make check-read1 TESTS="gdb.dwarf2/calling-convention.exp"

Change-Id: I42858b13db2cbd623c5c1739de65ad423e0c0938
---
 gdb/testsuite/gdb.dwarf2/calling-convention.exp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.dwarf2/calling-convention.exp b/gdb/testsuite/gdb.dwarf2/calling-convention.exp
index 0a11cb15c68..7ea9ae7c768 100644
--- a/gdb/testsuite/gdb.dwarf2/calling-convention.exp
+++ b/gdb/testsuite/gdb.dwarf2/calling-convention.exp
@@ -84,9 +84,9 @@ gdb_breakpoint "foo"
 gdb_continue_to_breakpoint "foo"
 
 gdb_test_multiple "return 35" "" {
-    -re ".*Function 'foo' does not follow the target calling convention.\r\nIf you continue, setting the return value will probably lead to unpredictable behaviors.\r\nMake foo return now?.*\\(y or n\\) $" {
-	send_gdb "n\n"
+    -re "Function 'foo' does not follow the target calling convention.\r\nIf you continue, setting the return value will probably lead to unpredictable behaviors.\r\nMake foo return now?.*\\(y or n\\) \$" {
 	pass $gdb_test_name
+	gdb_test "n" "Not confirmed" "Cancel return"
     }
 }
 

base-commit: 5321c31bc78379a33f07dc7bef9256d05b942ad7
-- 
2.25.1

Re: [PATCH] gdb/testsuite: Fix race in gdb.dwarf2/calling-convention.exp

[Bruno Larsen]

On 3/30/22 09:50, Lancelot SIX via Gdb-patches wrote:
> Pedro Alves warned me that there is a race in
> gdb.dwarf2/calling-convention.exp making the test sometimes fail on his
> setup.  This can be reliably reproduced using :
> 
>      make check-read1 TESTS="gdb.dwarf2/calling-convention.exp"
> 
> The relevant part of the gdb.log file is:
> 
>      return 35
>      Function 'foo' does not follow the target calling convention.
>      If you continue, setting the return value will probably lead to unpredictable behaviors.
>      Make foo return now? (y or n) PASS: gdb.dwarf2/calling-convention.exp: return 35
>      n
>      Not confirmed
>      (gdb) FAIL: gdb.dwarf2/calling-convention.exp: finish
> 
> The issue is that when doing the test for "return 35", the DejaGnu test
> sends "n" (to tell GDB not to perform the return action) but never
> consumes the "Not confirmed" acknowledgment sent by GDB.  Later, when
> trying to do the next test, DejaGnu tries to match the leftover output
> from the "return" test. As this output is not expected, the test fails.
> 
> Fix this by consuming the "Not confirmed\r\n$gdb_prompt " output.
> 
> While at cleaning up this test, also improve the regex for the return
> test:
> - Remove the leading ".*" pattern.
> - Replace the "$" (end of line marker) with "\$" because it does not
>    identify a TCL variable.
> 
> Tested on x86_64-gnu-linux, using
> 
> - make check TESTS="gdb.dwarf2/calling-convention.exp"
> - make check-read1 TESTS="gdb.dwarf2/calling-convention.exp"
> 
> Change-Id: I42858b13db2cbd623c5c1739de65ad423e0c0938

Thanks for looking at this! It had me quite confused. The change and explanation looks good to me, but I can't approve patches.
> ---
>   gdb/testsuite/gdb.dwarf2/calling-convention.exp | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.dwarf2/calling-convention.exp b/gdb/testsuite/gdb.dwarf2/calling-convention.exp
> index 0a11cb15c68..7ea9ae7c768 100644
> --- a/gdb/testsuite/gdb.dwarf2/calling-convention.exp
> +++ b/gdb/testsuite/gdb.dwarf2/calling-convention.exp
> @@ -84,9 +84,9 @@ gdb_breakpoint "foo"
>   gdb_continue_to_breakpoint "foo"
>   
>   gdb_test_multiple "return 35" "" {
> -    -re ".*Function 'foo' does not follow the target calling convention.\r\nIf you continue, setting the return value will probably lead to unpredictable behaviors.\r\nMake foo return now?.*\\(y or n\\) $" {
> -	send_gdb "n\n"
> +    -re "Function 'foo' does not follow the target calling convention.\r\nIf you continue, setting the return value will probably lead to unpredictable behaviors.\r\nMake foo return now?.*\\(y or n\\) \$" {
>   	pass $gdb_test_name
> +	gdb_test "n" "Not confirmed" "Cancel return"
>       }
>   }
>   
> 
> base-commit: 5321c31bc78379a33f07dc7bef9256d05b942ad7


-- 
Cheers!
Bruno Larsen

Re: [PATCH] gdb/testsuite: Fix race in gdb.dwarf2/calling-convention.exp

[Pedro Alves]

On 2022-03-30 13:50, Lancelot SIX via Gdb-patches wrote:
> Pedro Alves warned me that there is a race in
> gdb.dwarf2/calling-convention.exp making the test sometimes fail on his
> setup.  This can be reliably reproduced using :
> 
>     make check-read1 TESTS="gdb.dwarf2/calling-convention.exp"
> 
> The relevant part of the gdb.log file is:
> 
>     return 35
>     Function 'foo' does not follow the target calling convention.
>     If you continue, setting the return value will probably lead to unpredictable behaviors.
>     Make foo return now? (y or n) PASS: gdb.dwarf2/calling-convention.exp: return 35
>     n
>     Not confirmed
>     (gdb) FAIL: gdb.dwarf2/calling-convention.exp: finish
> 
> The issue is that when doing the test for "return 35", the DejaGnu test
> sends "n" (to tell GDB not to perform the return action) but never
> consumes the "Not confirmed" acknowledgment sent by GDB.  Later, when
> trying to do the next test, DejaGnu tries to match the leftover output
> from the "return" test. As this output is not expected, the test fails.
> 
> Fix this by consuming the "Not confirmed\r\n$gdb_prompt " output.
> 
> While at cleaning up this test, also improve the regex for the return
> test:
> - Remove the leading ".*" pattern.
> - Replace the "$" (end of line marker) with "\$" because it does not
>   identify a TCL variable.

I don't think the latter is necessary.  If it was, then the >3k instances
we have would be problematic:

 $ grep -rn " $\"" testsuite/ | wc -l
 3497

>  gdb_test_multiple "return 35" "" {
> -    -re ".*Function 'foo' does not follow the target calling convention.\r\nIf you continue, setting the return value will probably lead to unpredictable behaviors.\r\nMake foo return now?.*\\(y or n\\) $" {
> -	send_gdb "n\n"
> +    -re "Function 'foo' does not follow the target calling convention.\r\nIf you continue, setting the return value will probably lead to unpredictable behaviors.\r\nMake foo return now?.*\\(y or n\\) \$" {

This should escape the "?" in "now?" too, as '?' has special meaning in regexps.

The ".*" just before (y or n) isn't needed, right?

>  	pass $gdb_test_name
> +	gdb_test "n" "Not confirmed" "Cancel return"

Lowercase messages please.

Note gdb_test can handle this itself, it supports two extra arguments to handle questions.  Like so:

gdb_test "return 35" \
    "Not confirmed" \
    "return 35" \
    "Function 'foo' does not follow the target calling convention.\r\nIf you continue, setting the return value will probably lead to unpredictable behaviors.\r\nMake foo return now\\? \\(y or n\\) $" \
    "n"

Note that if gdb doesn't print the question and prints "Not confirmed" straight away, the test will still pass, because
the question is optional.  IMHO that's a gdb_test misfeature, though, which can lead to regressions without us noticing.
I wrote a patch to change that, and it caught a number of wrong things in the testsuite.  I'll send it a bit later.

Re: [PATCH] gdb/testsuite: Fix race in gdb.dwarf2/calling-convention.exp

[Lancelot SIX]

Hi,

> This should escape the "?" in "now?" too, as '?' has special meaning in regexps.

Yes, this one should definitely be escaped.

> 
> The ".*" just before (y or n) isn't needed, right?
> 

TBH, this is a pattern I used from somewhere else… And at the moment is 
sort of is required because of the non-escaped "?" you mentioned just 
above.  If the "?" is escaped properly, the ".*" can (should) be 
replaced by a plain space " ".

>>        pass $gdb_test_name
>> +     gdb_test "n" "Not confirmed" "Cancel return"
> 
> Lowercase messages please.
> 
> Note gdb_test can handle this itself, it supports two extra arguments to handle questions.  Like so:

Thanks, I was not aware of this.  This indeed simplified the all thing 
even further, so it should be preferred.

> 
> gdb_test "return 35" \
>      "Not confirmed" \
>      "return 35" \
>      "Function 'foo' does not follow the target calling convention.\r\nIf you continue, setting the return value will probably lead to unpredictable behaviors.\r\nMake foo return now\\? \\(y or n\\) $" \
>      "n"
> 
> Note that if gdb doesn't print the question and prints "Not confirmed" straight away, the test will still pass, because
> the question is optional.  IMHO that's a gdb_test misfeature, though, which can lead to regressions without us noticing.
> I wrote a patch to change that, and it caught a number of wrong things in the testsuite.  I'll send it a bit later.

Great! Thanks for doing this.

Best,
Lancelot.