Ryan, looks like the win32 side of the atomic rewrite may need some tighter ifdefs ?

What version of mingw32 is that?  I tried with mingw32 as shipped on Fedora 15 and it worked great.

The original offering from mingw.org (i686-pc-mingw32), not the 32bit
offering from the mingw-w64 project (i686-w64-mingw32) that is commonly
used on recent Linux distributions.

Both projects maintain a different set of headers (the original reason
of the mingw-w64 fork). Mingw.org has the w32api package, currently at
version 3.17-2. If I'm not mistaken, that's the same set of headers
also used by the Cygwin project.

tarball: http://sourceforge.net/projects/mingw/files/MinGW/BaseSystem/RuntimeLibrary/Win32-API/w32api-3.17/

or if you're more comfortable with cvs: http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/w32api/?cvsroot=src

Created attachment 190167 [details] [review]
Patch for additional #ifdef check

Hi,

Hopefully this patch would be helpful on the #ifdef side regarding the usage of Microsoft's Interlocked APIs, which AFAICT is Visual C++ only.

God Bless.

> Hopefully this patch would be helpful on the #ifdef side
> regarding the usage of Microsoft's Interlocked APIs,

Yes, I can build glib again with your patch using mingw.org's
toolchain. Thank you!

Well, that is, together with a fix for bug 650763 addressing an
error much later in the build.

> which AFAICT is Visual C++ only.

mingw.org's set of headers indeed lacks support for those.
But I think mingw-w64 does support them, or Ryan would not have
been able to build glib with Fedora's cross compiler, wouldn't he?

It seems like this is really a bug in only one version of mingw (which appears to have an internal inconsistency with itself).  GLib tries for compatibility with Windows, but I'm not sure how far we should go to work around idiosyncratic behaviours of particular Windows-emulation software (which is essentially what mingw is, since, AFAIK, it attempts to copy all new APIs that Microsoft adds).  I feel particularly strongly about this because the recent rewrite was done with the purpose of simplifying the ratsnest of #ifdef that used to be in that file.

From a practical standpoint, how long would it take to fix mingw32?

> It seems like this is really a bug in only one version of mingw (which
> appears to have an internal inconsistency with itself).

Well, the 32bit binaries on www.gtk.org/ftp.gnome.org have always been
built with mingw.org's toolchain, which is not only "one version of",
but it is the *only* project that get's to call itself MinGW.

The fork of that project, named MinGW-w64 (the similarity with MinGW
is unfortunately very confusing), is apparently becoming more popular
every day with various Linux distro's to build their cross-compilers.
Mainly because mingw-w64 provides support for both 32bit and 64bit
binaries, I guess. But make no mistake, it's a completely different animal.

> GLib tries for compatibility with Windows, but I'm not sure how far we
> should go to work around idiosyncratic behaviours of particular
> Windows-emulation software

Neither MinGW nor MinGW-w64 are emulators. The binaries produced with
these tool chains are native MS-Windows applications.

> (which is essentially what mingw is, since,
> AFAIK, it attempts to copy all new APIs that Microsoft adds).

They provide a Free/Open Source set of headers/libs targeting the
Microsoft C runtime (MSVCRT.DLL) that works with GCC/binutils etc.
If I understood things correctly we need this because the license
of Microsoft's headers would not allow us that kind of usage...

> I feel particularly strongly about this because the recent rewrite was
> done with the purpose of simplifying the ratsnest of #ifdef that used to
> be in that file.

I understand. Then maybe there's another solution than the proposed
#ifdef fix? Is it possible to test the presence/usability of MemoryBarrier +
(_)Interlocked(And|Or|Xor) in the configure script instead?

> From a practical standpoint, how long would it take to fix mingw32?

Honestly, that's not within my capabilities so I wouldn't know...

