Bug 37286

Summary: optionally hook into valgrind to make debugging message leaks feasible
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: enhancement    
Priority: medium CC: hp, ross, will
Version: 1.5Keywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.freedesktop.org/~smcv/dbus/log/?h=valgrind-37286
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 38005    
Bug Blocks: 36164    
Attachments: [1/7] Add support for inserting (a subset of) Valgrind client requests
[2/7] DBusMemPool: inform valgrind what we're up to
[3/7] dbus_message_cache_or_finalize: allow message cache to be disabled at runtime
DBusMessage: provide a hook to allow refcounting to be traced
DBusPendingCall: optionally trace refcount changes
DBusServer: optionally trace refs, unrefs
DBusConnection: optionally trace refs/unrefs
[4/7 v2] DBusMessage: provide a hook to allow refcounting to be traced
[5/7 v2] DBusPendingCall: optionally trace refcount changes
[6/7 v2] DBusServer: optionally trace refs, unrefs
[7/7 v2] DBusConnection: optionally trace refs/unrefs
[1/8 v2] Add support for inserting (a subset of) Valgrind client requests
[2/8 v2] DBusMemPool: inform valgrind what we're up to
[3/8 v2] dbus_message_cache_or_finalize: allow message cache to be disabled at runtime
[4/8] Provide a hook to allow refcounting to be traced
[5/8 v3] Add and use _dbus_message_trace_ref
[6/8 v3] add and use _dbus_pending_call_trace_ref
[7/8 v3] add and use _dbus_server_trace_ref
[8/8 v3] add and use _dbus_connection_trace_ref
Turn the non-valgrind code path into inline functions to avoid compiler warnings

Description Simon McVittie 2011-05-17 06:02:30 UTC
libdbus has some features which make tracking down DBusMessage leaks a pain:

