GNOME Bugzilla – Bug 698595
the valgrind/priv-before-instance bug
Last modified: 2013-04-26 16:00:57 UTC
There are lots of reasons to want the private data before the instance. The two biggest ones: - having the priv at a constant offset to the instance pointer means that we can stop putting ->priv pointers in our structures and instead statically store an integer offset and use it directly - it makes it possible to have GProperty without having code to jump through ->priv pointers (which would only work in the case that you actually have a ->priv pointer to jump through). This solves the main issue that is preventing GProperty from landing. Relatedly: in order for this priv-before-instance stuff to not cause Valgrind to become completely useless, we need to add some client requests to inform Valgrind of what's going on. We take a copy of BSD-licenced valgrind.h into glib (not to inflate our build-deps) and since we have it, use it to address another common Valgrind pain: disable gslice when running under it.
Created attachment 242153 [details] [review] Add a copy of valgrind.h to glib/ This is a BSD-licenced header file that is designed to be copy-pasted into programs. It will allow us to detect if we are running under Valgrind and send "client requests" to it. We will use this for a couple of reasons in upcoming patches.
Created attachment 242154 [details] [review] gslice: disable by default under valgrind All experienced GLib hackers know that G_SLICE=always-malloc is absolutely essential when valgrinding but many users of GLib don't know about this and get hit pretty hard when valgrinding their programs. When initialising gslice, add a check to see if we are running under valgrind and disable ourselves if we are. We only do the check in the case that G_SLICE= was not specified in the environment, so setting it to an empty string will prevent this default behaviour. I considered modifying gslice to use the VALGRIND_MALLOCLIKE_BLOCK client request in all cases in order to just mark the blocks properly but these calls are not free and gslice is pretty hyper-optimised. It's easier to just disable gslice completely and this way we only have to do one check during startup. It's also theoretically possible that someone might want to use valgrind to debug gslice, in which case the extra annotations would probably cause quite a lot of difficulty.
Created attachment 242155 [details] [review] gtype: put private data before the instance Classically, a GTypeInstance has had the following layout: [[[[GTypeInstance] GObject] TypeA] TypeB] [TypeAPrivate] [TypeBPrivate] where TypeB is a subclass of TypeA which is a GObject. Both TypeA and TypeB use pivate data. The main problem with this approach is that the offset between a pointer to an instance of TypeA and the TypeAPrivate is not constant: it changes depending on the depth of derivation and the size of the instance structures of the derived types. For example, changing the size of the TypeB structure in the above example would push the TypeAPrivate further along. This complicates the implementation of g_type_instance_get_private(). In particular, during object construction when the class pointer to the 'complete type' of the object is not yet stored in the header of the GTypeInstance, we need a lookup table in order to be able to implement g_type_instance_get_private() accurately. We can avoid this problem by storing the private data before the structures, in reverse order, like so: [TypeBPrivate] [TypeAPrivate] [[[[GTypeInstance] GObject] TypeA] TypeB] Now the distance between TypeA and TypeAPrivate depends only on the size of GObject and GTypeInstance, which are static. Even in the case of TypeB, the distance is not statically known but can be determined at runtime and is constant (because we will know the size of TypeAPrivate by the time we initialise TypeB and it won't change). This approach requires a slighty dirty trick: allocating extra memory _before_ the pointer we return from g_type_create_instance(). The main problem with this is that it will cause valgrind to behave very badly, reporting almost everything as "possibly lost". We can correct for this by including a few valgrind client requests in order to inform it that the start of the GTypeInstance should be considered a block of memory and that pointers to it should mean that this block is reachable. In order to make the private data reachable, we also declare it as a block and include an extra pointer from the end of the primary block pointing back at it. All of this is only done if we are running under Valgrind.
Review of attachment 242153 [details] [review]: ack
Review of attachment 242154 [details] [review]: ack
Review of attachment 242155 [details] [review]: looks good otherwise ::: gobject/gtype.c @@ +24,3 @@ #include "config.h" +#include <valgrind/valgrind.h> This does not use the included version. @@ +1925,3 @@ + g_slice_free1 (private_size + ivar_size + sizeof (gpointer), allocated); + + VALGRIND_FREELIKE_BLOCK (instance + sizeof (GTypeInstance), 0); Should just be instance @@ +4519,3 @@ } + return ((gchar *) instance) - node->data->instance.private_size; Soo beautiful!
Attachment 242153 [details] pushed as c8d56d7 - Add a copy of valgrind.h to glib/ Attachment 242154 [details] pushed as 00fbc2f - gslice: disable by default under valgrind Attachment 242155 [details] pushed as 31fde56 - gtype: put private data before the instance
I run Ubuntu 13.04. I generally install glib built from git master, which is either brave or foolish, depending on your perspective. :) Recently I updated glib and found a pretty significant regression in my desktop environment: whenever I close a terminal window or most other windows, unity-panel-service crashes. It restarts automatically, but I see all indicators on the system panel vanish and then reappear. I bisected glib and found that it broke with the change '31fde56 - gtype: put private data before the instance' referenced here. The unity-panel-service stack trace is below. It looks a bit gnarly and I haven't investigated further. But I thought you should know about this.
+ Trace 231861
By the way I've created a bug for the unity-panel-service crash downstream at https://bugs.launchpad.net/unity/+bug/1173262 .