[RFC] gdb/testsuite: Add framework for running under valgrind

Message ID 20190919042427.589-1-andrew.burgess@embecosm.com
State New
Headers show
Series
  • [RFC] gdb/testsuite: Add framework for running under valgrind
Related show

Commit Message

Andrew Burgess Sept. 19, 2019, 4:24 a.m.
This work isn't ready to be merged yet, but I wanted to share what I
had an get feedback.

Do people think this is something worth having the testsute?

What additonal features would need adding to make this really useful?

All feedback welcome,

Thanks,
Andrew

---

This patch adds a mechanism to run the GDB tests under valgrind.  We
can already achieve this by using the GDB make flag like this:

  make check RUNTESTFLAGS="GDB='valgrind --tool=memcheck $PWD/gdb'"

However, I think we can make life easier for the users so that we
automatically store the valgrind log files into the tests output
directory, with one log per run of GDB.  We can supply a suppression
file to valgrind to hide warnings from the guile garbage collector,
and by wrapping the up the developer no longer has to remember which
valgrind flags are needed.

One additional bonus is that by formalising where the valgrind logs
are placed we can summarise the results into a single valgrind.sum
file.

This patch introduces this mechanism.  The user can now do this:

  make check VALGRIND=1

to run the tests under valgrind.  There are two additional flags,
VALGRIND_COMMAND, which specifies a path to a valgrind binary (by
default the testsuite will just find valgrind on your $PATH), and
VALGRIND_EXTRA_ARGS, which takes a set of additional flags to pass to
valgrind, for example:

  make check VALGRIND=1 VALGRIND_COMMAND=/path/to/valgrind \
             VALGRIND_EXTRA_ARGS="--leak-check=full"

Each valgrind log file is stored into the output file as
'valgrind.out', 'valgrind.out.1', 'valgrind.out.2', etc. These
correspond to the 'gdb.cmd', 'gdb.cmd.1', 'gdb.cmd.2', etc files that
are created to track the GDB command lines.

At the end of each valgrind log is a line like this:

  ==25253== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 18125 from 133)

The testsuite makefile create a file called valgrind.sum, located
here:

  build/gdb/testsuite/valgrind.sum

With one line per test script, with output like this:

  gdb.btrace/dlopen.exp: 1 errors from 1 contexts
  gdb.btrace/enable.exp: 4 errors from 4 contexts
  gdb.btrace/enable-running.exp: 2 errors from 2 contexts
  gdb.btrace/exception.exp: 1 errors from 1 contexts
  gdb.btrace/function_call_history.exp: 2 errors from 2 contexts

The errors and contexts counts are extracted from each tests
valgrind.log files.  As any one test script can run GDB many times,
thus creating many valgrind.out files, the line presented in the
summary is the total number of errors and contexts over all valgrind
log files.

One current limitation is that the valgrind.sum file will find all
valgrind.log files, even if that test has not just been run - so stale
valgrind.log files will be found too.  This can be improved in the
future.  Additionally the summary file doesn't include a grand total
over all results yet.

gdb/ChangeLog:

	* contrib/gather-valgrind-results.sh: New file.

gdb/testsuite/ChangeLog:

	* Makefile.in: New control variables for valgrind testing.  Update
	'make check' related rules.
	* README: Add section on using valgrind.
	* lib/gdb.exp (default_gdb_spawn): Add valgrind wrapper.when
	needed.
	* valgrind-gdb.supp: New file.
---
 gdb/ChangeLog                          |   4 +
 gdb/contrib/gather-valgrind-results.sh | 215 +++++++++++++++++++++++++++++++++
 gdb/testsuite/ChangeLog                |   9 ++
 gdb/testsuite/Makefile.in              |  21 +++-
 gdb/testsuite/README                   |  53 ++++++++
 gdb/testsuite/lib/gdb.exp              |  38 +++++-
 gdb/testsuite/valgrind-gdb.supp        |  13 ++
 7 files changed, 348 insertions(+), 5 deletions(-)
 create mode 100755 gdb/contrib/gather-valgrind-results.sh
 create mode 100644 gdb/testsuite/valgrind-gdb.supp

-- 
2.14.5

Comments

Philippe Waroquiers Sept. 19, 2019, 7:15 p.m. | #1
On Thu, 2019-09-19 at 00:24 -0400, Andrew Burgess wrote:
> This work isn't ready to be merged yet, but I wanted to share what I

> had an get feedback.

> 

> Do people think this is something worth having the testsute?

I think this is a good idea.

> 

> What additonal features would need adding to make this really useful?

I have also done a setup to run GDB tests under valgrind,
with some differences in the approach:
As I understand, with the below, the valgrind output and the GDB
expect input/output are in different files.
The setup I am using puts the valgrind and gdb expect input/output
in the same file.  This can make it easier to associate an error
with the GDB command giving the problem.

The suppression file I am using suppresses the GC errors
using several supp entries mentioning only the leaf function.
It has also a bunch of supp entries for other problems.
The gdb_valgrind wrapper also loads python supp entries
(hardcoded path for now).


I am attaching the patches for the above (not ready
to merge, e.g. contains some hard-coded paths).


Thanks
Philippe


> 

> All feedback welcome,

> 

> Thanks,

> Andrew

> 

> ---

> 

> This patch adds a mechanism to run the GDB tests under valgrind.  We

> can already achieve this by using the GDB make flag like this:

> 

>   make check RUNTESTFLAGS="GDB='valgrind --tool=memcheck $PWD/gdb'"

> 

> However, I think we can make life easier for the users so that we

> automatically store the valgrind log files into the tests output

> directory, with one log per run of GDB.  We can supply a suppression

> file to valgrind to hide warnings from the guile garbage collector,

> and by wrapping the up the developer no longer has to remember which

> valgrind flags are needed.

> 

> One additional bonus is that by formalising where the valgrind logs

> are placed we can summarise the results into a single valgrind.sum

> file.

> 

> This patch introduces this mechanism.  The user can now do this:

> 

>   make check VALGRIND=1

> 

> to run the tests under valgrind.  There are two additional flags,

> VALGRIND_COMMAND, which specifies a path to a valgrind binary (by

> default the testsuite will just find valgrind on your $PATH), and

> VALGRIND_EXTRA_ARGS, which takes a set of additional flags to pass to

> valgrind, for example:

> 

>   make check VALGRIND=1 VALGRIND_COMMAND=/path/to/valgrind \

>              VALGRIND_EXTRA_ARGS="--leak-check=full"

> 

> Each valgrind log file is stored into the output file as

