Libguestfs - Jul 2018 - Re: [PATCH nbdkit] valgrind: Don't call dlclose when running under valgrind.

When valgrinding plugins, the following sequence can happen:

  plugin_free
   -> dlclose
     -> unloads the plugin and symbol table
  exit
   -> valgrind prints failures

(the same applies to filters).  When valgrind is printing the
failures, it looks up the addresses in the symbol table, but that has
been unloaded already so all you see are "???", resulting in useless
messages like:

  ==14189== 49,152 bytes in 3 blocks are possibly lost in loss record 69 of 69
  ==14189==    at 0x4C2EBAB: malloc (vg_replace_malloc.c:299)
  ==14189==    by 0x7928C79: ???
  ==14189==    by 0x7928FEE: ???
  ==14189==    by 0x784662B: ???
  ==14189==    by 0x79329BA: ???
  ==14189==    by 0x78BFAC0: ???
  ==14189==    by 0x78C59C5: ???
  ==14189==    by 0x7841C4B: ???
  ==14189==    by 0x75FA074: ???
  ==14189==    by 0x40A512: plugin_register (plugins.c:749)
  ==14189==    by 0x404436: open_plugin_so (main.c:740)
  ==14189==    by 0x404436: main (main.c:565)

We can improve this using the valgrind macro RUNNING_ON_VALGRIND to
prevent calling dlclose(3).

I have made this opt-in since we don't generally want to make
production nbdkit binaries which rely on probing valgrind.  Developers
have to enable it using ./configure --enable-valgrind.  I also took
this opportunity to improve developer documentation.

With this change you should see full stack traces from plugins, eg:

  ==2441== 32,768 bytes in 2 blocks are possibly lost in loss record 73 of 78
  ==2441==    at 0x4C2EBAB: malloc (vg_replace_malloc.c:299)
  ==2441==    by 0x7928C79: GetBlocks (tclThreadAlloc.c:1044)
  ==2441==    by 0x7928C79: TclpAlloc (tclThreadAlloc.c:358)
  ==2441==    by 0x7928FEE: TclpRealloc (tclThreadAlloc.c:514)
  ==2441==    by 0x784662B: Tcl_Realloc (tclCkalloc.c:1145)
  ==2441==    by 0x79329BA: Tcl_DStringSetLength (tclUtil.c:2819)
  ==2441==    by 0x78BFAC0: Tcl_ExternalToUtfDString (tclEncoding.c:1155)
  ==2441==    by 0x78C59C5: TclSetupEnv (tclEnv.c:124)
  ==2441==    by 0x7841C4B: Tcl_CreateInterp (tclBasic.c:907)
  ==2441==    by 0x75FA074: tcl_load (tcl.c:54)
  ==2441==    by 0x40A5E2: plugin_register (plugins.c:750)
  ==2441==    by 0x404436: open_plugin_so (main.c:740)
  ==2441==    by 0x404436: main (main.c:565)
---
 README          | 15 ++++++++++++---
 configure.ac    | 17 +++++++++++++++++
 src/Makefile.am |  3 ++-
 src/filters.c   |  3 ++-
 src/internal.h  |  8 ++++++++
 src/plugins.c   |  3 ++-
 6 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/README b/README
index 29e16c3..e51b564 100644
--- a/README
+++ b/README
@@ -105,7 +105,7 @@ To run the test suite:
 
 To test for memory leaks ('make check-valgrind'):
 
- - valgrind
+ - valgrind program and development headers
 
 For non-essential enhancements to the test suite:
 
@@ -121,7 +121,6 @@ After installing any dependencies:
   ./configure                    ./configure
   make                           make
   make check                     make check
-  make check-valgrind            make check-valgrind
 
 To run nbdkit from the source directory, use the top level ./nbdkit
 script.  It will run nbdkit and plugins from the locally compiled
