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

[Philippe Waroquiers <philippe dot waroquiers at skynet dot be>]

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*
> +}

Attachment: patch_valgrind.txt
Description: Text document