> 'valgrind.out', 'valgrind.out.1', 'valgrind.out.2', etc. These

> correspond to the 'gdb.cmd', 'gdb.cmd.1', 'gdb.cmd.2', etc files that

> are created to track the GDB command lines.

> 

> At the end of each valgrind log is a line like this:

> 

>   ==25253== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 18125 from 133)

> 

> The testsuite makefile create a file called valgrind.sum, located

> here:

> 

>   build/gdb/testsuite/valgrind.sum

> 

> With one line per test script, with output like this:

> 

>   gdb.btrace/dlopen.exp: 1 errors from 1 contexts

>   gdb.btrace/enable.exp: 4 errors from 4 contexts

>   gdb.btrace/enable-running.exp: 2 errors from 2 contexts

>   gdb.btrace/exception.exp: 1 errors from 1 contexts

>   gdb.btrace/function_call_history.exp: 2 errors from 2 contexts

> 

> The errors and contexts counts are extracted from each tests

> valgrind.log files.  As any one test script can run GDB many times,

> thus creating many valgrind.out files, the line presented in the

> summary is the total number of errors and contexts over all valgrind

> log files.

> 

> One current limitation is that the valgrind.sum file will find all

> valgrind.log files, even if that test has not just been run - so stale

> valgrind.log files will be found too.  This can be improved in the

> future.  Additionally the summary file doesn't include a grand total

> over all results yet.

> 

> gdb/ChangeLog:

> 

> 	* contrib/gather-valgrind-results.sh: New file.

> 

> gdb/testsuite/ChangeLog:

> 

> 	* Makefile.in: New control variables for valgrind testing.  Update

> 	'make check' related rules.

> 	* README: Add section on using valgrind.

> 	* lib/gdb.exp (default_gdb_spawn): Add valgrind wrapper.when

> 	needed.

> 	* valgrind-gdb.supp: New file.

> ---

>  gdb/ChangeLog                          |   4 +

>  gdb/contrib/gather-valgrind-results.sh | 215 +++++++++++++++++++++++++++++++++

>  gdb/testsuite/ChangeLog                |   9 ++

>  gdb/testsuite/Makefile.in              |  21 +++-

>  gdb/testsuite/README                   |  53 ++++++++

>  gdb/testsuite/lib/gdb.exp              |  38 +++++-

>  gdb/testsuite/valgrind-gdb.supp        |  13 ++

>  7 files changed, 348 insertions(+), 5 deletions(-)

>  create mode 100755 gdb/contrib/gather-valgrind-results.sh

>  create mode 100644 gdb/testsuite/valgrind-gdb.supp

> 

> diff --git a/gdb/contrib/gather-valgrind-results.sh b/gdb/contrib/gather-valgrind-results.sh

> new file mode 100755

> index 00000000000..9cbce030b16

> --- /dev/null

> +++ b/gdb/contrib/gather-valgrind-results.sh

> @@ -0,0 +1,215 @@

> +#! /bin/bash

> +

> +# Copyright (C) 2019 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/>;.

> +

> +# This script should be used by the testing framework once the tests

> +# have been run under valgrind.  This script looks for all of the

> +# valgrind.out files that have been generated and gathers the error

> +# summaries into a single file, one line per test script, like this:

> +#

> +#   gdb.xxx/example.exp: 10 errors from 5 contexts

> +#

> +# Note that a single test script might invoke GDB multiple times,

> +# resulting in multiple valgrind.out files for that test script.  In

> +# this case script will merge the results into a single line.  So the

> +# output directory for the above might have looked like this:

> +#

> +#   gdb/testsuite/outputs/gdb.xxx/example/valgrind.out

> +#   gdb/testsuite/outputs/gdb.xxx/example/valgrind.out.1

> +#   gdb/testsuite/outputs/gdb.xxx/example/valgrind.out.2

> +#

> +# The error summaries from all three of these files have been merged

> +# to give the total of 10 errors from 5 contexts.

> +

> +# ------------------------------------------------------------------#

> +

> +# This script should be passed the path to the outputs directory.

> +

> +outputs_dir=$1

> +

> +# Takes a single string argument.  Format the string as an error and

> +# exit.

> +function error () {

> +    echo "ERROR: $1"

> +    exit 1

> +}

> +

> +# Takes the absolute path to a 'gdb.cmd' file as produced by the GDB

> +# testsuite (including files with a version number, for example

> +# 'gdb.cmd.1') and prints a string that is the name of a script that

> +# was probably run to result in that command file.

> +#

> +# The script name (for example 'gdb.blah/example.exp') has to be

> +# guessed from the path to the command file as it is not recorded

> +# anywhere.

> +function cmd_file_to_testname () {

> +    local filename=$(dirname "$1")

> +    local p1=$(basename "$filename")

> +    filename=$(dirname "$filename")

> +    local p2=$(basename "$filename")

> +    echo "$p2/$p1.exp"

> +}

> +

> +# Takes the absolute path to a 'gdb.cmd' file as produced by the GDB

> +# testsuite (including files with a version number, for example

> +# 'gdb.cmd.1') and prints a string that is the absolute path for where

> +# we expect to find the corresponding 'valgrind.out' file.

> +function cmd_file_to_valgrind_file () {

> +    local filename=$1

> +    filename=$(echo $filename | sed -e 's/gdb.cmd/valgrind.out/')

> +    echo $filename

> +}

> +

> +# Takes the absolute path to a 'valgrind.out' file and produces a

> +# short summary of the results for that file.  The summary could be

> +# something like:

> +#

> +#    gdb.blah/example.exp:BAD

> +#

> +# If the valgrind output file is missing or corrupted.  If however the

> +# valgrind output file is as expected we'll get something like:

> +#

> +#    gdb.blah/example.exp:4;2

> +#

> +# The first number is the number of errors, and the second is the

> +# number of contexts from which the errors came.  This is extracted

> +# from valgrinds "ERROR SUMMARY" line in its output file.

> +function extract_valgrind_results () {

> +    local gdb_cmd_file=$1

> +

> +    local valgrind_file=$(cmd_file_to_valgrind_file "${gdb_cmd_file}")

> +    if [ ! "${valgrind_file}" -ot "${gdb_cmd_file}" ]

> +    then

> +        line=$(grep -m 1 "ERROR SUMMARY: " "${valgrind_file}")

> +        if [ -z "${line}" ]

> +        then

> +            echo "$testname:BAD"

> +        else

> +            line=$(echo $line \ | \

> +                       sed -e 's/.*ERROR SUMMARY: \([0-9]\+\) errors from \([0-9]\+\) contexts .*/\1;\2/')

> +            echo "$testname:$line"

> +        fi

> +    else

> +        echo "$testname:BAD"

> +    fi

> +}

