Re: [libstdc++/65033] Give alignment info to libatomic

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On 18/02/15 12:15 +0000, Jonathan Wakely wrote:
>On 12/02/15 13:23 -0800, Richard Henderson wrote:
>>When we fixed PR54005, making sure that atomic_is_lock_free returns the same
>>value for all objects of a given type, we probably should have changed the
>>interface so that we would pass size and alignment rather than size and object
>>pointer.
>>
>>Instead, we decided that passing null for the object pointer would be
>>sufficient.  But as this PR shows, we really do need to take alignment into
>>account.
>>
>>The following patch constructs a fake object pointer that is maximally
>>misaligned.  This allows the interface to both the builtin and to libatomic to
>>remain unchanged.  Which probably makes this back-portable to maintenance
>>releases as well.
>
>Am I right in thinking that another option would be to ensure that
>std::atomic<> objects are always suitably aligned? Would that make
>std::atomic<> slightly more compatible with a C11 atomic_int, where
>the _Atomic qualifier affects alignment?
>
>https://gcc.gnu.org/PR62259 suggests we might need to enforce
>alignment on std::atomic anyway, or am I barking up the wrong tree?
>

I've convinced myself that Richard's patch is correct in all cases,
but I think we also want this patch, to fix PR62259 and PR65147.

For the generic std::atomic<T> (i.e. not the integral or pointer
specializations) we should increase the alignment of atomic types that
have the same size as one of the standard integral types. This should
be consistent with what the C front end does for _Atomic, based on
what Joseph told me on IRC:

<jsm28> jwakely: _Atomic aligns 1/2/4/8/16-byte types the same as
        integer types of that size.
<jsm28> (Which may not be alignment = size, depending on the
        architecture.)

Ideally we'd use an attribute like Andrew describes at
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259#c4 but that's not
going to happen for GCC 5, so this just looks for an integral type of
the same size and uses its alignment.

Tested x86_64-linux, powerpc64le-linux.

I'll wait for RM approval for this and Richard's patch (which is OK
from a libstdc++ perspective).

[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 3755 bytes --]

commit bdcba837b42bbef3143ea59a0194bd3ef435dfcb
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Sep 3 15:39:53 2014 +0100

    	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.

diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
index cc4b5f1..5f02fe8 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -165,7 +165,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     struct atomic
     {
     private:
-      _Tp _M_i;
+      // Align 1/2/4/8/16-byte types the same as integer types of that size.
+      // This matches the alignment effects of the C11 _Atomic qualifier.
+      static constexpr int _S_alignment
+	= sizeof(_Tp) == sizeof(char)	   ? alignof(char)
+	: sizeof(_Tp) == sizeof(short)	   ? alignof(short)
+	: sizeof(_Tp) == sizeof(int)	   ? alignof(int)
+	: sizeof(_Tp) == sizeof(long)	   ? alignof(long)
+	: sizeof(_Tp) == sizeof(long long) ? alignof(long long)
+#ifdef _GLIBCXX_USE_INT128
+	: sizeof(_Tp) == sizeof(__int128)  ? alignof(__int128)
+#endif
+	: alignof(_Tp);
+
+      alignas(_S_alignment) _Tp _M_i;
 
       static_assert(__is_trivially_copyable(_Tp),
 		    "std::atomic requires a trivially copyable type");
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc b/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
index b59c6ba..806ccb1 100644
--- a/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
+++ b/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
@@ -27,4 +27,4 @@ struct X {
   char stuff[0]; // GNU extension, type has zero size
 };
 
-std::atomic<X> a;  // { dg-error "not supported" "" { target *-*-* } 173 }
+std::atomic<X> a;  // { dg-error "not supported" "" { target *-*-* } 186 }
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc b/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
new file mode 100644
index 0000000..cfe70b1
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
@@ -0,0 +1,56 @@
+// Copyright (C) 2015 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-require-atomic-builtins "" }
+// { dg-require-cstdint "" }
+// { dg-options "-std=gnu++11" }
+// { dg-do compile }
+
+#include <atomic>
+#include <cstdint>
+
+// libstdc++/62259
+
+struct twoints {
+  int a;
+  int b;
+};
+
+static_assert( alignof(std::atomic<twoints>) > alignof(int),
+               "std::atomic not suitably aligned" );
+
+// libstdc++/65147
+
+struct power_of_two_obj {
+    char c [8];
+};
+
+std::atomic<power_of_two_obj> obj1;
+
+static_assert( alignof(obj1) == alignof(std::int64_t),
+               "std::atomic not suitably aligned" );
+
+struct container_struct {
+   char c[1];
+   std::atomic<power_of_two_obj> ao;
+};
+
+container_struct obj2;
+
+static_assert( alignof(obj2.ao) > alignof(char),
+               "std::atomic not suitably aligned" );
+