RFA: valgrind and the test suite, take 2

RFA: valgrind and the test suite, take 2

[Tom Tromey]

This needs a documentation review.  I would also appreciate other
comments, as usual.

This patch adds support for valgrind to the test suite.  Unlike my last
patch along these lines, this one adds value above just setting GDB.

In particular, it adds a new test at each point where the tested gdb
exits.  If that gdb run caused valgrind to log any errors, the test
fails.

I ran the entire test suite this way on the compile farm and found a
number of problems (perhaps not all of our causing ... I'm still sorting
through the logs).

Tom

2010-02-19  Tom Tromey  <tromey@redhat.com>

	* gdbint.texinfo (Testsuite): Document use of VALGRIND setting.

2010-02-19  Tom Tromey  <tromey@redhat.com>

	* lib/gdb.exp: Recognize VALGRIND variable.
	(remote_spawn): New wrapper proc.
	(gdb_exit): Likewise.

diff --git a/gdb/doc/gdbint.texinfo b/gdb/doc/gdbint.texinfo
index 7741855..8b02ed8 100644
--- a/gdb/doc/gdbint.texinfo
+++ b/gdb/doc/gdbint.texinfo
@@ -7674,6 +7674,19 @@ HOME=`pwd` runtest \
   INTERNAL_GDBFLAGS=-nw
 @end smallexample
 
+@item @code{VALGRIND}
+
+It is useful to occasionally run @value{GDBN} under @code{valgrind},
+to find bugs that do not always result in an immediate failure.  This
+can be accomplished by setting @code{VALGRIND}.  The contents of this
+will be appended to the internal @code{valgrind} command line that the
+test suite computes.  @code{VALGRIND} can be empty.
+
+Setting this will add an additional check; each time @value{GDBN}
+exits, the contents of a the @code{valgrind} log file are examined
+and, if there was any output, the @samp{valgrind check} test will
+fail.
+
 @end itemize
 
 There are two ways to run the testsuite and pass additional parameters
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index a42d551..7bf1ea9 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -42,6 +42,14 @@ if ![info exists GDB] {
 }
 verbose "using GDB = $GDB" 2
 
+# If VALGRIND is set, we run gdb under valgrind.
+if {[info exists VALGRIND]} {
+    global outdir env vg_basename
+    set env(VGNAME) 0
+    set vg_basename "$outdir/valgrind."
+    set GDB "valgrind --quiet --trace-children=no --child-silent-after-fork=yes --log-file=${vg_basename}%q{VGNAME} $VALGRIND $GDB"
+}
+
 # GDBFLAGS is available for the user to set on the command line.
 # E.g. make check RUNTESTFLAGS=GDBFLAGS=mumble
 # Testcases may use it to add additional flags, but they must:
@@ -3139,6 +3147,50 @@ if {[info exists TRANSCRIPT]} {
   }
 }
 