> +

> +# This takes an absolute path to the outputs directory in the build

> +# tree, which is located somewhere like:

> +#

> +#    /projects/binutils-gdb/build/gdb/testsuite/outputs/

> +#

> +# And produces a list of summary results, one per valgrind output file

> +# found below the outputs directory.

> +function extract_raw_errors () {

> +    local output_dir=$1

> +

> +    while IFS= read -r cmd_file

> +    do

> +        gdb_cmd=$(cut -d' ' -f1 ${cmd_file})

> +

> +        re=\\bvalgrind\$

> +        if [[ "${gdb_cmd}" =~ $re ]]

> +        then

> +            # This GDB was run under valgrind, we expect to find a

> +            # valgrind.out file.  Failure to find one indicates

> +            # something went wrong.

> +            #

> +            # But first, lets figure out the name for this test.

> +            testname=$(cmd_file_to_testname "${cmd_file}")

> +

> +            # Now given the name of the GDB command file, find the

> +            # corresponding valgrind output file and extract the

> +            # results from it, and print them on one line.

> +            extract_valgrind_results "${cmd_file}"

> +        fi

> +    done < <(find "${outputs_dir}" -name "gdb.cmd*")

> +}

> +

> +# Some global state.

> +last_name=""

> +errors=0

> +contexts=0

> +is_bad=0

> +

> +# Takes a string that is the value part of a test summary line, this

> +# will either be the string "BAD" (without quotes), or two numbers

> +# like "4;2" (again without quotes).

> +#

> +# If the value is BAD, then the global state is updated to reflect

> +# this, while if the value is two numbers then these values are added

> +# into the global state.

> +function add_result () {

> +    local value=$1

> +

> +    # The following updates the global state.

> +    if [ $value == "BAD" ]

> +    then

> +        errors=0

> +        contexts=0

> +        is_bad=1

> +    elif [ $is_bad == 0 ]

> +    then

> +        e=$(echo $value | cut -d';' -f1)

> +        c=$(echo $value | cut -d';' -f2)

> +        errors=$((errors + e))

> +        contexts=$((contexts + c))

> +    fi

> +}

> +

> +# Print a line representing the current global result state.

> +function print_result () {

> +    if [ $is_bad == 1 ]

> +    then

> +        echo "${last_name}: Valgrind output missing or corrupted"

> +    else

> +        echo "${last_name}: ${errors} errors from $contexts contexts"

> +    fi

> +}

> +

> +# Check we have a sane outputs directory.

> +[[ -n "${outputs_dir}" && -d "${outputs_dir}" ]] \

> +    || error "invalid outputs directory '${outputs_dir}'"

> +

> +# Loop over all sorted summary lines and merge together all results

> +# for the same test file, then print the results.  Don't forget to

> +# print the last result after the loop.

> +while IFS= read -r result

> +do

> +    name=$(echo $result | cut -d: -f1)

> +    value=$(echo $result | cut -d: -f2)

> +

> +    if [ "$name" == "$last_name" ]

> +    then

> +        # Add this result to the last one.

> +        add_result $value

> +    else

> +        if [ -n "$last_name" ]

> +        then

> +            print_result

> +        fi

> +

> +        last_name=$name

> +        errors=0

> +        contexts=0

> +        is_bad=0

> +

> +        add_result $value

> +    fi

> +done < <(extract_raw_errors "$outputs_dir" | sort)

> +if [ -n "$last_name" ]

> +then

> +    print_result

> +fi

> diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in

> index 2beba053ee6..86aaf0234d3 100644

> --- a/gdb/testsuite/Makefile.in

> +++ b/gdb/testsuite/Makefile.in

> @@ -168,6 +168,18 @@ TIMESTAMP = $(if $(TS),| $(srcdir)/print-ts.py $(if $(TS_FORMAT),$(TS_FORMAT),),

>  gdb_debug = $(if $(GDB_DEBUG),GDB_DEBUG=$(GDB_DEBUG) ; export GDB_DEBUG ;,)

>  gdbserver_debug = $(if $(GDBSERVER_DEBUG),GDBSERVER_DEBUG=$(GDBSERVER_DEBUG) ; export GDBSERVER_DEBUG ;,)

>  

> +# Setup flags needed to activate the valgrind testing in the testsuite.

> +ifeq ($(VALGRIND),1)

> +  valgrind_var = VALGRIND=1 ; export VALGRIND ;

> +  valgrind_command_var = $(if $(VALGRIND_COMMAND),VALGRIND_COMMAND=$(VALGRIND_COMMAND) ; export VALGRIND_COMMAND ;,)

> +  valgrind_extra_args_var = $(if $(VALGRIND_EXTRA_ARGS),VALGRIND_EXTRA_ARGS=$(VALGRIND_EXTRA_ARGS) ; export VALGRIND_EXTRA_ARGS ;,)

> +  valgrind_build_sum = $(srcdir)/../contrib/gather-valgrind-results.sh $(PWD)/outputs/ > valgrind.sum

> +else

> +  valgrind_var =

> +  valgrind_command_var =

> +  valgrind_extra_args_var =

> +  valgrind_build_sum = true

> +endif

>  

>  # All the hair to invoke dejagnu.  A given invocation can just append

>  # $(RUNTESTFLAGS)

> @@ -176,6 +188,7 @@ DO_RUNTEST = \

>  	srcdir=${srcdir} ; export srcdir ; \

>  	EXPECT=${EXPECT} ; export EXPECT ; \

>  	EXEEXT=${EXEEXT} ; export EXEEXT ;  $(gdb_debug) $(gdbserver_debug) \

> +	$(valgrind_var) $(valgrind_command_var) $(valgrind_extra_args_var) \

>          $(RPATH_ENVVAR)=$$rootme/../../expect:$$rootme/../../libstdc++:$$rootme/../../tk/unix:$$rootme/../../tcl/unix:$$rootme/../../bfd:$$rootme/../../opcodes:$$$(RPATH_ENVVAR); \

>  	export $(RPATH_ENVVAR); \

>  	if [ -f $${rootme}/../../expect/expect ] ; then  \

> @@ -203,7 +216,10 @@ check-gdb.%:

>  	$(MAKE) check TESTS="gdb.$*/*.exp"

>  

>  check-single:

> -	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP)

> +	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP) ; \

