Bug 65147 - alignment of std::atomic object is not correct
Summary: alignment of std::atomic object is not correct
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.9.2
: P3 normal
Target Milestone: 5.0
Assignee: Not yet assigned to anyone
URL:
Keywords: ABI
Depends on:
Blocks:
 
Reported: 2015-02-20 19:47 UTC by Alexey Lapshin
Modified: 2015-04-22 12:59 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2015-03-20 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Lapshin 2015-02-20 19:47:57 UTC
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.

~/atomic_test$ cat unaligned_atomic.cpp

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

typedef struct {
    char c [8];
} power_of_two_obj;

typedef struct {
   char c[1];
   std::atomic<power_of_two_obj> ao;
} container_struct; 

int main ( void ) {

    std::atomic<power_of_two_obj> obj1;
    container_struct              obj2; 


    printf("\n Size and Alignment of std::atomic  object "); 
    printf(" : sizeof(obj1) %d alignof(obj1) %d ", sizeof(obj1), alignof(obj1) );

    printf("\n Size and Alignment of std::atomic member object "); 
    printf(" : sizeof(obj2.ao) %d alignof(obj2.ao) %d \n", sizeof(obj2.ao), alignof(obj2.ao) );

    return 0;
}

~/atomic_test$ g++ -v
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/opt/gcc/libexec/gcc/i386-pc-solaris2.11/4.9.2/lto-wrapper
Target: i386-pc-solaris2.11
Configured with: ./configure --prefix=/opt/gcc/
Thread model: posix
gcc version 4.9.2 (GCC) 

~/atomic_test$ g++ -O -latomic -std=c++11 unaligned_atomic.cpp -m32

~/atomic_test$ ./a.out

 Size and Alignment of std::atomic  object  : sizeof(obj1) 8 alignof(obj1) 1 
 Size and Alignment of std::atomic member object  : sizeof(obj2.ao) 8 alignof(obj2.ao) 1 

Alignment should be 8-bytes in above test case.

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

The bug was found on Solarix x86 -m32, but it could be on other platforms - SPARC, Linux, -m64.

There is another problem with similar test case. If size of object is not power of two then size of corresponding atomic object is also not power of two. According to  https://gcc.gnu.org/wiki/Atomic/GCCMM/UnalignedPolicy the size should be upsized in such case.
Comment 1 joseph@codesourcery.com 2015-02-20 21:19:32 UTC
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.
Comment 2 Jason Merrill 2015-03-20 21:15:40 UTC
This does seem like a bug.
Comment 3 Alexey Lapshin 2015-03-23 17:13:35 UTC
(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)
Comment 4 Jonathan Wakely 2015-03-23 17:25:31 UTC
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.
Comment 5 Jonathan Wakely 2015-03-26 19:27:34 UTC
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
Comment 6 Jonathan Wakely 2015-03-26 20:06:18 UTC
Fixed for gcc5.
Comment 7 Alexey Lapshin 2015-03-27 15:47:16 UTC
(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.
Comment 8 Jonathan Wakely 2015-03-27 18:31:19 UTC
(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.
Comment 9 Jonathan Wakely 2015-03-28 00:45:19 UTC
I don't have a fix for this yet, so let's re-open it ...
Comment 10 Jonathan Wakely 2015-03-30 10:35:39 UTC
(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).
Comment 11 Jonathan Wakely 2015-04-09 11:16:15 UTC
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
Comment 12 Jonathan Wakely 2015-04-09 11:18:34 UTC
Alexey, your testcases in comment 0 and comment 7 give the right results now.
Comment 13 Alexey Lapshin 2015-04-10 14:46:28 UTC
(In reply to Jonathan Wakely from comment #12)
> Alexey, your testcases in comment 0 and comment 7 give the right results now.

Thank You!
Comment 14 Jakub Jelinek 2015-04-22 11:59:17 UTC
GCC 5.1 has been released.
Comment 15 Jonathan Wakely 2015-04-22 12:59:53 UTC
This was fixed.