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

[Pedro Alves <palves@redhat.com>]

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