[PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp.

[PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp.

[Carl Love]

GDB maintainers:

The following patch fixes gdb tests errors with Valgrind in the test
gdb.base/valgrind-bt.exp.  The patch fixes an issue with the wait time
in the lib/valgrind.exp as well.  The lib fix also fixes test errors in
the gdb.base/valgrind-disp-step.exp and valgrind-infcall.exp.

The patch has been run on prototype ISA 3.1 hardware, Power 9 and Power
8BE.

Please let me know if the patch is acceptable to commit.

                      Carl build_plt_stub


---------------------------------------------------
[PATCH] Fix test case gdb.base/valgrind-bt.exp.

gdb/testsuite/ChangeLog:

	* gdb.base/valgrind-bt.exp: Add gdb_test "break main".
	Update expected string for gdb_test "bt".

	* lib/valgrind.exp: Increase vgdb wait from 1 to 5.
---
 gdb/testsuite/gdb.base/valgrind-bt.exp | 4 +++-
 gdb/testsuite/lib/valgrind.exp         | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/valgrind-bt.exp b/gdb/testsuite/gdb.base/valgrind-bt.exp
index bc6b0a384dd..206aa6360e6 100644
--- a/gdb/testsuite/gdb.base/valgrind-bt.exp
+++ b/gdb/testsuite/gdb.base/valgrind-bt.exp
@@ -31,6 +31,8 @@ if { [vgdb_start] == -1 } {
 
 set double_free [gdb_get_line_number "double-free"]
 
+gdb_test "break main" "Breakpoint 1 at 0x\[0-9a-fA-F\]+: file .*valgrind-bt.c, line \[0-9\]+." "Set bk in main"
+
 set test "continue"
 gdb_test_multiple "continue" $test {
     -re "Invalid free\\(\\).*: main \\(${srcfile}:$double_free\\)\r\n.*$gdb_prompt $" {
@@ -57,7 +59,7 @@ gdb_test_multiple "continue" $test {
 gdb_test_no_output "set height 0"
 gdb_test_no_output "set width 0"
 
-gdb_test "bt" "in main \\(.*\\) at .*${srcfile}:$double_free"
+gdb_test "bt" "#\[0-9 ]+main \\(.*\\) at .*${srcfile}.*"
 
 # Explicitly kill the program so it doesn't dump core when we quit->detach.
 gdb_test "kill" "" "kill program" "Kill the program being debugged.*y or n. $" "y"
diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
index c214491f7b8..5e0eaa33ee8 100644
--- a/gdb/testsuite/lib/valgrind.exp
+++ b/gdb/testsuite/lib/valgrind.exp
@@ -87,7 +87,7 @@ proc vgdb_start { {active_at_startup 1} } {
     # gdbserver and connect to it.
     gdb_test "disconnect" ".*"
 
-    set vgdbcmd "target remote | vgdb --wait=1 --pid=$vgdbpid"
+    set vgdbcmd "target remote | vgdb --wait=5 --pid=$vgdbpid"
     if { $active_at_startup } {
 	gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
     } else {
-- 
2.27.0

Re: [PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp.

[Keith Seitz]

On 4/16/21 11:15 AM, Carl Love via Gdb-patches wrote:

> diff --git a/gdb/testsuite/gdb.base/valgrind-bt.exp b/gdb/testsuite/gdb.base/valgrind-bt.exp
> index bc6b0a384dd..206aa6360e6 100644
> --- a/gdb/testsuite/gdb.base/valgrind-bt.exp
> +++ b/gdb/testsuite/gdb.base/valgrind-bt.exp
> @@ -31,6 +31,8 @@ if { [vgdb_start] == -1 } {
>  
>  set double_free [gdb_get_line_number "double-free"]
>  
> +gdb_test "break main" "Breakpoint 1 at 0x\[0-9a-fA-F\]+: file .*valgrind-bt.c, line \[0-9\]+." "Set bk in main"
> +
>  set test "continue"
>  gdb_test_multiple "continue" $test {
>      -re "Invalid free\\(\\).*: main \\(${srcfile}:$double_free\\)\r\n.*$gdb_prompt $" {

It is often much easier (and more desirable) to use the testsuite's
gdb_breakpoint to set "ordinary" breakpoints.

> @@ -57,7 +59,7 @@ gdb_test_multiple "continue" $test {
>  gdb_test_no_output "set height 0"
>  gdb_test_no_output "set width 0"
>  
> -gdb_test "bt" "in main \\(.*\\) at .*${srcfile}:$double_free"
> +gdb_test "bt" "#\[0-9 ]+main \\(.*\\) at .*${srcfile}.*"
>  
>  # Explicitly kill the program so it doesn't dump core when we quit->detach.
>  gdb_test "kill" "" "kill program" "Kill the program being debugged.*y or n. $" "y"
> diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
> index c214491f7b8..5e0eaa33ee8 100644
> --- a/gdb/testsuite/lib/valgrind.exp
> +++ b/gdb/testsuite/lib/valgrind.exp
> @@ -87,7 +87,7 @@ proc vgdb_start { {active_at_startup 1} } {
>      # gdbserver and connect to it.
>      gdb_test "disconnect" ".*"
>  
> -    set vgdbcmd "target remote | vgdb --wait=1 --pid=$vgdbpid"
> +    set vgdbcmd "target remote | vgdb --wait=5 --pid=$vgdbpid"
>      if { $active_at_startup } {
>  	gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
>      } else {
> 

The vgdb man page cautions (wrt --wait):

           Also, if you use the --wait argument in the GDB
           "target remote" command, you must set the GDB remotetimeout to a
           value bigger than the --wait argument value.

Out of curiosity, is this condition satisfied? When I run valgrind/vgdb manually,
the remotetimeout is 2, and the only place I see it set is in config/monitor.exp
when executables are downloaded to the target.

Keith

RE: [PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp.

[Carl Love]

Keith:

I changed the patch to use the gdb_breakpoint command as you suggested.

I was not aware of the vgdb man page comments on the wait times.  The
man page also talks about setting the set remotetimeout and max-invoke-
ms value.  I updated the patch to set these values.  I also found that
the wait time of 5 was higher than necessary and was able to reduce it
to 2.

The patch was retested and no new regression errors were found. 

Please let me know if the patch with these changes is accptable. 
Thanks.

                      Carl Love
 
------------------------------------------------------------------

Fix test case gdb.base/valgrind-bt.exp.

gdb/testsuite/ChangeLog:

	* gdb.base/valgrind-bt.exp: Add gdb_test "break main".
	Update expected string for gdb_test "bt".

	* lib/valgrind.exp: Add set remotetimeout 3.
	Increase vgdb wait from 1 to 2.
	Add max-invoke-ms option to vgdb command line.
---
 gdb/testsuite/gdb.base/valgrind-bt.exp | 4 +++-
 gdb/testsuite/lib/valgrind.exp         | 5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/valgrind-bt.exp b/gdb/testsuite/gdb.base/valgrind-bt.exp
index bc6b0a384dd..b559e433d72 100644
--- a/gdb/testsuite/gdb.base/valgrind-bt.exp
+++ b/gdb/testsuite/gdb.base/valgrind-bt.exp
@@ -31,6 +31,8 @@ if { [vgdb_start] == -1 } {
 
 set double_free [gdb_get_line_number "double-free"]
 
+gdb_breakpoint "main"
+
 set test "continue"
 gdb_test_multiple "continue" $test {
     -re "Invalid free\\(\\).*: main \\(${srcfile}:$double_free\\)\r\n.*$gdb_prompt $" {
@@ -57,7 +59,7 @@ gdb_test_multiple "continue" $test {
 gdb_test_no_output "set height 0"
 gdb_test_no_output "set width 0"
 
-gdb_test "bt" "in main \\(.*\\) at .*${srcfile}:$double_free"
+gdb_test "bt" "#\[0-9 ]+main \\(.*\\) at .*${srcfile}.*"
 
 # Explicitly kill the program so it doesn't dump core when we quit->detach.
 gdb_test "kill" "" "kill program" "Kill the program being debugged.*y or n. $" "y"
diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
index c214491f7b8..caabeda9730 100644
--- a/gdb/testsuite/lib/valgrind.exp
+++ b/gdb/testsuite/lib/valgrind.exp
@@ -82,12 +82,15 @@ proc vgdb_start { {active_at_startup 1} } {
 
     clean_restart $testfile
 
+    set vgdbcmd "set remotetimeout 3"
+
     # Make sure we're disconnected, in case we're testing with the
     # native-extended-gdbserver board, where gdb_start/gdb_load spawn
     # gdbserver and connect to it.
     gdb_test "disconnect" ".*"
 
-    set vgdbcmd "target remote | vgdb --wait=1 --pid=$vgdbpid"
+    set vgdbcmd "target remote | vgdb --wait=2 --max-invoke-ms=2500 --pid=$vgdbpid"
+
     if { $active_at_startup } {
 	gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
     } else {
-- 
2.27.0

Re: [PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp.

[Keith Seitz]

On 4/19/21 2:22 PM, Carl Love wrote:
> Keith:
> 
> I changed the patch to use the gdb_breakpoint command as you suggested.
> 
> I was not aware of the vgdb man page comments on the wait times.  The
> man page also talks about setting the set remotetimeout and max-invoke-
> ms value.  I updated the patch to set these values.  I also found that
> the wait time of 5 was higher than necessary and was able to reduce it
> to 2.

Excellent. Thank you for investigating this.

> Please let me know if the patch with these changes is accptable. 
> Thanks.

I am satisfied with this change (with below adjusted -- no need to repost),
but you will have to wait until a maintainer approves the patch.

Keith

> ------------------------------------------------------------------
> 
> Fix test case gdb.base/valgrind-bt.exp.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/valgrind-bt.exp: Add gdb_test "break main".
> 	Update expected string for gdb_test "bt".

The above is not strictly accurate anymore.

> 
> 	* lib/valgrind.exp: Add set remotetimeout 3.
> 	Increase vgdb wait from 1 to 2.
> 	Add max-invoke-ms option to vgdb command line.

Re: [PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp.

[Tom Tromey]

>>>>> "Carl" == Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:

Carl> Fix test case gdb.base/valgrind-bt.exp.

Carl> gdb/testsuite/ChangeLog:

Carl> 	* gdb.base/valgrind-bt.exp: Add gdb_test "break main".
Carl> 	Update expected string for gdb_test "bt".

Carl> 	* lib/valgrind.exp: Add set remotetimeout 3.
Carl> 	Increase vgdb wait from 1 to 2.
Carl> 	Add max-invoke-ms option to vgdb command line.

Thanks, this is ok.

Tom

Re: [PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp.

[Tom de Vries]

On 4/21/21 9:57 PM, Tom Tromey wrote:
>>>>>> "Carl" == Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Carl> Fix test case gdb.base/valgrind-bt.exp.
> 
> Carl> gdb/testsuite/ChangeLog:
> 
> Carl> 	* gdb.base/valgrind-bt.exp: Add gdb_test "break main".
> Carl> 	Update expected string for gdb_test "bt".
> 
> Carl> 	* lib/valgrind.exp: Add set remotetimeout 3.
> Carl> 	Increase vgdb wait from 1 to 2.
> Carl> 	Add max-invoke-ms option to vgdb command line.
> 
> Thanks, this is ok.

AFAICT, this caused https://sourceware.org/bugzilla/show_bug.cgi?id=27786 .

Thanks,
- Tom

Re: [PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp.

[Tom de Vries]

On 4/19/21 11:22 PM, Carl Love via Gdb-patches wrote:
> diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
> index c214491f7b8..caabeda9730 100644
> --- a/gdb/testsuite/lib/valgrind.exp
> +++ b/gdb/testsuite/lib/valgrind.exp
> @@ -82,12 +82,15 @@ proc vgdb_start { {active_at_startup 1} } {
>  
>      clean_restart $testfile
>  
> +    set vgdbcmd "set remotetimeout 3"
> +
>      # Make sure we're disconnected, in case we're testing with the
>      # native-extended-gdbserver board, where gdb_start/gdb_load spawn
>      # gdbserver and connect to it.
>      gdb_test "disconnect" ".*"
>  
> -    set vgdbcmd "target remote | vgdb --wait=1 --pid=$vgdbpid"
> +    set vgdbcmd "target remote | vgdb --wait=2 --max-invoke-ms=2500 --pid=$vgdbpid"
> +
>      if { $active_at_startup } {
>  	gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
>      } else {
> 

AFAICT, the first "set vgdbcmd" doesn't achieve anything.

The value is overwritten by the second "set vgdbcmd".

Thanks,
- Tom

Re: [PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp.

[Tom de Vries]

On 4/29/21 1:28 PM, Tom de Vries wrote:
> On 4/19/21 11:22 PM, Carl Love via Gdb-patches wrote:
>> diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
>> index c214491f7b8..caabeda9730 100644
>> --- a/gdb/testsuite/lib/valgrind.exp
>> +++ b/gdb/testsuite/lib/valgrind.exp
>> @@ -82,12 +82,15 @@ proc vgdb_start { {active_at_startup 1} } {
>>  
>>      clean_restart $testfile
>>  
>> +    set vgdbcmd "set remotetimeout 3"
>> +
>>      # Make sure we're disconnected, in case we're testing with the
>>      # native-extended-gdbserver board, where gdb_start/gdb_load spawn
>>      # gdbserver and connect to it.
>>      gdb_test "disconnect" ".*"
>>  
>> -    set vgdbcmd "target remote | vgdb --wait=1 --pid=$vgdbpid"
>> +    set vgdbcmd "target remote | vgdb --wait=2 --max-invoke-ms=2500 --pid=$vgdbpid"
>> +
>>      if { $active_at_startup } {
>>  	gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
>>      } else {
>>
> 
> AFAICT, the first "set vgdbcmd" doesn't achieve anything.
> 
> The value is overwritten by the second "set vgdbcmd".

This actually sets the remotetimeout, and increasing the value to 4
fixes the FAIL I reported:
...
diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
index caabeda9730..bba338f880c 100644
--- a/gdb/testsuite/lib/valgrind.exp
+++ b/gdb/testsuite/lib/valgrind.exp
@@ -82,7 +82,7 @@ proc vgdb_start { {active_at_startup 1} } {

     clean_restart $testfile

-    set vgdbcmd "set remotetimeout 3"
+    set_remotetimeout 4

     # Make sure we're disconnected, in case we're testing with the
     # native-extended-gdbserver board, where gdb_start/gdb_load spawn

...

Thanks,
- Tom

[gdb/testsuite] Fix timeout in gdb.base/valgrind-infcall-2.exp

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1381 bytes --]

[ was: Re: [PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp. ]

On 4/29/21 1:46 PM, Tom de Vries wrote:
> On 4/29/21 1:28 PM, Tom de Vries wrote:
>> On 4/19/21 11:22 PM, Carl Love via Gdb-patches wrote:
>>> diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
>>> index c214491f7b8..caabeda9730 100644
>>> --- a/gdb/testsuite/lib/valgrind.exp
>>> +++ b/gdb/testsuite/lib/valgrind.exp
>>> @@ -82,12 +82,15 @@ proc vgdb_start { {active_at_startup 1} } {
>>>  
>>>      clean_restart $testfile
>>>  
>>> +    set vgdbcmd "set remotetimeout 3"
>>> +
>>>      # Make sure we're disconnected, in case we're testing with the
>>>      # native-extended-gdbserver board, where gdb_start/gdb_load spawn
>>>      # gdbserver and connect to it.
>>>      gdb_test "disconnect" ".*"
>>>  
>>> -    set vgdbcmd "target remote | vgdb --wait=1 --pid=$vgdbpid"
>>> +    set vgdbcmd "target remote | vgdb --wait=2 --max-invoke-ms=2500 --pid=$vgdbpid"
>>> +
>>>      if { $active_at_startup } {
>>>  	gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
>>>      } else {
>>>
>>
>> AFAICT, the first "set vgdbcmd" doesn't achieve anything.
>>
>> The value is overwritten by the second "set vgdbcmd".
> 
> This actually sets the remotetimeout, and increasing the value to 4
> fixes the FAIL I reported:

Patch below fixes the FAIL.

Any comments?

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Fix-timeout-in-gdb.base-valgrind-infcall-2.exp.patch --]
[-- Type: text/x-patch, Size: 1460 bytes --]

[gdb/testsuite] Fix timeout in gdb.base/valgrind-infcall-2.exp

Since commit 6d5702a5eb3 "Fix test case gdb.base/valgrind-bt.exp" I run into:
...
FAIL: gdb.base/valgrind-infcall-2.exp: target remote for vgdb (timeout)
FAIL: gdb.base/valgrind-infcall-2.exp: monitor v.set gdb_output (timeout)
...

The commit adds this line in proc vgdb_start:
...
    set vgdbcmd "set remotetimeout 3"
...
which has no effect given that the value of var vgdbcmd is not used before
it's overwritten.  We can fix this by doing instead:
...
    set_remotetimeout 3
...

The FAIL I'm observing is fixed by increasing the remotetimeout value to 4.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2021-04-29  Tom de Vries  <tdevries@suse.de>

	PR testsuite/27786
	* lib/valgrind.exp (vgdb_start): Use set_remotetimeout.  Increase
	remotetimeout to 4.

---
 gdb/testsuite/lib/valgrind.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
index caabeda9730..bba338f880c 100644
--- a/gdb/testsuite/lib/valgrind.exp
+++ b/gdb/testsuite/lib/valgrind.exp
@@ -82,7 +82,7 @@ proc vgdb_start { {active_at_startup 1} } {
 
     clean_restart $testfile
 
-    set vgdbcmd "set remotetimeout 3"
+    set_remotetimeout 4
 
     # Make sure we're disconnected, in case we're testing with the
     # native-extended-gdbserver board, where gdb_start/gdb_load spawn

Re: [gdb/testsuite] Fix timeout in gdb.base/valgrind-infcall-2.exp

[Keith Seitz]

On 4/29/21 6:57 AM, Tom de Vries wrote:
> [ was: Re: [PATCH] GDB: Fix test case gdb.base/valgrind-bt.exp. ]
> 
> On 4/29/21 1:46 PM, Tom de Vries wrote:
>> On 4/29/21 1:28 PM, Tom de Vries wrote:
>>> On 4/19/21 11:22 PM, Carl Love via Gdb-patches wrote:
>>>> diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
>>>> index c214491f7b8..caabeda9730 100644
>>>> --- a/gdb/testsuite/lib/valgrind.exp
>>>> +++ b/gdb/testsuite/lib/valgrind.exp
>>>> @@ -82,12 +82,15 @@ proc vgdb_start { {active_at_startup 1} } {
>>>>  
>>>>      clean_restart $testfile
>>>>  
>>>> +    set vgdbcmd "set remotetimeout 3"
>>>> +
>>>>      # Make sure we're disconnected, in case we're testing with the
>>>>      # native-extended-gdbserver board, where gdb_start/gdb_load spawn
>>>>      # gdbserver and connect to it.
>>>>      gdb_test "disconnect" ".*"
>>>>  
>>>> -    set vgdbcmd "target remote | vgdb --wait=1 --pid=$vgdbpid"
>>>> +    set vgdbcmd "target remote | vgdb --wait=2 --max-invoke-ms=2500 --pid=$vgdbpid"
>>>> +
>>>>      if { $active_at_startup } {
>>>>  	gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
>>>>      } else {
>>>>
>>>
>>> AFAICT, the first "set vgdbcmd" doesn't achieve anything.
>>>
>>> The value is overwritten by the second "set vgdbcmd".
>>
>> This actually sets the remotetimeout, and increasing the value to 4
>> fixes the FAIL I reported:

Well done! I completely overlooked that. :-(

> 
> Patch below fixes the FAIL.
> 
> Any comments?

AFAICT, that looks like the correct way to do this.

Thank you for fixing this.

Keith

Re: [gdb/testsuite] Fix timeout in gdb.base/valgrind-infcall-2.exp

[Ulrich Weigand]

On Thu, Apr 29, 2021 at 03:57:41PM +0200, Tom de Vries wrote:

> Patch below fixes the FAIL.
> 
> Any comments?

I agree, this patch is OK.   Sorry for overlooking the problem,
and thanks for providing a fix!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com