GNOME Bugzilla – Bug 682818
atomic ops broken on mac/xcode
Last modified: 2013-11-10 17:08:57 UTC
Created attachment 222572 [details] [review] hack patch It seems that the atomic operation detection does not work for gcc 4.2.1, which is the latest version shipped with Xcode. Since the Mac is an important platform to support, I hope a workaround can be made for this unfortunately very old gcc version. I do not know the best way to fix this. What I have noticed is that this version of gcc does seem to understand the __sync_synchronized() instruction, and so if I force the code to compile anyway, regardless of the available compiler flags (read: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4), then the code builds and appears to work. At least I can run a complex GStreamer-based app without any apparent bugs. This does not prove synchronization is actually happening but I hope it is... Attached is a patch that somewhat forces things to work on Mac. It likely breaks other platforms though. Maybe someone else knows enough about this stuff to rework it in a way that it could be safe for everyone and be committed to glib. Filed as critical since the issue hoses GStreamer on Mac.
I already observed problems with this on OS X in February. I also didn't see so quickly how to fix it and wanted to get back to it at some point -- of course I should have put in a bug report. What happened to me was that the fallback atomic operations implemented using mutexes were compiled in because the detection failed, which (of course) caused a severe degradation in performance. Because of this I have been advising some OS X users to not upgrade to recent versions of glib. From what I saw, Apple gcc 4.2.1 appears to understand some of the intrinsics that were used but not all of them. It might be possible to use the same code with Apple gcc 4.2.1 but by using a different detection method. Note that Apple gcc 4.2.1 is a patched up version of gcc 4.2.1. Though better would be to use intrinsics or code that compile fine with Clang as well.
Please, keep in minds that it's not only Mac OS X issue. FreeBSD is affected also because also stuck on gcc-4.2.1 as "base" compiler (license issue).
<tpm> hrm, which reminds me of the atomic ops mess on OS/X, wonder if anything can be done about that (https://bugzilla.gnome.org/show_bug.cgi?id=682818) <ebassi> the proper way to fix it would be to use Clang's own atomic intrinsics <ebassi> instead of relying on the gcc compatibility layer <tpm> is it not an actual gcc that's being used here, though an older version? <ebassi> http://clang.llvm.org/docs/LanguageExtensions.html#__c11_atomic <ebassi> tpm: on MacOS I'd recommend compiling with clang, not gcc <ebassi> the heavily patched gcc Apple ships is not really nice to use, and it's stuck to 4.2.1 <ebassi> the compatibility layer has various holes <tpm> fair enough, I guess, though I wonder why people still use it then :) <ebassi> because we default to CC=gcc <ebassi> if you did CC=clang make, you'd end up using the gcc compatibility layer in clang, which does not have a bunch of needed #defines <ebassi> so the best option is to check if we're using clang, and check for clang features directly <tpm> right <ebassi> and just tell people to use clang as the default CC when building on MacOS
Adding perf keyword. Performance on GStreamer is affected "a lot" by this.
What about to exploit c99 external inlines? 1. configure checks whether atomic ops can be implemented properly with the current compiler (the one used for build glib itself) 2. if yes, then outline version of atomic functions generated (just like in the current stable glib), if not then fallback to mutex-based (again, just like in the current stable glib) 3. gatomic.h declares atomic op functions as external inline. 4. independently on the current compiler, gatomic.h provides implementation for these "external imlines" guarded by __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 (like in the current development glib) At the compile time of the program that _uses_ glib: 1. if compiler is fresh enough gcc, then it will pass thru guard, obtain inline body and obtain a chance to inline it. If for some reason inlining will be considered undesirable, then inline-body will be thrown out and usual call to the outline version will be emited. 2. if compiler is not a gcc, or is gcc but doesn't provide __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 define, then it again happily fall back to the outline version of atomic ops. And please don't say me that such approrach is unacceptable because requires compiler with some basic support of c99. My proposal does not narrow the set of compatible compilers, but extends it. The __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 supported 1. only by gcc, _and_ 2. only by the fairly modern versions of gcc In contrast to that, the inline-related part of c99 implemented not only by these "fairly modern versions of gcc", but also, at least, by gcc-4.2 and clang. As consequence, glib will continue to be useble on FreeBSD and Mac OS X. Alternativelly: you may just switch to c11 atomics. Yes, c11 compilers are not so widely available yet, but at least it's rely on standard instead of some obscure implementation specific define, Yes, gcc-4.2 continues to be out of luck, but clang obtains a chance. The same for the fresh enough gcc versions.
Created attachment 228635 [details] [review] configure: fix check for atomic operations I have different approach to fix the atomic operations check in configure. Right now we only checks for __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4, which seems to be missing in the shipped version of gcc in OS X and clang too. I propose changing this check, to check for __sync_bool_compare_and_swap and define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 if this checks succeed and the compiler does not define it.
Created attachment 228636 [details] [review] configure: fix check for atomic operations Fixed comments
Review of attachment 228636 [details] [review]: ::: configure.ac @@ +2378,3 @@ + [AC_DEFINE(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4, 1, [ compiler supports atomic operations])]) +fi + I don't really like this way of doing it, since it means that we end up with a confusing /* #undef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 */ in config.h.
*** Bug 668906 has been marked as a duplicate of this bug. ***
Lets go with this anyway.
Sorry for not looking at this for so long, but at FOSDEM Mitch finally convinced me that this problem will not easily go away (even if clang fixes their bug). The solution here is pretty close to what I was going to do anyway, although I would have used a separate define for it. I'm happy enough with this for now.
One note: the only reason that this works is because at compile time of apps using GLib, they will still call into the GLib API (and not use the inlines directly). GLib itself was built with this define, though, so its function implementations will do the atomic op for you. It's not perfect, but a perfect fix would require a sane/robust compile-time check and this solution is still much better than using a mutex to emulate.
Thanks for taking care of it Ryan :)
See bug 697017 for likely fallout of this change.
The test in configure.ac for __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is reversed. It's currently this... AC_TRY_COMPILE([], [__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4;], [], [AC_DEFINE(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4, 1, [ compiler supports atomic operations])]) where it should be.... AC_TRY_COMPILE([], [__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4;], [AC_DEFINE(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4, 1, [ compiler supports atomic operations])], []) which causes many of the reported problems to do with undefined symbols for __sync_fetch_and_add_4 etc.