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 :-)
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);
