Re: GAtomic ABI and win32 implementation

[Sebastian Wilhelmi <seppi seppi de>]

Moin Hans,

> > >       g_atomic_int_compare_and_exchange_fallback
> > >       g_atomic_int_exchange_and_add_fallback
> > >       g_atomic_int_get_fallback
> > >       g_atomic_pointer_compare_and_exchange_fallback
> > >       g_atomic_pointer_get_fallback
> >
> >They have to be exported in general, as we need them for e.g. non-gcc
> >compilers, which can't do the inline assembler stuff.
> Two of them were not exportable due to missing implementation, i.e.
> ;       g_atomic_int_get_fallback
> ;       g_atomic_pointer_get_fallback
> seem to be missing with my current configuration. Do you have tested the 
> none- GCC case so that this is just a win32 issue, I'm looking for ?

Fixed in CVS. Thanks.

> >ON win32 however
> >they are not needed, if you use the InterLocked* functions.
> 
> I'd rather like to implement all of them on win32 with the Interlocked*
> family of functions, rather than putting any win32 api into glib headers.
> 
> At least The GIMP and Dia are compiled on win32 with forced include
> of msvc_recommended_pragmas.h , especially in this case :
> #pragma warning(error:4013) /* 'function' undefined; assuming extern 
> returning int */
> which would break the build if noone includes <windows.h> before <glib.h>.
> 
> OTOH including windows.h would not only break due to namespace clashes but 
> also weaken the cross platform API glib provides ...

I see. In that case however you might actually be better off just using
the generic i486 functions. What's wrong with that? Using the
InterLocked* Functions in gatomic.c will be slower because of two nested
functions calls instead of an inline function. And the i486 version is
correct, too. The only problem of course is, if you compile with
non-gcc. I think, that's your motivation.....

In that case you could try the attached patch (untested..):

> >You might have to add casting (no idea, I have no windows around).
> >
> >Please run tests/atomic-test to test, whether it works.
> Will do, but running such tests without actually having a dual processor 
> machine   wont show any but the most simple errors. Wouldn't it be feasible 
> to at least extend the test to use multiple threads ?

I don't think, that would make sense. Triggering such a bug (if there is
one) is very hard and all I want to test with the atomic-test.c is,
whether there are no simple logical bugs in the functions (e.g. a ==
instead of a !=, happened to me once) and that pointers really will be
stored in full length.

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

Index: glib/gatomic.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gatomic.c,v
retrieving revision 1.1
diff -u -r1.1 gatomic.c
--- glib/gatomic.c	26 Feb 2004 14:30:34 -0000	1.1
+++ glib/gatomic.c	28 Feb 2004 16:42:10 -0000
@@ -23,6 +23,41 @@
 #include <glib.h>
 
 #ifdef G_THREADS_ENABLED
+# if defined (G_PLATFORM_WIN32)
+# include <windows.h>
+gint32   
+g_atomic_int_exchange_and_add (gint32   *atomic, 
+			       gint32    val)
+{
+  return InterlockedExchangeAdd (atomic, val);
+}
+
+void     
+g_atomic_int_add (gint32   *atomic, 
+		  gint32    val)
+{
+  InterlockedExchangeAdd (atomic, val);
+}
+
+gboolean 
+g_atomic_int_compare_and_exchange (gint32   *atomic, 
+				   gint32    oldval, 
+				   gint32    newval)
+{
+  return InterlockedCompareExchange (atomic, newval, oldval) == oldval;
+}
+
+gboolean 
+g_atomic_pointer_compare_and_exchange (gpointer *atomic, 
+				       gpointer  oldval, 
+				       gpointer  newval)
+{
+  return InterlockedCompareExchangePointer (atomic, newval, oldval) == oldval;
+}
+
+#  define g_atomic_int_get(atomic) (*(atomic))
+#  define g_atomic_pointer_get(atomic) (*(atomic))
+# else /* !G_PLATFORM_WIN32 */
 # if !defined (G_ATOMIC_USE_FALLBACK_IMPLEMENTATION)
 /* We have an inline implementation, which we can now use for the
  * fallback implementation. This fallback implementation is only used by
@@ -140,7 +175,7 @@
   return result;
 }
 
-static inline gint32
+gint32
 g_atomic_int_get_fallback (gint32 *atomic)
 {
   gint32 result;
@@ -152,7 +187,7 @@
   return result;
 }
 
-static inline gpointer
+gpointer
 g_atomic_pointer_get_fallback (gpointer *atomic)
 {
   gpointer result;
@@ -165,7 +200,8 @@
 }   
 
 
-# endif /* G_ATOMIC_USE_FALLBACK_IMPLEMENTATION */
+#  endif /* G_ATOMIC_USE_FALLBACK_IMPLEMENTATION */
+# else /* G_PLATFORM_WIN32 */
 #else /* !G_THREADS_ENABLED */
 gint32 g_atomic_int_exchange_and_add (gint32 *atomic, 
 				      gint32  val)
