GNOME Bugzilla – Bug 650935
G_GNUC_MAY_ALIAS and atomic ops
Last modified: 2011-05-28 20:17:08 UTC
c8e37b63e74fafdc1f299ec139677ad0e37676c3 contains a patch from Jakub Jelinek to fix strict aliasing warnings with glib by using G_GNUC_MAY_ALIAS https://bugzilla.redhat.com/show_bug.cgi?id=485914 contains the original report. The patch introduces annotations to the function call parameters and introduces a number of macros to insert the appropriate casts in order to avoid warnings. In the case of the integer atomic operations, the original report states that the intention is to allow them to be used with unsigned integers without GCC giving warnings. ISO C permits aliasing between signed/unsigned versions of the same type, however, so this is not an issue. I checked and GCC does not issue a warning, even with -Wstrict-aliasing=2. The introduced macros also provide inconsistent coverage of the available atomic operations. They cover the get and set operations but not the compare-and-exchange or add ones. The macros also have the feature of allow arbitrary pointer types to be used but this isn't documented (and it should be). I believe that we should remove the G_GNUC_MAY_ALIAS annotation from all of the functions dealing with integers and provide full macro coverage for arbitrary pointer types for all atomic pointer operations (and document this). It is unclear if we should allow the atomic integer operations to be used with unsigned integers without explicit casts from the user, but if we intend for this to be the case then we should support it uniformly for all operations.
It's also somewhat clear to me that this is completely broken: #ifndef G_ATOMIC_OP_MEMORY_BARRIER_NEEDED # define g_atomic_int_get(atomic) ((gint)*(atomic)) # define g_atomic_int_set(atomic, newval) ((void) (*(atomic) = (newval))) # define g_atomic_pointer_get(atomic) ((gpointer)*(atomic)) # define g_atomic_pointer_set(atomic, newval) ((void) (*(atomic) = (newval))) since G_ATOMIC_OP_MEMORY_BARRIER_NEEDED is only defined in the case that hardware reordering is a possibility and does not address compiler reordering. In the case that the pointer given to these macros is non-volatile then this is just a normal memory operation and (subject to aliasing analysis) the compiler is free to reorder loads and stores as it sees fit. That flies rather in the face of the documentation which includes the statement: "Also acts as a memory barrier." Without relying on GCC features, there are two possible ways to fix this: - remove this optimisation and use the functions in this case - cast the pointers to volatile-qualified versions of themselves The second solution may still be insufficient, however, since I believe the compiler is free to move non-volatile loads and stores across a volatile memory access (which in my opinion is still violating the user's expectation of a memory barrier having occurred).
It turns out that even within glib itself the atomic integer operations are widely used with unsigned integers. I suppose we should document this and support it properly (ie: for all functions).
Created attachment 188433 [details] [review] atomic: clean up macros, docs and strict aliasing
It'd be good to get Jakub's input on this.
Created attachment 188461 [details] [review] atomic: clean up macros, docs and strict aliasing
Comment on attachment 188461 [details] [review] atomic: clean up macros, docs and strict aliasing ugh. bugzilla appeared to reject this patch last night so i resubmitted it. ignore.
Created attachment 188463 [details] [review] Add a test case for atomic ops Make sure that the macros work properly with the range of types that they are documented to work with and ensure that no strict aliasing warnings are issued (even at the highest warning level). This test demonstrates a shortcoming of the pointer-casting/aliasing machinery for the pointer operations when used with gsize and -Wstrict-aliasing=2. I was unable to figure out how to fix that last night but I will continue to try. I'm not sure that it is safe to commit this since it uses the -Wstrict-aliasing=2 CFLAG unconditionally (and I don't know how non-GCC will respond to that).
Review of attachment 188433 [details] [review]: Would be good if the commit message were a bit more specific as to what the 'several issues' are. 'This is a macro' is a little misleading, since we are still providing function equivalents. Maybe 'This is also provided as a macro' ?
Review of attachment 188463 [details] [review]: So, this is just meant as a compile test ? I mean, looks like it would still 'succeed' if all the macros were defined as '(void)0' ?
(In reply to comment #9) > Review of attachment 188463 [details] [review]: > > So, this is just meant as a compile test ? I mean, looks like it would still > 'succeed' if all the macros were defined as '(void)0' ? I mostly attached it because it demonstrates an outstanding problem. I could turn it into a proper test case.
(In reply to comment #8) > Review of attachment 188433 [details] [review]: > 'This is a macro' is a little misleading, since we are still providing function > equivalents. Maybe 'This is also provided as a macro' ? I think we should have all of these functions "officially" as macros. As I see it, the functions that stand behind the macros are strictly implementation detail. We could rename them all to _impl, but I think that would be problematic from an ABI stability standpoint (since we'd have to keep the old names anyway).
I don't know if it's a GCC bug, if it's simply impossible or if I'm just missing something, but I can't find a way to make it possible to pass a 'gsize*' through any combination of casts into a function that takes any form of qualified gpointer*, no matter how many G_GNUC_MAY_ALIAS annotations I add. I'm starting to think that it would be easier to make the atomic-pointer functions take straight 'gpointer'. Then we wouldn't need any annotations or casting.
I do easily reproduce warnings, when I force the functions to be used in your atomic.c testcase, e.g (g_atomic_int_set)(&u) gives a warning. All of them seem to be due to -Wpointer-sign though, not due to -Wstrict-aliasing
commit 83821352657a9481dbff6ab04e8ae60566c17d5e Author: Ryan Lortie <desrt@desrt.ca> Date: Sat May 28 15:59:18 2011 -0400 glib: Rewrite gatomic.[ch] - remove all inline assembly versions - implement the atomic operations using either GCC intrinsics, the Windows interlocked API or a mutex-based fallback - drop gatomic-gcc.c since these are now defined in the header file. Adjust Makefile.am accordingly. - expand the set of operations: support 'get', 'set', 'compare and exchange', 'add', 'or', and 'xor' for both integers and pointers - deprecate g_atomic_int_exchange_and_add since g_atomic_int_add (as with all the new arithmetic operations) now returns the prior value - unify the use of macros: all functions are now wrapped in macros that perform the proper casts and checks - remove G_GNUC_MAY_ALIAS use; it was never required for the integer operations (since casting between pointers that only vary in signedness of the target is explicitly permitted) and we avoid the need for the pointer operations by using simple 'void *' instead of 'gpointer *' (which caused the 'type-punned pointer' warning) - provide function implementations of g_atomic_int_inc and g_atomic_int_dec_and_test: these were strictly macros before - improve the documentation to make it very clear exactly which types of pointers these operations may be used with - remove a few uses of the now-deprecated g_atomic_int_exchange_and_add - drop initialisation of gatomic from gthread (by using a GStaticMutex instead of a GMutex) - update glib.symbols and documentation sections files Closes #650823 and #650935