- Unused DBusMessage instances are returned to a cache instead of being
  freed; debug builds, at least, should allow this to be turned off
  (a runtime version of part of Bug #35090).

- DBusMessage is refcounted, so the stack trace when it's allocated is
  fairly meaningless - it's nearly always going to be the point at which
  the message is received. The actual leak comes later, and isn't seen by
  valgrind. GObject solves this with refdbg, but valgrind has its own way
  to produce backtraces, which might also be useful.

DBusMemPool is also not very valgrind-friendly: it makes valgrind unable to see whether we've leaked things.
Comment 1 Simon McVittie 2011-05-17 06:07:52 UTC
Created attachment 46808 [details] [review]
[1/7] Add support for inserting (a subset of) Valgrind client  requests

Usage:

./configure --without-valgrind (equivalent to the default)
./configure --with-valgrind
./configure --with-valgrind="-I /opt/valgrind/include"

If valgrind support is disabled, we define stub versions of the
Valgrind client requests I plan to use, so the actual code doesn't need
#ifdef hell.

---

Note that this has no runtime overhead if disabled, and small runtime overhead if enabled (currently 6 integer operations - valgrind detects these operations and behaves specially when it interprets them).

An alternative way to do this would be to copy valgrind.h and memcheck.h into libdbus directly (they're BSD-licensed), and define NVALGRIND (analogous to NDEBUG) if instrumentation is not wanted.
Comment 2 Simon McVittie 2011-05-17 06:08:33 UTC
Created attachment 46809 [details] [review]
[2/7] DBusMemPool: inform valgrind what we're up to

If we tell valgrind what we're doing, it can give better diagnostics.
Comment 3 Simon McVittie 2011-05-17 06:09:52 UTC
Created attachment 46810 [details] [review]
[3/7] dbus_message_cache_or_finalize: allow message cache to  be disabled at runtime

This should make it easier to diagnose message-related ref leaks,
use-after-free, etc. with Valgrind: for optimal results (and pessimal
performance), we want to avoid re-using memory blocks for as long as
possible.

For now this is conditional on DBUS_BUILD_TESTS. It could get its own
conditional if desired.

---

If desired this could also be combined with something like Bug #35090 to have a tristate at compile-time: always disabled / switchable at runtime / always enabled.
Comment 4 Simon McVittie 2011-05-17 06:10:28 UTC
Created attachment 46811 [details] [review]
DBusMessage: provide a hook to allow refcounting to be  traced

This has several functions:

* when under gdb, provide a function which can be used in breakpoints
* when not under valgrind and DBUS_MESSAGE_TRACE=1 is set, emit a
  _dbus_verbose when a message's refcount changes
* when under valgrind and DBUS_MESSAGE_TRACE=1 is set, emit a 
  VALGRIND_PRINTF_BACKTRACE when a message's refcount changes,
  which lets you see the complete history of each message to track down
  reference leaks
 
In addition to refcount changes, we also trace on ownership changes that
don't change the refcount.

Compile-time support is currently conditional on DBUS_ENABLE_VERBOSE_MODE,
but could be separated out if desired.
Comment 5 Simon McVittie 2011-05-17 06:10:44 UTC
Created attachment 46812 [details] [review]
DBusPendingCall: optionally trace refcount changes
Comment 6 Simon McVittie 2011-05-17 06:11:00 UTC
Created attachment 46813 [details] [review]
DBusServer: optionally trace refs, unrefs
Comment 7 Simon McVittie 2011-05-17 06:11:18 UTC
Created attachment 46814 [details] [review]
DBusConnection: optionally trace refs/unrefs
Comment 8 Simon McVittie 2011-06-23 07:51:28 UTC
Created attachment 48344 [details] [review]
[4/7 v2] DBusMessage: provide a hook to allow refcounting to be  traced

Rebased onto the "always use atomic ops" fixes from Bug #38005.
Comment 9 Simon McVittie 2011-06-23 07:51:52 UTC
Created attachment 48345 [details] [review]
[5/7 v2] DBusPendingCall: optionally trace refcount changes

Rebased onto the "always use atomic ops" fixes from Bug #38005.
Comment 10 Simon McVittie 2011-06-23 07:52:13 UTC
Created attachment 48346 [details] [review]
[6/7 v2] DBusServer: optionally trace refs, unrefs
Comment 11 Simon McVittie 2011-06-23 07:52:38 UTC
Created attachment 48347 [details] [review]
[7/7 v2] DBusConnection: optionally trace refs/unrefs

Rebased onto the "always use atomic ops" fixes from Bug #38005.
Comment 12 Simon McVittie 2011-09-19 08:08:34 UTC
Created attachment 51336 [details] [review]
[1/8 v2] Add support for inserting (a subset of) Valgrind client requests

Rebased onto master
Comment 13 Simon McVittie 2011-09-19 08:11:53 UTC
Created attachment 51337 [details] [review]
[2/8 v2] DBusMemPool: inform valgrind what we're up to
Comment 14 Simon McVittie 2011-09-19 08:42:29 UTC
Created attachment 51339 [details] [review]
[3/8 v2] dbus_message_cache_or_finalize: allow message cache to be disabled at runtime

Basically the same as Attachment #46810 [details]
Comment 15 Simon McVittie 2011-09-19 08:43:29 UTC
Created attachment 51340 [details] [review]
[4/8] Provide a hook to allow refcounting to be traced

This is designed to be used from a wrapper function, partly to supply
the same arguments every time for a particular class of object, and partly
to provide a more specific gdb breakpoint. It has several purposes:

* when under gdb, provide a function which can be used in breakpoints
* when not under valgrind and DBUS_MESSAGE_TRACE=1 is set, emit a
  _dbus_verbose when a message's refcount changes
* when under valgrind and DBUS_MESSAGE_TRACE=1 is set, emit a
  VALGRIND_PRINTF_BACKTRACE when a message's refcount changes,
  which lets you see the complete history of each message to track down
  reference leaks

---
New patch factoring out the common part of the old 4/7 to 7/7.
Comment 16 Simon McVittie 2011-09-19 08:44:18 UTC
Created attachment 51341 [details] [review]
[5/8 v3] Add and use _dbus_message_trace_ref
Comment 17 Simon McVittie 2011-09-19 08:44:59 UTC
Created attachment 51342 [details] [review]
[6/8 v3] add and use _dbus_pending_call_trace_ref
Comment 18 Simon McVittie 2011-09-19 08:47:10 UTC
Created attachment 51343 [details] [review]
[7/8 v3] add and use _dbus_server_trace_ref
Comment 19 Simon McVittie 2011-09-19 08:48:14 UTC
Created attachment 51344 [details] [review]
[8/8 v3] add and use _dbus_connection_trace_ref
Comment 20 Lennart Poettering 2012-02-09 16:37:30 UTC
Comment on attachment 51336 [details] [review]
[1/8 v2] Add support for inserting (a subset of) Valgrind client requests

Review of attachment 51336 [details] [review]:
-----------------------------------------------------------------

::: configure.ac
@@ +1144,5 @@
> +  AC_DEFINE([WITH_VALGRIND], [1], [Define to add Valgrind instrumentation])
> +fi
> +
> +AC_SUBST([VALGRIND_CFLAGS])
> +AC_SUBST([VALGRIND_LIBS])

The AC_SUBST is done in the PKG_CHECK_MODULES anyway and hence redundant.
Comment 21 Lennart Poettering 2012-02-09 16:38:58 UTC
Comment on attachment 51337 [details] [review]
[2/8 v2] DBusMemPool: inform valgrind what we're up to

Review of attachment 51337 [details] [review]:
-----------------------------------------------------------------

There's some whitespace spew in this one, but I wouldn't care.

(We really should make D-Bus whitespace clean. Dealing with witespace fuckups really makes D-Bus hard to work with)
Comment 22 Lennart Poettering 2012-02-09 16:41:21 UTC
Comment on attachment 51339 [details] [review]
[3/8 v2] dbus_message_cache_or_finalize: allow message cache to be disabled at runtime

Review of attachment 51339 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 23 Lennart Poettering 2012-02-09 16:45:54 UTC
Comment on attachment 51341 [details] [review]
[5/8 v3] Add and use _dbus_message_trace_ref

Review of attachment 51341 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 24 Lennart Poettering 2012-02-09 16:47:45 UTC
Comment on attachment 51340 [details] [review]
[4/8] Provide a hook to allow refcounting to be traced

Review of attachment 51340 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-internals.c
@@ +489,5 @@
> +      _dbus_assert (new_refcount >= 0);
> +      _dbus_assert (old_refcount >= 0);
> +      _dbus_assert (old_refcount > 0 || new_refcount > 0);
> +    }
> +

Hmm, an if branch just for asserts is something I haven't seen that often... I mean, the compiler will hopefully optimize this away when asserts are disabled, but it still makes me wonder when looking over this.

I understand that placing this all in a single assert would be ugly to read though, hence I guess it is OK.
Comment 25 Lennart Poettering 2012-02-09 16:48:57 UTC
Comment on attachment 51342 [details] [review]
[6/8 v3] add and use _dbus_pending_call_trace_ref

Review of attachment 51342 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 26 Lennart Poettering 2012-02-09 16:58:26 UTC
Comment on attachment 51343 [details] [review]
[7/8 v3] add and use _dbus_server_trace_ref

Review of attachment 51343 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 27 Lennart Poettering 2012-02-09 17:00:06 UTC
Comment on attachment 51344 [details] [review]
[8/8 v3] add and use _dbus_connection_trace_ref

Review of attachment 51344 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 28 Simon McVittie 2012-02-10 02:28:27 UTC
(In reply to comment #21)
> There's some whitespace spew in this one, but I wouldn't care.

Yeah, I'd vaguely been going for "remove trailing whitespace if I happen to be touching an adjacent line, but otherwise leave it alone".

(In reply to comment #24)
> I understand that placing this all in a single assert would be ugly to read
> though, hence I guess it is OK.

I'm mainly concerned about the quality of the assertion message if it fails: assertions of the form "assert (x && y && !z)" leave you wondering what was actually wrong.

(In reply to comment #20)
> > +AC_SUBST([VALGRIND_CFLAGS])
> > +AC_SUBST([VALGRIND_LIBS])
> 
> The AC_SUBST is done in the PKG_CHECK_MODULES anyway and hence redundant.

Ah, good to know. I'll audit our other uses of PKG_CHECK_MODULES for this.
Comment 29 Simon McVittie 2012-02-13 10:23:36 UTC
Fixed in git for 1.5.10, thanks for reviewing
Comment 30 Simon McVittie 2012-02-13 11:37:34 UTC
Created attachment 56984 [details] [review]
Turn the non-valgrind code path into inline functions to avoid compiler warnings

Recent gcc will warn if you have a statement that's just a macro
expanding to (0), but not if you have an inline stub function that
always returns 0, so let's do the latter.
Comment 31 Guillaume Desmottes 2012-02-20 05:16:35 UTC
Comment on attachment 56984 [details] [review]
Turn the non-valgrind code path into inline functions to avoid compiler warnings

Review of attachment 56984 [details] [review]:
-----------------------------------------------------------------

Maybe add a comment explaining this is done to keep gcc happy?
Comment 32 Simon McVittie 2012-02-21 07:06:43 UTC
(In reply to comment #31)
> Maybe add a comment explaining this is done to keep gcc happy?

Done and merged, thanks.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.