+# Support for running gdb under valgrind and checking the results.
+if {[info exists VALGRIND]} {
+    rename remote_spawn vg_remote_spawn
+    rename gdb_exit vg_gdb_exit
+
+    proc remote_spawn {args} {
+	global env vg_basename
+	incr env(VGNAME)
+	file delete -force ${vg_basename}$env(VGNAME)
+	return [uplevel vg_remote_spawn $args]
+    }
+
+    proc gdb_exit {args} {
+	global env vg_basename vg_reported
+
+	# We can sometimes see multiple consecutive calls to gdb_exit
+	# without an intervening spawn.  In this case we only want to
+	# report the valgrind results once.
+	if {![info exists vg_reported($env(VGNAME))]} {
+	    set vg_reported($env(VGNAME)) 1
+
+	    # FIXME: how can we name this test consistently across
+	    # runs?
+	    if {[file exists ${vg_basename}$env(VGNAME)]
+		&& [file size ${vg_basename}$env(VGNAME)] > 0} {
+		send_log "Valgrind output:\n"
+		set fd [open ${vg_basename}$env(VGNAME)]
+		send_log [read $fd]
+		close $fd
+		send_log "\n"
+		fail "valgrind check $env(VGNAME)"
+	    } else {
+		pass "valgrind check $env(VGNAME)"
+	    }
+	}
+
+	return [uplevel vg_gdb_exit $args]
+    }
+
+    # Valgrind slows everything down, so boost the default timeout.
+    global gdb_test_timeout
+    set gdb_test_timeout [expr {20 * $gdb_test_timeout}]
+}
+
 proc core_find {binfile {deletefiles {}} {arg ""}} {
     global objdir subdir
 

Re: RFA: valgrind and the test suite, take 2

[Pedro Alves]

On Friday 19 February 2010 17:23:52, Tom Tromey wrote:
> This patch adds support for valgrind to the test suite.  Unlike my last
> patch along these lines, this one adds value above just setting GDB.

Couldn't this be done with a board file?

-- 
Pedro Alves

Re: RFA: valgrind and the test suite, take 2

[Joel Brobecker]

> > This patch adds support for valgrind to the test suite.  Unlike my last
> > patch along these lines, this one adds value above just setting GDB.
> 
> Couldn't this be done with a board file?

It seems to be that it would be much more convenient to just be able
to defined VALGRIND to activate this feature.  board files are the only
way we have to provide setup-specific information (such as target name,
etc), but the stuff to get testing under valgrind should be the same
for everyone...

Similarly, I have always found unfortunate that gdb.reverse testing
is conditioned on a setting that gets set in a board file.  As a result
of this, I have never run the gdb.reverse testcases even though I could
have done so on my laptop.

-- 
Joel

Re: RFA: valgrind and the test suite, take 2

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Date: Fri, 19 Feb 2010 10:23:52 -0700
> 
> +@item @code{VALGRIND}
> +
> +It is useful to occasionally run @value{GDBN} under @code{valgrind},
> +to find bugs that do not always result in an immediate failure.  This
> +can be accomplished by setting @code{VALGRIND}.  The contents of this
> +will be appended to the internal @code{valgrind} command line that the
> +test suite computes.  @code{VALGRIND} can be empty.

This does not explain what could be the value of VALGRIND.  I needed
to read it several times before I realized that it should be a string
specifying various options and switches to the `valgrind' command.  So
how about this alternative wording:

 It is occasionally useful to run the @value{GDBN} test suite under
 @command{valgrind}, to find bugs that do not always result in an
 immediate failure of one of the tests.  This can be accomplished by
 setting @code{VALGRIND}.  The value should be a string specifying
 non-default command-line arguments and options to be passed to
 @code{valgrind}; it will be appended to the internal @code{valgrind}
 command line that the test suite computes.  An empty string causes
 the tests to be run under @command{valgrind} without modifying its
 command line.

Re: RFA: valgrind and the test suite, take 2

[Pedro Alves]

On Friday 19 February 2010 18:09:45, Joel Brobecker wrote:
> > > This patch adds support for valgrind to the test suite.  Unlike my last
> > > patch along these lines, this one adds value above just setting GDB.
> > 
> > Couldn't this be done with a board file?
> 
> It seems to be that it would be much more convenient to just be able
> to defined VALGRIND to activate this feature.  board files are the only
> way we have to provide setup-specific information (such as target name,
> etc), but the stuff to get testing under valgrind should be the same
> for everyone...
> 

Except when it won't, no?  E.g, I don't see a way to point at a
different valgrind binary than the one in the PATH, or a way to 
change the parameters that are hardcoded.

I took Tom's patch a converted it to the board file pasted
below.  I placed it amongst my other board files in the board
dir pointed at by DEJAGNU/site.exp, and ran it with

make check RUNTESTFLAGS="--target_board=valgrind.exp"

It has the same effect, it seems.  This technique can be
reused for other random ways to wrap gdb, not just valgrind.

Maybe we need a dir where we can place random board
files.  The board file used to test gdbserver locally could
got there as well.  "gdb/contrib", or a subdirectory within,
or somesuch.

> Similarly, I have always found unfortunate that gdb.reverse testing
> is conditioned on a setting that gets set in a board file.  As a result
> of this, I have never run the gdb.reverse testcases even though I could
> have done so on my laptop.

That is different.  Please do not mix things up.  That's about
testing a GDB functionality, _what_ to test, not about _how_ to test
GDB.  It is a deficiency of the testsuite that it doesn't detect
that on native testing on some targets it could use precord
for testing.  It has non complicated fix, and it has been
discussed before.  It just requires someone moving forward
fixing it.

-- 
Pedro Alves

========== valgrind.exp ============

set board_info(localhost,isremote) 0
load_generic_config "unix"

# The default compiler for this target.
set_board_info compiler "[find_gcc]"

global outdir vg_basename
set vg_basename "$outdir/valgrind."
global vg_count
set vg_count "0"

global GDB
set GDB "valgrind --quiet --trace-children=no --child-silent-after-fork=yes --log-file=${vg_basename}%q{VGCOUNT} $GDB"

# Support for running gdb under valgrind and checking the results.

rename remote_spawn vg_remote_spawn
proc remote_spawn {args} {
    global env vg_basename vg_count

    incr vg_count
    set env(VGCOUNT) $vg_count

    file delete -force ${vg_basename}$vg_count
    set res [uplevel vg_remote_spawn $args]

    return $res
}

rename gdb_exit vg_gdb_exit
proc gdb_exit {args} {
    global vg_basename vg_count vg_reported

    # We can sometimes see multiple consecutive calls to gdb_exit
    # without an intervening spawn.  In this case we only want to
    # report the valgrind results once.
    if {![info exists vg_reported($vg_count)]} {
	set vg_reported($vg_count) 1

	# FIXME: how can we name this test consistently across
	# runs?
	if {[file exists ${vg_basename}$vg_count]
	    && [file size ${vg_basename}$vg_count] > 0} {
	    send_log "Valgrind output:\n"
	    set fd [open ${vg_basename}$vg_count]
	    send_log [read $fd]
	    close $fd
	    send_log "\n"
	    fail "valgrind check $vg_count"
	} else {
	    pass "valgrind check $vg_count"
	}
    }

    return [uplevel vg_gdb_exit $args]
}

# Valgrind slows everything down, so boost the default timeout.
global gdb_test_timeout
set gdb_test_timeout [expr {20 * $gdb_test_timeout}]

Re: RFA: valgrind and the test suite, take 2

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> On Friday 19 February 2010 17:23:52, Tom Tromey wrote:
>> This patch adds support for valgrind to the test suite.  Unlike my last
>> patch along these lines, this one adds value above just setting GDB.

Pedro> Couldn't this be done with a board file?

Maybe, but then it won't be in the tree, and it won't be documented.

If that is an objection, could you explain why you'd prefer it to be
done that way?

The point of doing it this way is to encourage more frequent runs using
valgrind.  I find bugs with valgrind so often that I wish this mode of
running were the default.

Tom

Re: RFA: valgrind and the test suite, take 2

[Joel Brobecker]

> Maybe we need a dir where we can place random board files.  The board
> file used to test gdbserver locally could got there as well.
> "gdb/contrib", or a subdirectory within, or somesuch.

My only concern is to try to make it easy to enable this feature.
If we can achieve that with a board file, that's certainly fine
as far as I am concerned.

-- 
Joel

Re: RFA: valgrind and the test suite, take 2

[Pedro Alves]

On Friday 19 February 2010 20:02:40, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> On Friday 19 February 2010 17:23:52, Tom Tromey wrote:
> >> This patch adds support for valgrind to the test suite.  Unlike my last
> >> patch along these lines, this one adds value above just setting GDB.
> 
> Pedro> Couldn't this be done with a board file?
> 
> Maybe, but then it won't be in the tree, and it won't be documented.

Those are both fixable.  I don't know if you seen my other
more recent email.

> If that is an objection, could you explain why you'd prefer it to be
> done that way?

It's not an objection that I'll spend much more energy
defending, but FYI, I prefer not to hack specific solutions
when a more general solution already exists.  Another advantange
is that you can tweak it to your hearts content without
having to touch generic testsuite code.  If it allows things
that a board file doesn't, then it's another story.  But maybe
that could be considered a problem of missing hooks instead.

> The point of doing it this way is to encourage more frequent runs using
> valgrind.  

What's wrong with encouraging testing with a board file?
We do the same for local gdbserver testing.  We could come
up with N other board files that did similar things.

> I find bugs with valgrind so often that I wish this mode of
> running were the default.

If it's not going to be made the default, then that's
irrelevant.

-- 
Pedro Alves

Re: RFA: valgrind and the test suite, take 2

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Tom> If that is an objection, could you explain why you'd prefer it to be
Tom> done that way?

Pedro> It's not an objection that I'll spend much more energy
Pedro> defending, but FYI, I prefer not to hack specific solutions
Pedro> when a more general solution already exists.

It isn't more general to use a board file, because this patch works with
any board file.

>> I find bugs with valgrind so often that I wish this mode of
>> running were the default.

Pedro> If it's not going to be made the default, then that's
Pedro> irrelevant.

Ouch.  But true, I guess.

Tom

Re: RFA: valgrind and the test suite, take 2

[Pedro Alves]

On Friday 19 February 2010 21:08:51, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Tom> If that is an objection, could you explain why you'd prefer it to be
> Tom> done that way?
> 
> Pedro> It's not an objection that I'll spend much more energy
> Pedro> defending, but FYI, I prefer not to hack specific solutions
> Pedro> when a more general solution already exists.
> 
> It isn't more general to use a board file, because this patch works with
> any board file.

Yeah, I'll give you that.  Please do go ahead with your patch if
you still prefer it.

-- 
Pedro Alves