Improve finding memory leaks in GLib/GTK C (/C++) programs.
⇒ Improve GTK program quality and development time.
Situation:
GLib/Gtk does not free all of it’s acquired memory before application termination, because the operating system will free that memory anyway.
Valgrind or other tools claim detected memory leaks, that could actually be ignored.
Problem with the Situation
Depending on the tools one uses, it might be very difficult to distinguish between “ignorable leaks by glib/gtk” and user generated leaks, that require aid. Therefore, memory leaks might remain in program undetected.
Current workaround:
Memory leak detection tool, can be filled with suppression files. However, this suppression file needs to be available and available for the right version of glib/gtk the user is using. The suppression files might also not be enough and still let “ignorable” memory leaks through.
Suggested solution
I suggest a preprocessor flag, that would clean up all of GLib’s / GTK’s memory properly on application exit.
Discussion
I’d like to hear comments, how other people think about this. Maybe also a statement about the feasibility of this suggestion.
This has actually been discussed relatively recently here and (less recently) here.
tl;dr: It’s a huge amount of work, would likely result in libraries being broken or finding it easy to break other libraries, and wouldn’t provide much benefit over using a suppression file.
FWIW I consider the suppression file to be non-useful:
You have to remember a complex manual syntax to use it, passing the path to each desired suppression file on the command line.
It doesn’t scale. Even if GLib and GTK maintain suppression files, they’re hardly the only libraries in our stack. We’d need more files for pango, fontconfig, etc. etc. etc. All must be manually specified with a long command line.
It doesn’t work at all for asan, which is equally important.
I never use it.
The first two of those points could be fixed if valgrind were to support a standard suppressions directory that would be used automatically rather than manually, similar to how GLib’s gdb python extensions require zero effort to use. I bet 95% of our gdb extension users don’t even realize it exists. That would be a big step forward. But even that would do no good for asan, which is, again, just as important.
Normally it is not too hard to avoid leak complaints from valgrind. If you need a one-time “leak” of an object that should be valid for the duration of the program, you can keep a pointer to it in a static variable, then valgrind should not complain. GLib runs into trouble when it either doesn’t use a static variable to store the pointer, or keeps tricky pointers at offsets to the real allocated region.
Actually attempting to free memory in library destructors may break other libraries that rely on that memory in their own library destructors. So yes, that could potentially break other libraries in tricky ways.
But keeping a pointer to the “leaked” memory in a static variable without actually freeing it should be safe and uncontroversial.
I don’t want to re-do the same discussion. Instead, I’d like to get some more rationale, that I couldn’t find on the other topics ( I hope I didn’t overread them)
I don’t understand this reasoning because cleaning up memory would be the very last thing to do. If other libraries are still using it, it’s not the very last thing the programmer does. ⇒ Programming error
Can someone provide some numbers on how much time an implementation for a “dev-use only” cleanup function would take?
I have in mind something like I posted here (if possible):
Take a GQuark, for example, which is used as the basis of a lot of immutable identifiers in GLib. Data about GQuarks are stored in a hash table which is allocated once by GLib and never freed. It’s quite likely that any code run by any GLib-based library will use a GQuark, even during shutdown. That means the only safe place to free the GQuark hash table is right before return from main(). Even then, I don’t think that’s safe because there could be g_autoptr() variables defined in main() whose free functions will be called afterreturn from main(), and which could easily end up using a GQuark. There are probably also nasty interactions with linker destructors, depending on what order they’re run in compared to main() or destructors from other loaded libraries.
In order to do this safely you essentially have to free those allocations right before the heap itself is freed.
There is also anecdotal evidence (see a comment from Simon McVittie on one of those issues) that, when given a function which says ‘free everything’, programmers will misuse it and then file issues or complain when that subtly breaks everything. That’s an additional support cost which we would have to bear.
I suspect it would take lots of time. The places where GLib does one-time allocations are typically quite old, complex and fundamental parts of the type system. Any changes there may be complicated, and will certainly require thorough review.
I really think that it’s better to annotate the allocations at allocation time as one-time allocations, rather than trying to find an appropriate time to free them for all possible ways of running GLib code. If there’s a way to do that which works for valgrind and asan, that would be ideal.
I think the vast majority of cases can be handled as I said above: keep a pointer to the allocation in a static variable. I think that should generally placate both valgrind and asan. Of course, it’s not possible to do in all cases…
If so, that would be a great non-invasive solution.
I feel I was a bit negative and gatekeeper-y in my replies above. tl;dr: It would be fantastic if this could be fixed to make app developers’ lives easier, and minimally invasive merge requests which achieve that are very welcome. However, we have really limited review bandwidth, so it’s a waste of everyone’s time to work on fixes to this which are invasive or large, especially if they touch complex and critical code like the internals of GObject or GType. By all means look at making changes there, but if the resulting merge request is large or complicated, please don’t get your hopes up for it being reviewed, so be mindful of sinking too much time into exploring fixes if they look like they’re going that way.