@@ -157,8 +156,18 @@ nbdkit + plugins as a captive process, and tests them using
 libguestfs.  If there is a failure, look at the corresponding
 tests/*.log file for debug information.
 
-You can run the tests under valgrind, if it is available, using:
+Developers
+----------
 
+Install the valgrind program and development headers.
+
+Use:
+
+  ./configure --enable-gcc-warnings --enable-valgrind
+
+When testing use:
+
+  make check
   make check-valgrind
 
 Packager information
diff --git a/configure.ac b/configure.ac
index 25851b1..16e3250 100644
--- a/configure.ac
+++ b/configure.ac
@@ -166,6 +166,23 @@ AS_IF([test "$GNUTLS_LIBS" != ""],[
 dnl Check for valgrind.
 AC_CHECK_PROG([VALGRIND],[valgrind],[valgrind],[no])
 
+dnl If valgrind headers are available (optional).
+dnl Since this is only useful for developers, you have to enable
+dnl it explicitly using --enable-valgrind.
+AC_ARG_ENABLE([valgrind],
+    AS_HELP_STRING([--enable-valgrind], [enable Valgrind extensions, for
developers]),
+    [enable_valgrind=yes],
+    [enable_valgrind=no])
+AS_IF([test "x$enable_valgrind" = "xyes"],[
+    PKG_CHECK_MODULES([VALGRIND], [valgrind], [
+        AC_SUBST([VALGRIND_CFLAGS])
+        AC_SUBST([VALGRIND_LIBS])
+        AC_DEFINE([HAVE_VALGRIND],[1],[Valgrind headers found at compile time])
+    ],[
+        AC_MSG_ERROR([--enable-valgrind given, but Valgrind headers are not
available])
+    ])
+])
+
 dnl Bash completion.
 PKG_CHECK_MODULES([BASH_COMPLETION], [bash-completion >= 2.0], [
     bash_completion=yes
diff --git a/src/Makefile.am b/src/Makefile.am
index 7ead75c..915efe4 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -63,7 +63,8 @@ nbdkit_CPPFLAGS = \
 nbdkit_CFLAGS = \
 	-pthread \
 	$(WARNINGS_CFLAGS) \
-	$(GNUTLS_CFLAGS)
+	$(GNUTLS_CFLAGS) \
+	$(VALGRIND_CFLAGS)
 nbdkit_LDADD = \
 	$(GNUTLS_LIBS) \
 	-ldl
diff --git a/src/filters.c b/src/filters.c
index 3d2c07e..18948bc 100644
--- a/src/filters.c
+++ b/src/filters.c
@@ -80,7 +80,8 @@ filter_free (struct backend *b)
   if (f->filter.unload)
     f->filter.unload ();
 
-  dlclose (f->dl);
+  if (DO_DLCLOSE)
+    dlclose (f->dl);
   free (f->filename);
 
   unlock_unload ();
diff --git a/src/internal.h b/src/internal.h
index ec19841..96a68f2 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -94,6 +94,14 @@
 # endif
 #endif
 
+#if HAVE_VALGRIND
+# include <valgrind.h>
+/* http://valgrind.org/docs/manual/faq.html#faq.unhelpful */
+# define DO_DLCLOSE !RUNNING_ON_VALGRIND
+#else
+# define DO_DLCLOSE 1
+#endif
+
 #define container_of(ptr, type, member) ({                       \
       const typeof (((type *) 0)->member) *__mptr = (ptr);       \
       (type *) ((char *) __mptr - offsetof(type, member));       \
diff --git a/src/plugins.c b/src/plugins.c
index 23223b3..a56ad79 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -73,7 +73,8 @@ plugin_free (struct backend *b)
   if (p->plugin.unload)
     p->plugin.unload ();
 
-  dlclose (p->dl);
+  if (DO_DLCLOSE)
+    dlclose (p->dl);
   free (p->filename);
 
   unlock_unload ();
-- 
2.17.1

On Sun, Jul 01, 2018 at 12:50:46PM +0100, Richard W.M. Jones
wrote:
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 7ead75c..915efe4 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -63,7 +63,8 @@ nbdkit_CPPFLAGS = \
>  nbdkit_CFLAGS = \
>  	-pthread \
>  	$(WARNINGS_CFLAGS) \
> -	$(GNUTLS_CFLAGS)
> +	$(GNUTLS_CFLAGS) \
> +	$(VALGRIND_CFLAGS)
Just a note that this also has to be added to test_utils_CFLAGS at the
bottom of the same file, otherwise the tests fail to compile.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 07/01/2018 10:01 AM, Richard W.M. Jones wrote:
> On Sun, Jul 01, 2018 at 12:50:46PM +0100, Richard W.M. Jones wrote:
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 7ead75c..915efe4 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -63,7 +63,8 @@ nbdkit_CPPFLAGS = \
>>   nbdkit_CFLAGS = \
>>   	-pthread \
>>   	$(WARNINGS_CFLAGS) \
>> -	$(GNUTLS_CFLAGS)
>> +	$(GNUTLS_CFLAGS) \
>> +	$(VALGRIND_CFLAGS)
> 
> Just a note that this also has to be added to test_utils_CFLAGS at the
> bottom of the same file, otherwise the tests fail to compile.
With that fix, ACK.

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