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

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

[Richard Henderson]

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

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.

I believe that for all of our current systems, size_t == uintptr_t, so the
reinterpret_cast ought not generate warnings.

The test case is problematic, as there's currently no good place to put it.
The libstdc++ testsuite doesn't have the libatomic library path configured, and
the libatomic testsuite doesn't have the libstdc++ include paths configured.
Yet another example where we really need an install tree for testing.  Thoughts?


Ok?


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 2846 bytes --]

	* include/bits/atomic_base.h (__atomic_base<T>::is_lock_free): Build
	a fake pointer indicating type alignment.
	(__atomic_base<T *>::is_lock_free): Likewise.
	* include/std/atomic (atomic<T>::is_lock_free): Likewise.

diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index fe6524f..8104c98 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -346,11 +346,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       bool
       is_lock_free() const noexcept
-      { return __atomic_is_lock_free(sizeof(_M_i), nullptr); }
+      {
+	// Produce a fake, minimally aligned pointer.
+	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
+	return __atomic_is_lock_free(sizeof(_M_i), __a);
+      }
 
       bool
       is_lock_free() const volatile noexcept
-      { return __atomic_is_lock_free(sizeof(_M_i), nullptr); }
+      {
+	// Produce a fake, minimally aligned pointer.
+	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
+	return __atomic_is_lock_free(sizeof(_M_i), __a);
+      }
 
       _GLIBCXX_ALWAYS_INLINE void
       store(__int_type __i, memory_order __m = memory_order_seq_cst) noexcept
@@ -653,11 +661,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       bool
       is_lock_free() const noexcept
-      { return __atomic_is_lock_free(sizeof(__pointer_type), nullptr); }
+      {
+	// Produce a fake, minimally aligned pointer.
+	void *__a = reinterpret_cast<void *>(-__alignof(_M_p));
+	return __atomic_is_lock_free(sizeof(_M_p), __a);
+      }
 
       bool
       is_lock_free() const volatile noexcept
-      { return __atomic_is_lock_free(sizeof(__pointer_type), nullptr); }
+      {
+	// Produce a fake, minimally aligned pointer.
+	void *__a = reinterpret_cast<void *>(-__alignof(_M_p));
+	return __atomic_is_lock_free(sizeof(_M_p), __a);
+      }
 
       _GLIBCXX_ALWAYS_INLINE void
       store(__pointer_type __p,
diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
index 1a17427..cc4b5f1 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -198,11 +198,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       bool
       is_lock_free() const noexcept
-      { return __atomic_is_lock_free(sizeof(_M_i), nullptr); }
+      {
+	// Produce a fake, minimally aligned pointer.
+	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
+	return __atomic_is_lock_free(sizeof(_M_i), __a);
+      }
 
       bool
       is_lock_free() const volatile noexcept
-      { return __atomic_is_lock_free(sizeof(_M_i), nullptr); }
+      {
+	// Produce a fake, minimally aligned pointer.
+	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
+	return __atomic_is_lock_free(sizeof(_M_i), __a);
+      }
 
       void
       store(_Tp __i, memory_order __m = memory_order_seq_cst) noexcept

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 65033.cc --]
[-- Type: text/x-c++src; name="65033.cc", Size: 1106 bytes --]

// 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-options "-std=gnu++11" }

#include <atomic>
#include <testsuite_hooks.h>

struct S2 { char a[2]; };
struct S3 { char a[3]; };

int
main()
{
  std::atomic<S2> s2;
  std::atomic<S3> s3;

  VERIFY( s2.is_lock_free() == false );
  VERIFY( s3.is_lock_free() == false );
}

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

[Jonathan Wakely]

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?

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

[Jonathan Wakely]

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

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

[Richard Henderson]

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~

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

[Jonathan Wakely]

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

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

[Richard Henderson]

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~

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

[Jonathan Wakely]

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

On 25/03/15 12:04 -0700, Richard Henderson wrote:
>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)

Yes, that makes sense, thanks.  Here's what I plan to commit then.

This includes your fix to avoid decreasing alignment, but I didn't add
a test for that as I couldn't make it fail on any of the targets I
test on.

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

commit f796769ad20c0353490b9f1a7e019e2f0c1771fb
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 1a17427..42244bd 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -165,7 +165,23 @@ _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_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);
+
+      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..6f618a0 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 *-*-* } 189 }
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..cf5423a
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
@@ -0,0 +1,58 @@
+// 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>
+
+using std::int32_t;
+using std::int64_t;
+
+// libstdc++/62259
+
+struct twoints {
+  int32_t a;
+  int32_t b;
+};
+
+static_assert( alignof(std::atomic<twoints>) == alignof(int64_t),
+               "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(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(int64_t),
+               "std::atomic not suitably aligned" );

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

[Jonathan Wakely]

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

On 26/03/15 13:21 +0000, Jonathan Wakely wrote:
>This includes your fix to avoid decreasing alignment, but I didn't add
>a test for that as I couldn't make it fail on any of the targets I
>test on.

>commit f796769ad20c0353490b9f1a7e019e2f0c1771fb
>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.

My patch was not sufficient to fix 65147, because I didn't increase
the alignment of the std::atomic<integral type> specializations, and
std::atomic<16-byte type> is only aligned correctly if __int128 is
supported, which isn't true on x86 and other 32-bit targets.

This is the best I've come up with, does anyone have any better ideas
than the #else branch to hardcode alignment of 16-byte types to 16?


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

commit d0ccfb0523066c69f3d22d9cdd617a139c57f9e1
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Mar 30 14:28:01 2015 +0100

    	PR libstdc++/65147
    	* include/bits/atomic_base.h (__atomic_base): Align as underlying
    	type.
    	* include/std/atomic (atomic<T>): Hardcode alignment for 16-byte
    	types when __int128 is not available.
    	* testsuite/29_atomics/atomic/60695.cc: Adjust dg-error line number.
    	* testsuite/29_atomics/atomic/65147.cc: New.

diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index 8104c98..48931ac 100644
--- 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;
@@ -559,7 +559,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   /// Partial specialization for pointer types.
   template<typename _PTp>
-    struct __atomic_base<_PTp*>
+    struct alignas(_PTp*) __atomic_base<_PTp*>
     {
     private:
       typedef _PTp* 	__pointer_type;
diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
index 88c8b17..2b09477 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -175,6 +175,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	: sizeof(_Tp) == sizeof(long long) ? alignof(long long)
 #ifdef _GLIBCXX_USE_INT128
 	: sizeof(_Tp) == sizeof(__int128)  ? alignof(__int128)
+#else
+	: sizeof(_Tp) == 16  ? 16
 #endif
 	: 0;
 
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc b/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
index 6f618a0..f755be0 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 *-*-* } 189 }
+std::atomic<X> a;  // { dg-error "not supported" "" { target *-*-* } 191 }
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc b/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
new file mode 100644
index 0000000..bb92513
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
@@ -0,0 +1,42 @@
+// 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-options "-std=gnu++11" }
+// { dg-do compile }
+
+// PR libstdc++65147
+
+#include <atomic>
+
+static_assert( alignof(std::atomic<short>) == alignof(short),
+    "atomic short must be aligned like short" );
+
+static_assert( alignof(std::atomic<int>) == alignof(int),
+    "atomic int must be aligned like int" );
+
+static_assert( alignof(std::atomic<long>) == alignof(long),
+    "atomic long must be aligned like long" );
+
+static_assert( alignof(std::atomic<long long>) == alignof(long long),
+    "atomic long long must be aligned like long long" );
+
+struct S {
+  char s[16];
+};
+
+static_assert( alignof(std::atomic<S>) > 1,
+    "atomic 16-byte struct must not be aligned like char" );

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