> +	result=$$?; \

> +	$(valgrind_build_sum); \

> +	exit $$result

>  

>  check-single-racy:

>  	-rm -rf cache racy_outputs temp

> @@ -221,6 +237,7 @@ check-single-racy:

>  	  $(DO_RUNTEST) --outdir=racy_outputs/$$n $(RUNTESTFLAGS) \

>  	    $(expanded_tests_or_none) $(TIMESTAMP); \

>  	done; \

> +	$(valgrind_build_sum); \

>  	$(srcdir)/analyze-racy-logs.py \

>  	  `ls racy_outputs/*/gdb.sum` > racy.sum; \

>  	sed -n '/=== gdb Summary ===/,$$ p' racy.sum

> @@ -229,6 +246,7 @@ check-parallel:

>  	-rm -rf cache outputs temp

>  	$(MAKE) -k do-check-parallel; \

>  	result=$$?; \

> +	$(valgrind_build_sum); \

>  	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh \

>  	  `find outputs -name gdb.sum -print` > gdb.sum; \

>  	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \

> @@ -257,6 +275,7 @@ check-parallel-racy:

>  	    racy_outputs/$$n/gdb.log; \

>  	  sed -n '/=== gdb Summary ===/,$$ p' racy_outputs/$$n/gdb.sum; \

>  	done; \

> +	$(valgrind_build_sum); \

>  	$(srcdir)/analyze-racy-logs.py \

>  	  `ls racy_outputs/*/gdb.sum` > racy.sum; \

>  	sed -n '/=== gdb Summary ===/,$$ p' racy.sum

> diff --git a/gdb/testsuite/README b/gdb/testsuite/README

> index 4795df1f759..03d43f936f8 100644

> --- a/gdb/testsuite/README

> +++ b/gdb/testsuite/README

> @@ -139,6 +139,59 @@ tests" respectively.  "both" is the default.  GDB_PERFTEST_TIMEOUT

>  specify the timeout, which is 3000 in default.  The result of

>  performance test is appended in `testsuite/perftest.log'.

>  

> +Running Tests Under Valgrind

> +****************************

> +

> +GDB Testsuite includes features to enable easy testing under the

> +valgrind memory checking tool.  The simplest way to get started is to

> +pass VALGRIND=1 as part of the make check command line, like this:

> +

> +	make check VALGRIND=1

> +

> +This will run GDB under valgrind.  Each invokation of GDB will create

> +a new output file in the testsuite outputs directory, the first output

> +file will be valgrind.out, then valgrind.out.1, valgrind.out.2, etc.

> +The corresponding gdb.cmd file will be updated to include the valgrind

> +command line details (which includes the GDB command run under

> +valgrind).

> +

> +By default GDB just runs 'valgrind' and relies on finding a suitable

> +binary in your $PATH.  You can override this behaviour by passing the

> +VALGRIND_COMMAND flag make, like this:

> +

> +	make check VALGRIND=1 VALGRIND_COMMAND=/path/to/valgrind

> +

> +It is possible to add extra command line flags to valgrind like this:

> +

> +	make check VALGRIND=1 VALGRIND_EXTRA_ARGS="--leak-check=full"

> +

> +The default command line arguments used for valgrind are:

> +

> +	valgrind --tool=memcheck \

> +                 --log-file=/path/to/log \

> +                 --suppressions=/path/to/file

> +

> +Any extra flags provided by the user are appended to the valgrind

> +command line.

> +

> +Notice that by default a suppressions file is passed to valgrind, this

> +is because there are a large number of errors from the guile garbage

> +collector that make the results very noisy, the suppressions file

> +hides these errors.  The default suppressions file can be found in the

> +testsuite source directory here:

> +

> +	binutils-gdb/gdb/testsuite/valgrind-gdb.supp

> +

> +Finally, once a test run has completed a summary of the valgrind

> +results will be created into a file:

> +

> +	build/gdb/testsuite/valgrind.sum

> +

> +This summary file lists for each test script the number of valgrind

> +errors found.  This would be useful for comparing between two

> +different test runs and quickly spotting if any extra errors have

> +appeared.

> +

>  Testsuite Parameters

>  ********************

>  

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp

> index acbeb013768..008862ede24 100644

> --- a/gdb/testsuite/lib/gdb.exp

> +++ b/gdb/testsuite/lib/gdb.exp

> @@ -1695,9 +1695,39 @@ proc default_gdb_spawn { } {

>      global GDB

>      global INTERNAL_GDBFLAGS GDBFLAGS

>      global gdb_spawn_id

> +    global srcdir

>  

>      gdb_stop_suppressing_tests

>  

> +    set gdb_cmd $GDB

> +    if { [info exists ::env(VALGRIND)] == 1 && $::env(VALGRIND) == 1 } {

> +	set log_file \

> +	    [standard_output_file_with_gdb_instance valgrind.out]

> +	set sfile "$srcdir/valgrind-gdb.supp"

> +

> +	set valgrind_args "--tool=memcheck --log-file=${log_file}"

> +	if [file exists "${sfile}"] {

> +	    set valgrind_args "$valgrind_args --suppressions=${sfile}"

> +	}

> +	if { [info exists ::env(VALGRIND_EXTRA_ARGS)] == 1 } {

> +	    set valgrind_args "$valgrind_args $::env(VALGRIND_EXTRA_ARGS)"

> +	}

> +

> +	set valgrind_exe "valgrind"

> +	if { [info exists ::env(VALGRIND_COMMAND)] == 1 } {

> +	    set valgrind_exe $::env(VALGRIND_COMMAND)

> +	}

> +

> +	if ![is_remote host] {

> +	    if { [which $valgrind_exe] == 0 } then {

> +		perror "$valgrind_exe does not exist."

> +		exit 1

> +	    }

> +	}

> +

> +	set gdb_cmd "${valgrind_exe} ${valgrind_args} ${GDB}"

> +    }

> +

>      # Set the default value, it may be overriden later by specific testfile.

>      #

>      # Use `set_board_info use_gdb_stub' for the board file to flag the inferior

