diff mbox

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

Message ID 20150325162244.GF9755@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely March 25, 2015, 4:22 p.m. UTC 
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).

Comments

Richard Henderson March 25, 2015, 6:36 p.m. UTC | #1 
On 03/25/2015 09:22 AM, Jonathan Wakely wrote:
>      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;


Surely not by reducing a larger alignment applied to _Tp.
I.e.

  static constexpr int _S_min_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
	: 0;

  static constexpr int _S_alignment
	= _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp);



r~
Richard Henderson March 25, 2015, 6:39 p.m. UTC | #2 
On 03/25/2015 09:22 AM, Jonathan Wakely wrote:
> +static_assert( alignof(std::atomic<twoints>) > alignof(int),
> +               "std::atomic not suitably aligned" );

This is only true if int64_t has alignment larger than int32_t,
which is unfortunately not always the case.


r~
Jonathan Wakely March 25, 2015, 6:49 p.m. UTC | #3 
On 25/03/15 11:36 -0700, Richard Henderson wrote:
>On 03/25/2015 09:22 AM, Jonathan Wakely wrote:
>>      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;
>
>
>Surely not by reducing a larger alignment applied to _Tp.
>I.e.
>
>  static constexpr int _S_min_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
>	: 0;
>
>  static constexpr int _S_alignment
>	= _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp);

Doh, good catch. I'll make that change and add a test with a type that
has alignof(X) > sizeof(X).


On 25/03/15 11:39 -0700, Richard Henderson wrote:
>On 03/25/2015 09:22 AM, Jonathan Wakely wrote:
>> +static_assert( alignof(std::atomic<twoints>) > alignof(int),
>> +               "std::atomic not suitably aligned" );
>
>This is only true if int64_t has alignment larger than int32_t,
>which is unfortunately not always the case.

Huh, didn't realise that. I could change the tests to check it's
alignof(std::int64_t) as the next assertion does, but is it safe to
assume that struct twoints { int a; int b; } is exactly 64 bits
everywhere?

I'd prefer not to have the test say "if sizeof(twoints) ==
sizeof(long), test this, otherwise if sizeof(twoints) == ..."
Richard Henderson March 25, 2015, 7:04 p.m. UTC | #4 
On 03/25/2015 11:49 AM, Jonathan Wakely wrote:
> On 25/03/15 11:36 -0700, Richard Henderson wrote:
>> On 03/25/2015 09:22 AM, Jonathan Wakely wrote:
> On 25/03/15 11:39 -0700, Richard Henderson wrote:
>> On 03/25/2015 09:22 AM, Jonathan Wakely wrote:
>>> +static_assert( alignof(std::atomic<twoints>) > alignof(int),
>>> +               "std::atomic not suitably aligned" );
>>
>> This is only true if int64_t has alignment larger than int32_t,
>> which is unfortunately not always the case.
> 
> Huh, didn't realise that. I could change the tests to check it's
> alignof(std::int64_t) as the next assertion does, but is it safe to
> assume that struct twoints { int a; int b; } is exactly 64 bits
> everywhere?

Certainly not.  But if you're going to explicitly use int64_t elsewhere, you
might as well explicitly use int32_t as well.  Then I believe you can
reasonably assert

  alignof(twoint32) == alignof(int64_t)


r~
Hans-Peter Nilsson April 3, 2015, 3:04 a.m. UTC | #5 
On Wed, 25 Mar 2015, Jonathan Wakely wrote:
> 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:

Wrong.

> <jsm28> jwakely: _Atomic aligns 1/2/4/8/16-byte types the same as
>        integer types of that size.

No it doesn't!  It's "same or higher as".

> <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).
>

brgds, H-P
diff mbox

Patch

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" );
+