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 155884 - gatomic.c should be based on new SDK.
gatomic.c should be based on new SDK.
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: win32
unspecified
Other All
: Normal enhancement
: ---
Assigned To: gtk-win32 maintainers
gtk-win32 maintainers
: 322351 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2004-10-19 20:11 UTC by Kazuki Iwamoto
Modified: 2011-02-18 16:14 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
patch for gatomic.c (893 bytes, patch)
2004-10-19 20:12 UTC, Kazuki Iwamoto
none Details | Review
Use InterlockedCompareExchangePointer (1.86 KB, patch)
2005-01-06 17:12 UTC, John Ehresman
none Details | Review
Use default WINVER value (1.90 KB, patch)
2005-01-06 21:27 UTC, John Ehresman
none Details | Review

Description Kazuki Iwamoto 2004-10-19 20:11:43 UTC
Prototype of InterlockedCompareExchange was changed.

OLD : PVOID InterlockedCompareExchange(PVOID *Destination,PVOID Exchange,PVOID
Comperand);
NEW : LONG InterlockedCompareExchange(LONG volatile* Destination,LONG
Exchange,LONG Comperand);

So, if it's compled with new SDK, an error occurs.
It's easy to upgrade from old SDK.
But it's difficult to downgrade from new SDK.
I think that gatomic.c should be based on new SDK.
Comment 1 Kazuki Iwamoto 2004-10-19 20:12:41 UTC
Created attachment 32790 [details] [review]
patch for gatomic.c
Comment 2 Tor Lillqvist 2004-12-04 07:53:24 UTC
(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?
Comment 3 Hans Breuer 2004-12-04 08:54:24 UTC
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]
Comment 4 Tor Lillqvist 2004-12-04 17:46:46 UTC
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... 
Comment 5 John Ehresman 2005-01-06 17:12:58 UTC
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.
Comment 6 J. Ali Harlow 2005-01-06 17:30:39 UTC
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.
Comment 7 Hans Breuer 2005-01-06 17:43:25 UTC
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
Comment 8 John Ehresman 2005-01-06 18:06:22 UTC
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?
Comment 9 Hans Breuer 2005-01-06 20:45:58 UTC
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 ;-)
Comment 10 John Ehresman 2005-01-06 21:27:38 UTC
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.
Comment 11 Tor Lillqvist 2005-01-06 22:09:32 UTC
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?
Comment 12 Hans Breuer 2005-01-06 22:40:52 UTC
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 ;-)
Comment 13 Tor Lillqvist 2005-01-06 23:05:24 UTC
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?
Comment 14 Hans Breuer 2005-01-06 23:49:53 UTC
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.
Comment 15 Tor Lillqvist 2006-04-19 10:01:21 UTC
*** Bug 322351 has been marked as a duplicate of this bug. ***
Comment 16 Tor Lillqvist 2006-04-19 10:10:47 UTC
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.
Comment 17 John Ehresman 2006-04-19 12:11:21 UTC
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.
Comment 18 Tor Lillqvist 2006-04-19 12:32:58 UTC
Hmm. OK then, applying the last patch to glib-2-10 and HEAD.