(In reply to comment #5)
> 
> > which AFAICT is Visual C++ only.
> 
> mingw.org's set of headers indeed lacks support for those.
> But I think mingw-w64 does support them, or Ryan would not have
> been able to build glib with Fedora's cross compiler, wouldn't he?

Hi Dieter,

Sorry, I am not someone that is very knowledgeable in GCC/MinGW as I am not using it for Windows development... Can I know what version of GCC you are using?

I was trying to Google around regarding built-in atomic ops in GCC, and it seems
to me that only GCC 4.1 or later has this feature in them[1]-and normally the GCC included in the mingw32.org packages is GCC-3.4.5 unless one upgrades to the quite-recently released (ca. early this year or so) GCC-4.5.2 for MinGW, which most probably the MinGW website would mention on the steps involved.  It seems to me that is the condition for the G_ATOMIC_OP_USE_GCC_BUILTINS macro to be defined.

I think Ryan managed to build with the Fedora cross-compilers with the GCC built-in atomic ops as that system shipped with GCC-4.5.3/MinGW cross-compiler (as I just checked in my rather-stock Fedora15 installation-so the Windows Interlocked APIs were not used in his builds as a result).

The _MSC_VER check, as a result, is to avoid non-Visual C++ compilers to attempt to build the Interlocked API code as AFAICT it is supported only on Visual C++.

[1]:http://trac.mcs.anl.gov/projects/openpa/wiki/FAQ, under the subtitle "Why should I use this instead of GCC atomic intrinsics?"

(In reply to comment #8)
> I think Ryan managed to build with the Fedora cross-compilers with the GCC
> built-in atomic ops as that system shipped with GCC-4.5.3/MinGW cross-compiler
> (as I just checked in my rather-stock Fedora15 installation-so the Windows
> Interlocked APIs were not used in his builds as a result).

It's true that we will use the GCC operations in preference to the interlocked API, when possible.

However, during the testing, I built glib with the GCC atomic operations disabled for the purpose of testing the interlocked API.  The compile was successful except for one warning (and for this one warning it looks like mingw-w64 was in error since I double-checked with MSDN, and the code appeared correct).

(In reply to comment #8)
> Can I know what version of GCC you are using?

I'm still on gcc-4.5.0-1 but an update to gcc-4.5.2-1 is indeed
available on mingw.org. I've tested both versions (on 2 separate
MinGW/MSYS installations), without the #ifdef check, and they both
fail with the same undefined references to MemoryBarrier and InterlockedXX.

Tracing why gcc's built in atomic ops aren't used shows this in the
relevant part from config.log (same error with both gcc versions):

<snip>
configure:26230: checking whether to use assembler code for atomic operations
configure:26235: checking whether GCC supports build-in atomic intrinsics
configure:26252: gcc -mthreads -o conftest.exe -O1 -pipe -ggdb -mms-bitfields -Wall -I/d/dev/gnome.org/gnome-windows/prefix/gtk+-bundle/gtk+-bundle_2.24.0_win32/include -DG_DISABLE_SINGLE_INCLUDES  -L/d/dev/gnome.org/gnome-windows/prefix/gtk+-bundle/gtk+-bundle_2.24.0_win32/lib -Wl,--enable-auto-image-base conftest.c -lintl   >&5
d:\DOCUME~1\xhd8202\LOCALS~1\Temp\ccchyhnU.o: In function `main':
d:\dev\gnome.org\gnome-windows\checkout\glib\glib\src/conftest.c:100: undefined reference to `__sync_bool_compare_and_swap_4'
collect2: ld returned 1 exit status
configure:26252: $? = 1
configure: failed program was:
<snipped the actual code... />
configure:26260: result: no
configure:26269: result: i486
</snip>

Digging deeper I found a thread on mingw-users from the beginning
of the year [1], and adding -march=i486 -mtune=native to CFLAGS
the configure check now succeeds, also with both versions of gcc:

<snip>
configure:26230: checking whether to use assembler code for atomic operations
configure:26235: checking whether GCC supports build-in atomic intrinsics
configure:26252: gcc -mthreads -o conftest.exe -O1 -pipe -ggdb -march=i486 -mtune=native -mms-bitfields -Wall -I/d/dev/gnome.org/gnome-windows/prefix/gtk+-bundle/gtk+-bundle_2.24.0_win32/include -DG_DISABLE_SINGLE_INCLUDES  -L/d/dev/gnome.org/gnome-windows/prefix/gtk+-bundle/gtk+-bundle_2.24.0_win32/lib -Wl,--enable-auto-image-base conftest.c -lintl   >&5
configure:26252: $? = 0
configure:26260: result: yes
configure:26269: result: i486
</snip>

But I still get the the same undefined references to MemoryBarrier and
InterlockedXX.

[1] http://mingw-users.1079350.n2.nabble.com/Problem-with-sync-val-bool-compare-and-swap-in-current-MinGW-td5965743.html

Created attachment 190387 [details] [review]
Fix gatomic.c compilation on Windows (mingw.org GCC)

Ha, got it! Digging even deeper, the glib/glibconfig.h.win32.in
template does not mention G_ATOMIC_OP_USE_GCC_BUILTINS so it was
never even #defined in glib/glibconfig.h. That explains why I kept
seeing the Windows interlocked part being used instead of GCC
builtins in comment 10.

The attached patch leads to a successful build using GCC's
built-in atomic ops:
- without the proposed #ifdef patch
- with mingw.org's toolchain on a Windows XP machine
- with "-march=i486 -mtune=native" in CFLAGS
- with both gcc-4.5.0-1 and gcc-4.5.2-1
- and again, together with a fix for bug 650763 addressing a different issue.

But fails spectacularly with undefined references to multiple
of GCC's built-in functions for atomic memory access:
- without specifying -march and -mtune in CFLAGS
- but everything else as described above.

Further testing shows that the proposed -march=i486 might just
as well be i585 or i686, so the command sequence I'm using now
(based on Tor's build scripts included with the glib binaries on
ftp.gnome.org) is:

cd ${GLIB_SRC_DIR}

lt_cv_deplibs_check_method='pass_all' \
CC='gcc -mthreads' \
CPPFLAGS="-I${PLATFORM}/include" \
LDFLAGS="-L${PLATFORM}/lib -Wl,--enable-auto-image-base" \
CFLAGS="-O1 -pipe -ggdb -march=i686 -mtune=native" \
./configure \
--disable-silent-rules \
--disable-gtk-doc \
--prefix=${GLIB_DIST_DIR} &&

(cd glib &&
make glibconfig.h.win32 &&
make glibconfig-stamp &&
mv glibconfig.h glibconfig.h.autogened &&
cp glibconfig.h.win32 glibconfig.h) &&
PATH="${GLIB_DIST_DIR/bin}:${PATH}" make -j3 install

Comment on attachment 190387 [details] [review]
Fix gatomic.c compilation on Windows (mingw.org GCC)

Just realized my proposed patch is worthless as it also forces
usage of GCC's builtin atomic ops with mingw-w64. I'll try to
come up with a configure check so G_ATOMIC_OP_USE_GCC_BUILTINS is
not #defined when _InterlockedAnd is available (for the 32bit case)
or when InterlockedAnd is available (for the 64bit case).
Any pointers or insights on how this could be achieved are most welcome...

I hit this bug the other day.  Fixed in master.  Testing with MSVC is appreciated.

commit 349212211247781e7bec43b95cb57dddc1d0e5b1
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Tue Aug 9 19:43:34 2011 +0200

    Bug 652827 - glib-2.29.8 no longer builds with mingw.org's toolchain
    
    Check for Win32 atomic intrinsics.

Hi Behdad,

Thanks for looking into this part...The code does build successfully under MSVC,
but one more change is needed.

As a result of your patch, I updated config.h.win32.in so that the HAVE_WIN32_BUILTINS_FOR_ATOMIC_OPERATIONS is defined-the MSVC builds make use of config.h.win32.in (which in turns becomes config.h.win32 in a release tarball, which then is copied to config.h directly during the MSVC build), 
which is not generated (except for versioning stuff).

commit	77a10feafa4f77f1fcadb2ed1fd5da41dced3553
Author:	Chun-wei Fan <fanchunwei@src.gnome.org>	
Date:   2011-08-11 07:30:48 (GMT)

Bug 652827: Update config.h.win32.in

Add check macro for HAVE_WIN32_BUILTINS_FOR_ATOMIC_OPERATIONS, as it is now required for MSVC builds of glib/gatomic.c GLib 2.29.15+.

It is true that the MinGW cross-compiler on Linux systems will have HAVE_GCC_BUILTINS_FOR_ATOMIC_OPERATIONS and HAVE_WIN32_BUILTINS_FOR_ATOMIC_OPERATIONS defined during the completion of ./configure, but since this file is primarily meant for people compiling -on- Windows (and that the "native" Windows MinGW would neither ./configure to define HAVE_GCC_BUILTINS_FOR_ATOMIC_OPERATIONS and HAVE_WIN32_BUILTINS_FOR_ATOMIC_OPERATIONS), this file will be updated as it is for now at least until the situation for "native" Windows MinGW change. (please see Bug 652827 regarding this paragraph)

Thank you!

Ah, thanks.  Can you push?

Hi Behdad,

It has been pushed as of the time I wrote Comment 14.

Thank you!