Re: Running cc1 etc under valgrind (was Re: [PATCH 13/21] PR jit/63854: Add support for running make check-jit under valgrind)
On Wed, 2014-11-19 at 13:38 -0500, David Malcolm wrote: On Wed, 2014-11-19 at 09:57 -0700, Jeff Law wrote: On 11/19/14 03:46, David Malcolm wrote: This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present in the environment, all of the built client code using libgccjit.so is run under valgrind, with --leak-check=full. Hence: RUN_UNDER_VALGRIND= make check-jit will run all jit testcases under valgrind (taking 27 mins on my machine). Results are written to testsuite/jit/test-FOO.exe.valgrind.txt jit.exp automatically parses these result file, looking for lines of the form definitely lost: 11,316 bytes in 235 blocks indirectly lost: 352 bytes in 4 blocks in the valgrind log's summary footer, adding PASSes if they are zero bytes, and, for now generating XFAILs for non-zero bytes. Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's host_execute, but I don't see a clean way to fix that. This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving: # of expected passes 2481 # of expected failures 49 gcc/testsuite/ChangeLog: PR jit/63854 * jit.dg/jit.exp (report_leak): New. (parse_valgrind_logfile): New. (fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present in the environment, and if so, run the executable under valgrind, capturing valgrind's output to a logfile. Parse the log file, generating PASSes and XFAILs for the summary of leaks. OK for the trunk. FWIW, I'd love to see a mode where we can easily do this for the other testsuites as well. Many of the cleanups in these patches are called from toplev::finalize, or something called from there, so that they're called by libgccjit.so, but not called by cc1/cc1plus etc. In general, cc1 etc don't need to bother free-ing everything, and can instead simply exit. But if you're running under valgrind, you'd probably want them to call toplev::finalize before exiting, to make the valgrind log shorter. So perhaps cc1 etc could detect if they're being run under valgrind, and call toplev::finalize in the appropriate place? Or maybe this could be a command-line option? [I think I prefer autodetection, fwiw] It turns out that this isn't necessary (I think), since the pointers are typically still reachable.
[PATCH 13/21] PR jit/63854: Add support for running make check-jit under valgrind
This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present
in the environment, all of the built client code using libgccjit.so is
run under valgrind, with --leak-check=full.
Hence:
RUN_UNDER_VALGRIND= make check-jit
will run all jit testcases under valgrind (taking 27 mins on my
machine).
Results are written to testsuite/jit/test-FOO.exe.valgrind.txt
jit.exp automatically parses these result file, looking for lines of
the form
definitely lost: 11,316 bytes in 235 blocks
indirectly lost: 352 bytes in 4 blocks
in the valgrind log's summary footer, adding PASSes if they are zero
bytes, and, for now generating XFAILs for non-zero bytes.
Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's
host_execute, but I don't see a clean way to fix that.
This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving:
# of expected passes 2481
# of expected failures 49
gcc/testsuite/ChangeLog:
PR jit/63854
* jit.dg/jit.exp (report_leak): New.
(parse_valgrind_logfile): New.
(fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present
in the environment, and if so, run the executable under
valgrind, capturing valgrind's output to a logfile. Parse the
log file, generating PASSes and XFAILs for the summary of leaks.
---
gcc/testsuite/jit.dg/jit.exp | 79 +++-
1 file changed, 78 insertions(+), 1 deletion(-)
diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
index 531e929..26ab127 100644
--- a/gcc/testsuite/jit.dg/jit.exp
+++ b/gcc/testsuite/jit.dg/jit.exp
@@ -23,6 +23,48 @@ load_lib target-libpath.exp
load_lib gcc.exp
load_lib dejagnu.exp
+# Look for lines of the form:
+# definitely lost: 11,316 bytes in 235 blocks
+# indirectly lost: 352 bytes in 4 blocks
+# Ideally these would report zero bytes lost (which is a PASS);
+# for now, report non-zero leaks as XFAILs.
+proc report_leak {kind name logfile line} {
+set match [regexp $kind lost: .* $line result]
+if $match {
+ verbose Saw \$result\ within \$line\ 3
+ # Extract bytes and blocks.
+ # These can contain commas as well as numerals,
+ # but we only care about whether we have zero.
+ regexp $kind lost: (.+) bytes in (.+) blocks \
+ $result - bytes blocks
+ verbose bytes: '$bytes' 2
+ verbose blocks: '$blocks' 2
+ if { $bytes == 0 } {
+ pass $name: $logfile: $result
+ } else {
+ xfail $name: $logfile: $result
+ }
+}
+}
+
+proc parse_valgrind_logfile {name logfile} {
+verbose parse_valgrind_logfile: $logfile 2
+if [catch {set f [open $logfile]}] {
+ fail $name: unable to read $logfile
+ return
+}
+
+while { [gets $f line] = 0 } {
+ # Strip off the PID prefix e.g. ==7675==
+ set line [regsub ==\[0-9\]*== $line ]
+ verbose $line 2
+
+ report_leak definitely $name $logfile $line
+ report_leak indirectly $name $logfile $line
+}
+close $f
+}
+
# This is host_execute from dejagnu.exp commit
# 126a089777158a7891ff975473939f08c0e31a1c
# with the following patch applied, and renaming to fixed_host_execute.
@@ -49,8 +91,11 @@ load_lib dejagnu.exp
# if there was a problem.
#
proc fixed_host_execute {args} {
+global env
global text
global spawn_id
+global srcdir
+verbose srcdir: $srcdir 2
set timeoutmsg Timed out: Never got started,
set timeout 100
@@ -75,7 +120,28 @@ proc fixed_host_execute {args} {
# spawn the executable and look for the DejaGnu output messages from the
# test case.
# spawn -noecho -open [open |./${executable} r]
-spawn -noecho ./${executable} ${params}
+
+set run_under_valgrind [info exists env(RUN_UNDER_VALGRIND)]
+
+if $run_under_valgrind {
+ set valgrind_logfile ${executable}.valgrind.txt
+ set valgrind_params {valgrind}
+ lappend valgrind_params --leak-check=full
+ # srcdir is within gcc/testsuite; locate contrib relative to it:
+ lappend valgrind_params
--suppressions=${srcdir}/../../contrib/valgrind.supp
+ lappend valgrind_params --log-file=${valgrind_logfile}
+} else {
+ set valgrind_params {}
+}
+verbose valgrind_params: $valgrind_params 2
+
+set args ${valgrind_params}
+lappend args ./${executable}
+set args [concat $args ${params}]
+verbose args: $args 2
+
+eval spawn -noecho $args
+
expect_after full_buffer { error got full_buffer }
set prefix \[^\r\n\]*
@@ -147,6 +213,17 @@ proc fixed_host_execute {args} {
# force a close of the executable to be safe.
catch close
+# valgrind might not have finished writing the log out
+# before we parse it; need to wait for spawnee to
+# finish.
+catch wait reason
+verbose reason: $reason 2
+
+if $run_under_valgrind {
+ upvar 2 name name
+ parse_valgrind_logfile $name
Re: [PATCH 13/21] PR jit/63854: Add support for running make check-jit under valgrind
On 11/19/14 03:46, David Malcolm wrote: This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present in the environment, all of the built client code using libgccjit.so is run under valgrind, with --leak-check=full. Hence: RUN_UNDER_VALGRIND= make check-jit will run all jit testcases under valgrind (taking 27 mins on my machine). Results are written to testsuite/jit/test-FOO.exe.valgrind.txt jit.exp automatically parses these result file, looking for lines of the form definitely lost: 11,316 bytes in 235 blocks indirectly lost: 352 bytes in 4 blocks in the valgrind log's summary footer, adding PASSes if they are zero bytes, and, for now generating XFAILs for non-zero bytes. Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's host_execute, but I don't see a clean way to fix that. This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving: # of expected passes 2481 # of expected failures 49 gcc/testsuite/ChangeLog: PR jit/63854 * jit.dg/jit.exp (report_leak): New. (parse_valgrind_logfile): New. (fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present in the environment, and if so, run the executable under valgrind, capturing valgrind's output to a logfile. Parse the log file, generating PASSes and XFAILs for the summary of leaks. OK for the trunk. FWIW, I'd love to see a mode where we can easily do this for the other testsuites as well. All this work is definitely appreciated -- Jakub usually does similar stuff later in the release process, so you're offloading some stuff from him, which definitely helps. jeff
Re: [PATCH 13/21] PR jit/63854: Add support for running make check-jit under valgrind
On Wed, 2014-11-19 at 09:57 -0700, Jeff Law wrote: On 11/19/14 03:46, David Malcolm wrote: This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present in the environment, all of the built client code using libgccjit.so is run under valgrind, with --leak-check=full. Hence: RUN_UNDER_VALGRIND= make check-jit will run all jit testcases under valgrind (taking 27 mins on my machine). Results are written to testsuite/jit/test-FOO.exe.valgrind.txt jit.exp automatically parses these result file, looking for lines of the form definitely lost: 11,316 bytes in 235 blocks indirectly lost: 352 bytes in 4 blocks in the valgrind log's summary footer, adding PASSes if they are zero bytes, and, for now generating XFAILs for non-zero bytes. Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's host_execute, but I don't see a clean way to fix that. This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving: # of expected passes 2481 # of expected failures 49 gcc/testsuite/ChangeLog: PR jit/63854 * jit.dg/jit.exp (report_leak): New. (parse_valgrind_logfile): New. (fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present in the environment, and if so, run the executable under valgrind, capturing valgrind's output to a logfile. Parse the log file, generating PASSes and XFAILs for the summary of leaks. OK for the trunk. Thanks - though the patch I posted uses the contrib/valgrind.supp file, which I added before seeing the --enable-valgrind-annotations configure option that does a better job of this. So I'll revise it to remove that suppression file (and add some usage notes to the internals/index.rst document). FWIW, I'd love to see a mode where we can easily do this for the other testsuites as well. I suspect that the functions report_leak and parse_valgrind_logfile could be moved into a lib/valgrind.exp at some point if someone wants to reuse them - maybe adding a param to specify if we expect it to be clean (PASS) vs leaky (XFAIL) - I'm close to having most of the jit testcases be valgrind-clean. All this work is definitely appreciated -- Jakub usually does similar stuff later in the release process, so you're offloading some stuff from him, which definitely helps. Yeah - leaks are a much bigger issue for libgccjit.so than for e.g. cc1: people embedding it into long-running processes like, say, an X server aren't going to be happy about slow leaks. Dave
Re: [PATCH 13/21] PR jit/63854: Add support for running make check-jit under valgrind
On 11/19/14 10:11, David Malcolm wrote: Thanks - though the patch I posted uses the contrib/valgrind.supp file, which I added before seeing the --enable-valgrind-annotations configure option that does a better job of this. So I'll revise it to remove that suppression file (and add some usage notes to the internals/index.rst document). Sounds good. All this work is definitely appreciated -- Jakub usually does similar stuff later in the release process, so you're offloading some stuff from him, which definitely helps. Yeah - leaks are a much bigger issue for libgccjit.so than for e.g. cc1: people embedding it into long-running processes like, say, an X server aren't going to be happy about slow leaks. Yea, so imagine shoving libbfd into a link server, which had a GC collected class around bfd *. One of the key features of the link server was that it cached information across invocations. Sooo, we tended to never let go of the underlying BFD objects. Which wouldn't have been too bad, except that the underlying BFD objects tend to want to cache section contents. So this server sat on a mount point and anytime you referenced a file, it'd consult its little database of how do I build XYZ. You didn't have to reference too many files before the damn server ate all your memory. All in the name of automagically reordering executables... But I digress... Jeff
Running cc1 etc under valgrind (was Re: [PATCH 13/21] PR jit/63854: Add support for running make check-jit under valgrind)
On Wed, 2014-11-19 at 09:57 -0700, Jeff Law wrote: On 11/19/14 03:46, David Malcolm wrote: This commit updates jit.exp so that if RUN_UNDER_VALGRIND is present in the environment, all of the built client code using libgccjit.so is run under valgrind, with --leak-check=full. Hence: RUN_UNDER_VALGRIND= make check-jit will run all jit testcases under valgrind (taking 27 mins on my machine). Results are written to testsuite/jit/test-FOO.exe.valgrind.txt jit.exp automatically parses these result file, looking for lines of the form definitely lost: 11,316 bytes in 235 blocks indirectly lost: 352 bytes in 4 blocks in the valgrind log's summary footer, adding PASSes if they are zero bytes, and, for now generating XFAILs for non-zero bytes. Sadly this diverges jit.exp's fixed_host_execute further from DejaGnu's host_execute, but I don't see a clean way to fix that. This currently adds 63 PASSes and 49 XFAILs to jit.sum, giving: # of expected passes 2481 # of expected failures 49 gcc/testsuite/ChangeLog: PR jit/63854 * jit.dg/jit.exp (report_leak): New. (parse_valgrind_logfile): New. (fixed_host_execute): Detect if RUN_UNDER_VALGRIND is present in the environment, and if so, run the executable under valgrind, capturing valgrind's output to a logfile. Parse the log file, generating PASSes and XFAILs for the summary of leaks. OK for the trunk. FWIW, I'd love to see a mode where we can easily do this for the other testsuites as well. Many of the cleanups in these patches are called from toplev::finalize, or something called from there, so that they're called by libgccjit.so, but not called by cc1/cc1plus etc. In general, cc1 etc don't need to bother free-ing everything, and can instead simply exit. But if you're running under valgrind, you'd probably want them to call toplev::finalize before exiting, to make the valgrind log shorter. So perhaps cc1 etc could detect if they're being run under valgrind, and call toplev::finalize in the appropriate place? Or maybe this could be a command-line option? [I think I prefer autodetection, fwiw] Thoughts? Dave
