Libguestfs - Dec 2018 - Re: [PATCH nbdkit 2/4] valgrind: Add --show-leak-kinds=all and comprehensive list of suppressions.

On 12/2/18 10:39 AM, Richard W.M. Jones wrote:
> By default valgrind suppresses many leaks.  I'm not even sure exactly
> how it decides which ones to suppress, but certainly global variables
> pointing to malloc’d data are suppressed, which is not useful
> behaviour.
Here's my understanding of why that is the default - if you assign 
malloc()d storage into a global, then that storage is in scope right up 
until you call exit().  And it's faster (and often easier) to exit() a 
process with memory still allocated (the OS will clean it up anyways) 
than it is to manually chase down the memory cleanups yourself.
> 
> Tell valgrind to show all leaks.  It won't give an error on them.
> 
> However to do this we also need a much more comprehensive list of
> suppressions so that we don't constantly show unavoidable leaks in
> (eg) dlopen.  This adds separate files for each class of suppressions
> in a new valgrind/ directory.
> ---
>   .gitignore                                    |   1 +
>   Makefile.am                                   |   2 +-
>   configure.ac                                  |   3 +-
>   valgrind/Makefile.am                          |  47 ++++++++
>   valgrind/glibc.suppressions                   | 106 ++++++++++++++++++
>   .../nbdkit.suppressions                       |  21 +---
>   valgrind/perl.suppressions                    |  40 +++++++
>   wrapper.c                                     |   3 +-
>   8 files changed, 205 insertions(+), 18 deletions(-)
> 
> +++ b/valgrind/Makefile.am
> @@ -0,0 +1,47 @@
> +include $(top_srcdir)/common-rules.mk
> +
> +suppressions_files = $(wildcard *.suppressions)
A GNU make-ism - but you already mention requiring GNU make in README. 
Should we make ./configure error out hard if $MAKE is not GNU Make, 
rather than risking someone getting 80% though a build on BSD make and 
then choking when it gets here?
> +++ b/wrapper.c
> @@ -130,8 +130,9 @@ main (int argc, char *argv[])
>       passthru (VALGRIND);
>       passthru ("--vgdb=no");
>       passthru ("--leak-check=full");
> +    passthru ("--show-leak-kinds=all");
I could understand this if we were implementing a library and wanted to 
make it easier for some other user to call our library shutdown to 
reclaim all memory that we otherwise stashed in globals - but when we 
are just a standalone app, do we really need to worry about memory still 
reachable in globals, as that's not a true leak?
>       passthru ("--error-exitcode=119");
> -    passthru_format ("--suppressions=%s/valgrind-suppressions",
srcdir);
> +    passthru_format ("--suppressions=%s/valgrind/suppressions",
builddir);
Is this still right under VPATH?
>       passthru ("--trace-children=no");
>       passthru ("--run-libc-freeres=no");
>       passthru ("--num-callers=20");
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Mon, Dec 03, 2018 at 12:55:58PM -0600, Eric Blake
wrote:
> A GNU make-ism - but you already mention requiring GNU make in
> README. Should we make ./configure error out hard if $MAKE is not
> GNU Make, rather than risking someone getting 80% though a build on
> BSD make and then choking when it gets here?
Yes that would be a good idea as a separate patch.  I'm always annoyed
whenever I use *BSD and type "make" instead of "gmake".  In
fact I
have MAKEFLAGS=j<N> set in my environment and BSD make gives an error
about an unknown parameter which completely breaks ./configure too ...
> >+++ b/wrapper.c
> >@@ -130,8 +130,9 @@ main (int argc, char *argv[])
> >      passthru (VALGRIND);
> >      passthru ("--vgdb=no");
> >      passthru ("--leak-check=full");
> >+    passthru ("--show-leak-kinds=all");
> 
> I could understand this if we were implementing a library and wanted
> to make it easier for some other user to call our library shutdown
> to reclaim all memory that we otherwise stashed in globals - but
> when we are just a standalone app, do we really need to worry about
> memory still reachable in globals, as that's not a true leak?
Well it was a tidiness issue really.  The only actual error this
revealed was the memory leak in the bitmap patch
(https://www.redhat.com/archives/libguestfs/2018-December/msg00007.html),
and a lot of false positives which I worked my way through with a list
of suppressions.
> >      passthru ("--error-exitcode=119");
> >-    passthru_format
("--suppressions=%s/valgrind-suppressions", srcdir);
> >+    passthru_format
("--suppressions=%s/valgrind/suppressions", builddir);
> 
> Is this still right under VPATH?
I believe so ...?  Note this is the generated/concatenated file which
is why I changed srcdir to builddir.

Rich.
> >      passthru ("--trace-children=no");
> >      passthru ("--run-libc-freeres=no");
> >      passthru ("--num-callers=20");
> >
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 12/3/18 1:01 PM, Richard W.M. Jones wrote:
> 
>>>       passthru ("--error-exitcode=119");
>>> -    passthru_format
("--suppressions=%s/valgrind-suppressions", srcdir);
>>> +    passthru_format
("--suppressions=%s/valgrind/suppressions", builddir);
>>
>> Is this still right under VPATH?
> 
> I believe so ...?  Note this is the generated/concatenated file which
> is why I changed srcdir to builddir.
Yes, if it's generated, then builddir should be right.  But this is an 
independent fix, so I might have split it to another patch. (I guess I 
haven't got that far in my ongoing VPATH cleanups; up to now, my VPATH 
fixes have been the ones for issues pointed out by 'make distcheck', 
which doesn't run valgrind, as opposed to an actual VPATH build myself).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 12/3/18 12:55 PM, Eric Blake wrote:
> On 12/2/18 10:39 AM, Richard W.M. Jones wrote:
>> By default valgrind suppresses many leaks.  I'm not even sure
exactly
>> how it decides which ones to suppress, but certainly global variables
>> pointing to malloc’d data are suppressed, which is not useful
>> behaviour.
> 
> 
>> +include $(top_srcdir)/common-rules.mk
>> +
>> +suppressions_files = $(wildcard *.suppressions)
> 
> A GNU make-ism - but you already mention requiring GNU make in README. 
> Should we make ./configure error out hard if $MAKE is not GNU Make, 
> rather than risking someone getting 80% though a build on BSD make and 
> then choking when it gets here?
Worse, it fails in VPATH builds.  It resulted in 'make distcheck' 
hanging on a 'cat > valgrind.suppressions' command, because the
wildcard
didn't find anything in $(builddir), when it should have been looking in 
$(srcdir).


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Mon, Dec 03, 2018 at 10:06:17PM -0600, Eric Blake
wrote:
> On 12/3/18 12:55 PM, Eric Blake wrote:
> >On 12/2/18 10:39 AM, Richard W.M. Jones wrote:
> >>By default valgrind suppresses many leaks.  I'm not even sure
exactly
> >>how it decides which ones to suppress, but certainly global
variables
> >>pointing to malloc’d data are suppressed, which is not useful
> >>behaviour.
> >
> 
> >
> >>+include $(top_srcdir)/common-rules.mk
> >>+
> >>+suppressions_files = $(wildcard *.suppressions)
> >
> >A GNU make-ism - but you already mention requiring GNU make in
> >README. Should we make ./configure error out hard if $MAKE is not
> >GNU Make, rather than risking someone getting 80% though a build
> >on BSD make and then choking when it gets here?
> 
> Worse, it fails in VPATH builds.  It resulted in 'make distcheck'
> hanging on a 'cat > valgrind.suppressions' command, because the
> wildcard didn't find anything in $(builddir), when it should have
> been looking in $(srcdir).
I see you've fixed it already, thanks :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v