GNOME Bugzilla – Bug 790697
g_object_ref API should propagate parameter type
Last modified: 2017-12-09 00:58:54 UTC
It might be nice to propagate the return type of g_object_ref() and g_object_ref_sink() on GCC using typeof(). It might not catch a lot of bugs, but catching them at compile time is quite nice.
Created attachment 364172 [details] [review] gobject: add type propagation to gobject ref API Currently, g_object_ref() and g_object_ref_sink() return a gpointer which can mask issues when assigning to fields or returning from a function. To help catch these type of programming errors, we can propagate the type of the parameter through the function call on GCC using the typeof() C language extension. This will cause offending code to have a warning, but will continue to be source and binary compatible.
Created attachment 364173 [details] [review] gobject: fix typecasts via g_object_ref Now that g_object_ref() propagates the parameter type to the return value, we need to cast to ensure the result is warning free.
Created attachment 364239 [details] [review] gobject: add type propagation to gobject ref API Currently, g_object_ref() and g_object_ref_sink() return a gpointer which can mask issues when assigning to fields or returning from a function. To help catch these type of programming errors, we can propagate the type of the parameter through the function call on GCC using the typeof() C language extension. This will cause offending code to have a warning, but will continue to be source and binary compatible.
Created attachment 364241 [details] [review] gobject: fix typecasts via g_object_ref Now that g_object_ref() propagates the parameter type to the return value, we need to cast to ensure the result is warning free.
Review of attachment 364241 [details] [review]: Yes, this one can go in right now.
Review of attachment 364239 [details] [review]: I like this, but we need to be careful about how it’s deployed. There’s the potential for a lot of CI fallout and a very unhappy ebassi. This seems like the kind of thing which should be opt-in — but that means nobody would use it. My suggestion would be to make the typeof() usage conditional on GLIB_MAX_ALLOWED being defined and >= 2.56. That way, people explicitly opt in by bumping their GLIB_MAX_ALLOWED version, but it doesn’t need its own special enable-typeof()-checks flag to be added to every project (or, more likely, forgotten about by everyone).
Emmanuele, what do you think about #comment 6 from the CI point of view?
If we go ahead with this, I’d like it in early for 2.56 so we have plenty of compile testing against it.
Review of attachment 364239 [details] [review]: It's also something that would be caught more generally by that CLang gobject plugin right? An interesting question to me is - for a few nontrivial codebases, does this patch find any actual bugs? If we don't find any then I'm uncertain about turning this on by default. A specific reason not to is that many projects use a "-Werror only for certain warnings" approach, and for e.g. libostree we have -Werror=incompatible-pointer-types by default.
(In reply to Colin Walters from comment #9) > Review of attachment 364239 [details] [review] [review]: > > It's also something that would be caught more generally by that CLang > gobject plugin right? https://www.freedesktop.org/software/tartan/ Yes, if: - sufficient type hints are available (should be the case in any of the situations where this typeof() approach would throw an error, since the compiler is working with the same resolved types in both cases) - anyone takes the time to make sure Tartan detects this specific case - anyone actually has time to develop or maintain Tartan — I currently don’t :'-( - developers run Tartan on their code Generally, I’m in favour of the compiler catching bugs where possible, rather than deferring it to a separate static analysis tool which will get run less often. > An interesting question to me is - for a few nontrivial codebases, does this > patch > find any actual bugs? If we don't find any then I'm uncertain about turning > this on > by default. I just build OSTree against a patched GLib, and got the following: In file included from /opt/gnome/install/include/glib-2.0/gobject/gbinding.h:29:0, from /opt/gnome/install/include/glib-2.0/glib-object.h:23, from /opt/gnome/install/include/glib-2.0/gio/gioenums.h:28, from /opt/gnome/install/include/glib-2.0/gio/giotypes.h:28, from /opt/gnome/install/include/glib-2.0/gio/gio.h:26, from ./src/libotutil/otutil.h:24, from src/libostree/ostree-repo-file.c:24: src/libostree/ostree-repo-file.c: In function ‘ostree_repo_file_get_parent’: /opt/gnome/install/include/glib-2.0/gobject/gobject.h:513:33: warning: return from incompatible pointer type [-Wincompatible-pointer-types] #define g_object_ref(Obj) ((typeof(Obj)) (g_object_ref) (Obj)) ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/libostree/ostree-repo-file.c:511:10: note: in expansion of macro ‘g_object_ref’ return g_object_ref (self->parent); ^~~~~~~~~~~~ src/libostree/ostree-repo-file.c: In function ‘ostree_repo_file_resolve_relative_path’: /opt/gnome/install/include/glib-2.0/gobject/gobject.h:513:33: warning: return from incompatible pointer type [-Wincompatible-pointer-types] #define g_object_ref(Obj) ((typeof(Obj)) (g_object_ref) (Obj)) ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/libostree/ostree-repo-file.c:624:16: note: in expansion of macro ‘g_object_ref’ return g_object_ref (ostree_repo_file_get_root (self)); ^~~~~~~~~~~~ That’s a false positive, but is not so bad over a large code base. Compiling GTK+ produced 23 warnings, but I haven’t checked whether they’re true/false positives. Note that we should not expect to see any true positives from compiling existing code, since it’s all been tested and any bugs fixed already. The value of this patch would come from catching errors in new code before they get released. (Same as with static analysis: most of its value comes from running it on new code, not old code.) > A specific reason not to is that many projects use a "-Werror only for > certain warnings" > approach, and for e.g. libostree we have -Werror=incompatible-pointer-types > by > default. That would be caught if OSTree set a GLIB_MAX_ALLOWED, but it doesn’t seem to. Why not? We could also disable the typeof() stuff if GLIB_MAX_ALLOWED is not defined (as well as if it’s defined < 2.56).
I mostly just put up this review to see what other people thought about it, glad to see so much discussion! The other concern which I haven't really tested is the effect on g++/gtkmm. As for finding bugs, it was while tracking down something else that I came up with this patch. It didn't find the bug, but it did rule out an entire avenue of potential sources of the bug. I think that is useful. > libostree we have -Werror=incompatible-pointer-types by default. I'm sympathetic to this, but only a little since it's relying on a function for casting.
Comment on attachment 364241 [details] [review] gobject: fix typecasts via g_object_ref Pushed the small cleanup patch. Emmanuele, still waiting for your input on the GLIB_MAX_ALLOWED suggestion and what impact this will have (or what impact you would be happy with it having) on CI. Attachment 364241 [details] pushed as f44472e - gobject: fix typecasts via g_object_ref
Christian, if you could put together an updated version of the patch which checks GLIB_VERSION_MAX_ALLOWED (whoops, I was mistakenly calling it GLIB_MAX_ALLOWED before), we can probably push this forwards.
(In reply to Philip Withnall from comment #7) > Emmanuele, what do you think about #comment 6 from the CI point of view? We're just going to live with the CI fallout; very, very few projects use GLIB_VERSION_MIN_REQUIRED/GLIB_VERSION_MAX_ALLOWED during development. Fixing all the modules under CI to use those two macros, and fixing all the modules that generate compiler warnings, is basically the same amount of work. Plus, it's not like projects in GNOME don't generate compiler warnings already; in some cases, the maintainers simply disable -Werror by default.
I'm mostly okay with breaking things that are -Werror because if you sign up for that (or it's variants), that generally means you're okay with fixing things as new warnings are added. It's probably only a day of work for us to fix things in response to C-I. However, the code I'm most concerned about is glibmm/gtkmm given the slightly different semantics in typecasting in C++. So CC'ing Murray so he can chime in any thoughts. It's possible that being more strict about casts there already helps them.
I’ve also e-mailed the gtkmm list to see if anyone there has any thoughts. Let’s push this in a couple of weeks if there’s no feedback from the C++ people. https://mail.gnome.org/archives/gtkmm-list/2017-December/thread.html
https://github.com/ostreedev/ostree/pull/1363 Also I tried compiling NetworkManager with this, and it also failed; sent a patch: https://mail.gnome.org/archives/networkmanager-list/2017-December/msg00005.html > I'm mostly okay with breaking things that are -Werror because if you sign up for that (or it's variants), that generally means you're okay with fixing things as new warnings are added. I think -Werror is a *totally* different thing from a curated set of "-Werror=foo". https://mail.gnome.org/archives/desktop-devel-list/2012-July/msg00100.html The latter approach IMO has been working out OK so far. Anyways I'm not strictly opposed to this patch, but let's *at least* wait a cycle or two so that updates to projects like libostree/NetworkManager that are today using the "curated -Werror" approach are adapted?
(In reply to Colin Walters from comment #17) > Anyways I'm not strictly opposed to this patch, but let's *at least* wait a > cycle or two so that updates to projects like libostree/NetworkManager that > are today using the "curated -Werror" approach are adapted? I think the GLIB_VERSION_MAX_REQUIRED change would prevent build failures on libostree/NetworkManager until they bump that to 2.56.
The VMware hosted products are big consumers of the -mm bindings. I'll see if we can run this patch through the wringer and see if it breaks anything on our side.
(In reply to A. Walton from comment #19) > The VMware hosted products are big consumers of the -mm bindings. I'll see > if we can run this patch through the wringer and see if it breaks anything > on our side. All our product code came back clean against this, though we didn't get a chance to do a full rebuild of all of the OSS libraries against this patch due to our build farm having some maintenance fun. So maybe some drama recompiling glibmm - can't tell for now. Patch seems good to me though.
Created attachment 365235 [details] [review] gobject: add type propagation to gobject ref API Currently, g_object_ref() and g_object_ref_sink() return a gpointer which can mask issues when assigning to fields or returning from a function. To help catch these type of programming errors, we can propagate the type of the parameter through the function call on GCC using the typeof() C language extension. This will cause offending code to have a warning, but will continue to be source and binary compatible.
Created attachment 365236 [details] [review] gobject: add type propagation to gobject ref API Currently, g_object_ref() and g_object_ref_sink() return a gpointer which can mask issues when assigning to fields or returning from a function. To help catch these type of programming errors, we can propagate the type of the parameter through the function call on GCC using the typeof() C language extension. This will cause offending code to have a warning, but will continue to be source and binary compatible. This is only enabled when GLIB_VERSION_MAX_ALLOWED is 2.56 or greater.
I updated the patch to only work if enabled by GLIB_VERSION_MAX_ALLOWED, and to add some documentation. This is ready for final review, and I’m happy to push it afterwards to get a decent amount of testing during the rest of this cycle. My post to the glibmm mailing list hasn’t gone through moderation yet, so I’m inclined to think there won’t be much feedback there. I’m happy to push it without that feedback: we can always revert, and any changes done in modules which depend on GLib in the meantime won’t be invalidated.
Review of attachment 365236 [details] [review]: LGTM, but will let @chegert approve since you're changing his patch.
Review of attachment 365236 [details] [review]: LGTM
Shippingit. Thanks for the feedback and reviews everyone. Hopefully this doesn’t break much. Attachment 365236 [details] pushed as 3fae39a - gobject: add type propagation to gobject ref API
Also sent a mail to the lists announcing the change and giving details of how to fix any modules which break: https://mail.gnome.org/archives/desktop-devel-list/2017-December/msg00042.html
> Note that the new behaviour requires GCC, and is only enabled > if you have defined GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_56. Sorry if this is a silly question. But what happens if we're not using GCC? That's still supported, right?
(In reply to Daniel Boles from comment #28) > > Note that the new behaviour requires GCC, and is only enabled > > if you have defined GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_56. > > Sorry if this is a silly question. But what happens if we're not using GCC? > That's still supported, right? If you’re not using GCC (or if GLIB_VERSION_MAX_ALLOWED < GLIB_VERSION_2_56), the g_object_ref() behaviour will be as before (no type propagation).
Slight issue: GCC disables `typeof` when running with `-std=c99` and `-pedantic`, as we found out when building Avahi on Continuous: ``` In file included from /usr/include/glib-2.0/gobject/gbinding.h:29:0, from /usr/include/glib-2.0/glib-object.h:23, from ../../avahi-gobject/ga-client.h:23, from ga-client-enumtypes.c:4: /usr/include/glib-2.0/gobject/gobject.h: In function ‘g_set_object’: In file included from /usr/include/glib-2.0/gobject/gbinding.h:29:0, from /usr/include/glib-2.0/glib-object.h:23, from signals-marshal.h:5, from signals-marshal.c:1: /usr/include/glib-2.0/gobject/gobject.h: In function ‘g_set_object’: /usr/include/glib-2.0/gobject/gobject.h:513:34: warning: implicit declaration of function ‘typeof’ [-Wimplicit-function-declaration] #define g_object_ref(Obj) ((typeof(Obj)) (g_object_ref) (Obj)) ^ /usr/include/glib-2.0/gobject/gobject.h:726:5: note: in expansion of macro ‘g_object_ref’ g_object_ref (new_object); ^ ``` The GCC documentation says: ``` If you are writing a header file that must work when included in ISO C programs, write __typeof__ instead of typeof. ```
Created attachment 365243 [details] [review] Use escaped version of typeof When compiling code that includes gobject.h using GCC with the ISO standard, the `typeof` keyword is disabled, as it's a GCC extension. The GCC documentation recommends: > If you are writing a header file that must work when included in > ISO C programs, write __typeof__ instead of typeof. Which is precisely what we're going to do. Signed-off-by: Emmanuele Bassi <ebassi@gnome.org> Reviewed-by: Philip Withnall <withnall@endlessm.com>
Attachment 365243 [details] pushed as 9f3f089 - Use escaped version of typeof
Why use a gcc extension? Is there a reason why... #define g_object_ref(Obj) (0 ? (Obj) : (g_object_ref) (Obj)) ...isn't doing the job? That should also guarantee single-evaluation of side effects in Obj.
Created attachment 365256 [details] [review] Disable refcounting type propagation with C++ The type propagation breaks the GRefPtr.h class in WebKitGTK, and in any case existing C++ code calling the C API will need to perform an explicit cast, as there's no automatic promotion of pointer types to and from void*.
(In reply to Emmanuele Bassi (:ebassi) from comment #34) > Created attachment 365256 [details] [review] [review] > Disable refcounting type propagation with C++ Tested-by: GNOME Continuous https://git.gnome.org/browse/gnome-continuous/commit/?id=dd0d3a8766dc76530766138801974dc8c3071251 This fixes the WebKitGTK+ build.
(In reply to Morten Welinder from comment #33) > Why use a gcc extension? Is there a reason why... > > #define g_object_ref(Obj) (0 ? (Obj) : (g_object_ref) (Obj)) > > ...isn't doing the job? That should also guarantee single-evaluation of > side effects in Obj. The reason is fairly simplistic. I didn't think of it. If this works (and I agree it looks like it should), I would be perfectly fine with this style.
(In reply to Christian Hergert from comment #36) > > The reason is fairly simplistic. I didn't think of it. If this works (and I > agree it looks like it should), I would be perfectly fine with this style. Actually, after testing this, the (0?(Obj):g_object_ref(Obj)) style does not create the expected warnings. Perhaps GCC is removing the 0 condition before those checks occur.
Attachment 365256 [details] pushed as 1a6f648 - Disable refcounting type propagation with C++