| Message ID | 1416393981-39626-14-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
Various fixes/improvements to jit.exp Successfully bootstrapped & regrtested on x86_64-unknown-linux-gnu (Fedora 20). OK for trunk? (am not yet officially blessed as the JIT maintainer, so I require review for the non-obvious patches)
Various fixes/improvements to jit.exp Successfully bootstrapped & regrtested on x86_64-unknown-linux-gnu (Fedora 20). OK for trunk? (am not yet officially blessed as the JIT maintainer, so I require review for the non-obvious patches)
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.
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 $valgrind_logfile + } + return "" }