GNOME Bugzilla – Bug 683360
Remove "volatile" from G_DEFINE_*
Last modified: 2018-05-24 14:32:43 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.
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.
Created attachment 223488 [details] [review] G_DEFINE_QUARK: fix up some implementation issues
(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).
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...
Attachment 223488 [details] pushed as 73a100d - G_DEFINE_QUARK: fix up some implementation issues
How about the other occasions? gtype.h is still full of volatile's that are not needed.
Ping?
-- 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.