After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 790697 - g_object_ref API should propagate parameter type
g_object_ref API should propagate parameter type
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other Linux
: Normal enhancement
: 2.56
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-11-22 08:05 UTC by Christian Hergert
Modified: 2017-12-09 00:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gobject: add type propagation to gobject ref API (1.69 KB, patch)
2017-11-22 08:05 UTC, Christian Hergert
none Details | Review
gobject: fix typecasts via g_object_ref (3.80 KB, patch)
2017-11-22 08:05 UTC, Christian Hergert
none Details | Review
gobject: add type propagation to gobject ref API (2.09 KB, patch)
2017-11-22 23:11 UTC, Christian Hergert
none Details | Review
gobject: fix typecasts via g_object_ref (4.98 KB, patch)
2017-11-22 23:23 UTC, Christian Hergert
committed Details | Review
gobject: add type propagation to gobject ref API (2.14 KB, patch)
2017-12-08 10:47 UTC, Philip Withnall
none Details | Review
gobject: add type propagation to gobject ref API (2.82 KB, patch)
2017-12-08 10:52 UTC, Philip Withnall
committed Details | Review
Use escaped version of typeof (1.40 KB, patch)
2017-12-08 13:03 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
Disable refcounting type propagation with C++ (1.25 KB, patch)
2017-12-08 15:48 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review

Description Christian Hergert 2017-11-22 08:05:32 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.
Comment 1 Christian Hergert 2017-11-22 08:05:54 UTC
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.
Comment 2 Christian Hergert 2017-11-22 08:05:58 UTC
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.
Comment 3 Christian Hergert 2017-11-22 23:11:13 UTC
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.
Comment 4 Christian Hergert 2017-11-22 23:23:13 UTC
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.
Comment 5 Philip Withnall 2017-11-23 15:37:34 UTC
Review of attachment 364241 [details] [review]:

Yes, this one can go in right now.
Comment 6 Philip Withnall 2017-11-23 15:44:56 UTC
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).
Comment 7 Philip Withnall 2017-11-23 15:45:49 UTC
Emmanuele, what do you think about #comment 6 from the CI point of view?
Comment 8 Philip Withnall 2017-11-23 15:47:06 UTC
If we go ahead with this, I’d like it in early for 2.56 so we have plenty of compile testing against it.
Comment 9 Colin Walters 2017-11-23 18:40:18 UTC
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.
Comment 10 Philip Withnall 2017-11-23 19:46:12 UTC
(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).
Comment 11 Christian Hergert 2017-11-24 06:36:13 UTC
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 12 Philip Withnall 2017-12-04 11:44:10 UTC
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
Comment 13 Philip Withnall 2017-12-04 11:47:26 UTC
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.
Comment 14 Emmanuele Bassi (:ebassi) 2017-12-04 11:59:04 UTC
(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.
Comment 15 Christian Hergert 2017-12-04 23:57:53 UTC
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.
Comment 16 Philip Withnall 2017-12-05 12:43:01 UTC
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
Comment 17 Colin Walters 2017-12-05 15:59:23 UTC
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?
Comment 18 Philip Withnall 2017-12-05 16:30:11 UTC
(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.
Comment 19 A. Walton 2017-12-05 22:06:48 UTC
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.
Comment 20 A. Walton 2017-12-08 00:10:59 UTC
(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.
Comment 21 Philip Withnall 2017-12-08 10:47:27 UTC
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.
Comment 22 Philip Withnall 2017-12-08 10:52:13 UTC
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.
Comment 23 Philip Withnall 2017-12-08 10:55:24 UTC
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.
Comment 24 Colin Walters 2017-12-08 11:02:51 UTC
Review of attachment 365236 [details] [review]:

LGTM, but will let @chegert approve since you're changing his patch.
Comment 25 Christian Hergert 2017-12-08 11:07:23 UTC
Review of attachment 365236 [details] [review]:

LGTM
Comment 26 Philip Withnall 2017-12-08 11:15:41 UTC
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
Comment 27 Philip Withnall 2017-12-08 11:28:11 UTC
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
Comment 28 Daniel Boles 2017-12-08 12:08:56 UTC
> 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?
Comment 29 Philip Withnall 2017-12-08 12:24:53 UTC
(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).
Comment 30 Emmanuele Bassi (:ebassi) 2017-12-08 12:54:35 UTC
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.
```
Comment 31 Emmanuele Bassi (:ebassi) 2017-12-08 13:03:44 UTC
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>
Comment 32 Emmanuele Bassi (:ebassi) 2017-12-08 13:04:17 UTC
Attachment 365243 [details] pushed as 9f3f089 - Use escaped version of typeof
Comment 33 Morten Welinder 2017-12-08 15:33:05 UTC
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.
Comment 34 Emmanuele Bassi (:ebassi) 2017-12-08 15:48:45 UTC
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*.
Comment 35 Emmanuele Bassi (:ebassi) 2017-12-08 16:01:32 UTC
(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.
Comment 36 Christian Hergert 2017-12-09 00:28:34 UTC
(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.
Comment 37 Christian Hergert 2017-12-09 00:36:52 UTC
(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.
Comment 38 Emmanuele Bassi (:ebassi) 2017-12-09 00:58:41 UTC
Attachment 365256 [details] pushed as 1a6f648 - Disable refcounting type propagation with C++