[Richard Henderson]

On 03/31/2015 06:41 AM, Jonathan Wakely wrote:
> This is the best I've come up with, does anyone have any better ideas
> than the #else branch to hardcode alignment of 16-byte types to 16?

If there's no 16 byte type, are we convinced this matters?  I mean, there isn't
a 16-byte atomic instruction for 32-bit x86 (or any other 32-bit cpu of which I
am aware).  So we're forced to use a locking path anyway.


And if we do want the alignment, do we stop pretending with all the sizeof's
and alignof's and just use power-of-two size alignment for all of them, e.g.

  min_align = ((size & (size - 1)) || size > 16 ? 0 : size)


r~

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

[Jonathan Wakely]

On 31/03/15 07:54 -0700, Richard Henderson wrote:
>On 03/31/2015 06:41 AM, Jonathan Wakely wrote:
>> This is the best I've come up with, does anyone have any better ideas
>> than the #else branch to hardcode alignment of 16-byte types to 16?
>
>If there's no 16 byte type, are we convinced this matters?  I mean, there isn't
>a 16-byte atomic instruction for 32-bit x86 (or any other 32-bit cpu of which I
>am aware).  So we're forced to use a locking path anyway.

The C front end gives struct S { char s[16]; } 16 byte alignment, and
I'd like std::atomic<S> and _Atomic struct S to be layout compatible,
although it's not essential (or required by any standard).

And it matters most for the integral types, not arbitrary structs.

>And if we do want the alignment, do we stop pretending with all the sizeof's
>and alignof's and just use power-of-two size alignment for all of them, e.g.
>
>  min_align = ((size & (size - 1)) || size > 16 ? 0 : size)

Yeah, I wondered about that too. Joseph indicated there are targets
where C gives alignof(_Atomic X) != sizeof(X), which is why the target
hook exists, but maybe we can just not worry about those targets for
now.  For GCC 6 we can look into the new attribute Andrew did in the
atomics branch so that we can make std::atomic use the target hook
directly instead of trying to simulate its effects in C++ code.

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

[Richard Henderson]

On 03/31/2015 08:03 AM, Jonathan Wakely wrote:
> On 31/03/15 07:54 -0700, Richard Henderson wrote:
>> On 03/31/2015 06:41 AM, Jonathan Wakely wrote:
>>> This is the best I've come up with, does anyone have any better ideas
>>> than the #else branch to hardcode alignment of 16-byte types to 16?
>>
>> If there's no 16 byte type, are we convinced this matters?  I mean, there isn't
>> a 16-byte atomic instruction for 32-bit x86 (or any other 32-bit cpu of which I
>> am aware).  So we're forced to use a locking path anyway.
> 
> The C front end gives struct S { char s[16]; } 16 byte alignment...

Um, I'm pretty sure it doesn't.

	struct S { char s[16]; };
	int x = __alignof(struct S);

	.type	x, @object
	.size	x, 4
x:
	.long	1

What you're interpreting as alignment for that struct is probably optional
*additional* alignment from LOCAL_ALIGNMENT or LOCAL_DECL_ALIGNMENT or something.

> And it matters most for the integral types, not arbitrary structs.
> 
>> And if we do want the alignment, do we stop pretending with all the sizeof's
>> and alignof's and just use power-of-two size alignment for all of them, e.g.
>>
>>  min_align = ((size & (size - 1)) || size > 16 ? 0 : size)
> 
> Yeah, I wondered about that too. Joseph indicated there are targets
> where C gives alignof(_Atomic X) != sizeof(X), which is why the target
> hook exists, but maybe we can just not worry about those targets for
> now.

Those targets have alignof < sizeof.  So in a way we'd probably be doing them a
favor.  I know for instance that CRIS is in this category, where most data is
all byte aligned, but atomics must be naturally aligned.

And, as you note, just so long as we do the same thing for _Atomic once we
get that merged.


r~

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

[Jonathan Wakely]

On 31/03/15 08:13 -0700, Richard Henderson wrote:
>On 03/31/2015 08:03 AM, Jonathan Wakely wrote:
>> On 31/03/15 07:54 -0700, Richard Henderson wrote:
>>> On 03/31/2015 06:41 AM, Jonathan Wakely wrote:
>>>> This is the best I've come up with, does anyone have any better ideas
>>>> than the #else branch to hardcode alignment of 16-byte types to 16?
>>>
>>> If there's no 16 byte type, are we convinced this matters?  I mean, there isn't
>>> a 16-byte atomic instruction for 32-bit x86 (or any other 32-bit cpu of which I
>>> am aware).  So we're forced to use a locking path anyway.
>>
>> The C front end gives struct S { char s[16]; } 16 byte alignment...
>
>Um, I'm pretty sure it doesn't.
>
>	struct S { char s[16]; };
>	int x = __alignof(struct S);
>
>	.type	x, @object
>	.size	x, 4
>x:
>	.long	1
>
>What you're interpreting as alignment for that struct is probably optional
>*additional* alignment from LOCAL_ALIGNMENT or LOCAL_DECL_ALIGNMENT or something.

Sorry for not being clear, I meant __alignof(_Atomic struct S) is 16.

>> And it matters most for the integral types, not arbitrary structs.
>>
>>> And if we do want the alignment, do we stop pretending with all the sizeof's
>>> and alignof's and just use power-of-two size alignment for all of them, e.g.
>>>
>>>  min_align = ((size & (size - 1)) || size > 16 ? 0 : size)
>>
>> Yeah, I wondered about that too. Joseph indicated there are targets
>> where C gives alignof(_Atomic X) != sizeof(X), which is why the target
>> hook exists, but maybe we can just not worry about those targets for
>> now.
>
>Those targets have alignof < sizeof.  So in a way we'd probably be doing them a
>favor.  I know for instance that CRIS is in this category, where most data is
>all byte aligned, but atomics must be naturally aligned.

Aha, I wondered why CRIS overrides the atomic_align_for_mode hook when
it seemed to be giving them natural alignment anyway - I didn't
realise non-atomic types are only byte-aligned.

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

[Hans-Peter Nilsson]

On Thu, 26 Mar 2015, Jonathan Wakely wrote:

