Can you provide a test case or more details?

Created attachment 6056 [details]
Test app

gtk_color_selection_get_type() is __attribute__((const)) so
GCC thinks is can discard calls to it if they aren't used
anywhere. try 

volatile GType type;

I think the that the potential optimization from elimininating
repeated get_type() calls is probably worth the oddity, 
but it is certainly a gotcha that a lot of people are running
into.

Yes, that does fix it.

Though I don't like the use of G_GNUC_CONST here.
Unless it really does have significant performance advantages,
I'd take it out. It's not really correct.

(In reply to comment #4)
> Yes, that does fix it.
> 
> Though I don't like the use of G_GNUC_CONST here.
> Unless it really does have significant performance advantages,
> I'd take it out. It's not really correct.

That's right, we've been Doing It Wrong.

From the GCC manual:

`const'
     Many functions do not examine any values except their arguments,
     and have no effects except the return value.  Basically this is
     just slightly more strict class than the `pure' attribute below,
     since function is not allowed to read global memory.

But _get_type() functions obviously *do* read global memory, so it's wrong for
us to annotate them as const.  What we really mean is "pure":

`pure'
     Many functions have no effects except the return value and their
     return value depends only on the parameters and/or global
     variables.  Such a function can be subject to common subexpression
     elimination and loop optimization just as an arithmetic operator
     would be.

*** Bug 676100 has been marked as a duplicate of this bug. ***

The first call to a _get_type() function is neither const nor pure, because it has side effects. What we really want is __attribute__((idempotent)), which was suggested upstream a long time ago (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32911) but never went anywhere.

Calls to a _get_type() function after the first one can be called 'const' as legitimately as they can be called 'pure', because the static variable that the _get_type() function looks at never changes after the first call, so the optimizer doesn't need to worry about it.


(Marking _get_type() functions 'pure' instead of 'const' would basically be a no-op [relative to not marking them at all], because in code like:

  gtk_button_do_this (GTK_BUTTON (widget));
  gtk_button_do_that (GTK_BUTTON (widget));

you'd need to call gtk_button_get_type() both times if it was marked 'pure', because gtk_button_do_this() might have changed global memory. If it's marked 'const', you only need to call it the first time.)

(In reply to comment #7)

> (Marking _get_type() functions 'pure' instead of 'const' would basically be a
> no-op [relative to not marking them at all], because in code like:
> 
>   gtk_button_do_this (GTK_BUTTON (widget));
>   gtk_button_do_that (GTK_BUTTON (widget));
> 
> you'd need to call gtk_button_get_type() both times if it was marked 'pure',
> because gtk_button_do_this() might have changed global memory. If it's marked
> 'const', you only need to call it the first time.)

True.  

The obvious long term thing we want is for the compiler to be aware of the type hierarchy...a gcc gobject-introspection plugin.  Though there are also obvious issues with that owing to the fact that one has to compile the code to get the type data =/

More medium term, yes - Behdad's suggestion of 'idempotent' makes a lot of sense.  It's probably not hard to implement at all.

But this bug should remain open until we ship correct annotations on our _get_type() functions; telling people to use volatile to subvert the optimizer is not a good long term situation.

(In reply to comment #7)

> (Marking _get_type() functions 'pure' instead of 'const' would basically be a
> no-op [relative to not marking them at all], because in code like:
> 
>   gtk_button_do_this (GTK_BUTTON (widget));
>   gtk_button_do_that (GTK_BUTTON (widget));

FWIW in my code, I tend to hoist GObject-style casts manually.  I.e.:

GtkWidget *mywidget;
GtkButton *mywidget_button;

mywidget = foo_widget_new ();
mywidget_button = GTK_BUTTON (mywidget);

gtk_button_do_this (mywidget_button);
gtk_button_do_that (mywidget_button);

Because it also avoids grabbing a process-global mutex at every function call.

(Well actually, more often I say "screw that shit" and just use C casts, but sadly we rely too much on GObject casts in our ecosystem due to a lack of testing)

(In reply to comment #8)
> But this bug should remain open until we ship correct annotations on our
> _get_type() functions; telling people to use volatile to subvert the optimizer
> is not a good long term situation.

see also bug 605976

gdbus-codegen has this bug too.

I think at this point, we might as well declare that using G_GNUC_CONST on type functions is allowed/encouraged, and g_type_ensure() is the only supported way to force a type to be registered without creating actually an object of that type.

(In reply to Dan Winship from comment #12)
> I think at this point, we might as well declare that using G_GNUC_CONST on
> type functions is allowed/encouraged, and g_type_ensure() is the only
> supported way to force a type to be registered without creating actually an
> object of that type.

Probably, but if we recommend this, G_DECLARE_*_TYPE should then be changed to use G_GNUC_CONST. Best do that *now* before applications start relying on the opposite behavior.

This works *now* with the current GCC version, but what if a new optimization is added in the future that takes advantage of const functions and that breaks the whole g platform? For example interprocedural optimizations coupled with bundling…

It's safer to rely only on the API of GCC, that is, the interface, what is documented, not how it is currently used internally.

Although in practical terms G_GNUC_CONST is safe to use for the moment, and I agree with Dan (comment #12) that the only way to really ensure a type is available is to use g_type_ensure(), I don’t think we should add G_GNUC_CONST into all get_type() functions. As Sébastien says, its semantics aren’t correct for what we’d be using it for.

The real fix here is to push for support for some kind of __attribute__((idempotent)) in the compilers, and use that once it’s available. That’s bug #446565, which I’m going to mark this as a duplicate of.

*** This bug has been marked as a duplicate of bug 446565 ***

In the meantime, though, we can leave the existing G_GNUC_CONST attributes in place until they actually cause a problem.