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.
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.
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.
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.
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.
Created attachment 46812 [details] [review] DBusPendingCall: optionally trace refcount changes
Created attachment 46813 [details] [review] DBusServer: optionally trace refs, unrefs
Created attachment 46814 [details] [review] DBusConnection: optionally trace refs/unrefs
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.
Created attachment 48345 [details] [review] [5/7 v2] DBusPendingCall: optionally trace refcount changes Rebased onto the "always use atomic ops" fixes from Bug #38005.
Created attachment 48346 [details] [review] [6/7 v2] DBusServer: optionally trace refs, unrefs
Created attachment 48347 [details] [review] [7/7 v2] DBusConnection: optionally trace refs/unrefs Rebased onto the "always use atomic ops" fixes from Bug #38005.
Created attachment 51336 [details] [review] [1/8 v2] Add support for inserting (a subset of) Valgrind client requests Rebased onto master
Created attachment 51337 [details] [review] [2/8 v2] DBusMemPool: inform valgrind what we're up to
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]
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.
Created attachment 51341 [details] [review] [5/8 v3] Add and use _dbus_message_trace_ref
Created attachment 51342 [details] [review] [6/8 v3] add and use _dbus_pending_call_trace_ref
Created attachment 51343 [details] [review] [7/8 v3] add and use _dbus_server_trace_ref
Created attachment 51344 [details] [review] [8/8 v3] add and use _dbus_connection_trace_ref
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 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 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 on attachment 51341 [details] [review] [5/8 v3] Add and use _dbus_message_trace_ref Review of attachment 51341 [details] [review]: ----------------------------------------------------------------- Looks good.
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 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 on attachment 51343 [details] [review] [7/8 v3] add and use _dbus_server_trace_ref Review of attachment 51343 [details] [review]: ----------------------------------------------------------------- Looks good.
Comment on attachment 51344 [details] [review] [8/8 v3] add and use _dbus_connection_trace_ref Review of attachment 51344 [details] [review]: ----------------------------------------------------------------- Looks good.
(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.
Fixed in git for 1.5.10, thanks for reviewing
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 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?
(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.