[PATCH v2] [gdb/testsuite] Fix timeout in gdb.base/bg-execution-repeat.exp

[PATCH v2] [gdb/testsuite] Fix timeout in gdb.base/bg-execution-repeat.exp

[Tom de Vries]

I ran into the following test failure with test-case
gdb.base/bg-execution-repeat.exp:
...
(gdb) PASS: gdb.base/bg-execution-repeat.exp: c&: repeat bg command
^M
Breakpoint 2, foo () at bg-execution-repeat.c:23^M
23        return 0; /* set break here */^M
print 1^M
$1 = 1^M
(gdb) PASS: gdb.base/bg-execution-repeat.exp: c&: input still accepted
FAIL: gdb.base/bg-execution-repeat.exp: c&: breakpoint hit 2 (timeout)
...

The failure can be easily reproduced by adding a sleep 5 here:
...
+    sleep 5
     gdb_test "print 1" " = 1" "input still accepted"
...

There's a race in the test-case, between:
- the command handled in the foreground: the "print 1" command, and
- the command handled in the background: the continue command.

The current way of dealing with this is by putting the inferior to sleep for 5
seconds:
...
  foo ();
  sleep (5);
  foo ();
...
with the aim that the "print 1" command will win the race.

This method is both slow and unreliable.

Fix this by making the inferior wait till the "print 1" command is done.

This reduces running time from ~11s to ~1s.

