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 651514 - g_atomic_pointer_compare_and_exchange() incompatible change (sort of)
g_atomic_pointer_compare_and_exchange() incompatible change (sort of)
Status: RESOLVED WONTFIX
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: High critical
: ---
Assigned To: gtkdev
gtkdev
: 651576 651766 651793 652470 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-05-31 02:12 UTC by David Schleef
Modified: 2012-01-04 05:42 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description David Schleef 2011-05-31 02:12:54 UTC
We have the following code in gstelementfactory.c, with the (gpointer) cast presumably to avoid type-punning warnings:

  if (!g_atomic_pointer_compare_and_exchange (
    (gpointer) & oclass->elementfactory, NULL, factory))

This code now asserts at compile time due to recent gatomic header changes.  We're basically trying to figure out how to not trip a warning with both new and old glib.

IRC log:

<ds> desrt: yo
<desrt> what's up?
<ds> desrt: http://pastebin.com/gaVXpbAG

gstelementfactory.c: In function ‘gst_element_factory_create’:
gstelementfactory.c:397:8: error: size of array ‘Compile_Time_Assertion’ is negative
gstelementfactory.c:397:8: error: invalid use of void expression
gstelementfactory.c:397:8: error: incompatible type for argument 1 of ‘__sync_bool_compare_and_swap’

<ds> desrt: due to your recent gatomic hacking
<ds> desrt: one problem, you changed the prototype of ..
<ds> oh wait
<ds> lemme give you some context
<ds>   if (!g_atomic_pointer_compare_and_exchange (
<ds>           (gpointer) & oclass->elementfactory, NULL, factory))
<ds> that is the code that it fails on
<desrt> that makes perfect sense
<ds> Did you intend to change the prototype of g_atomic_pointer_compare_and_exchange?
<desrt> i actually didn't
<desrt> well, i did
<desrt> but not in a way that negatively impacts you
<ds> right
<desrt> the thing that's nailing you is the fact that i introduced a macro to do some additional checks
<ds> obviously, we want to change that to (gpointer *)
<desrt> better to remove it entirely
<ds> mmm, true.  not sure why it was like that.
<desrt> without the macro the cast was required
<desrt> and as a nice side effect you got a warning about type-punned pointers
<desrt> now you just give a pointer to an arbitrary pointer-sized object and it always works
<desrt> the problem with casting to (gpointer) is that (gpointer) has no particular size
<desrt> so the check fails
<ds> yeah, i understand what is going on.  I'm just checking if you intended to break it that way.
<desrt> well
<desrt> truthfully this is an incompatible API break in GLib
<desrt> which is something that we usually strive to avoid at all costs
<ds> right
<desrt> so it might be the case that i have to fix it to unbreak you
<ds> however, if it's "we're only breaking apps where they're doing something really stupid", I don't mind
<desrt> (and possibly others)
<desrt> well....
<desrt> what you were doing is necessitated by (what i consider) a deficiency of the old API and GCC's excessively pedantic behaviour
<desrt> so it's stupid, but it's not your fault :)
<ds> trying to keep -Wall -Werror compatibility across this change might be challenging, then
<ds> ok
<desrt> indeed
<ds> fine with me.  just wanted to know what's up.  thanks.
<desrt> one thing you could do is to put brackets around the function name
<desrt> that will prevent the macro evaluation (thereby skipping the checks)
<desrt> it will also reduce performance, though, since you miss the chance at inlining that the macro gives you
<desrt> please open a bug
<desrt> i see a couple of ways out of the mess
<desrt> one of them is to drop the extra checks
<desrt> the other is only to impose the extra checks if G_DISABLE_DEPRECATED is defined
<desrt> a 3rd option is to add a G_ATOMIC_BACKCOMPT define or something like that
<desrt> so that people can have -Well -Werror compatibility across the change if they're willing to add an extra #define
<desrt> there are a lot of possibilities
<desrt> and i'd like matthias's input
<ds> we can do a glib version check, too
<desrt> true.
<desrt> a cast to (volatile gpointer *) might do the trick on both
<desrt> could give that a try...
Comment 1 David Schleef 2011-05-31 02:21:44 UTC
The following patch (to GStreamer) seems to work with both old and new gatomic headers:

diff --git a/gst/gstelementfactory.c b/gst/gstelementfactory.c
index 0c3d8aa..1e3c304 100644
--- a/gst/gstelementfactory.c
+++ b/gst/gstelementfactory.c
@@ -395,7 +395,7 @@ gst_element_factory_create (GstElementFactory * factory, con
    */
   oclass = GST_ELEMENT_GET_CLASS (element);
   if (!g_atomic_pointer_compare_and_exchange (
-          (gpointer) & oclass->elementfactory, NULL, factory))
+        (volatile gpointer *)&oclass->elementfactory, NULL, factory))
     gst_object_unref (factory);

   GST_DEBUG ("created element \"%s\"", GST_PLUGIN_FEATURE_NAME (factory));
Comment 2 Matthias Clasen 2011-05-31 11:28:28 UTC
<desrt> one of them is to drop the extra checks
<desrt> the other is only to impose the extra checks if G_DISABLE_DEPRECATED is
defined
<desrt> a 3rd option is to add a G_ATOMIC_BACKCOMPT define or something like
that
<desrt> so that people can have -Well -Werror compatibility across the change

Of these, the G_DISABLE_DEPRECATED option seems the most palatable one to me. 
Of course, adding a release note about the possible breakage would be good. These go into README.in; add a section 'Notes about GLib 2.30'
Comment 3 Allison Karlitskaya (desrt) 2011-05-31 15:25:49 UTC
Since we're still in the unstable cycle, we will keep the API break for now.  If we receive more bugs about the break then we will fix it before the stable release (by using the G_DISABLE_DEPRECATED trick I mentioned).  If nobody else complains, we might just let this one slip past....

Is that okay with you, David?
Comment 4 David Schleef 2011-05-31 22:09:25 UTC
*** Bug 651576 has been marked as a duplicate of this bug. ***
Comment 5 Allison Karlitskaya (desrt) 2011-06-02 22:58:34 UTC
I find the warnings produced in bug 651575 very interesting...

What code generates those?  Is this a regression with the new changes?
Comment 6 David Schleef 2011-06-02 23:04:33 UTC
The code is in the patch.  It was not intended to work with the new changes.

However, I have so far been unable to find code that works both before and after the changes.  This is a problem (ahem, sort of), since we don't require the latest version of glib.  I say "sort of", since it triggers -Werror, which we prefer to compile with.
Comment 7 Sebastian Dröge (slomo) 2011-06-03 08:56:21 UTC
*** Bug 651766 has been marked as a duplicate of this bug. ***
Comment 8 Sergey V. Udaltsov 2011-06-03 19:08:28 UTC
jhbuild is essentially broken currently
Comment 9 Colin Walters 2011-06-03 19:19:07 UTC
*** Bug 651793 has been marked as a duplicate of this bug. ***
Comment 10 Colin Walters 2011-06-03 19:19:42 UTC
See bug 651793, where gcc 4.5.1 (Fedora 14) at least warns from constructs in glib itself too.
Comment 11 David Schleef 2011-06-04 17:45:44 UTC
This is the fix that was required for GStreamer.  It's kinda ugly:

  http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=f3eac7de22d576edf7d0e74830d8336f81dc376b
Comment 12 Tim-Philipp Müller 2011-06-13 16:23:59 UTC
*** Bug 652470 has been marked as a duplicate of this bug. ***
Comment 13 Matthias Clasen 2012-01-04 05:42:04 UTC
Too late now, anyway