> @@ -1707,8 +1737,8 @@ proc default_gdb_spawn { } {

>      # a specific different target protocol itself.

>      set use_gdb_stub [target_info exists use_gdb_stub]

>  

> -    verbose "Spawning $GDB $INTERNAL_GDBFLAGS $GDBFLAGS"

> -    gdb_write_cmd_file "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS"

> +    verbose "Spawning $gdb_cmd $INTERNAL_GDBFLAGS $GDBFLAGS"

> +    gdb_write_cmd_file "$gdb_cmd $INTERNAL_GDBFLAGS $GDBFLAGS"

>  

>      if [info exists gdb_spawn_id] {

>  	return 0

> @@ -1720,9 +1750,9 @@ proc default_gdb_spawn { } {

>  	    exit 1

>  	}

>      }

> -    set res [remote_spawn host "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]"]

> +    set res [remote_spawn host "$gdb_cmd $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]"]

>      if { $res < 0 || $res == "" } {

> -	perror "Spawning $GDB failed."

> +	perror "Spawning $gdb_cmd failed."

>  	return 1

>      }

>  

> diff --git a/gdb/testsuite/valgrind-gdb.supp b/gdb/testsuite/valgrind-gdb.supp

> new file mode 100644

> index 00000000000..f2a25eb0117

> --- /dev/null

> +++ b/gdb/testsuite/valgrind-gdb.supp

> @@ -0,0 +1,13 @@

> +{

> +  <guile_garbage_collection_1>

> +  Memcheck:Cond

> +  ...

> +  obj:*libgc*

> +}

> +

> +{

> +  <guile_garbage_collection_2>

> +  Memcheck:Value8

> +  ...

> +  obj:*libgc*

> +}
From 8ccd3c5a2cd7a959cced1eb5a6f2558329538e76 Mon Sep 17 00:00:00 2001
From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
Date: Thu, 3 Jan 2019 16:46:35 +0100
Subject: [PATCH 1/4] Do a proper mi_gdb_exit in some gdb.mi tests.

Some tests are missing a mi_gdb_exit at the end of the test, which means that
the 'normal non-mi gdb_exit' is done automatically at the end of the test.

Many tests that are executing a test several times with
different parameters, the call to mi_gdb_exit is done at the start
of the next test, which means that the possible errors/messages produced
during mi_gdb_exit are referencing the parameters of the new test,
rather than showing that the mi_gdb_exit was done to exit gdb in
the previous parameters.

This gives timeouts/problems when running under valgrind : a 'clean quit'
is needed to capture valgrind output.

gdb/testsuite/ChangeLog
2019-01-03 Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* gdb.mi/mi-break.exp: Call mi_gdb_exit after a test.
	gdb.mi/mi-exec-run.exp: Likewise.
	gdb.mi/mi-watch.exp: Likewise.
	gdb.mi/new-ui-mi-sync.exp: Likewise.
	gdb.mi/user-selected-context-sync.exp: Likewise.
---
 gdb/testsuite/gdb.mi/mi-break.exp             |  4 +--
 gdb/testsuite/gdb.mi/mi-exec-run.exp          |  4 +--
 gdb/testsuite/gdb.mi/mi-watch.exp             |  4 +--
 gdb/testsuite/gdb.mi/new-ui-mi-sync.exp       |  4 +--
 .../gdb.mi/user-selected-context-sync.exp     |  4 +--
 gdb/testsuite/lib/gdb.exp                     | 26 +++++++++++++++++++
 6 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index c517ce886f..bfbb3d9961 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -398,8 +398,6 @@ proc test_explicit_breakpoints {} {
 proc test_break {mi_mode} {
     global srcdir subdir binfile
 
-    mi_gdb_exit
-
     if {$mi_mode == "separate"} {
 	set start_ops "separate-mi-tty"
     } else {
@@ -427,6 +425,8 @@ proc test_break {mi_mode} {
     test_abreak_creation
 
     test_explicit_breakpoints
+
+    mi_gdb_exit
 }
 
 if [gdb_debug_enabled] {
diff --git a/gdb/testsuite/gdb.mi/mi-exec-run.exp b/gdb/testsuite/gdb.mi/mi-exec-run.exp
index eb4e00d881..6cb8810733 100644
--- a/gdb/testsuite/gdb.mi/mi-exec-run.exp
+++ b/gdb/testsuite/gdb.mi/mi-exec-run.exp
@@ -50,8 +50,6 @@ proc test {inftty_mode mi_mode force_fail} {
     global gdb_spawn_id gdb_main_spawn_id mi_spawn_id inferior_spawn_id
     global decimal
 
-    mi_gdb_exit
-
     set start_ops {}
     if {$inftty_mode == "separate"} {
 	lappend start_ops "separate-inferior-tty"
@@ -142,6 +140,8 @@ proc test {inftty_mode mi_mode force_fail} {
 	    switch_gdb_spawn_id $mi_spawn_id
 	}
     }
+
+    mi_gdb_exit
 }
 
 # Create a not-executable copy of the program, in order to exercise
diff --git a/gdb/testsuite/gdb.mi/mi-watch.exp b/gdb/testsuite/gdb.mi/mi-watch.exp
index 23cc178d71..7dea2f735f 100644
--- a/gdb/testsuite/gdb.mi/mi-watch.exp
+++ b/gdb/testsuite/gdb.mi/mi-watch.exp
@@ -143,8 +143,6 @@ proc test_watchpoint_all {mi_mode type} {
 	return
     }
 
-    mi_gdb_exit
-
     if {$mi_mode == "separate"} {
 	set start_ops "separate-mi-tty"
     } else {
@@ -172,6 +170,8 @@ proc test_watchpoint_all {mi_mode type} {
     #test_rwatch_creation_and_listing
     #test_awatch_creation_and_listing
     test_watchpoint_triggering
+
+    mi_gdb_exit
 }
 
 if [gdb_debug_enabled] {
diff --git a/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp b/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp
index 5560a8be96..d019007394 100644
--- a/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp
+++ b/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp
@@ -43,8 +43,6 @@ proc do_test {sync_command} {
     global gdb_spawn_id gdb_main_spawn_id mi_spawn_id inferior_spawn_id
     global gdb_prompt mi_gdb_prompt
 
-    mi_gdb_exit
-
     if {[mi_gdb_start "separate-mi-tty"] != 0} {
 	fail "could not start gdb"
 	return
@@ -113,6 +111,8 @@ proc do_test {sync_command} {
 	mi_gdb_test "" "456\\^.*state=\"stopped\".*" \
 	    "got -thread-info output and thread is stopped"
     }
+
+    mi_gdb_exit
 }
 
 foreach_with_prefix sync-command {"run" "continue"} {
diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
index 621b4c5163..992c67ab50 100644
--- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
+++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
@@ -385,8 +385,6 @@ proc_with_prefix test_setup { mode } {
 
     set any "\[^\r\n\]*"
 
-    mi_gdb_exit
-
     save_vars { GDBFLAGS } {
 	if { $mode == "non-stop" } {
 	    set GDBFLAGS [concat $GDBFLAGS " -ex \"set non-stop 1\""]
@@ -1277,4 +1275,6 @@ foreach_with_prefix mode { "all-stop" "non-stop" } {
 	test_cli_in_mi_thread $mode $exec_mode
 	test_cli_in_mi_frame $mode $exec_mode
     }
+
+    mi_gdb_exit
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 3a1f053cf8..8bf3a7b461 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1523,6 +1523,32 @@ proc gdb_reinitialize_dir { subdir } {
     }
 }
 
+#
+# show msg + a time stamp.
+#
+proc timed_log { msg } {
+    global verbose
+    # Set vvv to 0 if you want to always see such timed logs.
+    set vvv 3
+    if { $verbose >= $vvv } {
+	verbose "$msg [clock format [clock seconds] -format %H:%M:%S]" $vvv
+    }
+}
+
+#
+# There is a quirk encountered in dejagnu/expect/tcl when doing a
+# 'clean GDB quit': when a GDB has created detached inferiors,
+# after GDB has exit (and is defunct), the 'gdb_expect' in
+# default_gdb_exit does not see the GDB 'eof' till the exit of the
+# detached children.  If a child stays alive a long time, then a test
+# can take much longer and/or cause time outs in the default_gdb_exit.
+# The reason for this 'eof wait time' is that the children are keeping
+# the pty opened.
+# So, for tests that are launching such detached processes, either they
+# must be killed before exiting GDB, or the test program must close
+# fd 0/1/2, to ensure the child has closed the pty.
+# See e.g. gdb.base/pie-fork.exp and pie-fork.c.
+
 #
 # gdb_exit -- exit the GDB, killing the target program if necessary
 #
Simon Marchi Sept. 20, 2019, 2:03 a.m. | #2
On 2019-09-19 12:24 a.m., Andrew Burgess wrote:
> This work isn't ready to be merged yet, but I wanted to share what I

> had an get feedback.

> 

> Do people think this is something worth having the testsute?

> 

> What additonal features would need adding to make this really useful?

> 

> All feedback welcome,

> 

> Thanks,

> Andrew


Hi Andrew,

I think this is great.  I don't really mind how it's implemented under the hood,
hopefully Philippe and you can agree on which is the best solution.  I guess this
reporting is more for reporting leaks than invalid memory accesses, since invalid
memory accesses under valgrind cause the process to die, so you'd probably see it
as a test failure anyway.

A future improvement could also be to run gdbserver under valgrind when using
a board file that's gdbserver-based.

About leaks reported by valgrind, I am always wondering what we should do with allocations
that are made once for the whole lifetime of GDB.  Should we try to free them before exiting?
It might not be necessary, but an argument for free'ing them would be that they add noise to
the valgrind output, and may hide some more serious leaks.

Simon

Patch

diff --git a/gdb/contrib/gather-valgrind-results.sh b/gdb/contrib/gather-valgrind-results.sh
new file mode 100755
index 00000000000..9cbce030b16
--- /dev/null
+++ b/gdb/contrib/gather-valgrind-results.sh
@@ -0,0 +1,215 @@ 
+#! /bin/bash
+
+# Copyright (C) 2019 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/>.
+
+# This script should be used by the testing framework once the tests
+# have been run under valgrind.  This script looks for all of the
+# valgrind.out files that have been generated and gathers the error
+# summaries into a single file, one line per test script, like this:
+#
+#   gdb.xxx/example.exp: 10 errors from 5 contexts
+#
+# Note that a single test script might invoke GDB multiple times,
+# resulting in multiple valgrind.out files for that test script.  In
+# this case script will merge the results into a single line.  So the
+# output directory for the above might have looked like this:
+#
+#   gdb/testsuite/outputs/gdb.xxx/example/valgrind.out
+#   gdb/testsuite/outputs/gdb.xxx/example/valgrind.out.1
+#   gdb/testsuite/outputs/gdb.xxx/example/valgrind.out.2
+#
+# The error summaries from all three of these files have been merged
+# to give the total of 10 errors from 5 contexts.
+
+# ------------------------------------------------------------------#
+
+# This script should be passed the path to the outputs directory.
+
+outputs_dir=$1
+
+# Takes a single string argument.  Format the string as an error and
+# exit.
+function error () {
+    echo "ERROR: $1"
+    exit 1
+}
+
+# Takes the absolute path to a 'gdb.cmd' file as produced by the GDB
+# testsuite (including files with a version number, for example
+# 'gdb.cmd.1') and prints a string that is the name of a script that
+# was probably run to result in that command file.
+#
+# The script name (for example 'gdb.blah/example.exp') has to be
+# guessed from the path to the command file as it is not recorded
+# anywhere.
+function cmd_file_to_testname () {
+    local filename=$(dirname "$1")
+    local p1=$(basename "$filename")
+    filename=$(dirname "$filename")
+    local p2=$(basename "$filename")
+    echo "$p2/$p1.exp"
+}
+
+# Takes the absolute path to a 'gdb.cmd' file as produced by the GDB
+# testsuite (including files with a version number, for example
+# 'gdb.cmd.1') and prints a string that is the absolute path for where
+# we expect to find the corresponding 'valgrind.out' file.
+function cmd_file_to_valgrind_file () {
+    local filename=$1
+    filename=$(echo $filename | sed -e 's/gdb.cmd/valgrind.out/')
+    echo $filename
+}
+
+# Takes the absolute path to a 'valgrind.out' file and produces a
+# short summary of the results for that file.  The summary could be
+# something like:
+#
+#    gdb.blah/example.exp:BAD
+#
+# If the valgrind output file is missing or corrupted.  If however the
+# valgrind output file is as expected we'll get something like:
+#
+#    gdb.blah/example.exp:4;2
+#
+# The first number is the number of errors, and the second is the
+# number of contexts from which the errors came.  This is extracted
+# from valgrinds "ERROR SUMMARY" line in its output file.
+function extract_valgrind_results () {
+    local gdb_cmd_file=$1
+
+    local valgrind_file=$(cmd_file_to_valgrind_file "${gdb_cmd_file}")
+    if [ ! "${valgrind_file}" -ot "${gdb_cmd_file}" ]
+    then
+        line=$(grep -m 1 "ERROR SUMMARY: " "${valgrind_file}")
+        if [ -z "${line}" ]
+        then
+            echo "$testname:BAD"
+        else
+            line=$(echo $line \ | \
+                       sed -e 's/.*ERROR SUMMARY: \([0-9]\+\) errors from \([0-9]\+\) contexts .*/\1;\2/')
+            echo "$testname:$line"
+        fi
+    else
+        echo "$testname:BAD"
+    fi
+}
+
+# This takes an absolute path to the outputs directory in the build
+# tree, which is located somewhere like:
+#
+#    /projects/binutils-gdb/build/gdb/testsuite/outputs/
+#
+# And produces a list of summary results, one per valgrind output file
+# found below the outputs directory.
+function extract_raw_errors () {
+    local output_dir=$1
+
+    while IFS= read -r cmd_file
+    do
+        gdb_cmd=$(cut -d' ' -f1 ${cmd_file})
+
+        re=\\bvalgrind\$
+        if [[ "${gdb_cmd}" =~ $re ]]
+        then
+            # This GDB was run under valgrind, we expect to find a
+            # valgrind.out file.  Failure to find one indicates
+            # something went wrong.
+            #
+            # But first, lets figure out the name for this test.
+            testname=$(cmd_file_to_testname "${cmd_file}")
+
+            # Now given the name of the GDB command file, find the
+            # corresponding valgrind output file and extract the
+            # results from it, and print them on one line.
+            extract_valgrind_results "${cmd_file}"
+        fi
+    done < <(find "${outputs_dir}" -name "gdb.cmd*")
+}
+
+# Some global state.
+last_name=""
+errors=0
+contexts=0
+is_bad=0
+
+# Takes a string that is the value part of a test summary line, this
+# will either be the string "BAD" (without quotes), or two numbers
+# like "4;2" (again without quotes).
+#
+# If the value is BAD, then the global state is updated to reflect
+# this, while if the value is two numbers then these values are added
+# into the global state.
+function add_result () {
+    local value=$1
+
+    # The following updates the global state.
+    if [ $value == "BAD" ]
+    then
+        errors=0
+        contexts=0
+        is_bad=1
+    elif [ $is_bad == 0 ]
+    then
+        e=$(echo $value | cut -d';' -f1)
+        c=$(echo $value | cut -d';' -f2)
+        errors=$((errors + e))
+        contexts=$((contexts + c))
+    fi
+}
+
+# Print a line representing the current global result state.
+function print_result () {
+    if [ $is_bad == 1 ]
+    then
+        echo "${last_name}: Valgrind output missing or corrupted"
+    else
+        echo "${last_name}: ${errors} errors from $contexts contexts"
+    fi
+}
+
+# Check we have a sane outputs directory.
+[[ -n "${outputs_dir}" && -d "${outputs_dir}" ]] \
+    || error "invalid outputs directory '${outputs_dir}'"
+
+# Loop over all sorted summary lines and merge together all results
+# for the same test file, then print the results.  Don't forget to
+# print the last result after the loop.
+while IFS= read -r result
+do
+    name=$(echo $result | cut -d: -f1)
+    value=$(echo $result | cut -d: -f2)
+
+    if [ "$name" == "$last_name" ]
+    then
+        # Add this result to the last one.
+        add_result $value
+    else
+        if [ -n "$last_name" ]
+        then
+            print_result
+        fi
+
+        last_name=$name
+        errors=0
+        contexts=0
+        is_bad=0
+
+        add_result $value
+    fi
+done < <(extract_raw_errors "$outputs_dir" | sort)
+if [ -n "$last_name" ]
+then
+    print_result
+fi
diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 2beba053ee6..86aaf0234d3 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -168,6 +168,18 @@  TIMESTAMP = $(if $(TS),| $(srcdir)/print-ts.py $(if $(TS_FORMAT),$(TS_FORMAT),),
 gdb_debug = $(if $(GDB_DEBUG),GDB_DEBUG=$(GDB_DEBUG) ; export GDB_DEBUG ;,)
 gdbserver_debug = $(if $(GDBSERVER_DEBUG),GDBSERVER_DEBUG=$(GDBSERVER_DEBUG) ; export GDBSERVER_DEBUG ;,)
 
+# Setup flags needed to activate the valgrind testing in the testsuite.
+ifeq ($(VALGRIND),1)
+  valgrind_var = VALGRIND=1 ; export VALGRIND ;
+  valgrind_command_var = $(if $(VALGRIND_COMMAND),VALGRIND_COMMAND=$(VALGRIND_COMMAND) ; export VALGRIND_COMMAND ;,)
+  valgrind_extra_args_var = $(if $(VALGRIND_EXTRA_ARGS),VALGRIND_EXTRA_ARGS=$(VALGRIND_EXTRA_ARGS) ; export VALGRIND_EXTRA_ARGS ;,)
+  valgrind_build_sum = $(srcdir)/../contrib/gather-valgrind-results.sh $(PWD)/outputs/ > valgrind.sum
+else
+  valgrind_var =
+  valgrind_command_var =
+  valgrind_extra_args_var =
+  valgrind_build_sum = true
+endif
 
 # All the hair to invoke dejagnu.  A given invocation can just append
 # $(RUNTESTFLAGS)
@@ -176,6 +188,7 @@  DO_RUNTEST = \
 	srcdir=${srcdir} ; export srcdir ; \
 	EXPECT=${EXPECT} ; export EXPECT ; \
 	EXEEXT=${EXEEXT} ; export EXEEXT ;  $(gdb_debug) $(gdbserver_debug) \
+	$(valgrind_var) $(valgrind_command_var) $(valgrind_extra_args_var) \
         $(RPATH_ENVVAR)=$$rootme/../../expect:$$rootme/../../libstdc++:$$rootme/../../tk/unix:$$rootme/../../tcl/unix:$$rootme/../../bfd:$$rootme/../../opcodes:$$$(RPATH_ENVVAR); \
 	export $(RPATH_ENVVAR); \
 	if [ -f $${rootme}/../../expect/expect ] ; then  \
@@ -203,7 +216,10 @@  check-gdb.%:
 	$(MAKE) check TESTS="gdb.$*/*.exp"
 
 check-single:
-	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP)
+	$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP) ; \
+	result=$$?; \
+	$(valgrind_build_sum); \
+	exit $$result
 
 check-single-racy:
 	-rm -rf cache racy_outputs temp
@@ -221,6 +237,7 @@  check-single-racy:
 	  $(DO_RUNTEST) --outdir=racy_outputs/$$n $(RUNTESTFLAGS) \
 	    $(expanded_tests_or_none) $(TIMESTAMP); \
 	done; \
+	$(valgrind_build_sum); \
 	$(srcdir)/analyze-racy-logs.py \
 	  `ls racy_outputs/*/gdb.sum` > racy.sum; \
 	sed -n '/=== gdb Summary ===/,$$ p' racy.sum
@@ -229,6 +246,7 @@  check-parallel:
 	-rm -rf cache outputs temp
 	$(MAKE) -k do-check-parallel; \
 	result=$$?; \
+	$(valgrind_build_sum); \
 	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh \
 	  `find outputs -name gdb.sum -print` > gdb.sum; \
 	$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
@@ -257,6 +275,7 @@  check-parallel-racy:
 	    racy_outputs/$$n/gdb.log; \
 	  sed -n '/=== gdb Summary ===/,$$ p' racy_outputs/$$n/gdb.sum; \
 	done; \
+	$(valgrind_build_sum); \
 	$(srcdir)/analyze-racy-logs.py \
 	  `ls racy_outputs/*/gdb.sum` > racy.sum; \
 	sed -n '/=== gdb Summary ===/,$$ p' racy.sum
diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index 4795df1f759..03d43f936f8 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -139,6 +139,59 @@  tests" respectively.  "both" is the default.  GDB_PERFTEST_TIMEOUT
 specify the timeout, which is 3000 in default.  The result of
 performance test is appended in `testsuite/perftest.log'.
 
+Running Tests Under Valgrind
+****************************
+
+GDB Testsuite includes features to enable easy testing under the
+valgrind memory checking tool.  The simplest way to get started is to
+pass VALGRIND=1 as part of the make check command line, like this:
+
+	make check VALGRIND=1
+
+This will run GDB under valgrind.  Each invokation of GDB will create
+a new output file in the testsuite outputs directory, the first output
+file will be valgrind.out, then valgrind.out.1, valgrind.out.2, etc.
+The corresponding gdb.cmd file will be updated to include the valgrind
+command line details (which includes the GDB command run under
+valgrind).
+
+By default GDB just runs 'valgrind' and relies on finding a suitable
+binary in your $PATH.  You can override this behaviour by passing the
+VALGRIND_COMMAND flag make, like this:
+
+	make check VALGRIND=1 VALGRIND_COMMAND=/path/to/valgrind
+
+It is possible to add extra command line flags to valgrind like this:
+
+	make check VALGRIND=1 VALGRIND_EXTRA_ARGS="--leak-check=full"
+
+The default command line arguments used for valgrind are:
+
+	valgrind --tool=memcheck \
+                 --log-file=/path/to/log \
+                 --suppressions=/path/to/file
+
+Any extra flags provided by the user are appended to the valgrind
+command line.
+
+Notice that by default a suppressions file is passed to valgrind, this
+is because there are a large number of errors from the guile garbage
+collector that make the results very noisy, the suppressions file
+hides these errors.  The default suppressions file can be found in the
+testsuite source directory here:
+
+	binutils-gdb/gdb/testsuite/valgrind-gdb.supp
+
+Finally, once a test run has completed a summary of the valgrind
+results will be created into a file:
+
+	build/gdb/testsuite/valgrind.sum
+
+This summary file lists for each test script the number of valgrind
+errors found.  This would be useful for comparing between two
+different test runs and quickly spotting if any extra errors have
+appeared.
+
 Testsuite Parameters
 ********************
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index acbeb013768..008862ede24 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1695,9 +1695,39 @@  proc default_gdb_spawn { } {
     global GDB
     global INTERNAL_GDBFLAGS GDBFLAGS
     global gdb_spawn_id
+    global srcdir
 
     gdb_stop_suppressing_tests
 
+    set gdb_cmd $GDB
+    if { [info exists ::env(VALGRIND)] == 1 && $::env(VALGRIND) == 1 } {
+	set log_file \
+	    [standard_output_file_with_gdb_instance valgrind.out]
+	set sfile "$srcdir/valgrind-gdb.supp"
+
+	set valgrind_args "--tool=memcheck --log-file=${log_file}"
+	if [file exists "${sfile}"] {
+	    set valgrind_args "$valgrind_args --suppressions=${sfile}"
+	}
+	if { [info exists ::env(VALGRIND_EXTRA_ARGS)] == 1 } {
+	    set valgrind_args "$valgrind_args $::env(VALGRIND_EXTRA_ARGS)"
+	}
+
+	set valgrind_exe "valgrind"
+	if { [info exists ::env(VALGRIND_COMMAND)] == 1 } {
+	    set valgrind_exe $::env(VALGRIND_COMMAND)
+	}
+
+	if ![is_remote host] {
+	    if { [which $valgrind_exe] == 0 } then {
+		perror "$valgrind_exe does not exist."
+		exit 1
+	    }
+	}
+
+	set gdb_cmd "${valgrind_exe} ${valgrind_args} ${GDB}"
+    }
+
     # Set the default value, it may be overriden later by specific testfile.
     #
     # Use `set_board_info use_gdb_stub' for the board file to flag the inferior
@@ -1707,8 +1737,8 @@  proc default_gdb_spawn { } {
     # a specific different target protocol itself.
     set use_gdb_stub [target_info exists use_gdb_stub]
 
-    verbose "Spawning $GDB $INTERNAL_GDBFLAGS $GDBFLAGS"
-    gdb_write_cmd_file "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS"
+    verbose "Spawning $gdb_cmd $INTERNAL_GDBFLAGS $GDBFLAGS"
+    gdb_write_cmd_file "$gdb_cmd $INTERNAL_GDBFLAGS $GDBFLAGS"
 
     if [info exists gdb_spawn_id] {
 	return 0
@@ -1720,9 +1750,9 @@  proc default_gdb_spawn { } {
 	    exit 1
 	}
     }
-    set res [remote_spawn host "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]"]
+    set res [remote_spawn host "$gdb_cmd $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]"]
     if { $res < 0 || $res == "" } {
-	perror "Spawning $GDB failed."
+	perror "Spawning $gdb_cmd failed."
 	return 1
     }
 
diff --git a/gdb/testsuite/valgrind-gdb.supp b/gdb/testsuite/valgrind-gdb.supp
new file mode 100644
index 00000000000..f2a25eb0117
--- /dev/null
+++ b/gdb/testsuite/valgrind-gdb.supp
@@ -0,0 +1,13 @@ 
+{
+  <guile_garbage_collection_1>
+  Memcheck:Cond
+  ...
+  obj:*libgc*
+}
+
+{
+  <guile_garbage_collection_2>
+  Memcheck:Value8
+  ...
+  obj:*libgc*
+}