GNOME Bugzilla – Bug 627423
Make GLib valgrind friendly
Last modified: 2018-05-24 12:40:45 UTC
Right now, the GLib library stack gets in the way for finding memleaks. It would be useful if there was a function, say, g_mem_shutdown() that would free all memory "leaked" by GLib.
Created attachment 168322 [details] [review] Proof of concept patch Five new functions are introduced here void g_mem_shutdown (void); gpointer g_mem_free_at_shutdown (gpointer mem); gpointer g_mem_slice_free_at_shutdown (gsize block_size, gpointer mem_block); gpointer g_mem_notify_at_shutdown (gpointer user_data, GDestroyNotify callback); void g_mem_remove_notify_at_shutdown (gpointer user_data, GDestroyNotify callback); g_mem_shutdown() is supposed to be called as the very last thing before main() exits. The reason g_mem_free_at_shutdown() returns a gpointer is for convenient usage / modification such as return g_mem_free_at_shutdown (g_new0 (Blah, num));
Example: [davidz@x61 tests]$ tail -14 shutdown-test.c int main (int argc, char *argv[]) { gint ret; ret = 0; g_type_init (); g_mem_shutdown (); return ret; } [davidz@x61 tests]$ make V=0 shutdown-test && ./shutdown-test && G_SLICE=always-malloc G_DEBUG=gc-friendly valgrind --leak-check=full --show-reachable=yes .libs/lt-shutdown-test make: `shutdown-test' is up to date. ==28253== Memcheck, a memory error detector ==28253== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al. ==28253== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info ==28253== Command: .libs/lt-shutdown-test ==28253== ==28253== ==28253== HEAP SUMMARY: ==28253== in use at exit: 0 bytes in 0 blocks ==28253== total heap usage: 409 allocs, 409 frees, 41,535 bytes allocated ==28253== ==28253== All heap blocks were freed -- no leaks are possible ==28253== ==28253== For counts of detected and suppressed errors, rerun with: -v ==28253== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 6 from 6)
$ diffstat glib-valgrind-friendly-proof-of-concept.patch glib/gdataset.c | 36 ++++++++++++++++- glib/gmem.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++ glib/gmem.h | 10 ++++ glib/gthread.c | 12 +++++ glib/gutils.c | 5 +- gobject/gparam.c | 2 gobject/gparamspecs.c | 9 ++++ gobject/gsignal.c | 21 ++++++++++ gobject/gtype.c | 86 ++++++++++++++++++++++++++++++++--------- gobject/gvalue.c | 8 +++ gthread/gthread-posix.c | 6 +- 11 files changed, 270 insertions(+), 25 deletions(-) (needless to say, this is a only a proof-of-concept - there are many other leaks that needs fixing too.)
Do you have any plans for how to handle threads (both created internally in say gio and possibly by the app?)
(In reply to comment #4) > Do you have any plans for how to handle threads (both created internally in say > gio and possibly by the app?) Yup - my initial idea was to just bail if more than the main thread exists and I still think this is sound. However, we probably need some way for libs and apps to get notified when they need e.g. worker threads - for example, GDBus needs that. Perhaps something like gpointer g_mem_notify_pre_shutdown (gpointer user_data, GDestroyNotify); that is like g_mem_notify_at_shutdown() that is called as the first thing of g_mem_shutdown(), e.g. void g_mem_shutdown (void) { for (callback, user_data) in pre_shutdown_callbacks: callback (user_data); if (num_threads() > 1) g_error ("stray threads detected, fail"); for (callback, user_data) in at_shutdown_callbacks: callback (user_data); /* free GMemShutdown resources */ } Do you think this could work?
see also bug 335126 it would be nice if, rather than actually freeing anything, we could just tell valgrind, "this memory block is expected to be in the still-reachable set at exit time". (Because then you don't need to worry about interactions between the various GDestroyNotifies, etc.)
What are the chances of something wanting to use this recently freed memory from an atexit(3)?
I'm really not in favour of the freelist stuff. Counting maybe.
Created attachment 168529 [details] [review] Updated patch Here's an updated patch The following code is now valgrind clean (AType is a trivial GObject subclass) and some of the gobject/gtype.c changes are cleaner. int main (int argc, char *argv[]) { gint ret; GObject *o; ret = 0; g_type_init (); o = g_object_new (a_type_get_type (), NULL); g_object_unref (o); g_mem_shutdown (); return ret; }
Review of attachment 168529 [details] [review]: ::: gobject/gobject.c @@ +322,1 @@ } This looks like a straight leak fix that should be committed independently ? ::: gobject/gparam.c @@ +1397,3 @@ info.n_preallocs = pspec_info->n_preallocs; info.instance_init = (GInstanceInitFunc) pspec_info->instance_init; + cinfo = g_mem_free_at_shutdown (g_new (ParamSpecClassInfo, 1)); Wouldn't it be more correct to use a class_finalize for this ?
Review of attachment 168529 [details] [review]: ::: glib/gdataset.c @@ +1050,3 @@ to strdup so that we fill our blocks at least 50%. */ if (len > QUARK_STRING_BLOCK_SIZE / 2) + return g_mem_free_at_shutdown (g_strdup (string)); We can probably get away without this. The strings that are duped here end up as keys in the hash table above, so you can free them from there. Just make sure to only free the strings that are > QUARK_STRING_BLOCK_SIZE / 2
Review of attachment 168529 [details] [review]: ::: glib/gmem.c @@ +1277,3 @@ + guint n; + + g_mutex_lock (gmem_shutdown_mutex); I wonder if using a g_mutex here is a good idea, considering one of the shutdown notifiers will be the one for this very mutex...
Compare with desrts patch here: bug 609281
Other bugs asking for this: bug 64096
(In reply to comment #8) > I'm really not in favour of the freelist stuff. Counting maybe. Although I really liked this approach at first, a few things came to mind after a good night's sleep: 1) It will lower the mental threshold for having global state where this is a bad architectural choice, but laziness makes it a very tempting thing to do. 2) Also lowers the threshold for premature optimizations using global state. 3) We cannot use it for cleaning up GSlice or GThread because the "free-list" implementation depends on these. So some kind of explicit deinit would be needed there. 4) Ordering might be a bit tricky. If two modules allocate resources lazily, and one depends on the other, the first might clean up right before it is used by the second one as part of its cleanup. This is probably mitigated by not allowing arbitrary callbacks on shutdown, but simply g_free/g_slice_free.
(In reply to comment #6) > see also bug 335126 > > it would be nice if, rather than actually freeing anything, we could just tell > valgrind, "this memory block is expected to be in the still-reachable set at > exit time". (Because then you don't need to worry about interactions between > the various GDestroyNotifies, etc.) While that would solve the Valgrind problem, it won't work with other profiling tools. Another issue is that libraries that use a private statically linked copy of GLib will not want to leave any garbage behind in the process. This is very important for projects like http://code.google.com/p/frida-ire/, where a shared library with its own copy of GLib statically linked in gets injected into already running processes, and later unloaded. And this could happen many times (every time the user of this debugger attaches to a process), and won't be too good for long-running processes, which will eventually run out of OS resources (TLS keys, memory, etc.).
(In reply to comment #5) > (In reply to comment #4) > > Do you have any plans for how to handle threads (both created internally in say > > gio and possibly by the app?) > > Yup - my initial idea was to just bail if more than the main thread exists and > I still think this is sound. Completely agree. Only apps (like test-suites) that know they've done their homework cleaning up things will want to call this API.
(In reply to comment #7) > What are the chances of something wanting to use this recently freed memory > from an atexit(3)? I think that's an acceptable compromise that users of g_mem_shutdown() will have to live with, IMHO. Don't think it would be acceptable if g_mem_shutdown() was automatically called from __attribute__((__destructor__)) or DllMain on DLL_PROCESS_DETACH, though.
For the record, I am entirely uninterested in use cases that involve 'privately linked, static copies' of glib.
Created attachment 171606 [details] [review] Another approach Here's another approach, though I'm not happy with all of it, but thought I'd throw it out there now that things are stable and 100% leak-free in my own applications. (Which includes both client- and server-side GDBus.)
Created attachment 171607 [details] [review] GDBus: Implement teardown of shared thread GDBus: Implement teardown of shared thread This bugfix is needed for leak-free GIO.
Created attachment 171608 [details] [review] GDBus: Implement teardown of shared thread GDBus: Implement teardown of shared thread This bugfix is needed for proper teardown of GIO.
Created attachment 171609 [details] [review] Fix leak in GFileAttributeInfoList Bugfix needed for leak-free GIO.
Created attachment 171610 [details] [review] Fix leak in GWinHttpVfs Bugfix needed for leak-free GIO.
Review of attachment 171608 [details] [review]: Worker may drop the last reference and hence try to join itself. Will investigate.
(In reply to comment #19) > For the record, I am entirely uninterested in use cases that involve 'privately > linked, static copies' of glib. Ok. Sorry for being so overly enthusiastic about this, it's just because I'm a very happy user of GLib in a specialized application where this is the only option. It is definitely not a good idea for regular applications, but I think it's a valuable thing to support given that it doesn't really impact regular use-cases in any negative way.
Created attachment 171620 [details] [review] Another approach Rolled GDBus shared thread teardown into the mix, not sure if there's a better way to fix this, so for now just keep an extra ref which gets unreffed as part of deinit.
*** Bug 609281 has been marked as a duplicate of this bug. ***
*** Bug 64096 has been marked as a duplicate of this bug. ***
*** Bug 651211 has been marked as a duplicate of this bug. ***
contrast bug 666114, which suggests using a valgrind suppressions file to hide the leaks. As mentioned in bug 666114 comment 20, I'm thinking now that the "free stuff on shutdown" approach is probably better. Of the two patches here, Ole's explicit shutdown stuff seems more likely to do the right thing than David's generic shutdown function registry (which seems like it would be doomed to clean things up in the wrong order some of the time... especially if it was getting used outside of glib as well). Some of the dup'ed bugs suggested using destructors to run the cleanup code, which would save us from even needing to add any new API at all (except maybe a gboolean g_mem_should_cleanup_on_exit() or whatever).
From the other bug: I favour a private API (for libglib use only) that runs as part of a dtor in libglib. In the event that the ultra-cleanup environment variable is not set, it would do nothing. If it is set, it would run the passed function at library unload time. We could then do something similar for libgobject and libgio. The only thing I'd make public would be a boolean API for "should do memory cleanup at shutdown?". Otherwise, let other libraries worry about themselves when it comes to keeping a list of who to call....
If made public, I like to use this in pango too. But public or private, dtor or not, just do it for freaking god's sake... Lets aim for actually closing this bug.
I certainly agree with this sentiment. My code has to run for years without restarting - and I can't do that unless I test for memory leaks when I _do_ exit my test cases. Glib makes that painful and ugly. Every release and many changes to my code add new exception cases to give Valgrind - just because of glib "leaks".
When I said "Every release" I meant "Every release of glib".
While this is good for testing, it's ironically bad for real usage, because it's quite simply far more efficient to just call _exit() (or be killed by a signal) and let the kernel clean everything up at once. This effect isn't too terrible for just one process, but since GNOME logins are still a bit of a swarm of processes, what you really don't want is e.g. on logout to wake up every process, have it call exit() and thus trigger destructors, call free() on a bunch of stuff, only to then later _exit(). So I like the environment variable approach over destructors.
the "free" branch of git://git.mysterion.org/glib (http://git.mysterion.org/glib/log/?h=free) has some patches from 6 months ago based on Ole's last patch. (It has a destructor that only does the cleanup if "cleanup" was specified in G_DEBUG.) To pre-emptively respond to the most obvious criticism of that code: the horror that is g_type_deinit() cannot actually be improved on much; trying to unref and free the class structs in any obvious way leads to crashes because of assumptions throughout the code that static types will never be freed. Fixing that would require making changes to some very tricky code, just to handle this use case which only ever occurs while exiting test programs. The ugly deinit() code seemed preferable to that.
the libglib.so cleanup code is mostly sane though... maybe we could start with that, and at least somewhat improve things for valgrinders. Or maybe a mix of this and the valgrind suppressions bug, with actual cleanup for some things, and a suppressions file for others
Environment variables are a _horrible_ way to configure applications - for example the ugly way debug was suppressed. For those applications that this is a concern for, let them have a debug flag or "check memory" flag. Please, don't make any more ugly environment variables - they completely violate the principle of least surprise. I don't think it should be built into exit, or similar, but provide a function to call that cleans everything up that isn't currently active. No one would call such a function unless they wanted this effect. If you don't want your program to do this, then don't call it. If you're concerned that your program might exit a lot, then put it on a debug flag. Feel free to document it that this is the proper way to use it. Most people won't bother to ever call this function. Don't make them do a g_setenv in order to actually make it work. Putting an environment variable to trigger this behavior means that you assume that your programmers are really stupid - which of course, just makes them more stupid.
Alan, you are missing the point in my opinion. Whoever is running valgrind is concerned about the cleanup, NOT the app developer. Forcing every app to add a switch and a function call at the end is plain wrong. What I want is to be able to set a couple env vars (I already have to set G_SLICE=always-malloc) when running valgrind, to get clean results. I don't want to have to modify my code. Now, it would be nice to also add a function, for apps that want to use it, but that doesn't address all usecases.
OK. I understand your concern. But it's also the case that the application developer will likely have to change their code to clean up things they know they'll need for the lifetime of the application - or it won't be clean. I'm not convinced you won't have to modify those apps anyway - or at least 90% of them. _Please_ also add a function (which is what I want, obviously), and _please_ don't make it so that I _also_ have to set an environment variable. [You've probably figured out I'm not a fan of the way you handled debug]. Given the fact that you have a large body of code with a number of semi-unresponsive developers being tested by other people (which is common in open source), having the env var would be a good way to make it so you can document what they still need to clean up. I got that. Thanks for clarifying! My particular case is that my code [http://assimmon.org] will run for the lifetime of the OS image - so I won't exit until shutdown. I'm already cleaning up my data structures and making sure I have zero active objects - but I'd love it if I could also clean up the glib structures as well. Then if I had any stray mallocs of strings that I lost track of, they would be very obvious. I don't think I knew about G_SLICE=always-malloc - that's good to know. I just put in a lot of exceptions for the glib code. I know, it's so odd that it's using glib but not a GUI app - but glib is so darn _handy_. I really love it.
Dan, how about you go ahead and push this in? If Matthias objects, you can revert.
(In reply to comment #42) > Dan, how about you go ahead and push this in? If Matthias objects, you can > revert. There are a few things here: * GLib isn't branched and GNOME 3.8 is in hard code freeze * Even then, it'd be good to at least attach patches here and have someone actually review them
Ya... srsly... GLib doesn't work this way.
Also: there is a very large unsolved problem here: the idea of where the freeing should be done. If we have a function that is called just before main() returns and we free a bunch of stuff inside of GObject then dtors (will run after main() returns) may call back into GObject and cause crashes. Doing it from the library dtor also has disadvantages: each separate library would have to maintain its own list in order for this to work properly and register its own dtor to do the work.
(In reply to comment #45) > If we have a function that is called just before main() returns ...then you can't valgrind a program without modifying it to call that function. I guess you could just have every program always call it (expecting it be a no-op in most cases), but that would be a weird regression to have a mandatory end-of-main function call, just after we got rid of the mandatory start-of-main call to g_type_init(). > Doing it from the library dtor also has disadvantages: each separate library > would have to maintain its own list in order for this to work properly and > register its own dtor to do the work. I'm not sure what you mean about "maintain its own list" (that would be any different between the explicit function and dtor cases). The dtor solution seemed to be nice as far as I got, though it's possible that there'd be messy interdependencies between libraries at a higher level that wouldn't work well with the strictly-ordered cleanup.
I'm working on a solution right now along these lines: #define G_CLEANUP_DEFINE \ GCleanupList _glib_cleanup_list; \ __attribute__((destructor)) static void _glib_do_cleanup (void) { \ g_cleanup_list_clear (&_glib_cleanup_list); \ } #define G_CLEANUP_ADD(data, func) \ G_STMT_START { \ extern GCleanupList _glib_cleanup_list; \ g_cleanup_list_add (&_glib_cleanup_list, (data), (func)); \ if (0) (func)(data); \ } G_STMT_END #define G_CLEANUP_REMOVE(data, func) \ G_STMT_START { \ extern GCleanupList _glib_cleanup_list; \ g_cleanup_list_remove (&_glib_cleanup_list, (data), (func)); \ if (0) (func)(data); \ } G_STMT_END and docs: /** * G_CLEANUP_DEFINE: * * Sets up the GLib memory cleanup infrastructure for a shared library * or executable program. This macro should be used exactly once per * shared library or executable. * * The macro defines a linked list to which cleanup functions will be * added if memory cleanup has been enabled. It also defines a * destructor function to free the items in this list on the current * module being unloaded (usually after main() returns). * * The list is declared with a visibility that makes it accessible to * other files in the same module but not visible outside of the module * (ie: G_GNUC_INTERNAL). The exact name or type used for the list is * an implementation detail that may be changed; the only supported way * to add items to the list is by way of G_CLEANUP_ADD(). * * Since: 2.38 **/ /** * G_CLEANUP_ADD: * @function: the function to call * @data: the data to free * * Marks an item to be freed when performing memory cleanup. * * If memory cleanup is enabled then @function will be called on @data * at the time that destructors are being run for the current module * (ie: at program exit or module unload). * * In order for this to work, G_CLEANUP_DEFINE needs to be used exactly * once somewhere in a source file in your module. * * Adding the same @data more than once is undefined. * * Since: 2.38 **/ /** * G_CLEANUP_REMOVE: * @function: the function that was to have been called * @data: the data that would have been freed * * Reverses the effect of a previous use of G_CLEANUP_ADD(). * * As an example, you might need to use this in the case of needing to * realloc() something that had been previously marked for cleanup. * * Since: 2.38 **/ Where: void g_cleanup_list_add (GCleanupList *list, GDestroyNotify func, gpointer data) { GCleanupNode *node; if (!g_cleanup_enabled) return; ... implementation ... } After thinking about it I believe this sort of approach is the only one that's sufficiently correct and also convenient enough that people would want to use it. I'm flexible on if we should actually create GCleanupList as a public API or just hard-code the functionality of the functions into the macros themselves... I sort of prefer having the API because one would expect that g_cleanup_list_add() is threadsafe and doing that inside of a macro just gets nasty, particularly when it comes to dealing with removes. The only issue I'm still really caught on is how pedantic we want to be about what it means to cleanup. Imagine this library: static gpointer my_state; void init_my_library (void) { my_state = g_malloc(); } The question comes: what is the proper way to do cleanup? Is it like so: void init_my_library (void) { my_state = g_malloc(); G_CLEANUP_ADD (g_free, my_state); } or is it like so: static void cleanup_my_library (void) { g_clear_pointer (&my_state, g_free); } void init_my_library (void) { my_state = g_malloc (); G_CLEANUP_ADD (cleanup_my_library); /* note: no data pointer */ } The second one seems sort of inconvenient but may actually be nicer for large libraries that have lots of stuff that needs to be freed. I guess it is better to have the data pointer to allow the possibility of both (and make it reasonable to give NULL for it -- so I should remove my remarks above about repeated data being undefined). This also means that it should be called "user_data" rather than "data" and the arguments should _definitely_ be in the normal callback order (ie: user_data follows callback) not the DestroyNotify order (ie: destructor follows data).
(In reply to comment #47) > GCleanupList _glib_cleanup_list; \ I think we have a convention that "glib" is only used in the names of glib-internal things. > After thinking about it I believe this sort of approach is the only one that's > sufficiently correct and also convenient enough that people would want to use > it. I don't think you can make 100% LIFO cleanup work. Eg, it's not safe to free a mutex until you know that you're not going to run any more code that might look at it, but there's no way to determine when that would be based only on G_CLEANUP_ADD calls. More generally, the fact that Resource X was first used after Resource Y does not guarantee that it won't still be needed after Resource Y is freed.
> I think we have a convention that "glib" is only used in the names of > glib-internal things. Note that this is not a symbol exported from libglib. It's a symbol that gets added to each module that uses our clean-up mechanism. > I don't think you can make 100% LIFO cleanup work. I'm not saying that this approach automatically solves everything, but it does solve the hard part: inter-module dependencies. dtors are run in a way such that a library does not get dtor'd until after all libraries using it already have been. This way we won't do something like tearing down GObject before GIO has cleaned up after itself. Solving the dependencies within a particular library is left unsolved, but in general: if things have no relationship to each other then you can free them in any order. If ordering is importing, add a cleanup function that calls stuff in the right order. One bit of trouble I run into now: all along I've been implicitly relying on the fact that destructors are clearly working well enough inside of GLib for us to start depending on them in a public header. This is true, but I notice this: Some compilers need #pragma to handle this, which does not work with macros, so the way you need to use this is (for constructors): #ifdef G_DEFINE_CONSTRUCTOR_NEEDS_PRAGMA #pragma G_DEFINE_CONSTRUCTOR_PRAGMA_ARGS(my_constructor) #endif G_DEFINE_CONSTRUCTOR(my_constructor) static void my_constructor(void) { in gconstructor.h which obviously makes this problematic to use from G_CLEANUP_DEFINE. This is apparently only needed for Visual Studio pre-2008 (which lacked _Pragma). I wonder If I care enough... Even if we need the pragma ugliness we could just instruct people "copy/paste this ugly bit of code into your library somewhere".
For now there's a pretty simple workaround: cleanup is only supported on compilers that allow for #pragma-less declaration of destructors. It's not like this is a feature that programs depend on... Meanwhile, bug 696504 considers the possibility of gconstructor becoming #pragma-free.
I've pushed a reasonably fleshed-out branch as wip/gcleanup. It should give an idea of the API as well as showing its use from libglib itself. A small indication that we're not completely wasting our time here is that I actually found a leak that was missed by valgrind before all this... The interaction with threading is... difficult, to say the least. It's clear at this point that, in cleanup mode, we will have to join worker threads at exit. Part of this is to clear the resources in use by those threads, but another part of it is to ensure that the thread-local-storage in use by those threads gets properly freed. Even then, we have a bit of a problem: the GPrivate data for the main thread itself is never freed. I briefly toyed with the idea of creating a new thread and killing the main thread in order to force the freeing before realising how crazy this is. I've added a g_private_reset() that can be used as a GDestroyNotify against a GPrivate to finish the cleanup in the main thread. The branch currently does this incorrectly: installing the handler each time the private is initialised (ie: per-thread) instead of once globally. This is going to need some more thinking... maybe I can find some insanely clever trick to get this working properly automatically... Also: with all the other noise gone, g_mutex_impl_new() and friends are sticking out quite badly on the valgrind output now. This is a tricky situation. The situation will go away on Linux when we get our native operations, but other platforms will still have this noise. We can't really solve it using G_CLEANUP_ADD() from the GMutex code, either, because that would install the cleanups in libglib for a mutex that may well be defined somewhere else. At least it's easy enough to install a valgrind suppression for these few special cases...
okay.. GMutex thing is actually easier than I thought. We don't actually have to clear the GMutex structure -- only the impl, and that's under control of GLib. Pushed another patch to deal with that.
Any ideas how this interacts with the C++ atexit cleanup of static objects ? if we have eg. struct Foo { ... ~Foo() { g_slice_free (GFooThing, member_baa); } }; static Foo global_foo; Then it would be nice if we could be certain that this was cleaned up before these glib atexit things etc. to avoid the inevitable issues there. Indeed - last I looked (and FWIW in the past I was rather an advocate for this sort of debug / shutdown method) - there were some really awful, twisty problems here that make it really hard to do it right in the general case. Still - if it is a debug-only method, enabled only for simple apps, that does nothing (ie. not hurting exit performance) unless some environment variable is set it can't do much harm I guess :-)
(In reply to comment #53) > Any ideas how this interacts with the C++ atexit cleanup of static objects ? if > we have eg. > > struct Foo { > ... > ~Foo() { g_slice_free (GFooThing, member_baa); } > }; > > static Foo global_foo; Destructors are handled in the order that you would expect, as per module dependency, so the c++ destructors will run first, then after all modules that depend on GLib are done cleaning up, GLib's destructors would run (causing gslice to de-init).
wip/gcleanup seems generally plausible to me, though it might make sense to finish cleanupifying glib before committing, to make sure it's all going to work. I would make g_cleanup_enabled an exported variable, and get rid of g_cleanup_is_enabled(). And then wrap G_CLEANUP_ADD, etc in "if (G_UNLIKELY (g_cleanup_enabled)) { ... }". Yes, then the code is behaving differently in the cleanup and non-cleanup cases, but I think that's preferable to doing the work to maintain the cleanup list when it's never going to be used... G_CLEANUP_DEFINE's docs should clarify that you put it at the top level of the .c file. Is LIFO-ness guaranteed within a given library? If so, that should be documented.
I generally want to avoid having exported global variable if possible. They're kinda strange -- the way they work if used from another program is that the program makes a private copy of it and records a relocation for copying GLib's copy into their private copy (which they then interpose over top of GLib's copy). This is why we had to use -Bsymbolic-functions and not -Bsymbolic. As far as 'not doing the work', note: void g_cleanup_list_add (GCleanupList *list, GCleanupFunc cleanup_func, gpointer user_data) { GCleanupNode *node; if (!g_cleanup_enabled) return; I figured that we may as well just have one call instead of a call to find out if we should make another call.... LIFO should be guaranteed, I guess....
(In reply to comment #56) > I generally want to avoid having exported global variable if possible... but we already use some, and we can't stop, for ABI reasons, so all the strangeness is going to be present either way. > As far as 'not doing the work', note: Well, that requires a function call still in the "no-op" case. But also, the reason I didn't notice that check was that I didn't look there, because I was assuming that the generic methods were supposed to work even if cleanup was not enabled. If they only work in the same situations as the macros themselves work in, then why wouldn't you want to just use the macros / why expose the possibility of multiple cleanup lists in the API? (Example of where you'd want to use GCleanupList in a non-G_DEBUG=cleanup situation: the filename_free_list you just added to gtestutils.c.) (Another possible example: G_MAIN_CONTEXT_CLEANUP_ADD(), which adds something to the currently-running GMainContext's cleanup_list, which will get cleared at the end of the current g_main_context_iterate()...)
We shouldn't need G_SLICE=always-malloc to make valgrind work either. I had an attempt at writing annotations a while back. See http://valgrind.10908.n7.nabble.com/Mempool-annotation-for-glib-gslice-td42665.html
Oh, I think the valgrind issues are fixed. Will try that again: https://bugs.kde.org/show_bug.cgi?id=254420
(In reply to comment #58) > We shouldn't need G_SLICE=always-malloc to make valgrind work either. As of https://git.gnome.org/browse/glib/commit/?id=00fbc2f0ce2fb65da1027485707fbac59b91a1ef you don't.
That just makes the declaration unnecessary. David was talking about making it unnecessary for gslice to switch modes at all.
Indeed. Although I wonder now if it is really necessary. I was thinking that switching to always-malloc mode would mask issues with people using g_slice for allocation and g_free() to free, and other similar errors — but that kind of thing isn't going to last long without being discovered even without valgrind, is it? Perhaps the always-malloc mode is fine.
Can someone look into bug 680832 also?
(In reply to comment #62) > Indeed. Although I wonder now if it is really necessary. I was thinking that > switching to always-malloc mode would mask issues with people using g_slice for > allocation and g_free() to free, and other similar errors — but that kind of > thing isn't going to last long without being discovered even without valgrind, > is it? Quite possibly won't last long. Still, gslice could do better there. See https://bugzilla.gnome.org/show_bug.cgi?id=335126#c23
Comment on attachment 168529 [details] [review] Updated patch Obsoleted by wip/gcleanup branch.
I've rebased wip/gcleanup, and will post additional work on this here as patches. Or Ryan, let me know if I should just push additional patches to wip/gcleanup?
Created attachment 258655 [details] [review] gcleanup: Fix noisy errors about taking address of static variables These are -Waddress errors, which make sense in general. We can do the NULL checks inside of the g_cleanup_xxx functions. ie: gthread.c: In function 'g_thread_self': gthread.c:1006:147: warning: the comparison will always evaluate as 'true' for the address of 'g_thread_specific_private' will never be NULL [-Waddress] G_CLEANUP_ADD (&g_thread_specific_private, g_private_reset); ^ gthread.c:1006:303: warning: the address of 'g_thread_specific_private' will always evaluate as 'true' [-Waddress] G_CLEANUP_ADD (&g_thread_specific_private, g_private_reset); ^
Created attachment 258656 [details] [review] gcleanup: Add in more gtestutils.c cleanups
Created attachment 258657 [details] [review] gcleanup: Add some debugging code for tracing gcleanup I used this while developing the remander of these patches, and it's very helpful to track things down. May need a more dynamic way to enable, such as using one of the slots in GCleanupList.
Created attachment 258658 [details] [review] gmain: Fix use of uninitialized memory in sigaction structure
Created attachment 258659 [details] [review] gcleanup: Use three passes to do cleanup This allows certain handlers to postpone their cleanup until later as long as these second handlers guarantee not to add anything else. Three passes is what's needed to make this scheme work. This is a simple solution for the GPrivate problem, and does not require g_private_reset(), which ended up leaking anyway, when shutdown handers used thread specific data. This patch also fixes crahses when g_mutex_init() or other threading xxx_init() functions were used. g_mutex_clear() removes the cleanup handler. Lastly by doing this GCleanupList does not require a lock that itself needs to be cleaned up.
Created attachment 258660 [details] [review] gthread: Remove g_private_reset() It's not necessary now that we're doing 3 cleanup passes.
Created attachment 258661 [details] [review] gerror: Don't leak when warning about overwriting an error While not strictly necessary, this fixes a false positive leak in the tests.
Created attachment 258662 [details] [review] gmain: Use a simpler method for cleanup of glib_worker thread Using g_main_context_invoke() here hits way too much code that has already been cleaned up at this point. Besides, this is simpler.
Created attachment 258663 [details] [review] option-context: Fix leaks in tests
Created attachment 258664 [details] [review] private: Fix memory leak in tests Don't use g_private_new(), it's deprecated, and leaks by definition.
Created attachment 258665 [details] [review] glib-tap.mk: Add a memcheck target to run valgrind
With these patches glib/tests passes valgrind --leak-check, as long as you have an appropriate default.supp for your glibc/ditsro. In Fedora that lives in: /usr/lib64/valgrind/default.supp To run valgrind on the tests: $ make -C glib/tests memcheck This has a multiple cleanup pass approach, where cleanup handlers add further handlers. This is actively used by GMutex, GPrivate and friends. More on that below. This fixes the GPrivate problem mentioned in comment #51, and no longer need ugly things like g_private_reset(). In addition it makes GCleanupList lockless, using atomic pointer ops instead. Please look over the atomic ops to make sure I got it right. About the passes: cleanup handlers can add other handlers. a) handlers often do this when they are the first thing to use a static GMutex, for example. This happens very often during our tests. b) In addition, to solve the GPrivate issue noted above, (as well as GMutex use-after-free problems) we have them do a two stage cleanup, where the first time their handler is called they simply add another handler for the next pass. Thus it can be demonstrated that a maxmimum of 3 passes are needed to make this work for libglib. We can choose whether we want to add a field for max number of passes in GCleanupList, but it seems that 3 is appropriate for any non-abusive use case. Yes, I *really* did try to make this work without passes. But it was extremely ugly, with cleanup implementation details leaking all over libglib. In addition there was still the GPrivate problem. So I think this is a good tradeoff between having a bit more complexity in the cleanup code vs. complexity everywhere else.
Review of attachment 171620 [details] [review]: This patch to longer applies, so I'm going to change its status. In general I think Ryan's approach makes more sense and is extensible to other glib based libraries.
(In reply to comment #78) > Yes, I *really* did try to make this work without passes. But it was extremely > ugly, with cleanup implementation details leaking all over libglib. FTR, I've re-pushed to wip/free an older branch that does entirely explicit cleanup, rather than cleanup-list-based cleanup, for comparison of ugliness. (But I think that with the addition of multiple passes, the cleanup-list-based approach will work, and end up being less ugly.)
Some more notes (for myself): * G_CLEANUP_REMOVE shouldn't be necessary. It's in efficient, and really people should only be G_CLEANUP_ADD'ing stuff that lives until module unload. * Because of GPrivate having a user defined callback, we probably need a max of 4 passes for libglib.
Created attachment 258694 [details] [review] glib-tap.mk: Add a memcheck target to run valgrind
Created attachment 258695 [details] [review] gcleanup: Export correctly and add proper version numbers
Created attachment 258696 [details] [review] gthread-posix: Don't use gslice allocate GRecMutex This leads to problems during cleanup, and seems strange to have locks defined in terms of things that need locking.
Created attachment 258697 [details] [review] boxed: Fix double free in boxed unit tests
Created attachment 258698 [details] [review] qdata: Fix leak in qdata unit tests
Created attachment 258699 [details] [review] signals: Fix memory leaks in signals unit tests
Created attachment 258700 [details] [review] gcleanup: Drop the G_CLEANUP_REMOVE() function and related func It shouldn't be necessary. Callers who want to stop or change a cleanup, can just keep around a pointer that they change or nullify. We *are* talking about static/global data here after all. In addition it was hard to get lockless operations here right, and G_CLEANUP_REMOVE implementation was really inefficient. We don't want to encourage people using this for things they really should be freeing themselves. Lastly clean up some old documentation
Created attachment 258701 [details] [review] gcleanup: Five passes are the cleanup maximum This is because GPrivate needs to be freed after the common folk, and yet still has a notify callback, which itself could need up to three passes to complete. This makes me itch to make this a parameter. Lets see.
Consider these accepted-commit_now: 292da62 gslice: don't misuse g_mutex_init() b0af517 grand: restructure a bit b43a0f7 tests: fix leak in mainloop test f7d7de8 gcleanup: Add in more gtestutils.c cleanups b0db989 gerror: Don't leak when warning about overwriting an error 27eb3f1 qdata: Fix leak in qdata unit tests a722110 signals: Fix memory leaks in signals unit tests > 04a2781 gcleanup: Fix noisy errors about taking address of static variables > These are -Waddress errors, which make sense in general. We can do > the NULL checks inside of the g_cleanup_xxx functions. except that you're checking that func != NULL, not that data != NULL > 51163a9 gcleanup: Add some debugging code for tracing gcleanup These make the functions more awkward to use for non-G_CLEANUP_ADD uses, which again raises the question of whether the functions are intended for generic use or not (comment 57). >+ fprintf (stderr, "GLib-Cleanup: added: %s %p\n", code_loc, user_data); g_printerr(). Or were you intentionally avoiding glib API? You should mention that in a comment if so. > b0db18d gmain: Fix use of uninitialized memory in sigaction structure The memset() is overkill. I think you just need "action.sa_flags = 0;". It's already clearing sa_mask. > 4c2c451 gcleanup: Use three passes to do cleanup I don't think any fixed maximum number of passes can work for all libraries, since the number of passes needed depends on how the different data types in the library are intertwined. It also seems inelegant to me to register cleanup functions that just say "haha, no, just kidding, here's the real cleanup function to do later". Possible alternative (not fully thought-through): could you specify the ordering statically? eg, G_CLEANUP_ADD(func, data, stage)? (Or have G_CLEANUP_ADD() imply stage 1, and have G_CLEANUP_ADD_LATE() for later stages.) And the library organizes its cleanup such that cleanup functions can call functions that will schedule cleanups for later stages, but not functions that will schedule cleanups in the current stage or earlier (and g_cleanup_add() would assert that.) (We'd probably want the stages to be an enum, not just hardcoded integers, in case we needed to add another stage in the middle later.) > c27b349 gmain: Use a simpler method for cleanup of glib_worker thread Hm... I think you need to do something (atomics, mutex, etc) to guarantee that the worker thread sees the updated value of glib_worker_quit_requested. > 3128498 option-context: Fix leaks in tests hm... what's the leak? > ad37106 private: Fix memory leak in tests No real reason to make it static. Also, it would be more current-glib-idiomatic to drop the GPrivate* variable and just use "&private" > 7ddbe43 gthread-posix: Don't use gslice allocate GRecMutex "allocateD" >+ if G_UNLIKELY (mutex == NULL) (I hate when people do this. It makes it look like it's some sort of special syntax extension, rather than just a macro definition that happens to expand to something with parentheses around it.) > fa64dbc boxed: Fix double free in boxed unit tests >- t = g_thread_self (); >+ t = g_thread_ref (g_thread_self ()); > g_value_take_boxed (&value, t); Just use g_value_set_boxed() instead of g_boxed_take_value().
Attachment 258661 [details] pushed as 2672228 - gerror: Don't leak when warning about overwriting an error Attachment 258698 [details] pushed as b49344c - qdata: Fix leak in qdata unit tests Attachment 258699 [details] pushed as 27da079 - signals: Fix memory leaks in signals unit tests
(In reply to comment #90) > Consider these accepted-commit_now: > > 292da62 gslice: don't misuse g_mutex_init() > b0af517 grand: restructure a bit > b43a0f7 tests: fix leak in mainloop test > b0db989 gerror: Don't leak when warning about overwriting an error > 27eb3f1 qdata: Fix leak in qdata unit tests > a722110 signals: Fix memory leaks in signals unit tests Done, except for this one: > f7d7de8 gcleanup: Add in more gtestutils.c cleanups > > 04a2781 gcleanup: Fix noisy errors about taking address of static variables > > > These are -Waddress errors, which make sense in general. We can do > > the NULL checks inside of the g_cleanup_xxx functions. > > except that you're checking that func != NULL, not that data != NULL Yes, true. I should change the commit message. Accepting NULL data is not the end of the world. > > 51163a9 gcleanup: Add some debugging code for tracing gcleanup > > These make the functions more awkward to use for non-G_CLEANUP_ADD uses, which > again raises the question of whether the functions are intended for generic use > or not (comment 57). > > >+ fprintf (stderr, "GLib-Cleanup: added: %s %p\n", code_loc, user_data); > > g_printerr(). Or were you intentionally avoiding glib API? You should mention > that in a comment if so. Yes, that's the case. Avoiding (most) glib API. In particular g_print, g_printerr, or g_log all take locks. Will mention this explicitly. > > b0db18d gmain: Fix use of uninitialized memory in sigaction structure > > The memset() is overkill. I think you just need "action.sa_flags = 0;". It's > already clearing sa_mask. Roger. > > 4c2c451 gcleanup: Use three passes to do cleanup > > I don't think any fixed maximum number of passes can work for all libraries, > since the number of passes needed depends on how the different data types in > the library are intertwined. > > It also seems inelegant to me to register cleanup functions that just say > "haha, no, just kidding, here's the real cleanup function to do later". > > Possible alternative (not fully thought-through): could you specify the > ordering statically? eg, G_CLEANUP_ADD(func, data, stage)? (Or have > G_CLEANUP_ADD() imply stage 1, and have G_CLEANUP_ADD_LATE() for later stages.) > And the library organizes its cleanup such that cleanup functions can call > functions that will schedule cleanups for later stages, but not functions that > will schedule cleanups in the current stage or earlier (and g_cleanup_add() > would assert that.) > > (We'd probably want the stages to be an enum, not just hardcoded integers, in > case we needed to add another stage in the middle later.) Yeah, I agree. I'll think about this more. It would be nice to have just two stages, normal and 'late'. But GPrivate ruins that. Will rework this to be less haphazard. > > c27b349 gmain: Use a simpler method for cleanup of glib_worker thread > > Hm... I think you need to do something (atomics, mutex, etc) to guarantee that > the worker thread sees the updated value of glib_worker_quit_requested. Nod. > > 3128498 option-context: Fix leaks in tests > > hm... what's the leak? The fact that g_option_context_parse() discards values from argv (and moves following values into their place. When argv arrives via main(), that's not a problem. When we use a strv with heap allocated strings, then it leaks. > > ad37106 private: Fix memory leak in tests > > No real reason to make it static. Also, it would be more current-glib-idiomatic > to drop the GPrivate* variable and just use "&private" K. > > 7ddbe43 gthread-posix: Don't use gslice allocate GRecMutex > > "allocateD" > > >+ if G_UNLIKELY (mutex == NULL) > > (I hate when people do this. It makes it look like it's some sort of special > syntax extension, rather than just a macro definition that happens to expand to > something with parentheses around it.) Well that's the style throughout the entirety of gthread-posix.c. > > fa64dbc boxed: Fix double free in boxed unit tests > > >- t = g_thread_self (); > >+ t = g_thread_ref (g_thread_self ()); > > g_value_take_boxed (&value, t); > > Just use g_value_set_boxed() instead of g_boxed_take_value(). Makes sense.
(In reply to comment #90) > > 51163a9 gcleanup: Add some debugging code for tracing gcleanup > > These make the functions more awkward to use for non-G_CLEANUP_ADD uses, which > again raises the question of whether the functions are intended for generic use > or not (comment 57). Marginally. Passing %NULL as @code_loc isn't the end of the world.
Have reworked things further in the wip/gcleanup branch. And put it to use, in all of libglib, libgobject and about half of gio (including gdbus). About 100 patches, found quite a few leaks and finalization issues in code, and obviously removed tons of leaks in tests. I'm splitting them up for review. This bug will track the actual GCleanup implementation. Other bugs will track its use (one per sub-library) as well as leak and patches.
Created attachment 259354 [details] [review] gconstructor: install as a proper header
Created attachment 259356 [details] [review] gcleanup: Implementation of GCleanupList and associated macros ... So here's the reworked GCleanup patch. Some notes. Now need to define a -DG_CLEANUP_SCOPE similar to -DG_LOG_DOMAIN. Without this defined no cleanup in built. The reason for this is so that macros like G_DEFINE_TYPE and G_PRIVATE_INIT can push stuff onto the appropriate GCleanupList. Anything with a callback needs to be cleaned up in the GCleanupList for the module in which the callback is defined. So we use G_CLEANUP_SCOPE to facilitate that. Why not just use _glib_cleanup_list? Because we can't assume that's defined in macros like G_DEFINE_TYPE and friends. So making it explicit which cleanup list the definitions are part of is important. G_CLEANUP_SCOPE is auto-defined to NULL if not defined, similar to G_LOG_DOMAIN. All the g_cleanup_list_xxx() functions accept NULL GCleanupList and do a noop. So this works as you would expect, no definition == no cleanup. Added concept of phases as Dan suggested. As we discovered earlier, LIFO ordering just plain doesn't work. The reason for this is that we extensively use init-on-demand in glib. For example, mutexes need to be cleaned up as one of the last things, but they're initialized on demand. Added macros G_CLEANUP_IN_PHASE and G_CLEANUP_FUNC_IN_PHASE Renamed g_claenup_list_add() to g_cleanup_list_push() Removed G_CLEANUP_REMOVE, remove is less done, and should just use the g_cleanup_list_remove() function. Made g_cleanup_list_remove() work during g_cleanup_list_clear() and made all of it completely thread safe. This is necessary for various things like GSubProcess cleanup and GThreadPool unused threads. Made g_cleanup_list_remove() fast, but not leak if another push follows. G_CLEANUP() is nicer than G_CLEANUP_ADD(), and is the most commonly used macro, so made it short. There are debug facilities, but I might take them out before we merge this. Removed preconditions in G_CLEANUP_ADD(). Very noisy compiler warnings. Updated docs for all this.
Created attachment 259357 [details] [review] gcleanup: Implementation of GCleanupList and associated macros Add a new type GCleanupList that stores a list of things to "clean up" when g_cleanup_list_clear() is called. More importantly, define some macros (G_CLEANUP_ADD, etc) that facilitate conditionally building a per-library/executable cleanup list if G_DEBUG=cleanup is specified. The cleanup list is run at destructor time. Concept and initial work: Ryan Lortie <desrt@desrt.ca>
(In reply to comment #97) > Created an attachment (id=259357) [details] [review] > gcleanup: Implementation of GCleanupList and associated macros > > Add a new type GCleanupList that stores a list of things to "clean up" > when g_cleanup_list_clear() is called. To me, the _clear() name suggests that it wipes the list without calling anything. How about _clean(), since the object is called cleanup?
Review of attachment 259357 [details] [review]: A few comments. ::: glib/gcleanup.h @@ +77,3 @@ + */ + +extern GCleanupList G_CLEANUP_SCOPE[1]; This is really bad. G_CLEANUP_SCOPE is defined to something like "glib". Defining a variable with that name is a horrible idea. Should define this to something that expands to something like _glib_cleanup_scope or something. Also, about the comment, I think in C, a constant pointer the way you want is the definition of an array. So I don't see this as a "C doesn't allow that" thing. Suggest removing the comment. @@ +118,3 @@ + if (0) (func) (data); \ + } G_STMT_END +#define G_CLEANUP_IN_PHASE(data, func, phase) \ If G_CLEANUP_SCOPE is not defined, I prefer that trying to add cleanup items fails at compile-time, to catch programmer errors. Though, I see how useful it is to be able to disable cleanup. Perhaps use a separate G_DISABLE_CLEANUP for that? ::: glib/glib-init.h @@ +27,3 @@ extern GLogLevelFlags g_log_always_fatal; extern GLogLevelFlags g_log_msg_prefix; +gboolean g_cleanup_enabled; extern?
Thanks for taking a look. (In reply to comment #99) > Review of attachment 259357 [details] [review]: > > A few comments. > > ::: glib/gcleanup.h > @@ +77,3 @@ > + */ > + > +extern GCleanupList G_CLEANUP_SCOPE[1]; > > This is really bad. G_CLEANUP_SCOPE is defined to something like "glib". > Defining a variable with that name is a horrible idea. Should define this to > something that expands to something like _glib_cleanup_scope or something. That was a misleading typo in the documentation. Fixed. See the first patch on bug #711744 for an example. It's easy to use an underscore if desired. > Also, about the comment, I think in C, a constant pointer the way you want is > the definition of an array. So I don't see this as a "C doesn't allow that" > thing. Suggest removing the comment. Makes sense. Done. > @@ +118,3 @@ > + if (0) (func) (data); \ > + } G_STMT_END > +#define G_CLEANUP_IN_PHASE(data, func, phase) \ > > If G_CLEANUP_SCOPE is not defined, I prefer that trying to add cleanup items > fails at compile-time, to catch programmer errors. We need to be able to use G_CLEANUP() in macros in glib and other header files (like G_DEFINE_TYPE or G_PRIVATE_INIT), so that they add a cleanup to the appropriate GCleanupList if cleanup is enabled in the project using glib. Usage of this on bug #711744. However obviously those same header files still need to be usable without cleanup. So that's what G_CLEANUP_SCOPE is used for. > ::: glib/glib-init.h > @@ +27,3 @@ > extern GLogLevelFlags g_log_always_fatal; > extern GLogLevelFlags g_log_msg_prefix; > +gboolean g_cleanup_enabled; > > extern? Done
(In reply to comment #98) > (In reply to comment #97) > > Created an attachment (id=259357) [details] [review] [details] [review] > > gcleanup: Implementation of GCleanupList and associated macros > > > > Add a new type GCleanupList that stores a list of things to "clean up" > > when g_cleanup_list_clear() is called. > > To me, the _clear() name suggests that it wipes the list without calling > anything. How about _clean(), since the object is called cleanup? Sounds good. Done.
Created attachment 259395 [details] [review] gcleanup: Implementation of GCleanupList and associated macros Add a new type GCleanupList that stores a list of things to "clean up" when g_cleanup_list_clear() is called. More importantly, define some macros (G_CLEANUP, etc) that facilitate conditionally building a per-library/executable cleanup list if G_DEBUG=cleanup is specified. The cleanup list is run at destructor time. -DG_CLEANUP_SCOPE defines the name of the cleanup list and enables the feature for a given module. Concept and initial work: Ryan Lortie <desrt@desrt.ca>
Created attachment 259413 [details] [review] gcleanup: Implementation of GCleanupList and associated macros Updated patch logging and a logic fix on item reuse.
The dependent tree of bugs implement cleanup throughout libglib, libgmodule, libgobject, and about half of libgio. All reworked and pushed to wip/gcleanup, a little over 100 patches in all so far. I've renamed the old wip/gcleanup to wip/gcleanup-desrt A 'make memcheck' target is implemented. For the first three, all related tests pass without leaks. libgio is pretty tough, given gdbus, but I've done about half of it. Further work is needed on libgio for some corner cases with threads and worker tasks, that I've seen crop up. But in general the current gcleanup patch approach gives the flexibility to make a complex code base provably not leak. Actual leaks, crashers, races, discovered during this work are separated into separate patches and bugs, so they can be merged early on.
On GCleanupList: I'm thinking maybe changing the term s/phase/priority/ ... that's a better more familiar term for what it does. Nearly half the patches (mostly ones that fix leaks, crashes) have been merged. Thanks for the reviews so far.
After discussion on IRC, prototyped a facility for cleaning up certain qdata from a GDataList when a cleanup scope is cleaned. Needs more testing ... on wip/gcleanup: https://git.gnome.org/browse/glib/commit/?h=wip/gcleanup&id=6f5c379d5902d2f008a8fc7c936d9e2d6175c25a Removed the wip/gcleanup commit for g_type_set_qdata_full(). We don't need it. People can use gcleanup to clean up such per-GType qdata.
It would be good to rebase the branch to get the already-committed patches out of the way. > gcleanup: Implementation of GCleanupScope and associated macros >+ * G_CLEANUP_SCOPE: need to state that it must be a valid C identifier >+ * For example, you might use this in its Makefile.am: >+ * |[ >+ * INCLUDES = -DG_CLEANUP_SCOPE=my_app_cleanup >+ * ]| Makefile.am: warning: 'INCLUDES' is the old name for 'AM_CPPFLAGS' (or '*_CPPFLAGS') >+ * G_CLEANUP_PHASE_GRAVEYARD: >+ * >+ * Special extremely late cleanup items. Rarely used outside of GLib. By >+ * convention, cleanup items running in this phase should not only use lower >+ * level facilitities, and not run other parts of the library's code. I don't think you can safely claim it's "rarely used outside of GLib" until we've ported several non-GLib modules to use GCleanup... "should not only use" should be "should only use", but isn't that statement true for all phases? The cleanup functions in a given phase can't cause cleanups from any earlier phase to be added? >+ * NOTE: most glib functions are off limits in this function without "in this FILE" >+/* As good a place as any to put this... */ >+G_CLEANUP_DEFINE; glib-init.c would be better really, to more clearly distinguish the generic gcleanup mechanism from libglib's specific use of it. >+ * We use the bit locks, as they don't need cleanup themselves. That's only true on platforms that have both futex(2) and built-in atomic operations. If they lack either of those, then bitlocks will sometimes end up using GMutex. (Likewise throughout gcleanup.c; atomic ops will use GMutexes on some platforms. Though we might just want to declare that gcleanup won't work on such platforms.) >+ * Item removal is optimized for removal during cleanup. However in the case >+ * of repeated removal/push during the source of the process (ie: before >+ * cleanup has begun), we don't want the GCleanupScope to become a memory >+ * leak. >+ * >+ * So we reuse removed nodes here. This is a lot of complication for a use case that you explicitly discourage anyway... > memcheck: Add a 'make memcheck' target that runs valgrind are you sure those pthread_create / pthread_exit suppressions are needed, and not caused by something glib is still failing to clean up?
See bug 711744 for the current effort at this *** This bug has been marked as a duplicate of bug 711744 ***
This is not a duplicate bug #711744 is about implementing GCleanup in the glib/glib subdirectory for libglib.so.
*** Bug 740317 has been marked as a duplicate of this bug. ***
*** Bug 696201 has been marked as a duplicate of this bug. ***
*** Bug 759580 has been marked as a duplicate of this bug. ***
-- 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/333.