[Bug libstdc++/62259] New: atomic class doesn't enforce required alignment on powerpc64

[Bug libstdc++/62259] New: atomic class doesn't enforce required alignment on powerpc64

[saugustine at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259

            Bug ID: 62259
           Summary: atomic class doesn't enforce required alignment on
                    powerpc64
           Product: gcc
           Version: 4.9.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: saugustine at google dot com

Created attachment 33396
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33396&action=edit
small reproducer

The attached short program results in a bus error on powerpc64 top of trunk at
-O0, but I believe is a bug that would be exposed on many targets, going back
at least to 4.9. 

It succeeds--probably by luck--on 4.8.

The key is that the atomic struct is eight-bytes in size, but only four byte
aligned, and gcc takes no care to subsequently align it. GCC chooses an ldarx
instruction, which requires eight-byte alignment.

Although https://gcc.gnu.org/wiki/Atomic/GCCMM/UnalignedPolicy doesn't directly
address this case, a careful reading leads me to believe that this is intended
to work.

My uneducated guess is that the template at <atomic>:189 should either use
&_M_i in calls to __atomic_is_lock_free (instead of nullptr) or should add
alignment as necessary. Not sure how that is intended to be done. If I fix
<atomic> to pass the pointer, then gcc chooses to call out to an atomic library
function, which gcc doesn't provide.

google ref b/17136920

g++ -std=c++11 t.cc -O0

#include <atomic>

struct twoints {
int a;
int b;
};

int main() {
  // unalign subsequent variables
  char b0 = 'a';
  twoints one = { 1 };
  twoints two = { 2 };
  std::atomic<twoints> a(one);
  twoints e = { 0 };

  a.compare_exchange_strong(e, two, std::memory_order_acq_rel);
  return 0;
}

[Bug libstdc++/62259] atomic class doesn't enforce required alignment on powerpc64

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259

Ulrich Weigand <uweigand at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |uweigand at gcc dot gnu.org

--- Comment #1 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Indeed, when running a simple test program:

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

struct twoints {
  int a;
  int b;
};

int main(void) {
   printf("%d\n", __alignof__ (twoints));
   printf("%d\n", __alignof__ (std::atomic<twoints>));
   return 0;
}

we see that the GCC only requires 4 bytes of alignment for the atomic type.

However, with the equivalent C11 code using the _Atomic keyword

#include <stdatomic.h>
#include <stdio.h>

struct twoints {
  int a;
  int b;
};

int main() {
   printf("%d\n", __alignof__ (struct twoints));
   printf("%d\n", __alignof__ (_Atomic (struct twoints)));
   return 0;
}

we get an alignment requirement of 8 bytes for the atomic type.

In the C case, this is done by the compiler front-end where it implements the
_Atomic keyword.  In the C++ case, it seems the compiler doesn't really get
involved, as it's all done in plain C++ in standard library code ...

I suspect the intent was that for C++, we likewise ought to have an increased
alignment requirement for the type, but I'm not sure how to implement this in
the library.  Need some of the library experts to comment here.

[Bug libstdc++/62259] atomic class doesn't enforce required alignment on powerpc64

[dje at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259

David Edelsohn <dje at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-09-03
                 CC|                            |dje at gcc dot gnu.org,
                   |                            |redi at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #2 from David Edelsohn <dje at gcc dot gnu.org> ---
Confirmed.

[Bug libstdc++/62259] atomic class doesn't enforce required alignment on powerpc64

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to saugustine from comment #0)
> My uneducated guess is that the template at <atomic>:189 should either use
> &_M_i in calls to __atomic_is_lock_free (instead of nullptr) or should add
> alignment as necessary. Not sure how that is intended to be done. If I fix
> <atomic> to pass the pointer, then gcc chooses to call out to an atomic
> library function, which gcc doesn't provide.

GCC does provide it, in libatomic, so -latomic should work.

But I just tried your suggested change and saw no effect: I didn't need
libatomic and I still got a bus error.

I suppose what we want is the equivalent of this, but the _Atomic keyword isn't
valid in C++:

--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -161,7 +161,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     struct atomic
     {
     private:
-      _Tp _M_i;
+      alignas(alignof(_Atomic _Tp)) _Tp _M_i;

       // TODO: static_assert(is_trivially_copyable<_Tp>::value, "");

[Bug libstdc++/62259] atomic class doesn't enforce required alignment on powerpc64

[amacleod at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259

Andrew Macleod <amacleod at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amacleod at redhat dot com

--- Comment #4 from Andrew Macleod <amacleod at redhat dot com> ---
Yeah... up until now, CRIS was the only port that this was an issue for.

The original C11 work had an extension  __attribute__(atomic)  which would do
the same thing the _Atomic keyword does for non C11 compilation, and the type
in the libstdc++ atomic classes would be given this attribute.

When jsm took over the C11 integration, this attribute code added extra testing
and code paths that were beyond the scope of what he was doing with C11, so it
was left behind.

my original mothballing note was
https://gcc.gnu.org/ml/gcc/2013-09/msg00240.html

we could probably track down the parts he didn't integrate from the branch if
someone wanted to work with them and get them up to snuff.

[Bug libstdc++/62259] atomic class doesn't enforce required alignment on powerpc64

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |alexey.lapshin at oracle dot com

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
*** Bug 65149 has been marked as a duplicate of this bug. ***

[Bug libstdc++/62259] atomic class doesn't enforce required alignment on powerpc64

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Author: redi
Date: Thu Mar 26 19:27:02 2015
New Revision: 221703

URL: https://gcc.gnu.org/viewcvs?rev=221703&root=gcc&view=rev
Log:
    PR libstdc++/62259
    PR libstdc++/65147
    * include/std/atomic (atomic<T>): Increase alignment for types with
    the same size as one of the integral types.
    * testsuite/29_atomics/atomic/60695.cc: Adjust dg-error line number.
    * testsuite/29_atomics/atomic/62259.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc
      - copied, changed from r221701,
trunk/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/atomic
    trunk/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc

[Bug libstdc++/62259] atomic class doesn't enforce required alignment on powerpc64

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |5.0

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for gcc5.

[Bug libstdc++/62259] atomic class doesn't enforce required alignment on powerpc64

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259

--- Comment #8 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Andrew Macleod from comment #4)
> Yeah... up until now, CRIS was the only port that this was an issue for.

And JFTR, the resolution to this PR doesn't solve the similar issue there. (To
wit, increasing alignment up to *that of an integer of* the natural size which
is not the same as "increasing alignment up to the natural size" which would
work.)

> The original C11 work had an extension  __attribute__(atomic)  which would
> do the same thing the _Atomic keyword does for non C11 compilation, and the
> type in the libstdc++ atomic classes would be given this attribute.

This would work.