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 683360 - Remove "volatile" from G_DEFINE_*
Remove "volatile" from G_DEFINE_*
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-09-04 19:58 UTC by Behdad Esfahbod
Modified: 2018-05-24 14:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
G_DEFINE_QUARK: fix up some implementation issues (1.81 KB, patch)
2012-09-05 00:54 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Behdad Esfahbod 2012-09-04 19:58:50 UTC
In code like this:

#define G_DEFINE_QUARK(QN, q_n)                                         \ 
GQuark                                                                  \ 
q_n##_quark (void)                                                      \ 
{                                                                       \ 
  static volatile gsize g_define_quark__volatile = 0;                   \ 
  if (g_once_init_enter (&g_define_quark__volatile))                    \ 
    {                                                                   \ 
      GQuark g_define_quark = g_quark_from_string (#QN);                \ 
      g_once_init_leave (&g_define_quark__volatile, g_define_quark);    \ 
    }                                                                   \ 
  return g_define_quark__volatile;                                      \ 
} 

We shouldn't be using "volatile".  What volatile means is that "the value may change at any moment".  That's true when you are dealing with the same variable from multiple threads without the correct primitives.  When we use g_once_init, gatomic, etc, ie. primitives designed exactly to address this case, the value is not changing unexpectedly anymore.  As such, we should NOT mark them volatile.

In C, volatile really is only useful with IO ports and other hardware memory mappings.  In more recent C++ volatile variables have been respec'ed to have acquire/release semantics which is more useful, as in, you can use them and get some atomicity without bothering with atomic ops.
Comment 1 Allison Karlitskaya (desrt) 2012-09-05 00:48:50 UTC
I disagree with your argument, but I think your conclusion is correct.

In particular, this part: 

  What volatile means is that "the value may change at any moment".

That's not true.  What volatile really means is that a write must be a write and a read must be a read, without optimisations.  That's why we often use volatile as a way of ensuring that a write occurs (and that the function call from which we get the value is not optimised away).

In any case, the use of it here is totally bogus.  We get all of the guarantees that we need from the g_once_init_enter().

A few more notes:

  - why not _from_static_string()?

  - why bother at all with the g_once?  We can do this without any
    synchronisation because we already have locking in GQuark.  If we
    try to create the quark twice at the same time, we will just assign
    the same value twice.  No problem.
Comment 2 Allison Karlitskaya (desrt) 2012-09-05 00:54:49 UTC
Created attachment 223488 [details] [review]
G_DEFINE_QUARK: fix up some implementation issues
Comment 3 Behdad Esfahbod 2012-09-05 00:58:17 UTC
(In reply to comment #1)
> I disagree with your argument, but I think your conclusion is correct.
> 
> In particular, this part: 
> 
>   What volatile means is that "the value may change at any moment".
> 
> That's not true.  What volatile really means is that a write must be a write
> and a read must be a read, without optimisations.  That's why we often use
> volatile as a way of ensuring that a write occurs (and that the function call
> from which we get the value is not optimised away).

Right.  By "may change at any moment" I meant in a hand-wavy way that "a write;read sequence cannot be optimized".


> In any case, the use of it here is totally bogus.  We get all of the guarantees
> that we need from the g_once_init_enter().
> 
> A few more notes:
> 
>   - why not _from_static_string()?

Agreed.


>   - why bother at all with the g_once?  We can do this without any
>     synchronisation because we already have locking in GQuark.  If we
>     try to create the quark twice at the same time, we will just assign
>     the same value twice.  No problem.

It's safe with quarks, but I think it used to be aproblem with type registration (enums and objects, perhaps error domains when we add them too).
Comment 4 Matthias Clasen 2012-09-05 02:52:35 UTC
Review of attachment 223488 [details] [review]:

Sure, looks ok to me; I was just copying the implementation of EGG_DEFINE_QUARK without too much thinking...
Comment 5 Allison Karlitskaya (desrt) 2012-09-08 18:08:16 UTC
Attachment 223488 [details] pushed as 73a100d - G_DEFINE_QUARK: fix up some implementation issues
Comment 6 Behdad Esfahbod 2012-09-08 18:21:36 UTC
How about the other occasions?  gtype.h is still full of volatile's that are not needed.
Comment 7 Behdad Esfahbod 2013-08-26 23:35:23 UTC
Ping?
Comment 8 GNOME Infrastructure Team 2018-05-24 14:32:43 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/600.