GNOME Bugzilla – Bug 666114
should have infrastructure to run its tests under valgrind
Last modified: 2018-05-24 13:36:03 UTC
Bug #666113 could have been avoided if the GLib tests were semi-regularly run under valgrind. Patches on the way. Also, it'd be nice if we could install valgrind suppression files for at least some of the "intentional, one-per-process" allocations; at the moment, telepathy-glib and GStreamer both invent their own.
Created attachment 203372 [details] [review] Add valgrind infrastructure and empty suppression files "make valgrind" runs the tests (if any) under valgrind. The suppressions files used are also installed for the benefit of third-party tests, unless --without-valgrind-dir was passed to ./configure. The default location is ${libdir}/valgrind (matching the suppressions files installed by valgrind, ncurses and Python on Debian systems), but can be changed via ./configure --with-valgrind-dir=/where/ever.
Created attachment 203373 [details] [review] Fill in the valgrind suppression file for GLib Based on those in GStreamer and telepathy-glib.
Created attachment 203374 [details] [review] Suppress leaks caused by g_environ_setenv() Setting environment variables can leak, because this is the only feasible way to avoid potentially freeing non-malloc'd memory.
Created attachment 203375 [details] [review] Suppress GPrivate leaks: it cannot be freed
Created attachment 203376 [details] [review] Fill in some GObject suppressions
Created attachment 203377 [details] [review] Fill in some GIO-related suppressions
Is there really any advantage to having three separate suppression files? It just seems to make it more annoying to use, since you have to specify --suppressions= three times.
(In reply to comment #7) > Is there really any advantage to having three separate suppression files? Perhaps not, I'll squash them into one at the top level. (The idea was to have implementation details of libgfoo confined to the gfoo/ directory - but one suppressions file for all of GLib/GObject/GIO seems fine, really.)
Created attachment 203517 [details] [review] [1/2 v2] Add valgrind infrastructure and an empty suppression file
Created attachment 203518 [details] [review] [2/2 v2] Fill in the valgrind suppression file --- This isn't everything by any means, but it's a good start...
Comment on attachment 203517 [details] [review] [1/2 v2] Add valgrind infrastructure and an empty suppression file >+# analogous to AM_CFLAGS >+OUR_VALGRIND_FLAGS = \ >+# user can also override VALGRIND_FLAGS. Potentially useful ones That's actually backwards from how CFLAGS/AM_CFLAGS works. But this seems saner. I'd just remove the first comment.
Comment on attachment 203518 [details] [review] [2/2 v2] Fill in the valgrind suppression file >+{ >+ GTest initialization >+ Memcheck:Leak >+ ... >+ fun:g_test_init >+ fun:main >+} "fun:main" isn't really needed there. (And someone might call g_test_init() via a wrapper of some sort...) >+{ >+ GSLice initialization >+ Memcheck:Leak >+ ... >+ fun:g_*alloc* >+ fun:g_slice_init* >+ fun:g_slice_alloc* >+} g_slice_init() only mallocs if you don't do G_SLICE=always-malloc (ironically enough :-), in which case the suppressions file isn't going to be very useful anyway. >+{ >+ g_type_register_static >+ Memcheck:Leak >+ ... >+ fun:g_type_register_static >+} do g_*_register_static* and you can get a bunch more >+{ >+ g_param_spec_* >+ Memcheck:Leak >+ ... >+ fun:g_type_class_ref >+ ... >+ fun:g_param_spec_* >+} My initial thought was that this was too broad, but then I noticed that in http://git.gnome.org/browse/libsoup/tree/tests/libsoup.supp I have *all* calls to g_type_class_ref() marked as leaky... I guess 99% of the time types never get freed, so suppressing these warnings is worth losing the ability to catch refcounting bugs in the 1% case. [I'm planning to merge any remaining things from libsoup.supp into the glib suppressions file after this goes in and I update glib-networking and libsoup to make use of it.] > # -------------- Third-party --------------------------------------------- What is this section for? If it's "leaks in libraries that glib depends on", then shouldn't the getpwnam_r() leak be moved here?
(In reply to comment #11) > (From update of attachment 203517 [details] [review]) > >+# analogous to AM_CFLAGS > >+OUR_VALGRIND_FLAGS = \ > > >+# user can also override VALGRIND_FLAGS. Potentially useful ones > > That's actually backwards from how CFLAGS/AM_CFLAGS works. But this seems > saner. I'd just remove the first comment. Perhaps my comment was unclear (the "also" is a bit misleading); my intention was that OUR_VALGRIND_FLAGS contains settings by the Makefile.am author, whereas VALGRIND_FLAGS contains settings from the user (make VALGRIND_FLAGS="--track-fds" or whatever). This is analogous to Automake CFLAGS: AM_CFLAGS contains settings by the Makefile.am author, and CFLAGS is freely overridable by the user (automake-1.11.info.gz §27.6 Flag Variables Ordering). "User" here means whoever is compiling GLib.
(In reply to comment #12) > "fun:main" isn't really needed there. Good point. > g_slice_init() only mallocs if you don't do G_SLICE=always-malloc (ironically > enough :-), in which case the suppressions file isn't going to be very useful > anyway. OK, I'll drop that one. I think it came from telepathy-glib, probably from before we had much experience with valgrind. > do g_*_register_static* and you can get a bunch more Good idea. > My initial thought was that this was too broad, but then I noticed that in > http://git.gnome.org/browse/libsoup/tree/tests/libsoup.supp I have *all* calls > to g_type_class_ref() marked as leaky... I guess 99% of the time types never > get freed Perhaps we should split out a second suppressions file for things that are a bigger hammer than is really justified, including "all classes are allowed to leak"? Then developers of things that actually use dynamic types (GStreamer) could avoid using that one. > [I'm planning to merge any remaining things from libsoup.supp into the glib > suppressions file after this goes in and I update glib-networking and libsoup > to make use of it.] This is exactly what I was hoping would happen :-) > > # -------------- Third-party --------------------------------------------- > > What is this section for? If it's "leaks in libraries that glib depends on", > then shouldn't the getpwnam_r() leak be moved here? Yes, that's what it's for, and yes, the getpwnam_r() bit should move there.
(In reply to comment #13) > > That's actually backwards from how CFLAGS/AM_CFLAGS works. But this seems > > saner. I'd just remove the first comment. > > Perhaps my comment was unclear Huh, no, after reading the autoconf docs, it appears that I've just always misinterpreted the purpose of AM_CFLAGS... :-}
(In reply to comment #12) > "fun:main" isn't really needed there. fixed > g_slice_init() only mallocs if you don't do G_SLICE=always-malloc (ironically > enough :-), in which case the suppressions file isn't going to be very useful > anyway. fixed > do g_*_register_static* and you can get a bunch more fixed > >+ fun:g_type_class_ref > >+ ... > >+ fun:g_param_spec_* > > My initial thought was that this was too broad, but then I noticed that in > http://git.gnome.org/browse/libsoup/tree/tests/libsoup.supp I have *all* calls > to g_type_class_ref() marked as leaky... left as-is for the moment > > # -------------- Third-party --------------------------------------------- > > What is this section for? If it's "leaks in libraries that glib depends on", > then shouldn't the getpwnam_r() leak be moved here? done
Created attachment 204679 [details] [review] [2/2 v3] Fill in the valgrind suppression file
What's stopping this from being committed? +{ + unsetting environment variables can leak, this is unavoidable + Memcheck:Leak + ... + fun:g_strdup_printf + fun:g_environ_setenv +} This one is a false suppression, hiding a real leak: bug 669412.
Comment on attachment 204679 [details] [review] [2/2 v3] Fill in the valgrind suppression file >+{ >+ shared global default g_main_context >+ Memcheck:Leak >+ ... >+ fun:g_main_context_new >+ fun:g_main_context_default >+} This could lead to leaking data related to sources that are attached to the main context... although I guess that's unlikely, since in most cases, if you accidentally leave a source un-destroyed, it will end up triggering eventually, and presumably cause a crash, and then you'll figure it out. (Though, eg, GCancellableSource would never trigger if you leaked it, since you'd presumably no longer have access to its GCancellable.) But anyway... probably ok? >+ unsetting environment variables can leak, this is unavoidable as someone else noted, there were some bugs in how this was implemented. g_environ_setenv and g_environ_unsetenv should only be allowed to leak when called from g_setenv / g_unsetenv. >+{ >+ GPrivate cannot be freed, as per the docs >+ Memcheck:Leak >+ ... >+ fun:g_slice_alloc >+ fun:g_private_new >+} I don't think the "g_slice_alloc" is needed/useful there. >+{ >+ g_param_spec_* >+ Memcheck:Leak >+ ... >+ fun:g_type_class_ref >+ ... >+ fun:g_param_spec_* >+} I think you can make that "fun:g_param_spec_internal", and then drop the "fun:g_type_class_ref". >+{ >+ type_iface_vtable_base_init_Wm >+ Memcheck:Leak >+ ... >+ fun:type_iface_vtable_base_init_Wm >+ fun:g_type_class_ref >+} I'm not sure the g_type_class_ref adds anything there. (And type_iface_vtable_base_init_Wm is called from some other places.)
Having played around with this a bit, I'm thinking that actually freeing all the leaked memory (eg, bug 627423) may be a better approach than just suppressing leaks. At least in some cases. Eg, the suppressions file here includes: +{ + shared global default g_main_context + Memcheck:Leak + ... + fun:g_main_context_new + fun:g_main_context_default +} But then valgrind will still report that the default context's cached_poll_array member is leaked (because that gets created in g_main_context_iterate(), not g_main_context_new()). And there's no way to suppress that leak report for *just* the default context; we'd have to suppress it for *all* contexts, which would mean that it could end up hiding a real leak if the cached_poll_array cleanup got broken. Whereas just freeing the default context when the program exits solves this problem, and doesn't create any new ones. (Though that approach may not work for all cases... (gtype?))
(In reply to comment #20) > Having played around with this a bit, I'm thinking that actually freeing all > the leaked memory (eg, bug 627423) may be a better approach than just > suppressing leaks. If you can make that work reliably, great; but failing that, suppressing is better than nothing. If you don't like my choice of suppressions, can we at least have my first patch (with a comments-only suppressions file), so you can `make valgrind` and just get a lot of warnings? That would help to spot everything else that Valgrind notices (e.g. uses of uninitialized or freed memory), even if leaks are ignored. (It might need updates/merging for master, though; it's been a while.)
Created attachment 222537 [details] [review] Makefile.decl: update valgrind rules --verbose seems to just be a bother, so remove that. Add --show-reachable=yes, because that turns up important information too. Bump --num-callers up to 40, since in my experience 20 is occasionally too small to figure things out.
Created attachment 222538 [details] [review] glib-2.0.supp: updates... Also, move around a necessary leak in gtestutils into a place where it's easier to ignore reliably.
Comment on attachment 222538 [details] [review] glib-2.0.supp: updates... >+{ >+ glib worker thread >+ Memcheck:Leak >+ ... >+ fun:g_main_context_iteration >+ fun:glib_worker_main >+} (This was me being lazy; we definitely don't want to suppress *all* leaks inside g_main_context_iteration(), since that would suppress leaks in all source callbacks in the worker thread.) >+{ >+ auto-initialization of static GMutex >+ Memcheck:Leak >+ ... >+ fun:g_mutex_impl_new >+ fun:g_mutex_get_impl >+} so, since the docs explicitly say that you aren't supposed to free static mutexes/conds/etc, this is an example of something that maybe we would want to keep a suppressions file for...
I think this file should also take into consideration the false positive in bug 683094 related to g_closure_new_simple().
this is being worked on as part of bug 711744 *** This bug has been marked as a duplicate of bug 711744 ***
*** Bug 743541 has been marked as a duplicate of this bug. ***
Bug 711744 is going to take a long time (along with bug #627423 and bug #680832). In the meantime, it would be useful if GLib shipped a canonical Valgrind suppression file, as the patches here implement, so that people don’t keep writing their own. There is an up-to-date one here, which we might want to roll some changes back from: https://github.com/dtrebbien/GNOME.supp.git
Created attachment 338808 [details] [review] glib: Add installed Valgrind suppressions file for GLib and GIO While we cannot get Valgrind to automatically load this suppression file for applications which link to GLib (https://bugs.kde.org/show_bug.cgi?id=160905), we can at least install it on systems in a shared directory, so that developers can use a standardised (and up-to-date) suppressions file for GLib, rather than rolling their own. The file will typically be installed to: /usr/share/glib-2.0/valgrind/glib.supp Distributors: it is recommended that this suppression file be installed as part of the development package for GLib in your distribution.
Here's a slapdash attempt at this. It uses the suppressions file from Walbottle, which might not contain all of the suppressions from previous patches on this bug, but makes those tests (and those of a few other projects I've used it in) run Valgrind-clean. I would be unable to test other suppressions rules as easily. It provides a single file containing rules for both GLib and GIO, because making developers pass --suppressions=glib.supp --suppressions=gio.supp seems too cumbersome for any gain of keeping the symbols separate for the two libraries. The .supp file is currently in the root git directory for the same reason. Feedback very welcome.
I'm now a huge fan of `-fsanitize=address` (from the https://github.com/google/sanitizers/wiki suite).
Review of attachment 338808 [details] [review]: Yeah, let's just try this.
Comment on attachment 338808 [details] [review] glib: Add installed Valgrind suppressions file for GLib and GIO Attachment 338808 [details] pushed as a24f57b glib: Add installed Valgrind suppressions file for GLib and GIO
I’ve pushed the suppressions file (and it will be installed in $datadir by GLib). In order to close this bug I still need to update the patches adding a Valgrind mode for `make check`. I will try and find time for that; will probably end up using AX_VALGRIND_CHECK (https://www.gnu.org/software/autoconf-archive/ax_valgrind_check.html).
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/487.