Index: glib/gatomic.h
===================================================================
RCS file: /cvs/gnome/glib/glib/gatomic.h,v
retrieving revision 1.3
diff -u -r1.3 gatomic.h
--- glib/gatomic.h	26 Feb 2004 17:31:38 -0000	1.3
+++ glib/gatomic.h	28 Feb 2004 16:42:11 -0000
@@ -35,7 +35,20 @@
 G_BEGIN_DECLS
  
 #ifdef G_THREADS_ENABLED
-
+# if defined (G_PLATFORM_WIN32)
+gint32   g_atomic_int_exchange_and_add         (gint32   *atomic, 
+						gint32    val);
+void     g_atomic_int_add                      (gint32   *atomic, 
+						gint32    val);
+gboolean g_atomic_int_compare_and_exchange     (gint32   *atomic, 
+						gint32    oldval, 
+						gint32    newval);
+gboolean g_atomic_pointer_compare_and_exchange (gpointer *atomic, 
+						gpointer  oldval, 
+						gpointer  newval);
+#  define g_atomic_int_get(atomic) (*(atomic))
+#  define g_atomic_pointer_get(atomic) (*(atomic))
+# else /* !G_PLATFORM_WIN32 */
 gint32   g_atomic_int_exchange_and_add_fallback         (gint32   *atomic, 
 							 gint32    val);
 void     g_atomic_int_add_fallback                      (gint32   *atomic, 
@@ -46,8 +59,7 @@
 gboolean g_atomic_pointer_compare_and_exchange_fallback (gpointer *atomic, 
 							 gpointer  oldval, 
 							 gpointer  newval);
-
-# if defined (__GNUC__)
+#  if defined (__GNUC__)
 #   if defined (G_ATOMIC_INLINED_IMPLEMENTATION_I486)
 /* Adapted from CVS version 1.10 of glibc's sysdeps/i386/i486/bits/atomic.h 
  */
@@ -491,10 +503,10 @@
 #   else /* !G_ATOMIC_INLINED_IMPLEMENTATION_... */
 #     define G_ATOMIC_USE_FALLBACK_IMPLEMENTATION
 #   endif /* G_ATOMIC_INLINED_IMPLEMENTATION_... */
-# else /* !__GNU__ */
+#  else /* !__GNU__ */
 #   define G_ATOMIC_USE_FALLBACK_IMPLEMENTATION
-# endif /* __GNUC__ */
-# ifdef G_ATOMIC_USE_FALLBACK_IMPLEMENTATION
+#  endif /* __GNUC__ */
+#  ifdef G_ATOMIC_USE_FALLBACK_IMPLEMENTATION
 #  define g_atomic_int_exchange_and_add					\
     g_atomic_int_exchange_and_add_fallback
 #  define g_atomic_int_add						\
@@ -507,7 +519,7 @@
     g_atomic_int_get_fallback
 #  define g_atomic_pointer_get 						\
     g_atomic_pointer_get_fallback
-# else /* !G_ATOMIC_USE_FALLBACK_IMPLEMENTATION */
+#  else /* !G_ATOMIC_USE_FALLBACK_IMPLEMENTATION */
 static inline gint32
 g_atomic_int_get (gint32 *atomic)
 {
@@ -523,7 +535,8 @@
   G_ATOMIC_MEMORY_BARRIER ();
   return result;
 }   
-# endif /* G_ATOMIC_USE_FALLBACK_IMPLEMENTATION */
+#  endif /* G_ATOMIC_USE_FALLBACK_IMPLEMENTATION */
+# endif /* G_PLATFORM_WIN32 */
 #else /* !G_THREADS_ENABLED */
 gint32 g_atomic_int_exchange_and_add (gint32 *atomic, gint32  val);
 # define g_atomic_int_add(atomic, val) (void)(*(atomic) += (val))