Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option --db-attach

Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option --db-attach

[Pedro Alves]

On 10/24/2018 12:13 PM, Tom de Vries wrote:
> Hi,
> 
> When running valgrind-db-attach.exp with valgrind version 3.13.0, we get:
> ...
> PASS: gdb.base/valgrind-db-attach.exp: spawn valgrind
> valgrind: Unknown option: --db-attach=yes
> valgrind: Use --help for more information or consult the user manual.
> ERROR: Process no longer exists
> UNRESOLVED: gdb.base/valgrind-db-attach.exp: valgrind started
> ...
> 
> The valgrind option --db-attach has been deprecated in version 3.10.0, and
> removed in version 3.11.0.
> 

But was it replaced with / renamed to something else equivalent,
or the functionality completely eliminated?

If the latter, I don't see much value in keeping the
test case around going forward.  Especially if we have no
comment in the source indicating when it stopped being useful...
It ends up just being dead weight and future gdb developers
won't even realize.

Thanks,
Pedro Alves

> Fix valgrind-db-attach.exp to replace the ERROR/UNRESOLVED with:
> ...
> UNSUPPORTED: gdb.base/valgrind-db-attach.exp: valgrind started
> ...
> 
> Tested on x86_64-linux.
> 
> Committed as obvious.
> 
> Thanks,
> - Tom
> 
> [gdb/testsuite] Handle removed valgrind option --db-attach
> 
> 2018-10-24  Tom de Vries  <tdevries@suse.de>
> 
> 	* gdb.base/valgrind-db-attach.exp: Handle removed support for
> 	--db-attach in valgrind.
> 
> ---
>  gdb/testsuite/gdb.base/valgrind-db-attach.exp | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.base/valgrind-db-attach.exp b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
> index 3be6af5ca9..3e40283a95 100644
> --- a/gdb/testsuite/gdb.base/valgrind-db-attach.exp
> +++ b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
> @@ -51,6 +51,10 @@ set use_gdb_stub 1
>  set test "valgrind started"
>  # The trailing '.' differs for different memcheck versions.
>  gdb_test_multiple "" $test {
> +    -re "valgrind: Unknown option: --db-attach=yes" {
> +	unsupported $test
> +	return -1
> +    }
>      -re "Memcheck, a memory error detector\\.?\r\n" {
>  	pass $test
>      }
> 

Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option --db-attach

[Tom de Vries]

On 10/24/18 6:29 PM, Pedro Alves wrote:
> On 10/24/2018 12:13 PM, Tom de Vries wrote:
>> Hi,
>>
>> When running valgrind-db-attach.exp with valgrind version 3.13.0, we get:
>> ...
>> PASS: gdb.base/valgrind-db-attach.exp: spawn valgrind
>> valgrind: Unknown option: --db-attach=yes
>> valgrind: Use --help for more information or consult the user manual.
>> ERROR: Process no longer exists
>> UNRESOLVED: gdb.base/valgrind-db-attach.exp: valgrind started
>> ...
>>
>> The valgrind option --db-attach has been deprecated in version 3.10.0, and
>> removed in version 3.11.0.
>>
> 
> But was it replaced with / renamed to something else equivalent,
> or the functionality completely eliminated?
> 

From http://valgrind.org/docs/manual/dist.news.html:
...
* ================== DEPRECATED FEATURES =================

* --db-attach is now deprecated and will be removed in the next
  valgrind feature release.  The built-in GDB server capabilities are
  superior and should be used instead. Learn more here:

http://valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.gdbserver
...

I'd say the functionality was replaced with vgdb, which is exercised in
other test-cases (valgrind-infcall.exp, valgrind-disp-step.exp).

> If the latter, I don't see much value in keeping the
> test case around going forward.  Especially if we have no
> comment in the source indicating when it stopped being useful...

I could add this (similar to the "valgrind is not >= 3.7.0" comment in
valgrind-infcall.exp):
...
diff --git a/gdb/testsuite/gdb.base/valgrind-db-attach.exp
b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
index 3e40283a95..dd13b4d170 100644
--- a/gdb/testsuite/gdb.base/valgrind-db-attach.exp
+++ b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
@@ -52,6 +52,7 @@ set test "valgrind started"
 # The trailing '.' differs for different memcheck versions.
 gdb_test_multiple "" $test {
     -re "valgrind: Unknown option: --db-attach=yes" {
+       # valgrind is not <= 3.10.0.
        unsupported $test
        return -1
     }
...
?

> It ends up just being dead weight and future gdb developers
> won't even realize.
> 

True. I can also remove the testcase.

Thanks,
- Tom

Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option --db-attach

[Philippe Waroquiers]

On Wed, 2018-10-24 at 17:29 +0100, Pedro Alves wrote:
> On 10/24/2018 12:13 PM, Tom de Vries wrote:
> > Hi,
> > 
> > When running valgrind-db-attach.exp with valgrind version 3.13.0, we get:
> > ...
> > PASS: gdb.base/valgrind-db-attach.exp: spawn valgrind
> > valgrind: Unknown option: --db-attach=yes
> > valgrind: Use --help for more information or consult the user manual.
> > ERROR: Process no longer exists
> > UNRESOLVED: gdb.base/valgrind-db-attach.exp: valgrind started
> > ...
> > 
> > The valgrind option --db-attach has been deprecated in version 3.10.0, and
> > removed in version 3.11.0.
> > 
> 
> But was it replaced with / renamed to something else equivalent,
> or the functionality completely eliminated?
--db-attach option functionality was removed, as it was not very
reliable and had a bunch of limitations e.g. not supporting threads.

Instead, the gdbserver embedded in valgrind allows the user debug a process
when valgrind reports an error.

I have put on my list of things to do to convert valgrind-attach
test to use vgdb (but the conversion is probably not trivial).

Philippe

Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option --db-attach

[Tom de Vries]

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

On 10/24/18 10:50 PM, Philippe Waroquiers wrote:
> On Wed, 2018-10-24 at 17:29 +0100, Pedro Alves wrote:
>> On 10/24/2018 12:13 PM, Tom de Vries wrote:
>>> Hi,
>>>
>>> When running valgrind-db-attach.exp with valgrind version 3.13.0, we get:
>>> ...
>>> PASS: gdb.base/valgrind-db-attach.exp: spawn valgrind
>>> valgrind: Unknown option: --db-attach=yes
>>> valgrind: Use --help for more information or consult the user manual.
>>> ERROR: Process no longer exists
>>> UNRESOLVED: gdb.base/valgrind-db-attach.exp: valgrind started
>>> ...
>>>
>>> The valgrind option --db-attach has been deprecated in version 3.10.0, and
>>> removed in version 3.11.0.
>>>
>>
>> But was it replaced with / renamed to something else equivalent,
>> or the functionality completely eliminated?
> --db-attach option functionality was removed, as it was not very
> reliable and had a bunch of limitations e.g. not supporting threads.
> 
> Instead, the gdbserver embedded in valgrind allows the user debug a process
> when valgrind reports an error.
> 
> I have put on my list of things to do to convert valgrind-attach
> test to use vgdb (but the conversion is probably not trivial).

The valgrind-db-attach.exp test-case is very similar to the
valgrind-infcall.exp test-case (using vgdb), so I gave it a try by
putting the two alongside, and doing a copy/paste/replace.

OK for trunk (perhaps with a rename to valgrind-bt.{exp,c})?

[ Or perhaps first factor out a vgdb_start/stop or some such from
valgrind-disp-step.exp and valgrind-infcall.exp, and then use those
procs in valgrind-db-attach.exp instead? ]

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Rewrite-valgrind-db-attach.exp-to-use-vgdb.patch --]
[-- Type: text/x-patch, Size: 4694 bytes --]

[gdb/testsuite] Rewrite valgrind-db-attach.exp to use vgdb

The valgrind option --db-attach has been deprecated in version 3.10.0, and
removed in version 3.11.0, so the valgrind-db-attach.exp testcase is
unsupported starting version 3.11.0.

Rewrite the test-case to use vgdb instead (making it supported starting
version 3.7.0).

Tested on x86_64-linux with and without --target_board=native-gdbserver.

2018-10-25  Tom de Vries  <tdevries@suse.de>

	* gdb.base/valgrind-db-attach.exp: Rewrite to use vgdb.

---
 gdb/testsuite/gdb.base/valgrind-db-attach.exp | 80 ++++++++++++++++++---------
 1 file changed, 55 insertions(+), 25 deletions(-)

diff --git a/gdb/testsuite/gdb.base/valgrind-db-attach.exp b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
index 3e40283a95..348f74379d 100644
--- a/gdb/testsuite/gdb.base/valgrind-db-attach.exp
+++ b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
@@ -23,16 +23,8 @@ if {[build_executable $testfile.exp $testfile $srcfile {debug}] == -1} {
     return -1
 }
 
-gdb_exit
-
-# remote_spawn breaks the command on each whitespace despite possible quoting.
-# Use backslash-escaped whitespace there instead:
-
-set db_command "--db-command=$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts] %f %p"
-regsub -all " " $db_command "\\ " db_command
-
 set test "spawn valgrind"
-set cmd "valgrind --db-attach=yes $db_command $binfile"
+set cmd "valgrind --vgdb-error=0 $binfile"
 set res [remote_spawn host $cmd]
 if { $res < 0 || $res == "" } {
     verbose -log "Spawning $cmd failed."
@@ -43,18 +35,13 @@ pass $test
 # Declare GDB now as running.
 set gdb_spawn_id $res
 
-# GDB spawned by `valgrind --db-attach=yes' stops already after the startup is
-# executed, like with non-extended gdbserver.  It is also not correct to
-# run/attach the inferior.
+# GDB started by vgdb stops already after the startup is executed, like with
+# non-extended gdbserver.  It is also not correct to run/attach the inferior.
 set use_gdb_stub 1
 
 set test "valgrind started"
 # The trailing '.' differs for different memcheck versions.
 gdb_test_multiple "" $test {
-    -re "valgrind: Unknown option: --db-attach=yes" {
-	unsupported $test
-	return -1
-    }
     -re "Memcheck, a memory error detector\\.?\r\n" {
 	pass $test
     }
@@ -72,23 +59,63 @@ gdb_test_multiple "" $test {
 	unsupported $test
 	return -1
     }
+    -re "valgrind: Bad option.*--vgdb-error=0" {
+	# valgrind is not >= 3.7.0.
+	unsupported $test
+	return -1
+    }
+}
+
+set test "vgdb prompt"
+# The trailing '.' differs for different memcheck versions.
+gdb_test_multiple "" $test {
+    -re "  (target remote | \[^\r\n\]*/vgdb \[^\r\n\]*)\r\n" {
+	set vgdbcmd $expect_out(1,string)
+	pass $test
+    }
 }
 
+# Do not kill valgrind.
+set valgrind_spawn_id [board_info host fileid]
+unset gdb_spawn_id
+set board [host_info name]
+unset_board_info fileid
+
+clean_restart $testfile
+
+# 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" ".*"
+
+gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
+
+gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"
+
 set double_free [gdb_get_line_number "double-free"]
 
-set test "attach to debugger"
-gdb_test_multiple "" $test {
-    -re "Invalid free\\(\\).*: main \\(${srcfile}:$double_free\\)\r\n.*---- Attach to debugger \\? --- \[^\r\n\]* ---- " {
-	send_gdb "y\r"
+set test "continue"
+gdb_test_multiple "continue" $test {
+    -re "Invalid free\\(\\).*: main \\(${srcfile}:$double_free\\)\r\n.*$gdb_prompt $" {
+	pass $test
+    }
+    -re "Remote connection closed.*\r\n$gdb_prompt $" {
+	fail "$test (remote connection closed)"
+	# Only if valgrind got stuck.
+	kill_wait_spawned_process $valgrind_spawn_id
+	return -1
     }
-    -re "---- Attach to debugger \\? --- \[^\r\n\]* ---- " {
-	send_gdb "n\r"
-	exp_continue
+    -re "The program is not being run\\.\r\n$gdb_prompt $" {
+	fail "$test (valgrind vgdb has terminated)"
+	# Only if valgrind got stuck.
+	kill_wait_spawned_process $valgrind_spawn_id
+	return -1
+    }
+    -re "\r\n$gdb_prompt $" {
+	pass "$test (false warning)"
     }
 }
 
-gdb_test "" ".*" "eat first prompt"
-
 # Initialization from default_gdb_start.
 gdb_test_no_output "set height 0"
 gdb_test_no_output "set width 0"
@@ -97,3 +124,6 @@ gdb_test "bt" "in main \\(.*\\) at .*${srcfile}:$double_free"
 
 # 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"
+
+# Only if valgrind got stuck.
+kill_wait_spawned_process $valgrind_spawn_id

Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option --db-attach

[Pedro Alves]

On 10/25/2018 11:09 AM, Tom de Vries wrote:
> On 10/24/18 10:50 PM, Philippe Waroquiers wrote:
>> On Wed, 2018-10-24 at 17:29 +0100, Pedro Alves wrote:
>>> But was it replaced with / renamed to something else equivalent,
>>> or the functionality completely eliminated?
>> --db-attach option functionality was removed, as it was not very
>> reliable and had a bunch of limitations e.g. not supporting threads.
>>
>> Instead, the gdbserver embedded in valgrind allows the user debug a process
>> when valgrind reports an error.
>>
>> I have put on my list of things to do to convert valgrind-attach
>> test to use vgdb (but the conversion is probably not trivial).
> 
> The valgrind-db-attach.exp test-case is very similar to the
> valgrind-infcall.exp test-case (using vgdb), so I gave it a try by
> putting the two alongside, and doing a copy/paste/replace.
> 
> OK for trunk

OK with me.  Thanks for doing this!

> (perhaps with a rename to valgrind-bt.{exp,c})?

Yeah, that sounds good.  But I'd do it as follow up, to help
archaeology, by helping git's renaming detection algorithm.

> [ Or perhaps first factor out a vgdb_start/stop or some such from
> valgrind-disp-step.exp and valgrind-infcall.exp, and then use those
> procs in valgrind-db-attach.exp instead? ]
That sounds like a very good idea.  I'd be fine to push the
current patch in as-is, and do that re-factoring on top, though,
if you'd like.

Pedro Alves

[PATCH OB][gdb/testsuite] Move valgrind-db-attach.{c,exp} to valgrind-bt.{c,exp}

[Tom de Vries]

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

[ was: Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option
--db-attach ]
On 10/25/18 2:16 PM, Pedro Alves wrote:
>> (perhaps with a rename to valgrind-bt.{exp,c})?
> Yeah, that sounds good.  But I'd do it as follow up, to help
> archaeology, by helping git's renaming detection algorithm.

Done. Committed as attached.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Move-valgrind-db-attach.-c-exp-to-valgrind-bt.-c-exp.patch --]
[-- Type: text/x-patch, Size: 1072 bytes --]

[gdb/testsuite] Move valgrind-db-attach.{c,exp} to valgrind-bt.{c,exp}

Now that valgrind-db-attach.exp no longer use --db-attach, rename
valgrind-db-attach.{c,exp} to valgrind-bt.{c,exp}.

2018-10-05  Tom de Vries  <tdevries@suse.de>

    * gdb.base/valgrind-db-attach.c: Rename to ...
    * gdb.base/valgrind-bt.c: ... this.
    * gdb.base/valgrind-db-attach.exp: Rename to ...
    * gdb.base/valgrind-bt.exp: ... this.

---
 gdb/testsuite/gdb.base/{valgrind-db-attach.c => valgrind-bt.c}     | 0
 gdb/testsuite/gdb.base/{valgrind-db-attach.exp => valgrind-bt.exp} | 0
 2 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.base/valgrind-db-attach.c b/gdb/testsuite/gdb.base/valgrind-bt.c
similarity index 100%
rename from gdb/testsuite/gdb.base/valgrind-db-attach.c
rename to gdb/testsuite/gdb.base/valgrind-bt.c
diff --git a/gdb/testsuite/gdb.base/valgrind-db-attach.exp b/gdb/testsuite/gdb.base/valgrind-bt.exp
similarity index 100%
rename from gdb/testsuite/gdb.base/valgrind-db-attach.exp
rename to gdb/testsuite/gdb.base/valgrind-bt.exp

[PATCH][gdb/testsuite] Factor out lib/valgrind.exp

[Tom de Vries]

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

[ was: Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option
--db-attach ]

On 10/25/18 2:16 PM, Pedro Alves wrote:
>> [ Or perhaps first factor out a vgdb_start/stop or some such from
>> valgrind-disp-step.exp and valgrind-infcall.exp, and then use those
>> procs in valgrind-db-attach.exp instead? ]
> That sounds like a very good idea. 

How about this?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Factor-out-lib-valgrind.exp.patch --]
[-- Type: text/x-patch, Size: 12745 bytes --]

[gdb/testsuite] Factor out lib/valgrind.exp

Factor out common code related to vgdb setup and cleanup in valgrind-bt.exp,
valgrind-disp-step.exp and gdb.base/valgrind-infcall.exp.

Tested on x86_64-linux with and without --target_board=native-gdbserver.

2018-10-25  Tom de Vries  <tdevries@suse.de>

	* lib/valgrind.exp: New file.
	(vgdb_start, vgdb_stop): New procs, factored out of ...
	* gdb.base/valgrind-bt.exp: ... here, ...
	* gdb.base/valgrind-disp-step.exp: ... here and ...
	* gdb.base/valgrind-infcall.exp: ... here.

---
 gdb/testsuite/gdb.base/valgrind-bt.exp        |  74 +-----------------
 gdb/testsuite/gdb.base/valgrind-disp-step.exp |  74 +-----------------
 gdb/testsuite/gdb.base/valgrind-infcall.exp   |  74 +-----------------
 gdb/testsuite/lib/valgrind.exp                | 103 ++++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 210 deletions(-)

diff --git a/gdb/testsuite/gdb.base/valgrind-bt.exp b/gdb/testsuite/gdb.base/valgrind-bt.exp
index 348f74379d..27d1bbc5da 100644
--- a/gdb/testsuite/gdb.base/valgrind-bt.exp
+++ b/gdb/testsuite/gdb.base/valgrind-bt.exp
@@ -13,6 +13,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+load_lib valgrind.exp
+
 if [is_remote target] {
     # The test always runs locally.
     return 0
@@ -23,74 +25,7 @@ if {[build_executable $testfile.exp $testfile $srcfile {debug}] == -1} {
     return -1
 }
 
-set test "spawn valgrind"
-set cmd "valgrind --vgdb-error=0 $binfile"
-set res [remote_spawn host $cmd]
-if { $res < 0 || $res == "" } {
-    verbose -log "Spawning $cmd failed."
-    unsupported $test
-    return -1
-}
-pass $test
-# Declare GDB now as running.
-set gdb_spawn_id $res
-
-# GDB started by vgdb stops already after the startup is executed, like with
-# non-extended gdbserver.  It is also not correct to run/attach the inferior.
-set use_gdb_stub 1
-
-set test "valgrind started"
-# The trailing '.' differs for different memcheck versions.
-gdb_test_multiple "" $test {
-    -re "Memcheck, a memory error detector\\.?\r\n" {
-	pass $test
-    }
-    -re "valgrind: failed to start tool 'memcheck' for platform '.*': No such file or directory" {
-	unsupported $test
-	return -1
-    }
-    -re "valgrind: wrong ELF executable class" {
-	unsupported $test
-	return -1
-    }
-    -re "command not found" {
-	# The spawn succeeded, but then valgrind was not found - e.g. if
-	# we spawned SSH to a remote system.
-	unsupported $test
-	return -1
-    }
-    -re "valgrind: Bad option.*--vgdb-error=0" {
-	# valgrind is not >= 3.7.0.
-	unsupported $test
-	return -1
-    }
-}
-
-set test "vgdb prompt"
-# The trailing '.' differs for different memcheck versions.
-gdb_test_multiple "" $test {
-    -re "  (target remote | \[^\r\n\]*/vgdb \[^\r\n\]*)\r\n" {
-	set vgdbcmd $expect_out(1,string)
-	pass $test
-    }
-}
-
-# Do not kill valgrind.
-set valgrind_spawn_id [board_info host fileid]
-unset gdb_spawn_id
-set board [host_info name]
-unset_board_info fileid
-
-clean_restart $testfile
-
-# 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" ".*"
-
-gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
-
-gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"
+vgdb_start
 
 set double_free [gdb_get_line_number "double-free"]
 
@@ -125,5 +60,4 @@ gdb_test "bt" "in main \\(.*\\) at .*${srcfile}:$double_free"
 # 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"
 
-# Only if valgrind got stuck.
-kill_wait_spawned_process $valgrind_spawn_id
+vgdb_stop
diff --git a/gdb/testsuite/gdb.base/valgrind-disp-step.exp b/gdb/testsuite/gdb.base/valgrind-disp-step.exp
index 6ce16444cf..bbf495e3dc 100644
--- a/gdb/testsuite/gdb.base/valgrind-disp-step.exp
+++ b/gdb/testsuite/gdb.base/valgrind-disp-step.exp
@@ -18,6 +18,8 @@
 # really tests is that GDB falls back to in-line stepping
 # automatically instead of getting stuck or crashing.
 
+load_lib valgrind.exp
+
 if [is_remote target] {
     # The test always runs locally.
     return 0
@@ -28,74 +30,7 @@ if {[build_executable "failed to build" $testfile $srcfile {debug}] == -1} {
     return -1
 }
 
-set test "spawn valgrind"
-set cmd "valgrind --vgdb-error=0 $binfile"
-set res [remote_spawn host $cmd]
-if { $res < 0 || $res == "" } {
-    verbose -log "Spawning $cmd failed."
-    unsupported $test
-    return -1
-}
-pass $test
-# Declare GDB now as running.
-set gdb_spawn_id $res
-
-# GDB started by vgdb stops already after the startup is executed, like with
-# non-extended gdbserver.  It is also not correct to run/attach the inferior.
-set use_gdb_stub 1
-
-set test "valgrind started"
-# The trailing '.' differs for different memcheck versions.
-gdb_test_multiple "" $test {
-    -re "Memcheck, a memory error detector\\.?\r\n" {
-	pass $test
-    }
-    -re "valgrind: failed to start tool 'memcheck' for platform '.*': No such file or directory" {
-	unsupported $test
-	return -1
-    }
-    -re "valgrind: wrong ELF executable class" {
-	unsupported $test
-	return -1
-    }
-    -re "command not found" {
-	# The spawn succeeded, but then valgrind was not found - e.g. if
-	# we spawned SSH to a remote system.
-	unsupported $test
-	return -1
-    }
-    -re "valgrind: Bad option.*--vgdb-error=0" {
-	# valgrind is not >= 3.7.0.
-	unsupported $test
-	return -1
-    }
-}
-
-set test "vgdb prompt"
-# The trailing '.' differs for different memcheck versions.
-gdb_test_multiple "" $test {
-    -re "  (target remote | \[^\r\n\]*/vgdb \[^\r\n\]*)\r\n" {
-	set vgdbcmd $expect_out(1,string)
-	pass $test
-    }
-}
-
-# Do not kill valgrind.
-set valgrind_pid [exp_pid -i [board_info host fileid]]
-unset gdb_spawn_id
-set board [host_info name]
-unset_board_info fileid
-
-clean_restart $testfile
-
-# 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" ".*"
-
-gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
-
-gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"
+vgdb_start
 
 gdb_test_no_output "set displaced-stepping off"
 gdb_breakpoint "main" "breakpoint at main"
@@ -132,5 +67,4 @@ foreach displaced { "off" "on" } {
     }
 }
 
-# Only if valgrind got stuck.
-remote_exec host "kill -9 ${valgrind_pid}"
+vgdb_stop
diff --git a/gdb/testsuite/gdb.base/valgrind-infcall.exp b/gdb/testsuite/gdb.base/valgrind-infcall.exp
index 0a3774a489..53588e880b 100644
--- a/gdb/testsuite/gdb.base/valgrind-infcall.exp
+++ b/gdb/testsuite/gdb.base/valgrind-infcall.exp
@@ -13,6 +13,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+load_lib valgrind.exp
+
 if [is_remote target] {
     # The test always runs locally.
     return 0
@@ -23,74 +25,7 @@ if {[build_executable $testfile.exp $testfile $srcfile {debug}] == -1} {
     return -1
 }
 
-set test "spawn valgrind"
-set cmd "valgrind --vgdb-error=0 $binfile"
-set res [remote_spawn host $cmd]
-if { $res < 0 || $res == "" } {
-    verbose -log "Spawning $cmd failed."
-    unsupported $test
-    return -1
-}
-pass $test
-# Declare GDB now as running.
-set gdb_spawn_id $res
-
-# GDB started by vgdb stops already after the startup is executed, like with
-# non-extended gdbserver.  It is also not correct to run/attach the inferior.
-set use_gdb_stub 1
-
-set test "valgrind started"
-# The trailing '.' differs for different memcheck versions.
-gdb_test_multiple "" $test {
-    -re "Memcheck, a memory error detector\\.?\r\n" {
-	pass $test
-    }
-    -re "valgrind: failed to start tool 'memcheck' for platform '.*': No such file or directory" {
-	unsupported $test
-	return -1
-    }
-    -re "valgrind: wrong ELF executable class" {
-	unsupported $test
-	return -1
-    }
-    -re "command not found" {
-	# The spawn succeeded, but then valgrind was not found - e.g. if
-	# we spawned SSH to a remote system.
-	unsupported $test
-	return -1
-    }
-    -re "valgrind: Bad option.*--vgdb-error=0" {
-	# valgrind is not >= 3.7.0.
-	unsupported $test
-	return -1
-    }
-}
-
-set test "vgdb prompt"
-# The trailing '.' differs for different memcheck versions.
-gdb_test_multiple "" $test {
-    -re "  (target remote | \[^\r\n\]*/vgdb \[^\r\n\]*)\r\n" {
-	set vgdbcmd $expect_out(1,string)
-	pass $test
-    }
-}
-
-# Do not kill valgrind.
-set valgrind_spawn_id [board_info host fileid]
-unset gdb_spawn_id
-set board [host_info name]
-unset_board_info fileid
-
-clean_restart $testfile
-
-# 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" ".*"
-
-gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
-
-gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"
+vgdb_start
 
 set continue_count 1
 set loop 1
@@ -130,5 +65,4 @@ gdb_test_multiple $test $test {
     }
 }
 
-# Only if valgrind got stuck.
-kill_wait_spawned_process $valgrind_spawn_id
+vgdb_stop
diff --git a/gdb/testsuite/lib/valgrind.exp b/gdb/testsuite/lib/valgrind.exp
new file mode 100644
index 0000000000..bc3754cc06
--- /dev/null
+++ b/gdb/testsuite/lib/valgrind.exp
@@ -0,0 +1,103 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Generic subroutines for handling valgrind vgdb server
+
+#
+# Start a vgdb server, and connect gdb to it.
+#
+proc vgdb_start { } {
+    global binfile use_gdb_stub board testfile
+    global valgrind_spawn_id gdb_spawn_id
+
+    set test "spawn valgrind"
+    set cmd "valgrind --vgdb-error=0 $binfile"
+    set res [remote_spawn host $cmd]
+    if { $res < 0 || $res == "" } {
+	verbose -log "Spawning $cmd failed."
+	unsupported $test
+	return -1
+    }
+    pass $test
+    # Declare GDB now as running.
+    set gdb_spawn_id $res
+
+    # GDB started by vgdb stops already after the startup is executed, like with
+    # non-extended gdbserver.  It is also not correct to run/attach the inferior.
+    set use_gdb_stub 1
+
+    set test "valgrind started"
+    # The trailing '.' differs for different memcheck versions.
+    gdb_test_multiple "" $test {
+	-re "Memcheck, a memory error detector\\.?\r\n" {
+	    pass $test
+	}
+	-re "valgrind: failed to start tool 'memcheck' for platform '.*': No such file or directory" {
+	    unsupported $test
+	    return -1
+	}
+	-re "valgrind: wrong ELF executable class" {
+	    unsupported $test
+	    return -1
+	}
+	-re "command not found" {
+	    # The spawn succeeded, but then valgrind was not found - e.g. if
+	    # we spawned SSH to a remote system.
+	    unsupported $test
+	    return -1
+	}
+	-re "valgrind: Bad option.*--vgdb-error=0" {
+	    # valgrind is not >= 3.7.0.
+	    unsupported $test
+	    return -1
+	}
+    }
+
+    set test "vgdb prompt"
+    # The trailing '.' differs for different memcheck versions.
+    gdb_test_multiple "" $test {
+	-re "  (target remote | \[^\r\n\]*/vgdb \[^\r\n\]*)\r\n" {
+	    set vgdbcmd $expect_out(1,string)
+	    pass $test
+	}
+    }
+
+    # Do not kill valgrind.
+    set valgrind_spawn_id [board_info host fileid]
+    unset gdb_spawn_id
+    set board [host_info name]
+    unset_board_info fileid
+
+    clean_restart $testfile
+
+    # 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" ".*"
+
+    gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
+
+    gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"
+}
+
+#
+# Stop vgdb server
+#
+proc vgdb_stop { } {
+    global valgrind_spawn_id
+
+    # Only if valgrind got stuck.
+    kill_wait_spawned_process $valgrind_spawn_id
+}

Re: [PATCH][gdb/testsuite] Factor out lib/valgrind.exp

[Pedro Alves]

On 10/25/2018 04:08 PM, Tom de Vries wrote:
> [ was: Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option
> --db-attach ]
> 
> On 10/25/18 2:16 PM, Pedro Alves wrote:
>>> [ Or perhaps first factor out a vgdb_start/stop or some such from
>>> valgrind-disp-step.exp and valgrind-infcall.exp, and then use those
>>> procs in valgrind-db-attach.exp instead? ]
>> That sounds like a very good idea. 
> How about this?
> 

Thanks!

> -
> -gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"
> +vgdb_start

These vgdb_start need to handle the case of vgdb_start
returning early on error.

> +++ b/gdb/testsuite/lib/valgrind.exp
> @@ -0,0 +1,103 @@
> +# Copyright 2018 Free Software Foundation, Inc.

This is mostly preexisting code, so please bring forth the
copyright dates of the original files/code.

> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Generic subroutines for handling valgrind vgdb server

Write full sentences.  I.e., add missing period.

> +
> +#
> +# Start a vgdb server, and connect gdb to it.

Please document the proc's return.

> +#
> +proc vgdb_start { } {
> +    global binfile use_gdb_stub board testfile
> +    global valgrind_spawn_id gdb_spawn_id
> +
> +    set test "spawn valgrind"
> +    set cmd "valgrind --vgdb-error=0 $binfile"
> +    set res [remote_spawn host $cmd]
> +    if { $res < 0 || $res == "" } {
> +	verbose -log "Spawning $cmd failed."
> +	unsupported $test
> +	return -1
> +    }
> +    pass $test
> +    # Declare GDB now as running.
> +    set gdb_spawn_id $res
> +
> +    # GDB started by vgdb stops already after the startup is executed, like with
> +    # non-extended gdbserver.  It is also not correct to run/attach the inferior.
> +    set use_gdb_stub 1
> +
> +    set test "valgrind started"
> +    # The trailing '.' differs for different memcheck versions.
> +    gdb_test_multiple "" $test {
> +	-re "Memcheck, a memory error detector\\.?\r\n" {
> +	    pass $test
> +	}
> +	-re "valgrind: failed to start tool 'memcheck' for platform '.*': No such file or directory" {
> +	    unsupported $test
> +	    return -1
> +	}
> +	-re "valgrind: wrong ELF executable class" {
> +	    unsupported $test
> +	    return -1
> +	}
> +	-re "command not found" {
> +	    # The spawn succeeded, but then valgrind was not found - e.g. if
> +	    # we spawned SSH to a remote system.
> +	    unsupported $test
> +	    return -1
> +	}
> +	-re "valgrind: Bad option.*--vgdb-error=0" {
> +	    # valgrind is not >= 3.7.0.
> +	    unsupported $test
> +	    return -1

These return -1's etc. need to be handled by the caller.

> +	}
> +    }
> +
> +    set test "vgdb prompt"
> +    # The trailing '.' differs for different memcheck versions.
> +    gdb_test_multiple "" $test {
> +	-re "  (target remote | \[^\r\n\]*/vgdb \[^\r\n\]*)\r\n" {
> +	    set vgdbcmd $expect_out(1,string)
> +	    pass $test
> +	}
> +    }
> +
> +    # Do not kill valgrind.
> +    set valgrind_spawn_id [board_info host fileid]
> +    unset gdb_spawn_id
> +    set board [host_info name]
> +    unset_board_info fileid
> +
> +    clean_restart $testfile
> +
> +    # 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" ".*"
> +
> +    gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
> +
> +    gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"

Note this is relying on returning what gdb_test returns.

> +}
> +
> +#
> +# Stop vgdb server

Add period.

> +#
> +proc vgdb_stop { } {
> +    global valgrind_spawn_id
> +
> +    # Only if valgrind got stuck.
> +    kill_wait_spawned_process $valgrind_spawn_id
> +}

OK with the above fixed.

Thanks,
Pedro Alves

Re: [PATCH][gdb/testsuite] Factor out lib/valgrind.exp

[Philippe Waroquiers]

Tom,
Thanks for doing all this.
An additional minor comment below ...

Also, for your information, before a new valgrind release,
I am re-running the valgrind gdbserver tests with various
gdb versions, and before a gdb release, I am re-running
these same valgrind tests with this new gdb candidate release.

Philippe


On Thu, 2018-10-25 at 16:25 +0100, Pedro Alves wrote:
> On 10/25/2018 04:08 PM, Tom de Vries wrote:
> > [ was: Re: [OB PATCH][gdb/testsuite] Handle removed valgrind option
> > --db-attach ]
...
> > +
> > +    set test "vgdb prompt"
> > +    # The trailing '.' differs for different memcheck versions.
Spurious 'trailing' comment ?

> > +    gdb_test_multiple "" $test {
> > +	-re "  (target remote | \[^\r\n\]*/vgdb \[^\r\n\]*)\r\n" {
> > +	    set vgdbcmd $expect_out(1,string)
> > +	    pass $test