Bug 65147

On Fri, 20 Feb 2015, alexey.lapshin at oracle dot com wrote:

> According to the documentation -
> https://gcc.gnu.org/wiki/Atomic/GCCMM/UnalignedPolicy alignment of atomic
> object should match it`s size.
> Alignment in the test case below does not match with documentation.

Not in itself a bug to fail to follow preliminary plans.  *But*:

> This behavior also differs from gcc. Gcc aligns 8-bytes objects to 8-bytes.

Differences between C and C++ atomics might be a bug - there may be intent 
for them to be ABI-compatible (although that didn't get implemented), 
unlike other aspects of the early plans you point to where intent changed 
over the course of implementation.

This does seem like a bug.

(In reply to Jason Merrill from comment #2)
> This does seem like a bug.

What is a proper behavior for G++ in this case ?

should it always align std::atomic object of size 8 at 8 bytes ?

Or should G++ just never inline implementation for atomic routine ?

(see bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65149 when implementation of atomic routine was inlined for incorrectly aligned atomic object, which leads to BusError)

I think std::atomic<T> should increase the alignment of its T member. That will have the advantage of being layout-compatible with _Atomic T.

Author: redi
Date: Thu Mar 26 19:27:02 2015
New Revision: 221703

URL: https://gcc.gnu.org/viewcvs?rev=221703&root=gcc&view=rev
Log:
	PR libstdc++/62259
	PR libstdc++/65147
	* include/std/atomic (atomic<T>): Increase alignment for types with
	the same size as one of the integral types.
	* testsuite/29_atomics/atomic/60695.cc: Adjust dg-error line number.
	* testsuite/29_atomics/atomic/62259.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
      - copied, changed from r221701, trunk/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/atomic
    trunk/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc

Fixed for gcc5.

(In reply to Jonathan Wakely from comment #5)
> Author: redi
> Date: Thu Mar 26 19:27:02 2015
> New Revision: 221703
> 
> URL: https://gcc.gnu.org/viewcvs?rev=221703&root=gcc&view=rev
> Log:
> 	PR libstdc++/62259
> 	PR libstdc++/65147
> 	* include/std/atomic (atomic<T>): Increase alignment for types with
> 	the same size as one of the integral types.
> 	* testsuite/29_atomics/atomic/60695.cc: Adjust dg-error line number.
> 	* testsuite/29_atomics/atomic/62259.cc: New.
> 
> Added:
>     trunk/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
>       - copied, changed from r221701,
> trunk/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
> Modified:
>     trunk/libstdc++-v3/ChangeLog
>     trunk/libstdc++-v3/include/std/atomic
>     trunk/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc


It looks like this fix makes alignment of atomic object to be the same as alignment of integral non-atomic object of the same size.

The gcc behavior is different it makes alignment of atomic objects of sizes 1,2,4,8,16 to match with size :

G++ :
$ cat all.cc

#include <atomic>
#include <stdio.h>

typedef struct {
   char c[16];
} S16;

int main ( void ) {
   std::atomic<char>      ac;
   std::atomic<short>     as;
   std::atomic<long>      al;
   std::atomic<long long> all;
   std::atomic<S16>       a16;

   printf("\n sizeof(ac) %d alignof(ac) %d",  sizeof(ac), alignof(ac) );
   printf("\n sizeof(as) %d alignof(as) %d",  sizeof(as), alignof(as) );
   printf("\n sizeof(al) %d alignof(al) %d",  sizeof(al), alignof(al) );
   printf("\n sizeof(all) %d alignof(all) %d",  sizeof(all), alignof(all) );
   printf("\n sizeof(a16) %d alignof(a16) %d",  sizeof(a16), alignof(a16) );
   printf("\n");
}
$g++ -latomic -std=c++11 -m32 all.cc 
$./a.out

 sizeof(ac) 1 alignof(ac) 1
 sizeof(as) 2 alignof(as) 2
 sizeof(al) 4 alignof(al) 4
 sizeof(all) 8 alignof(all) 4
 sizeof(a16) 16 alignof(a16) 1


gcc : 

$ cat all.c

#include <stdatomic.h>
#include <stdio.h>

typedef struct {
   char c[16];
} S16;

int main ( void ) {
   _Atomic char      ac;
   _Atomic short     as;
   _Atomic long      al;
   _Atomic long long all;
   _Atomic S16       a16;

   printf("\n sizeof(ac) %d alignof(ac) %d",  sizeof(ac), __alignof__(ac) );
   printf("\n sizeof(as) %d alignof(as) %d",  sizeof(as), __alignof__(as) );
   printf("\n sizeof(al) %d alignof(al) %d",  sizeof(al), __alignof__(al) );
   printf("\n sizeof(all) %d alignof(all) %d",  sizeof(all), __alignof__(all) );
   printf("\n sizeof(a16) %d alignof(a16) %d",  sizeof(a16), __alignof__(a16) );

   printf("\n");
}
$ gcc -latomic -std=c11 -m32 all.c 
$ ./a.out

 sizeof(ac) 1 alignof(ac) 1
 sizeof(as) 2 alignof(as) 2
 sizeof(al) 4 alignof(al) 4
 sizeof(all) 8 alignof(all) 8
 sizeof(a16) 16 alignof(a16) 16
avl@ficus:~/atomic_test$ 

Note 8-bytes and 16-bytes objects aligned at their size at -m32.

(In reply to Alexey Lapshin from comment #7)
> It looks like this fix makes alignment of atomic object to be the same as
> alignment of integral non-atomic object of the same size.

That was the intention, yes. Because I understood that to be what _Atomic does.

> The gcc behavior is different it makes alignment of atomic objects of sizes
> 1,2,4,8,16 to match with size :

Oh.

I don't have a fix for this yet, so let's re-open it ...

(In reply to Alexey Lapshin from comment #7)
> It looks like this fix makes alignment of atomic object to be the same as
> alignment of integral non-atomic object of the same size.

Actually it only did that for non-integral atomic objects, e.g. I didn't do anything to change std::atomic<long long>.

> The gcc behavior is different it makes alignment of atomic objects of sizes
> 1,2,4,8,16 to match with size :

That's not strictly true, there is a target hook (atomic_align_for_mode) which specifies the alignment for 1/2/4/8/16-byte objects, and the result is not necessarily the same as the size. Or so I'm told. That's why I used the nested conditional expressions with alignof(integral type) instead of just using alignas(sizeof(T)).

> $g++ -latomic -std=c++11 -m32 all.cc 
> $./a.out
> 
>  sizeof(ac) 1 alignof(ac) 1
>  sizeof(as) 2 alignof(as) 2
>  sizeof(al) 4 alignof(al) 4
>  sizeof(all) 8 alignof(all) 4
>  sizeof(a16) 16 alignof(a16) 1

There are two problems here.

The first is that alignof(std::atomic<long long>) is less than alignof(long long), and my recent changes didn't address that. That is easy to fix:

--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -235,7 +235,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // 8 bytes, since that is what GCC built-in functions for atomic
   // memory access expect.
   template<typename _ITp>
-    struct __atomic_base
+    struct alignas(_ITp) __atomic_base
     {
     private:
       typedef _ITp     __int_type;


The second problem is that alignof(struct S16) is not increased. That's because libstdc++ doesn't support __int128 on x86, so this bit of code doesn't do anything:

#ifdef _GLIBCXX_USE_INT128
	: sizeof(_Tp) == sizeof(__int128)  ? alignof(__int128)
#endif

I'm not sure how to fix this. Maybe we should just bodge it like this and hope it is valid for all important targets:

#ifdef _GLIBCXX_USE_INT128
	: sizeof(_Tp) == sizeof(__int128)  ? alignof(__int128)
#else
	: sizeof(_Tp) == 16  ? 16
#endif

(The real solution is a new attribute that uses the target hook, so we can guarantee the same result as the C front end, but it's too late to do that for GCC 5).

Author: redi
Date: Thu Apr  9 11:15:44 2015
New Revision: 221945

URL: https://gcc.gnu.org/viewcvs?rev=221945&root=gcc&view=rev
Log:
2015-04-09  Jonathan Wakely  <jwakely@redhat.com>
	    Richard Henderson  <rth@redhat.com>

	PR libstdc++/65147
	* include/bits/atomic_base.h (__atomic_base<_ITp>): Increase
	alignment.
	* include/std/atomic (atomic): For types with a power of two size set
	alignment to at least the size.
	* testsuite/29_atomics/atomic/60695.cc: Adjust dg-error line number.
	* testsuite/29_atomics/atomic/65147.cc: New.
	* testsuite/29_atomics/atomic_integral/65147.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
      - copied, changed from r221943, trunk/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
    trunk/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc
      - copied, changed from r221943, trunk/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/atomic_base.h
    trunk/libstdc++-v3/include/std/atomic
    trunk/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc

Alexey, your testcases in comment 0 and comment 7 give the right results now.

(In reply to Jonathan Wakely from comment #12)
> Alexey, your testcases in comment 0 and comment 7 give the right results now.

Thank You!

GCC 5.1 has been released.

This was fixed.