Created attachment 32790 [details] [review]
patch for gatomic.c

(A sidenote: The GLib binaries that I distribute are compiled with gcc, so they 
are compiled with the gcc-only inline assembly and not the Win32 API in 
gatomic.c.)

Hans, what do you think about the issue here? You use MSVC6 (or 5?), don't you, 
but do you still use the new Platform SDK headers?

For gtk+ related stuff it is usually msvc6 (5 for my now broken win98 notebook).
At work the newest version (7.1?). I've noticed the issue some time ago
but don't think using ans Sdk update should be enforced.
IMHO not only downgrading the sdk is hard, but updating as well (300 MB
download, requires internet explorer, ...). As long as the only really
supported msvc compiler are 12 and 13 (i.e. the last one linking to
msvcrt.dll) the Sdk from their installation CDs should be used.
[Sounds a lot like notabug]

I.e. a patch that would use some #ifdef to work with both the headers included 
with MSVC6 and the newest one would be nice... 

Created attachment 35564 [details] [review]
Use InterlockedCompareExchangePointer

Attached patch only uses InterlockedCompareExchangePointer if
HAVE_INTERLOCKED_COMPARE_EXCHANGE_POINTER is defined.  Unfortunately, I
couldn't find any symbol from windows.h to indicate that it should be defined
or not, so I added a comment telling people to comment the #define out if
needed.  Not ideal, but it may be the best we can do without resorting to a
configure test.

Note that gfileutils currently requires headers from the new SDK, so glib
currently doesn't build without this patch.

Surely this would be better done as a trivial AC_TRY_COMPILE configure test.
Apart from anything else this would mean the pre-processor switch would go in
config.h where people will tend to look for it.

isn't WINVER meant to be used for such? The SDK provided
with vc6 defines it to 0x0400 if it isn't defined before.

To your patch:
- IMO changing the source to configure is not an option here ...
- ... at least not if is required for the 'official' compilers
  (i.e. if at all the logic would need to be inverted;)
- Also shouldn't your patch be merging the GLIB_SIZEOF_VOID_P != 4 case?
- if WINVER does not fit the purpose probably some definition
  should be added to the makefile - or better build/win32/make.msc

I agree that using configure is not a realistic option (in theory having a
configure might be a good idea, but it's a lot of work just for this).

Unfortunately WINVER isn't the symbol to use here.  MS decided to change the
function declaration independant of what WINVER is set to.

The GLIB_SIZEOF_VOID_P is a separate problem for 64 bit windows.  I think there
are other api's for 64 bit pointers.

Should HAVE_INTERLOCKED_COMPARE_EXCHANGE_POINTER be defined in make.msc or in
config.h.win32.in?

Even if the SDK you are using does not define WINVER > 0x400 (could you please
look?) to me it looks reasonable to assume that people updating their SDK do
this to get newer API - which sometimes is protected by WINVER.

So what about :

#if (WINVER > 0x0400) || (GLIB_SIZEOF_VOID_P != 4)
  return InterlockedCompareExchangePointer (atomic, newval, oldval) == oldval;
#else
  /* If this does not compile with the SDK used you should define WINVER to
   * at least 0x0401 in your build/win32/make.msc like
   *   CC = cl -G5 $(OPTIMIZE) $(CRUNTIME) -DWINVER=0x0401 -W3 -nologo
   * should work. For more information about this mess ask google, e.g.
   *   InterlockedCompareExchange signature
   */
  return InterlockedCompareExchange (atomic, newval, oldval) == oldval;
#endif

Of course this should only be used if it does not break the gtk+ build
too bad. But if it work it could probably be used at more places.
The multi monitor ugliness springs to mind ;-)

Created attachment 35581 [details] [review]
Use default WINVER value

The new patch uses the default value of WINVER to determine if
InterlockedCompareExchangePointer is available, which will work in WINVER is
not defined before windows.h is included (which can be done if you want only
the api's available in an older version of win32 declared).  Since it's
unlikely that this will be needed in the gatomic.c file, this should be pretty
safe.  I also removed the sizeof(void*) == 4 from the
InterlockedCompareExchangePointer branch of the ifdef.

Sorry, I wasn't thinking of WINVER in terms of the default value that is set if
it's not defined before windows.h.  I think it's pretty safe to use this type
of test in .c files and in private headers.

How hard would it be to simply translate the gcc-style inline assembly to MSVC-
style inline assembly? I mean, we are talking about just two machine 
instructions in each of four functions. (g_atomic_int_exchange_and_add
(): "lock; xaddl", g_atomic_int_add(): "lock; addl", 
g_atomic_int_compare_and_exchange() and g_atomic_pointer_compare_and_exchange
():  "lock; cmpxchgl".) Anybody here done any inline assembly in MSVC, or know 
somebody that can do it?

From my experience it aint that simple, at least not if you take multi processor
machines into account (having had trouble with exactly this function at work a
few years ago). And no the assembler wasn't by me and I don't recall anything of it.
But I was the one removing it again ;-)

Hmm, you mean that you suspect the machine code as used in the i486 gcc inline 
assemby doesn't work reliably on multi-processor machines?

At least reverse engineering of InterlockedExchange() on single and multi
processor NT did give different results and we had crashes with two
processors. But as already noted assembly is out of my scope.

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

Re: comment #6: Using configure tests for this doesn't help, as those who build with MSVC don't run configure scripts.

How about simply removing the troublesome Win32 API implementation altogether, and let it use the DEFINE_WITH_MUTEXES ifdef branch when building with MSVC? The official binaries are built with gcc anyway, and thus use the inline assembly implementation.

What's wrong with the last patch attached to this bug?   It might need some updating, but I'd be more than willing to do so.  I don't think we want to drop the usage of InterlockedExchange because mutex's are almost certainly slower.

Hmm. OK then, applying the last patch to glib-2-10 and HEAD.