Re: Apparent thread-safety bug in Glib 2.0 docs

[Sebastian Wilhelmi <seppi seppi de>]

Hi Christopher,

> http://developer.gnome.org/doc/API/2.0/glib/glib-Threads.html#id2941682
> 
> It lists 4 examples while talking about thread-safe code.
> Unfortunately, the 3rd & 4th examples, though labeled as thread safe
> appear to not be thread safe.
> 
> The problem that I'm perceiving is that the "current_number" variable
> is not declared as volatile. While this might seem nit picky, without
> this flag the compiler may very well generate incorrect code. Given
> that this is a common programmer error, I think it'd be a good idea to
> either elaborate this point in the documentation, or at least correct
> the examples. Thoughts?

You are right. This is an oversight. I'll propose adding the keyword in
both examples and the following note:

        You have to declare variables, which are to be used by different
        threads, volatile in some circumstances. This is to prevent the
        compiler from optimizing access to those variables in a way,
        that makes a seemingly correct multi-threaded program not work.

        In particular all non-const static function variables, which are
        accessed by different threads have to be declared volatile. For
        global static or non-static variables this is in general not
        necessary, if they are protected by mutexes.

Would that be a good (and short, we should go into to much detail)
description?

Additionally I checked all of GLib and indeed found some places, where a
volatile would be needed according to the above mentioned rule.

The patch for both (documentaion and the fixes) is attached. Is is OK to
commit to HEAD and glib-2-2, Owen?

Bye,
Sebastian
-- 
Sebastian Wilhelmi                 |            här ovanför alla molnen
mailto:seppi seppi de              |     är himmlen så förunderligt blå
http://seppi.de                    |

Index: docs/reference/glib/tmpl/threads.sgml
===================================================================
RCS file: /cvs/gnome/glib/docs/reference/glib/tmpl/threads.sgml,v
retrieving revision 1.41
diff -u -r1.41 threads.sgml
--- docs/reference/glib/tmpl/threads.sgml	30 Jul 2003 22:31:23 -0000	1.41
+++ docs/reference/glib/tmpl/threads.sgml	25 Oct 2003 10:23:31 -0000
@@ -516,7 +516,7 @@
 
   int give_me_next_number (<!-- -->)
   {
-    static int current_number = 0;
+    static volatile int current_number = 0;
     int ret_val;
 
     g_mutex_lock (give_me_next_number_mutex);
@@ -538,6 +538,22 @@
 g_mutex_new() requires that. Use a #GStaticMutex instead.
 </para>
 
+<note>
+<para>
+You have to declare variables, which are to be used by different
+threads, volatile in some circumstances. This is to prevent the
+compiler from optimizing access to those variables in a way, that
+makes a seemingly correct multi-threaded program not work.
+</para>
+
+<para>
+In particular all non-const static function variables, which are
+accessed by different threads have to be declared volatile. For global
+static or non-static variables this is in general not necessary, if
+they are protected by mutexes.  
+</para> 
+</note>
+
 <para>
 A #GMutex should only be accessed via the following functions.
 </para>
@@ -656,7 +672,7 @@
 <programlisting>
   int give_me_next_number (<!-- -->)
   {
-    static int current_number = 0;
+    static volatile int current_number = 0;
     int ret_val;
     static GStaticMutex mutex = G_STATIC_MUTEX_INIT;
 
Index: glib/gmessages.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gmessages.c,v
retrieving revision 1.58
diff -u -r1.58 gmessages.c
--- glib/gmessages.c	25 Aug 2003 16:36:03 -0000	1.58
+++ glib/gmessages.c	25 Oct 2003 10:23:35 -0000
@@ -345,7 +345,7 @@
 		   GLogFunc	  log_func,
 		   gpointer	  user_data)
 {
-  static guint handler_id = 0;
+  static volatile guint handler_id = 0;
   GLogDomain *domain;
   GLogHandler *handler;
   
Index: glib/gthread.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gthread.c,v
retrieving revision 1.35
diff -u -r1.35 gthread.c
--- glib/gthread.c	8 Jul 2003 23:43:37 -0000	1.35
+++ glib/gthread.c	25 Oct 2003 10:23:36 -0000
@@ -437,7 +437,7 @@
 {
   GRealThread *self = (GRealThread*) g_thread_self ();
   GArray *array;
-  static guint next_index = 0;
+  static volatile guint next_index = 0;
   GStaticPrivateNode *node;
 
   array = self->private_data;
Index: glib/gutf8.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gutf8.c,v
retrieving revision 1.38
diff -u -r1.38 gutf8.c
--- glib/gutf8.c	25 Jul 2003 21:32:47 -0000	1.38
+++ glib/gutf8.c	25 Oct 2003 10:23:39 -0000
@@ -358,7 +358,7 @@
 static GHashTable *
 get_alias_hash (void)
 {
-  static GHashTable *alias_hash = NULL;
+  static volatile GHashTable *alias_hash = NULL;
   const char *aliases;
 
   G_LOCK (aliases);
Index: glib/gwin32.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gwin32.c,v
retrieving revision 1.36
diff -u -r1.36 gwin32.c
--- glib/gwin32.c	25 Jul 2003 21:32:47 -0000	1.36
+++ glib/gwin32.c	25 Oct 2003 10:23:40 -0000
@@ -550,7 +550,7 @@
 static gchar *
 get_package_directory_from_module (gchar *module_name)
 {
-  static GHashTable *module_dirs = NULL;
+  static volatile GHashTable *module_dirs = NULL;
   G_LOCK_DEFINE_STATIC (module_dirs);
   HMODULE hmodule = NULL;
   gchar *fn;
@@ -649,7 +649,7 @@
 g_win32_get_package_installation_directory (gchar *package,
 					    gchar *dll_name)
 {
-  static GHashTable *package_dirs = NULL;
+  static volatile GHashTable *package_dirs = NULL;
   G_LOCK_DEFINE_STATIC (package_dirs);
   gchar *result = NULL;
   gchar *key;
Index: gobject/gsignal.c
===================================================================
RCS file: /cvs/gnome/glib/gobject/gsignal.c,v
retrieving revision 1.53
diff -u -r1.53 gsignal.c
--- gobject/gsignal.c	12 Sep 2003 20:33:31 -0000	1.53
+++ gobject/gsignal.c	25 Oct 2003 10:23:45 -0000
@@ -814,7 +814,7 @@
 			    gpointer            hook_data,
 			    GDestroyNotify      data_destroy)
 {
-  static gulong seq_hook_id = 1;
+  static volatile gulong seq_hook_id = 1;
   SignalNode *node;
   GHook *hook;
   SignalHook *signal_hook;
Index: gthread/gthread-win32.c
===================================================================
RCS file: /cvs/gnome/glib/gthread/gthread-win32.c,v
retrieving revision 1.6
diff -u -r1.6 gthread-win32.c
--- gthread/gthread-win32.c	25 Nov 2002 23:08:27 -0000	1.6
+++ gthread/gthread-win32.c	25 Oct 2003 10:23:46 -0000
@@ -552,7 +552,7 @@
 static void
 g_thread_impl_init ()
 {
-  static gboolean beenhere = FALSE;
+  static volatile gboolean beenhere = FALSE;
   HMODULE kernel32;
 
   if (beenhere)