I also verified that the test-case still triggers on the original problem by
applying this gdb/infcmd.c patch:
...
-strip_bg_char (const char *args, int *bg_char_p)
+strip_bg_char (const char *_args, int *bg_char_p)
 {
-  const char *p;
+  char *args = const_cast<char *>(_args);
+  char *p;

   if (args == nullptr || *args == '\0')
     {
@@ -210,6 +211,7 @@ strip_bg_char (const char *args, int *bg_char_p)
       p--;
       while (p > args && isspace (p[-1]))
 	p--;
+      *p = '\0';
...

Tested on x86_64-linux, with make-check-all.sh.

PR testsuite/31794
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31794
---
 gdb/testsuite/gdb.base/bg-execution-repeat.c   | 18 +++++++++++++++++-
 gdb/testsuite/gdb.base/bg-execution-repeat.exp |  9 +++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/bg-execution-repeat.c b/gdb/testsuite/gdb.base/bg-execution-repeat.c
index 2caa7d442f6..d5b48ee4f94 100644
--- a/gdb/testsuite/gdb.base/bg-execution-repeat.c
+++ b/gdb/testsuite/gdb.base/bg-execution-repeat.c
@@ -23,11 +23,27 @@ foo (void)
   return 0; /* set break here */
 }
 
+static volatile int do_wait;
+
+static void
+wait (void)
+{
+  while (do_wait)
+    usleep (10 * 1000);
+}
+
 int
 main (void)
 {
+  alarm (60);
+
   foo ();
-  sleep (5);
+
+  do_wait = 1;
+  wait ();
+  /* do_wait set to 0 externally.  */
+
   foo ();
+
   return 0;
 }
diff --git a/gdb/testsuite/gdb.base/bg-execution-repeat.exp b/gdb/testsuite/gdb.base/bg-execution-repeat.exp
index a4cc7daa702..35ddb34cd8f 100644
--- a/gdb/testsuite/gdb.base/bg-execution-repeat.exp
+++ b/gdb/testsuite/gdb.base/bg-execution-repeat.exp
@@ -68,6 +68,15 @@ proc test {continue_cmd} {
     # stopped.
     gdb_test "print 1" " = 1" "input still accepted"
 
+    # With gdbserver, we cannot set memory while the inferior is running, so
+    # enable the "set var" command with an interrupt / continue& pair.
+    gdb_test -no-prompt-anchor "interrupt"
+
+    # Allow the breakpoint to trigger.
+    gdb_test -no-prompt-anchor "set var do_wait=0"
+
+    gdb_test -no-prompt-anchor "continue&"
+
     # Make sure we see a stop after the print, and not before.  Don't
     # expect a prompt, as we had resumed the inferior in the background.
     set test "breakpoint hit 2"

base-commit: b5929e7aa0195a0656a63da95d5eccbb73b5b173
-- 
2.35.3

Re: [PATCH v2] [gdb/testsuite] Fix timeout in gdb.base/bg-execution-repeat.exp

[Guinevere Larsen]

On 6/20/24 8:56 AM, Tom de Vries wrote:
> I ran into the following test failure with test-case
> gdb.base/bg-execution-repeat.exp:
> ...
> (gdb) PASS: gdb.base/bg-execution-repeat.exp: c&: repeat bg command
> ^M
> Breakpoint 2, foo () at bg-execution-repeat.c:23^M
> 23        return 0; /* set break here */^M
> print 1^M
> $1 = 1^M
> (gdb) PASS: gdb.base/bg-execution-repeat.exp: c&: input still accepted
> FAIL: gdb.base/bg-execution-repeat.exp: c&: breakpoint hit 2 (timeout)
> ...
>
> The failure can be easily reproduced by adding a sleep 5 here:
> ...
> +    sleep 5
>       gdb_test "print 1" " = 1" "input still accepted"
> ...
>
> There's a race in the test-case, between:
> - the command handled in the foreground: the "print 1" command, and
> - the command handled in the background: the continue command.
>
> The current way of dealing with this is by putting the inferior to sleep for 5
> seconds:
> ...
>    foo ();
>    sleep (5);
>    foo ();
> ...
> with the aim that the "print 1" command will win the race.
>
> This method is both slow and unreliable.
>
> Fix this by making the inferior wait till the "print 1" command is done.
>
> This reduces running time from ~11s to ~1s.
>
> I also verified that the test-case still triggers on the original problem by
> applying this gdb/infcmd.c patch:
> ...
> -strip_bg_char (const char *args, int *bg_char_p)
> +strip_bg_char (const char *_args, int *bg_char_p)
>   {
> -  const char *p;
> +  char *args = const_cast<char *>(_args);
> +  char *p;
>
>     if (args == nullptr || *args == '\0')
>       {
> @@ -210,6 +211,7 @@ strip_bg_char (const char *args, int *bg_char_p)
>         p--;
>         while (p > args && isspace (p[-1]))
>   	p--;
> +      *p = '\0';
> ...
>
> Tested on x86_64-linux, with make-check-all.sh.
>
> PR testsuite/31794
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31794
> ---
>   gdb/testsuite/gdb.base/bg-execution-repeat.c   | 18 +++++++++++++++++-
>   gdb/testsuite/gdb.base/bg-execution-repeat.exp |  9 +++++++++
>   2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.base/bg-execution-repeat.c b/gdb/testsuite/gdb.base/bg-execution-repeat.c
> index 2caa7d442f6..d5b48ee4f94 100644
> --- a/gdb/testsuite/gdb.base/bg-execution-repeat.c
> +++ b/gdb/testsuite/gdb.base/bg-execution-repeat.c
> @@ -23,11 +23,27 @@ foo (void)
>     return 0; /* set break here */
>   }
>   
> +static volatile int do_wait;
> +
> +static void
> +wait (void)
> +{
> +  while (do_wait)
> +    usleep (10 * 1000);
> +}
> +
>   int
>   main (void)
>   {
> +  alarm (60);
> +
>     foo ();
> -  sleep (5);
> +
> +  do_wait = 1;
> +  wait ();
> +  /* do_wait set to 0 externally.  */
> +
>     foo ();
> +
>     return 0;
>   }
> diff --git a/gdb/testsuite/gdb.base/bg-execution-repeat.exp b/gdb/testsuite/gdb.base/bg-execution-repeat.exp
> index a4cc7daa702..35ddb34cd8f 100644
> --- a/gdb/testsuite/gdb.base/bg-execution-repeat.exp
> +++ b/gdb/testsuite/gdb.base/bg-execution-repeat.exp
> @@ -68,6 +68,15 @@ proc test {continue_cmd} {
>       # stopped.
>       gdb_test "print 1" " = 1" "input still accepted"
>   
> +    # With gdbserver, we cannot set memory while the inferior is running, so
> +    # enable the "set var" command with an interrupt / continue& pair.
> +    gdb_test -no-prompt-anchor "interrupt"
> +
> +    # Allow the breakpoint to trigger.
> +    gdb_test -no-prompt-anchor "set var do_wait=0"
> +
> +    gdb_test -no-prompt-anchor "continue&"
> +
>       # Make sure we see a stop after the print, and not before.  Don't
>       # expect a prompt, as we had resumed the inferior in the background.
>       set test "breakpoint hit 2"
>
> base-commit: b5929e7aa0195a0656a63da95d5eccbb73b5b173

Thanks for this, I always like when we can make tests run faster. Testes 
with unix, native-gdbserver and native-extended-gdbserver with both gcc 
and clang.

Just a suggestion, on line 45 there is a use of gdb_test_multiple with a 
prompt with no anchor. It seems to me that that test could be 
substituted for `gdb_test -no-prompt-anchor` too.

Either way, this patch is a good change, Reviewed-By: Guinevere Larsen 
<blarsen@redhat.com>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

Re: [PATCH v2] [gdb/testsuite] Fix timeout in gdb.base/bg-execution-repeat.exp

[Tom de Vries]

On 6/25/24 16:19, Guinevere Larsen wrote:
> On 6/20/24 8:56 AM, Tom de Vries wrote:
>> I ran into the following test failure with test-case
>> gdb.base/bg-execution-repeat.exp:
>> ...
>> (gdb) PASS: gdb.base/bg-execution-repeat.exp: c&: repeat bg command
>> ^M
>> Breakpoint 2, foo () at bg-execution-repeat.c:23^M
>> 23        return 0; /* set break here */^M
>> print 1^M
>> $1 = 1^M
>> (gdb) PASS: gdb.base/bg-execution-repeat.exp: c&: input still accepted
>> FAIL: gdb.base/bg-execution-repeat.exp: c&: breakpoint hit 2 (timeout)
>> ...
>>
>> The failure can be easily reproduced by adding a sleep 5 here:
>> ...
>> +    sleep 5
>>       gdb_test "print 1" " = 1" "input still accepted"
>> ...
>>
>> There's a race in the test-case, between:
>> - the command handled in the foreground: the "print 1" command, and
>> - the command handled in the background: the continue command.
>>
>> The current way of dealing with this is by putting the inferior to 
>> sleep for 5
>> seconds:
>> ...
>>    foo ();
>>    sleep (5);
>>    foo ();
>> ...
>> with the aim that the "print 1" command will win the race.
>>
>> This method is both slow and unreliable.
>>
>> Fix this by making the inferior wait till the "print 1" command is done.
>>
>> This reduces running time from ~11s to ~1s.
>>
>> I also verified that the test-case still triggers on the original 
>> problem by
>> applying this gdb/infcmd.c patch:
>> ...
>> -strip_bg_char (const char *args, int *bg_char_p)
>> +strip_bg_char (const char *_args, int *bg_char_p)
>>   {
>> -  const char *p;
>> +  char *args = const_cast<char *>(_args);
>> +  char *p;
>>
>>     if (args == nullptr || *args == '\0')
>>       {
>> @@ -210,6 +211,7 @@ strip_bg_char (const char *args, int *bg_char_p)
>>         p--;
>>         while (p > args && isspace (p[-1]))
>>       p--;
>> +      *p = '\0';
>> ...
>>
>> Tested on x86_64-linux, with make-check-all.sh.
>>
>> PR testsuite/31794
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31794
>> ---
>>   gdb/testsuite/gdb.base/bg-execution-repeat.c   | 18 +++++++++++++++++-
>>   gdb/testsuite/gdb.base/bg-execution-repeat.exp |  9 +++++++++
>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/bg-execution-repeat.c 
>> b/gdb/testsuite/gdb.base/bg-execution-repeat.c
>> index 2caa7d442f6..d5b48ee4f94 100644
>> --- a/gdb/testsuite/gdb.base/bg-execution-repeat.c
>> +++ b/gdb/testsuite/gdb.base/bg-execution-repeat.c
>> @@ -23,11 +23,27 @@ foo (void)
>>     return 0; /* set break here */
>>   }
>> +static volatile int do_wait;
>> +
>> +static void
>> +wait (void)
>> +{
>> +  while (do_wait)
>> +    usleep (10 * 1000);
>> +}
>> +
>>   int
>>   main (void)
>>   {
>> +  alarm (60);
>> +
>>     foo ();
>> -  sleep (5);
>> +
>> +  do_wait = 1;
>> +  wait ();
>> +  /* do_wait set to 0 externally.  */
>> +
>>     foo ();
>> +
>>     return 0;
>>   }
>> diff --git a/gdb/testsuite/gdb.base/bg-execution-repeat.exp 
>> b/gdb/testsuite/gdb.base/bg-execution-repeat.exp
>> index a4cc7daa702..35ddb34cd8f 100644
>> --- a/gdb/testsuite/gdb.base/bg-execution-repeat.exp
>> +++ b/gdb/testsuite/gdb.base/bg-execution-repeat.exp
>> @@ -68,6 +68,15 @@ proc test {continue_cmd} {
>>       # stopped.
>>       gdb_test "print 1" " = 1" "input still accepted"
>> +    # With gdbserver, we cannot set memory while the inferior is 
>> running, so
>> +    # enable the "set var" command with an interrupt / continue& pair.
>> +    gdb_test -no-prompt-anchor "interrupt"
>> +
>> +    # Allow the breakpoint to trigger.
>> +    gdb_test -no-prompt-anchor "set var do_wait=0"
>> +
>> +    gdb_test -no-prompt-anchor "continue&"
>> +
>>       # Make sure we see a stop after the print, and not before.  Don't
>>       # expect a prompt, as we had resumed the inferior in the 
>> background.
>>       set test "breakpoint hit 2"
>>
>> base-commit: b5929e7aa0195a0656a63da95d5eccbb73b5b173
> 
> Thanks for this, I always like when we can make tests run faster. Testes 
> with unix, native-gdbserver and native-extended-gdbserver with both gcc 
> and clang.
> 

Hi Gwen,

thanks for the review.

> Just a suggestion, on line 45 there is a use of gdb_test_multiple with a 
> prompt with no anchor. It seems to me that that test could be 
> substituted for `gdb_test -no-prompt-anchor` too.
>

I've dealt with this in a separate patch ( 
https://sourceware.org/pipermail/gdb-patches/2024-June/210220.html ).


> Either way, this patch is a good change, Reviewed-By: Guinevere Larsen 
> <blarsen@redhat.com>
> 

Pushed.

Thanks,
- Tom