--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
+static_assert(alignof(obj1) == alignof(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(int64_t),
+               "std::atomic not suitably aligned" );

I'd suggest replacing == with >= or compare with
alignof(atomic_int64_t) (figuratively).

brgds, H-P

patch fix issue 1 with "[libstdc++/65033] Give alignment info to libatomic"

[Hans-Peter Nilsson]

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

Ever since aligmnent was made sane-ish, 62259.cc has failed for
reasons obvious in the patch.  Can I please commit this?

	* testsuite/29_atomics/atomic/62259.cc: Assert atomic
	alignment is larger-equal, not equal, to default alignment.

Index: libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
===================================================================
--- libstdc++-v3/testsuite/29_atomics/atomic/62259.cc	(revision 222036)
+++ libstdc++-v3/testsuite/29_atomics/atomic/62259.cc	(working copy)
@@ -33,7 +33,7 @@ struct twoints {
   int32_t b;
 };
 
-static_assert( alignof(std::atomic<twoints>) == alignof(int64_t),
+static_assert( alignof(std::atomic<twoints>) >= alignof(int64_t),
                "std::atomic not suitably aligned" );
 
 // libstdc++/65147
@@ -44,7 +44,7 @@ struct power_of_two_obj {
 
 std::atomic<power_of_two_obj> obj1;
 
-static_assert( alignof(obj1) == alignof(int64_t),
+static_assert( alignof(obj1) >= alignof(int64_t),
                "std::atomic not suitably aligned" );
 
 struct container_struct {
@@ -54,5 +54,5 @@ struct container_struct {
 
 container_struct obj2;
 
-static_assert( alignof(obj2.ao) == alignof(int64_t),
+static_assert( alignof(obj2.ao) >= alignof(int64_t),
                "std::atomic not suitably aligned" );

brgds, H-P

Re: patch fix issue 1 with "[libstdc++/65033] Give alignment info to libatomic"

[Jonathan Wakely]

On 13/04/15 06:45 +0200, Hans-Peter Nilsson wrote:
>Ever since aligmnent was made sane-ish, 62259.cc has failed for
>reasons obvious in the patch.  Can I please commit this?

Yes, OK for trunk and the gcc-5-branch, thanks.

Issue 2 with "[libstdc++/65033] Give alignment info to libatomic"

[Hans-Peter Nilsson]

(check_cxx_fundamental_alignment_constraints is Dodji's, others
CC:ed were already in the thread)

Looking into those atomic things and running tests for cris-elf,
I get FAIL for
libstdc++-v3/testsuite/29_atomics/atomic/65147.cc, specifically

struct S16 {
   char c[16];
};

static_assert( alignof(std::atomic<S16>) >= 16,
    "atomic<S16> must be aligned to at least its size" );

which just isn't aligned for cris-elf.  Its aligmnent is 1; the
default.  Trying to investigate using:

#include <iostream>
using std::cout;
using std::endl;
struct xx {
  alignas (16) char x[16];
};

xx ai;

int main(void)
{
  cout << "alignof(ai): " << __alignof__(ai)
       << endl;
}

yields:
b.cc:5:25: warning: requested alignment 16 is larger than 8 [-Wattributes]
   alignas (16) char x[16];

which is mysterious (where does the 8 come from?), until I grep
the error string and find
c-family/c-common.c:check_cxx_fundamental_alignment_constraints.

In there, I see target macros used, among them
BIGGEST_ALIGNMENT.  This is 8 for cris-elf: the *bit alignment*
(there's a bug there already; bits not bytes) of the biggest
*required* alignment (modulo atomics) for types, not the biggest
*supported* alignment.  So, the wrong macro (and unit) is used.
Similarly, BIGGEST_FIELD_ALIGNMENT is about *require*, not
*support*.  Changing either macro is also an ABI change.

Why not allow the presumably most relaxed value for types, like
for __attribute__ ((__aligned__())), i.e. MAX_OFILE_ALIGNMENT,
then a tighter alignment check when declaring an object?

Right now, MAX_OFILE_ALIGNMENT is only used in
check_cxx_fundamental_alignment_constraints when the scope is
*known* to be file-scope and I know MAX_OFILE_ALIGNMENT sounds
like it's only for file-scope variables, but well, that's what
we have here, so the error is wrong.

So, into what shall BIGGEST_ALIGNMENT change in
check_cxx_fundamental_alignment_constraints?

brgds, H-P

Re: Issue 2 with "[libstdc++/65033] Give alignment info to libatomic"

[Joseph Myers]

On Mon, 13 Apr 2015, Hans-Peter Nilsson wrote:

> b.cc:5:25: warning: requested alignment 16 is larger than 8 [-Wattributes]
>    alignas (16) char x[16];
> 
> which is mysterious (where does the 8 come from?), until I grep
> the error string and find
> c-family/c-common.c:check_cxx_fundamental_alignment_constraints.
> 
> In there, I see target macros used, among them
> BIGGEST_ALIGNMENT.  This is 8 for cris-elf: the *bit alignment*
> (there's a bug there already; bits not bytes) of the biggest
> *required* alignment (modulo atomics) for types, not the biggest
> *supported* alignment.  So, the wrong macro (and unit) is used.
> Similarly, BIGGEST_FIELD_ALIGNMENT is about *require*, not
> *support*.  Changing either macro is also an ABI change.
> 
> Why not allow the presumably most relaxed value for types, like
> for __attribute__ ((__aligned__())), i.e. MAX_OFILE_ALIGNMENT,
> then a tighter alignment check when declaring an object?

When I was making sure C diagnosed unsupported extended alignments, my 
conclusion was that the support for realigning the stack means that 
MAX_OFILE_ALIGNMENT is supported everywhere (except for malloc, of 
course).  See <https://gcc.gnu.org/ml/gcc-patches/2013-11/msg02187.html> 
for my analysis.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Richard Henderson]

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~

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

[Hans-Peter Nilsson]

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

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

[Jonathan Wakely]

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.
>
>I believe that for all of our current systems, size_t == uintptr_t, so the
>reinterpret_cast ought not generate warnings.
>
>The test case is problematic, as there's currently no good place to put it.
>The libstdc++ testsuite doesn't have the libatomic library path configured, and
>the libatomic testsuite doesn't have the libstdc++ include paths configured.
>Yet another example where we really need an install tree for testing.  Thoughts?
>
>
>Ok?

OK for trunk.

>
>r~

>	* include/bits/atomic_base.h (__atomic_base<T>::is_lock_free): Build
>	a fake pointer indicating type alignment.
>	(__atomic_base<T *>::is_lock_free): Likewise.
>	* include/std/atomic (atomic<T>::is_lock_free): Likewise.

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

[Hans-Peter Nilsson]

On Thu, 12 Feb 2015, Richard Henderson wrote:
> When we fixed PR54005,

Hm, there's confusion.  When was this fixed?  (Not fixed
AFAICT.)  Maybe you mean PR54004, but there was no note there
either.  Or maybe there's a typo and you meant some other PR and
that PR54005 is supposedly fixed by this patch (committed as
r221701)

...but it doesn't seem right: you use a specific object when
deducing the alignment for the fake-pointer, so it's used anyway
and is_lock_free must not be object-specific (despite its name)
and only type-specific as mandated by the standard (see PR).

To wit, deduce from the known-alignment of _Tp, not
known-alignment of _M_i. Or is this what happens; they're the
same?  Why then use __alignof(_M_i) (the object-alignment)
instead of _S_alignment (the deduced alas insufficiently
increased type-alignment)?

> making sure that atomic_is_lock_free returns the same
> value for all objects of a given type,

(That would work but it doesn't seem to be the case.)

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

Regarding what's actually needed, alignment of an atomic type
should always be *forced to be at least the natural alignment of
of the object* (with non-power-of-two sized-objects rounded up)
and until then atomic types won't work for targets where the
non-atomic equivalents have less alignment (as straddling a
page-boundary won't be lock-less-atomic anywhere where
straddling a page-boundary may cause a non-atomic-access...) So,
not target-specific except for targets that require even
higher-than-natural alignment.

> The following patch constructs a fake object pointer that is maximally
> misaligned.

(With regards to the known object alignment of the _M_i object.)

>  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.
>
> I believe that for all of our current systems, size_t == uintptr_t, so the
> reinterpret_cast ought not generate warnings.
>
> The test case is problematic, as there's currently no good place to put it.
> The libstdc++ testsuite doesn't have the libatomic library path configured, and
> the libatomic testsuite doesn't have the libstdc++ include paths configured.
> Yet another example where we really need an install tree for testing.  Thoughts?

brgds, H-P

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

[Hans-Peter Nilsson]

On Thu, 2 Apr 2015, Hans-Peter Nilsson wrote:
> Why then use __alignof(_M_i) (the object-alignment)
> instead of _S_alignment (the deduced alas insufficiently
> increased type-alignment)?

(The immediate reason is that _S_alignment wasn't there until a
later revision, but the gist of the question remains. :-)

> > making sure that atomic_is_lock_free returns the same
> > value for all objects of a given type,
>
> (That would work but it doesn't seem to be the case.)
>
> > 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.
>
> Regarding what's actually needed, alignment of an atomic type
> should always be *forced to be at least the natural alignment of
> of the object* (with non-power-of-two sized-objects rounded up)
> and until then atomic types won't work for targets where the
> non-atomic equivalents have less alignment (as straddling a
> page-boundary won't be lock-less-atomic anywhere where
> straddling a page-boundary may cause a non-atomic-access...) So,
> not target-specific except for targets that require even
> higher-than-natural alignment.

So, can we do something like this instead for gcc5?
(Completely untested and may be syntactically, namespacingly and
cxxstandardversionly flawed.)

Index: include/std/atomic
===================================================================
--- include/std/atomic	(revision 221849)
+++ include/std/atomic	(working copy)
@@ -165,16 +165,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     struct atomic
     {
     private:
-      // Align 1/2/4/8/16-byte types the same as integer types of that size.
+      // Align 1/2/4/8/16-byte types to the natural alignment of that size.
       // This matches the alignment effects of the C11 _Atomic qualifier.
       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)
+      = sizeof(_Tp) == sizeof(char)	   ? max(sizeof(char), alignof(char))
+	: sizeof(_Tp) == sizeof(short)	   ? max(sizeof(short), alignof(short))
+	: sizeof(_Tp) == sizeof(int)	   ? max(sizeof(int), alignof(int))
+	: sizeof(_Tp) == sizeof(long)	   ? max(sizeof(long), alignof(long))
+	: sizeof(_Tp) == sizeof(long long) ? max(sizeof(long long), alignof(long long))
 #ifdef _GLIBCXX_USE_INT128
-	: sizeof(_Tp) == sizeof(__int128)  ? alignof(__int128)
+	: sizeof(_Tp) == sizeof(__int128)  ? max(sizeof(__int128), alignof(__int128))
 #endif
 	: 0;

@@ -216,7 +216,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       is_lock_free() const noexcept
       {
 	// Produce a fake, minimally aligned pointer.
-	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
+	void *__a = reinterpret_cast<void *>(-_S_alignment);
 	return __atomic_is_lock_free(sizeof(_M_i), __a);
       }

@@ -224,7 +224,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       is_lock_free() const volatile noexcept
       {
 	// Produce a fake, minimally aligned pointer.
-	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
+	void *__a = reinterpret_cast<void *>(-_S_alignment);
 	return __atomic_is_lock_free(sizeof(_M_i), __a);
       }

Index: include/bits/atomic_base.h
===================================================================
--- include/bits/atomic_base.h	(revision 221849)
+++ include/bits/atomic_base.h	(working copy)
@@ -240,7 +240,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     private:
       typedef _ITp 	__int_type;

-      __int_type 	_M_i;
+      // Align 1/2/4/8/16-byte types to the natural alignment of that size.
+      // This matches the alignment effects of the C11 _Atomic qualifier.
+      static constexpr int _S_min_alignment
+      = sizeof(_Tp) == sizeof(char)	   ? max(sizeof(char), __alignof(char))
+	: sizeof(_Tp) == sizeof(short)	   ? max(sizeof(short), __alignof(short))
+	: sizeof(_Tp) == sizeof(int)	   ? max(sizeof(int), __alignof(int))
+	: sizeof(_Tp) == sizeof(long)	   ? max(sizeof(long), __alignof(long))
+	: sizeof(_Tp) == sizeof(long long) ? max(sizeof(long long), __alignof(long long))
+#ifdef _GLIBCXX_USE_INT128
+	: sizeof(_Tp) == sizeof(__int128)  ? max(sizeof(__int128), __alignof(__int128))
+#endif
+	: 0;
+
+      static constexpr int _S_alignment
+        = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : __alignof(_Tp);
+
+      __int_type 	_M_i __attribute ((__aligned(_S_alignment)));

     public:
       __atomic_base() noexcept = default;
@@ -348,7 +364,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       is_lock_free() const noexcept
       {
 	// Produce a fake, minimally aligned pointer.
-	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
+	void *__a = reinterpret_cast<void *>(-_S_alignment);
 	return __atomic_is_lock_free(sizeof(_M_i), __a);
       }

@@ -356,7 +372,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       is_lock_free() const volatile noexcept
       {
 	// Produce a fake, minimally aligned pointer.
-	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
+	void *__a = reinterpret_cast<void *>(-_S_alignment);
 	return __atomic_is_lock_free(sizeof(_M_i), __a);
       }


brgds, H-P

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

[Jonathan Wakely]

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

On 03/04/15 05:24 -0400, Hans-Peter Nilsson wrote:
>On Thu, 2 Apr 2015, Hans-Peter Nilsson wrote:
>> Why then use __alignof(_M_i) (the object-alignment)
>> instead of _S_alignment (the deduced alas insufficiently
>> increased type-alignment)?

Isn't the object aligned to _S_alignment?

>
>(The immediate reason is that _S_alignment wasn't there until a
>later revision, but the gist of the question remains. :-)


>> > making sure that atomic_is_lock_free returns the same
>> > value for all objects of a given type,
>>
>> (That would work but it doesn't seem to be the case.)

Because we haven't done anything to increase the alignment for the
__atomic_base<_ITp> specialization yet, see the additional patch at:

https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01664.html

(Although that's insufficient as you point out, because it should
depend on the size too.)

>> > 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.
>>
>> Regarding what's actually needed, alignment of an atomic type
>> should always be *forced to be at least the natural alignment of
>> of the object* (with non-power-of-two sized-objects rounded up)
>> and until then atomic types won't work for targets where the
>> non-atomic equivalents have less alignment (as straddling a
>> page-boundary won't be lock-less-atomic anywhere where
>> straddling a page-boundary may cause a non-atomic-access...) So,
>> not target-specific except for targets that require even
>> higher-than-natural alignment.
>
>So, can we do something like this instead for gcc5?
>(Completely untested and may be syntactically, namespacingly and
>cxxstandardversionly flawed.)
>
>Index: include/std/atomic
>===================================================================
>--- include/std/atomic	(revision 221849)
>+++ include/std/atomic	(working copy)
>@@ -165,16 +165,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     struct atomic
>     {
>     private:
>-      // Align 1/2/4/8/16-byte types the same as integer types of that size.
>+      // Align 1/2/4/8/16-byte types to the natural alignment of that size.
>       // This matches the alignment effects of the C11 _Atomic qualifier.
>       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)
>+      = sizeof(_Tp) == sizeof(char)	   ? max(sizeof(char), alignof(char))
>+	: sizeof(_Tp) == sizeof(short)	   ? max(sizeof(short), alignof(short))
>+	: sizeof(_Tp) == sizeof(int)	   ? max(sizeof(int), alignof(int))
>+	: sizeof(_Tp) == sizeof(long)	   ? max(sizeof(long), alignof(long))
>+	: sizeof(_Tp) == sizeof(long long) ? max(sizeof(long long), alignof(long long))
> #ifdef _GLIBCXX_USE_INT128
>-	: sizeof(_Tp) == sizeof(__int128)  ? alignof(__int128)
>+	: sizeof(_Tp) == sizeof(__int128)  ? max(sizeof(__int128), alignof(__int128))
> #endif
> 	: 0;

Instead of changing every case in the condition to include sizeof why
not just do it afterwards using sizeof(_Tp), in the _S_alignment
calculation?

We know sizeof(_Tp) == sizeof(corresponding integer type) because
that's the whole point of the conditionals! See attachment.

>@@ -216,7 +216,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       is_lock_free() const noexcept
>       {
> 	// Produce a fake, minimally aligned pointer.
>-	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
>+	void *__a = reinterpret_cast<void *>(-_S_alignment);
> 	return __atomic_is_lock_free(sizeof(_M_i), __a);
>       }

If _M_i is aligned to _S_alignment then what difference does the
change above make?

It doesn't matter if the value is per-object if we've forced all such
objects to have the same alignment, does it?

Or is it different if a std::atomic<T> is included in some other
struct and the user forces a different alignment on it? I don't think
we really need to support that, users shouldn't be doing that.


>Index: include/bits/atomic_base.h
>===================================================================
>--- include/bits/atomic_base.h	(revision 221849)
>+++ include/bits/atomic_base.h	(working copy)
>@@ -240,7 +240,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     private:
>       typedef _ITp 	__int_type;
>
>-      __int_type 	_M_i;
>+      // Align 1/2/4/8/16-byte types to the natural alignment of that size.
>+      // This matches the alignment effects of the C11 _Atomic qualifier.
>+      static constexpr int _S_min_alignment
>+      = sizeof(_Tp) == sizeof(char)	   ? max(sizeof(char), __alignof(char))
>+	: sizeof(_Tp) == sizeof(short)	   ? max(sizeof(short), __alignof(short))
>+	: sizeof(_Tp) == sizeof(int)	   ? max(sizeof(int), __alignof(int))
>+	: sizeof(_Tp) == sizeof(long)	   ? max(sizeof(long), __alignof(long))
>+	: sizeof(_Tp) == sizeof(long long) ? max(sizeof(long long), __alignof(long long))
>+#ifdef _GLIBCXX_USE_INT128
>+	: sizeof(_Tp) == sizeof(__int128)  ? max(sizeof(__int128), __alignof(__int128))
>+#endif
>+	: 0;
>+
>+      static constexpr int _S_alignment
>+        = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : __alignof(_Tp);
>+
>+      __int_type 	_M_i __attribute ((__aligned(_S_alignment)));

This is massively overcomplicated. Unlike the generic template in
<atomic> that handles arbitrary types, here in <bits/atomic_base.h> we
know for a fact that _ITp is one of the standard integer types so we
don't need to do the silly dance to find a standard integer type of
the same size.

The attached patch against trunk should have the same result with much
less effort.

It doesn't include the changes to the reinterpret_cast<void *>
expressions to produce a minimally aligned pointer, but I think this
is progress, thanks :-)


[-- Attachment #2: atomic-align.txt --]
[-- Type: text/plain, Size: 1612 bytes --]

diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index 8104c98..79769cf 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -240,7 +240,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     private:
       typedef _ITp 	__int_type;
 
-      __int_type 	_M_i;
+      static constexpr int _S_alignment =
+	sizeof(_ITp) > alignof(_ITp) ? sizeof(_ITp) : alignof(_ITp);
+
+      alignas(_S_alignment) __int_type _M_i;
 
     public:
       __atomic_base() noexcept = default;
diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
index 88c8b17..81f29d8 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -165,9 +165,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     struct atomic
     {
     private:
-      // Align 1/2/4/8/16-byte types the same as integer types of that size.
+      // Align 1/2/4/8/16-byte types the natural alignment of that size.
       // This matches the alignment effects of the C11 _Atomic qualifier.
-      static constexpr int _S_min_alignment
+      static constexpr int _S_int_alignment
 	= sizeof(_Tp) == sizeof(char)	   ? alignof(char)
 	: sizeof(_Tp) == sizeof(short)	   ? alignof(short)
 	: sizeof(_Tp) == sizeof(int)	   ? alignof(int)
@@ -178,6 +178,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #endif
 	: 0;
 
+      static constexpr int _S_min_alignment
+	= _S_int_alignment > sizeof(_Tp) ? _S_int_alignment : sizeof(_Tp);
+
       static constexpr int _S_alignment
         = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp);
 

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

[Richard Henderson]

On 04/03/2015 07:13 AM, Jonathan Wakely wrote:
> +++ b/libstdc++-v3/include/std/atomic
> @@ -165,9 +165,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      struct atomic
>      {
>      private:
> -      // Align 1/2/4/8/16-byte types the same as integer types of that size.
> +      // Align 1/2/4/8/16-byte types the natural alignment of that size.
>        // This matches the alignment effects of the C11 _Atomic qualifier.
> -      static constexpr int _S_min_alignment
> +      static constexpr int _S_int_alignment
>  	= sizeof(_Tp) == sizeof(char)	   ? alignof(char)
>  	: sizeof(_Tp) == sizeof(short)	   ? alignof(short)
>  	: sizeof(_Tp) == sizeof(int)	   ? alignof(int)
> @@ -178,6 +178,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>  #endif
>  	: 0;
>  
> +      static constexpr int _S_min_alignment
> +	= _S_int_alignment > sizeof(_Tp) ? _S_int_alignment : sizeof(_Tp);
> +

This doesn't work for non-power-of-two sized _Tp.

Again, we don't have *any* target for which alignof(integer) > sizeof(integer).
So if you care about forcing natural alignment, don't bother with the alignof
on the integrals, as you're doing with _S_int_alignment at the moment.


r~

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

[Jonathan Wakely]

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

On 03/04/15 12:13 -0700, Richard Henderson wrote:
>On 04/03/2015 07:13 AM, Jonathan Wakely wrote:
>> +++ b/libstdc++-v3/include/std/atomic
>> @@ -165,9 +165,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>      struct atomic
>>      {
>>      private:
>> -      // Align 1/2/4/8/16-byte types the same as integer types of that size.
>> +      // Align 1/2/4/8/16-byte types the natural alignment of that size.
>>        // This matches the alignment effects of the C11 _Atomic qualifier.
>> -      static constexpr int _S_min_alignment
>> +      static constexpr int _S_int_alignment
>>  	= sizeof(_Tp) == sizeof(char)	   ? alignof(char)
>>  	: sizeof(_Tp) == sizeof(short)	   ? alignof(short)
>>  	: sizeof(_Tp) == sizeof(int)	   ? alignof(int)
>> @@ -178,6 +178,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>  #endif
>>  	: 0;
>>
>> +      static constexpr int _S_min_alignment
>> +	= _S_int_alignment > sizeof(_Tp) ? _S_int_alignment : sizeof(_Tp);
>> +
>
>This doesn't work for non-power-of-two sized _Tp.
>
>Again, we don't have *any* target for which alignof(integer) > sizeof(integer).
>So if you care about forcing natural alignment, don't bother with the alignof
>on the integrals, as you're doing with _S_int_alignment at the moment.

OK, the attached patch uses the simpler version you proposed, so
integral types and non-integral types with size 1/2/4/8/16 are aligned
to at least their size.

What about the __atomic_base<_PTp*> partial specialization for
pointers, do we need to use alignas on its data member, or are
pointers always aligned appropriately on all targets?

And what about these lines:

  void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
  return __atomic_is_lock_free(sizeof(_M_i), __a);

Do we still need that if we use alignas on the data members?
If we do, do you agree with HP that it should use _S_alignment not
__alignof(_M_i)? That seems redundant to me once _M_i has been given a
fixed alignment with alignas.


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

commit 18fe6cb4dd74e5e5e4586ec10adba997e2e28c81
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Apr 3 15:14:40 2015 +0100

    2015-04-07  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/65147.cc: New.
    	* testsuite/29_atomics/atomic_integral/65147.cc: New.

diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index 8104c98..79769cf 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -240,7 +240,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     private:
       typedef _ITp 	__int_type;
 
-      __int_type 	_M_i;
+      static constexpr int _S_alignment =
+	sizeof(_ITp) > alignof(_ITp) ? sizeof(_ITp) : alignof(_ITp);
+
+      alignas(_S_alignment) __int_type _M_i;
 
     public:
       __atomic_base() noexcept = default;
diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
index 88c8b17..125e37a 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -165,18 +165,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     struct atomic
     {
     private:
-      // 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.
+      // Align 1/2/4/8/16-byte types to at least their size.
       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;
+	= (sizeof(_Tp) & (sizeof(_Tp) - 1)) || sizeof(_Tp) > 16
+	? 0 : sizeof(_Tp);
 
       static constexpr int _S_alignment
         = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp);
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc b/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
new file mode 100644
index 0000000..e05ec17
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
@@ -0,0 +1,28 @@
+// 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-options "-std=gnu++11" }
+// { dg-do compile }
+
+#include <atomic>
+
+struct S16 {
+   char c[16];
+};
+
+static_assert( alignof(std::atomic<S16>) >= 16,
+    "atomic<S16> must be aligned to at least its size" );
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc b/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc
new file mode 100644
index 0000000..a5f5b74
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc
@@ -0,0 +1,32 @@
+// 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-options "-std=gnu++11" }
+// { dg-do compile }
+
+#include <atomic>
+
+static_assert( alignof(std::atomic<char>) >= sizeof(char),
+    "atomic<char> must be aligned to at least its size" );
+static_assert( alignof(std::atomic<short>) >= sizeof(short),
+    "atomic<short> must be aligned to at least its size" );
+static_assert( alignof(std::atomic<int>) >= sizeof(int),
+    "atomic<int> must be aligned to at least its size" );
+static_assert( alignof(std::atomic<long>) >= sizeof(long),
+    "atomic<long> must be aligned to at least its size" );
+static_assert( alignof(std::atomic<long long>) >= sizeof(long long),
+    "atomic<long long> must be aligned to at least its size" );

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

[Jonathan Wakely]

On 07/04/15 14:14 +0100, Jonathan Wakely wrote:
>On 03/04/15 12:13 -0700, Richard Henderson wrote:
>>On 04/03/2015 07:13 AM, Jonathan Wakely wrote:
>>>+++ b/libstdc++-v3/include/std/atomic
>>>@@ -165,9 +165,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>     struct atomic
>>>     {
>>>     private:
>>>-      // Align 1/2/4/8/16-byte types the same as integer types of that size.
>>>+      // Align 1/2/4/8/16-byte types the natural alignment of that size.
>>>       // This matches the alignment effects of the C11 _Atomic qualifier.
>>>-      static constexpr int _S_min_alignment
>>>+      static constexpr int _S_int_alignment
>>> 	= sizeof(_Tp) == sizeof(char)	   ? alignof(char)
>>> 	: sizeof(_Tp) == sizeof(short)	   ? alignof(short)
>>> 	: sizeof(_Tp) == sizeof(int)	   ? alignof(int)
>>>@@ -178,6 +178,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>> #endif
>>> 	: 0;
>>>
>>>+      static constexpr int _S_min_alignment
>>>+	= _S_int_alignment > sizeof(_Tp) ? _S_int_alignment : sizeof(_Tp);
>>>+
>>
>>This doesn't work for non-power-of-two sized _Tp.
>>
>>Again, we don't have *any* target for which alignof(integer) > sizeof(integer).
>>So if you care about forcing natural alignment, don't bother with the alignof
>>on the integrals, as you're doing with _S_int_alignment at the moment.
>
>OK, the attached patch uses the simpler version you proposed, so
>integral types and non-integral types with size 1/2/4/8/16 are aligned
>to at least their size.

Committed to trunk.

>
>What about the __atomic_base<_PTp*> partial specialization for
>pointers, do we need to use alignas on its data member, or are
>pointers always aligned appropriately on all targets?
>
>And what about these lines:
>
> void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
> return __atomic_is_lock_free(sizeof(_M_i), __a);
>
>Do we still need that if we use alignas on the data members?
>If we do, do you agree with HP that it should use _S_alignment not
>__alignof(_M_i)? That seems redundant to me once _M_i has been given a
>fixed alignment with alignas.
>

>commit 18fe6cb4dd74e5e5e4586ec10adba997e2e28c81
>Author: Jonathan Wakely <jwakely@redhat.com>
>Date:   Fri Apr 3 15:14:40 2015 +0100
>
>    2015-04-07  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/65147.cc: New.
>    	* testsuite/29_atomics/atomic_integral/65147.cc: New.
>
>diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
>index 8104c98..79769cf 100644
>--- a/libstdc++-v3/include/bits/atomic_base.h
>+++ b/libstdc++-v3/include/bits/atomic_base.h
>@@ -240,7 +240,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     private:
>       typedef _ITp 	__int_type;
> 
>-      __int_type 	_M_i;
>+      static constexpr int _S_alignment =
>+	sizeof(_ITp) > alignof(_ITp) ? sizeof(_ITp) : alignof(_ITp);
>+
>+      alignas(_S_alignment) __int_type _M_i;
> 
>     public:
>       __atomic_base() noexcept = default;
>diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
>index 88c8b17..125e37a 100644
>--- a/libstdc++-v3/include/std/atomic
>+++ b/libstdc++-v3/include/std/atomic
>@@ -165,18 +165,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     struct atomic
>     {
>     private:
>-      // 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.
>+      // Align 1/2/4/8/16-byte types to at least their size.
>       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;
>+	= (sizeof(_Tp) & (sizeof(_Tp) - 1)) || sizeof(_Tp) > 16
>+	? 0 : sizeof(_Tp);
> 
>       static constexpr int _S_alignment
>         = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp);
>diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc b/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
>new file mode 100644
>index 0000000..e05ec17
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
>@@ -0,0 +1,28 @@
>+// 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-options "-std=gnu++11" }
>+// { dg-do compile }
>+
>+#include <atomic>
>+
>+struct S16 {
>+   char c[16];
>+};
>+
>+static_assert( alignof(std::atomic<S16>) >= 16,
>+    "atomic<S16> must be aligned to at least its size" );
>diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc b/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc
>new file mode 100644
>index 0000000..a5f5b74
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc
>@@ -0,0 +1,32 @@
>+// 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-options "-std=gnu++11" }
>+// { dg-do compile }
>+
>+#include <atomic>
>+
>+static_assert( alignof(std::atomic<char>) >= sizeof(char),
>+    "atomic<char> must be aligned to at least its size" );
>+static_assert( alignof(std::atomic<short>) >= sizeof(short),
>+    "atomic<short> must be aligned to at least its size" );
>+static_assert( alignof(std::atomic<int>) >= sizeof(int),
>+    "atomic<int> must be aligned to at least its size" );
>+static_assert( alignof(std::atomic<long>) >= sizeof(long),
>+    "atomic<long> must be aligned to at least its size" );
>+static_assert( alignof(std::atomic<long long>) >= sizeof(long long),
>+    "atomic<long long> must be aligned to at least its size" );

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

[Hans-Peter Nilsson]

On Fri, 3 Apr 2015, Jonathan Wakely wrote:

> On 03/04/15 05:24 -0400, Hans-Peter Nilsson wrote:
> > On Thu, 2 Apr 2015, Hans-Peter Nilsson wrote:
> > > Why then use __alignof(_M_i) (the object-alignment)
> > > instead of _S_alignment (the deduced alas insufficiently
> > > increased type-alignment)?
>
> Isn't the object aligned to _S_alignment?

We did specify that with the alignas.  Is the alignof always
exactly the same as an alignas, if one is specified?  (And will
that not change in a future amendment, standard and/or
implementation?)  Either way, is there a test-case to guard all
this?

Those questions wouldn't even be asked if we used _S_alignment
for the fake-pointer too, just as a matter of defensive
programming.

> Instead of changing every case in the condition to include sizeof why
> not just do it afterwards using sizeof(_Tp), in the _S_alignment
> calculation?

Doh.

> We know sizeof(_Tp) == sizeof(corresponding integer type) because
> that's the whole point of the conditionals! See attachment.
>
> > @@ -216,7 +216,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >       is_lock_free() const noexcept
> >       {
> > 	// Produce a fake, minimally aligned pointer.
> > -	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
> > +	void *__a = reinterpret_cast<void *>(-_S_alignment);
> > 	return __atomic_is_lock_free(sizeof(_M_i), __a);
> >       }
>
> If _M_i is aligned to _S_alignment then what difference does the
> change above make?
>
> It doesn't matter if the value is per-object if we've forced all such
> objects to have the same alignment, does it?
>
> Or is it different if a std::atomic<T> is included in some other
> struct and the user forces a different alignment on it? I don't think
> we really need to support that, users shouldn't be doing that.

Why do we even need to ask those questions, when the patch takes
care of the per-type business without doubt?

> The attached patch against trunk should have the same result with much
> less effort.
>
> It doesn't include the changes to the reinterpret_cast<void *>
> expressions to produce a minimally aligned pointer, but I think this
> is progress, thanks :-)

Progress is good. :)

brgds, H-P

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

[Jonathan Wakely]

On 05/04/15 21:07 -0400, Hans-Peter Nilsson wrote:
>On Fri, 3 Apr 2015, Jonathan Wakely wrote:
>
>> On 03/04/15 05:24 -0400, Hans-Peter Nilsson wrote:
>> > On Thu, 2 Apr 2015, Hans-Peter Nilsson wrote:
>> > > Why then use __alignof(_M_i) (the object-alignment)
>> > > instead of _S_alignment (the deduced alas insufficiently
>> > > increased type-alignment)?
>>
>> Isn't the object aligned to _S_alignment?
>
>We did specify that with the alignas.  Is the alignof always
>exactly the same as an alignas, if one is specified?  (And will
>that not change in a future amendment, standard and/or
>implementation?)  Either way, is there a test-case to guard all
>this?

The language guarantees that's what alignas() does, if the argument is
a valid alignment (which it must be if we derive it from some other
type's alignment).

>Those questions wouldn't even be asked if we used _S_alignment
>for the fake-pointer too, just as a matter of defensive
>programming.
>
>> Instead of changing every case in the condition to include sizeof why
>> not just do it afterwards using sizeof(_Tp), in the _S_alignment
>> calculation?
>
>Doh.
>
>> We know sizeof(_Tp) == sizeof(corresponding integer type) because
>> that's the whole point of the conditionals! See attachment.
>>
>> > @@ -216,7 +216,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >       is_lock_free() const noexcept
>> >       {
>> > 	// Produce a fake, minimally aligned pointer.
>> > -	void *__a = reinterpret_cast<void *>(-__alignof(_M_i));
>> > +	void *__a = reinterpret_cast<void *>(-_S_alignment);
>> > 	return __atomic_is_lock_free(sizeof(_M_i), __a);
>> >       }
>>
>> If _M_i is aligned to _S_alignment then what difference does the
>> change above make?
>>
>> It doesn't matter if the value is per-object if we've forced all such
>> objects to have the same alignment, does it?
>>
>> Or is it different if a std::atomic<T> is included in some other
>> struct and the user forces a different alignment on it? I don't think
>> we really need to support that, users shouldn't be doing that.
>
>Why do we even need to ask those questions, when the patch takes
>care of the per-type business without doubt?

Well if we know the object is guaranteed to be correctly aligned we
might not even need a fake, minimally aligned pointer. We could go
back to passing &_M_i or just a null pointer to __atomic_is_lock_free.

The whole point of alignas() is to fix the alignment to a known value.

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

[Hans-Peter Nilsson]

On Tue, 7 Apr 2015, Jonathan Wakely wrote:
> On 05/04/15 21:07 -0400, Hans-Peter Nilsson wrote:
> > On Fri, 3 Apr 2015, Jonathan Wakely wrote:
> >
> > > On 03/04/15 05:24 -0400, Hans-Peter Nilsson wrote:
> > > > On Thu, 2 Apr 2015, Hans-Peter Nilsson wrote:
> > > > > Why then use __alignof(_M_i) (the object-alignment)
> > > > > instead of _S_alignment (the deduced alas insufficiently
> > > > > increased type-alignment)?
> > >
> > > Isn't the object aligned to _S_alignment?
> >
> > We did specify that with the alignas.  Is the alignof always
> > exactly the same as an alignas, if one is specified?  (And will
> > that not change in a future amendment, standard and/or
> > implementation?)  Either way, is there a test-case to guard all
> > this?
>
> The language guarantees that's what alignas() does, if the argument is
> a valid alignment (which it must be if we derive it from some other
> type's alignment).

I'm more worried about alignof reporting a higher value for a
specific object than alignas to be wrong.

Your question quoted just below seems to indicate a similar
worry.

> > > Or is it different if a std::atomic<T> is included in some other
> > > struct and the user forces a different alignment on it? I don't think
> > > we really need to support that, users shouldn't be doing that.
> >
> > Why do we even need to ask those questions, when the patch takes
> > care of the per-type business without doubt?
>
> Well if we know the object is guaranteed to be correctly aligned we
> might not even need a fake, minimally aligned pointer. We could go
> back to passing &_M_i or just a null pointer to __atomic_is_lock_free.

The target has a say in __atomic_is_lock_free and could impose
some crazy pointer-value-specific condition like "only in the
first half of a page" (anything can happen to work around chip
errata) so I suggest staying with an alignment-generated
pointer.  No, I only dreamt that up, shoot it down if we only
care for current targets.

> The whole point of alignas() is to fix the alignment to a known value.

And alignof and __alignof to report *exactly* that, I hope?

brgds, H-P

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

[Jonathan Wakely]

On 07/04/15 06:51 -0400, Hans-Peter Nilsson wrote:
>On Tue, 7 Apr 2015, Jonathan Wakely wrote:
>> On 05/04/15 21:07 -0400, Hans-Peter Nilsson wrote:
>> > On Fri, 3 Apr 2015, Jonathan Wakely wrote:
>> >
>> > > On 03/04/15 05:24 -0400, Hans-Peter Nilsson wrote:
>> > > > On Thu, 2 Apr 2015, Hans-Peter Nilsson wrote:
>> > > > > Why then use __alignof(_M_i) (the object-alignment)
>> > > > > instead of _S_alignment (the deduced alas insufficiently
>> > > > > increased type-alignment)?
>> > >
>> > > Isn't the object aligned to _S_alignment?
>> >
>> > We did specify that with the alignas.  Is the alignof always
>> > exactly the same as an alignas, if one is specified?  (And will
>> > that not change in a future amendment, standard and/or
>> > implementation?)  Either way, is there a test-case to guard all
>> > this?
>>
>> The language guarantees that's what alignas() does, if the argument is
>> a valid alignment (which it must be if we derive it from some other
>> type's alignment).
>
>I'm more worried about alignof reporting a higher value for a
>specific object than alignas to be wrong.

That shouldn't be possible because the C++ standard says it's an error
to use alignas with a less strict alignment than would be used if it
was omitted, i.e. an error to use alignas with a value less than the
result alignof would give.  However, G++ doesn't reject it (PR65685).

It still won't be possible here, because the alignas value we use is
not less than alignof(_Tp).

>Your question quoted just below seems to indicate a similar
>worry.

I was thinking about cases like this:

  struct __attribute__((packed)) Bad {
    char c;
    std::atomic<long long> a;
  };

But G++ ignores the packed attribute here, which is good (Clang
doesn't seem to ignore it, and mis-aligns the atomic).

>> > > Or is it different if a std::atomic<T> is included in some other
>> > > struct and the user forces a different alignment on it? I don't think
>> > > we really need to support that, users shouldn't be doing that.
>> >
>> > Why do we even need to ask those questions, when the patch takes
>> > care of the per-type business without doubt?
>>
>> Well if we know the object is guaranteed to be correctly aligned we
>> might not even need a fake, minimally aligned pointer. We could go
>> back to passing &_M_i or just a null pointer to __atomic_is_lock_free.
>
>The target has a say in __atomic_is_lock_free and could impose
>some crazy pointer-value-specific condition like "only in the
>first half of a page" (anything can happen to work around chip
>errata) so I suggest staying with an alignment-generated
>pointer.  No, I only dreamt that up, shoot it down if we only
>care for current targets.
>
>> The whole point of alignas() is to fix the alignment to a known value.
>
>And alignof and __alignof to report *exactly* that, I hope?

Yes, alignas sets the alignment requirement, alignof gets it.

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

[Hans-Peter Nilsson]

On Tue, 7 Apr 2015, Jonathan Wakely wrote:
> On 07/04/15 06:51 -0400, Hans-Peter Nilsson wrote:
> > On Tue, 7 Apr 2015, Jonathan Wakely wrote:
> > > On 05/04/15 21:07 -0400, Hans-Peter Nilsson wrote:
> > > > We did specify that with the alignas.  Is the alignof always
> > > > exactly the same as an alignas, if one is specified?  (And will
> > > > that not change in a future amendment, standard and/or
> > > > implementation?)  Either way, is there a test-case to guard all
> > > > this?
> > >
> > > The language guarantees that's what alignas() does, if the argument is
> > > a valid alignment (which it must be if we derive it from some other
> > > type's alignment).
> >
> > I'm more worried about alignof reporting a higher value for a
> > specific object than alignas to be wrong.
>
> That shouldn't be possible because the C++ standard says it's an error
> to use alignas with a less strict alignment than would be used if it
> was omitted, i.e. an error to use alignas with a value less than the
> result alignof would give.  However, G++ doesn't reject it (PR65685).
>
> It still won't be possible here, because the alignas value we use is
> not less than alignof(_Tp).

That's not what I meant.  My worry is there being a case where
alignof yields a *higher* value than the one that the alignas
specified.

> > Your question quoted just below seems to indicate a similar
> > worry.
>
> I was thinking about cases like this:
>
>  struct __attribute__((packed)) Bad {
>    char c;
>    std::atomic<long long> a;
>  };
>
> But G++ ignores the packed attribute here, which is good (Clang
> doesn't seem to ignore it, and mis-aligns the atomic).

I was more thinking of something like:

#include <atomic>
#include <iostream>
using std::cout;
using std::endl;

struct SoSo {
 double d;
 int x alignas(sizeof(int));
};

SoSo s __attribute__((__aligned__(16)));

int main(void)
{
  cout << "alignof(s): " << alignof(s) << endl;
  cout << "alignof(s.d): " << alignof(s.d) << endl;
  cout << "alignof(s.x): " << alignof(s.x) << endl;
}

in which I fear s.x would get an alignof the same as the s.d or
s, now or after a while, i.e. higher than specified.

(I get for cris-elf at revision 221891:
alignof(s): 16
alignof(s.d): 1
alignof(s.x): 4
which is kind-of-expected except I thought s.d would get the s
alignment so that just leaves it open whether that could
possibly change.)

brgds, H-P

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

[Jonathan Wakely]

On 07/04/15 10:51 -0400, Hans-Peter Nilsson wrote:
>I was more thinking of something like:
>
>#include <atomic>
>#include <iostream>
>using std::cout;
>using std::endl;
>
>struct SoSo {
> double d;
> int x alignas(sizeof(int));
>};
>
>SoSo s __attribute__((__aligned__(16)));
>
>int main(void)
>{
>  cout << "alignof(s): " << alignof(s) << endl;
>  cout << "alignof(s.d): " << alignof(s.d) << endl;
>  cout << "alignof(s.x): " << alignof(s.x) << endl;
>}
>
>in which I fear s.x would get an alignof the same as the s.d or
>s, now or after a while, i.e. higher than specified.
>
>(I get for cris-elf at revision 221891:
>alignof(s): 16
>alignof(s.d): 1
>alignof(s.x): 4
>which is kind-of-expected except I thought s.d would get the s
>alignment so that just leaves it open whether that could
>possibly change.)

The docs are clear that alignof(s.x) is not related to its position in
struct SoSo: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html

I'm not going to worry about that behaviour changing.

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

[Hans-Peter Nilsson]

On Tue, 7 Apr 2015, Jonathan Wakely wrote:
> The docs are clear that alignof(s.x) is not related to its position in
> struct SoSo: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
>
> I'm not going to worry about that behaviour changing.

'kthen thanks, quite clear; I should have checked that myself.

BTW; I hope nobody uses alignof the atomic types for e.g. manual
memory management, because any inner alignment of the atomic
payload isn't exposed; they get the default alignment:

#include <atomic>
#include <iostream>
using std::cout;
using std::endl;

std::atomic_int ai;
std::atomic_llong all;
std::atomic_intmax_t aim;

int main(void)
{
  cout << "alignof(ai): " << alignof(ai) << " .is_lock_free(): " << ai.is_lock_free() << endl;
}

alignof(ai): 1 .is_lock_free(): 1

I'd expect
alignof(ai): 4 .is_lock_free(): 1

No... wait, that's because atomic_base.h doesn't have the
natural-alignment fix, so it's still broken for
less-than-natural-alignment targets.  But will be fixed?

brgds, H-P

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

[Jonathan Wakely]

On 07/04/15 23:58 -0400, Hans-Peter Nilsson wrote:
>I'd expect
>alignof(ai): 4 .is_lock_free(): 1
>
>No... wait, that's because atomic_base.h doesn't have the
>natural-alignment fix, so it's still broken for
>less-than-natural-alignment targets.  But will be fixed?

Yes, with my uncommitted patch to add the alignas specifier to
__atomic_base<_ITp> I get your expected output.