[Bug c/65146] New: alignment of _Atomic structure member is not correct

[Bug c/65146] New: alignment of _Atomic structure member is not correct

[alexey.lapshin at oracle dot com]

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

            Bug ID: 65146
           Summary: alignment of _Atomic structure member is not correct
           Product: gcc
           Version: 4.9.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: alexey.lapshin at oracle dot com

Alignment of single _Atomic object match with documentation :
https://gcc.gnu.org/wiki/Atomic/GCCMM/UnalignedPolicy . 
Alignment of _Atomic structure member does not match. 

~/atomic_test$ cat unaligned_atomic.c

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

typedef struct {
    char c [8];
} power_of_two_obj;

typedef struct {
   char c[1];
   _Atomic power_of_two_obj ao;
} container_struct; 

int main ( void ) {

    _Atomic power_of_two_obj obj1;
    container_struct         obj2; 


    printf("\n Size and Alignment of _Atomic  object "); 
    printf(" : sizeof(obj1) %d __alignof__(obj1) %d ", sizeof(obj1),
__alignof__(obj1) );

    printf("\n Size and Alignment of _Atomic member object "); 
    printf(" : sizeof(obj2.ao) %d __alignof__(obj2.ao) %d \n", sizeof(obj2.ao),
__alignof__(obj2.ao) );

    return 0;
}

~/atomic_test$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/opt/gcc/libexec/gcc/i386-pc-solaris2.11/4.9.2/lto-wrapper
Target: i386-pc-solaris2.11
Configured with: ./configure --prefix=/opt/gcc/
Thread model: posix
gcc version 4.9.2 (GCC) 

~/atomic_test$ gcc -O -latomic -std=c11 unaligned_atomic.c -m32

~/atomic_test$ ./a.out

 Size and Alignment of _Atomic  object  : sizeof(obj1) 8 __alignof__(obj1) 8 
 Size and Alignment of _Atomic member object  : sizeof(obj2.ao) 8
__alignof__(obj2.ao) 4 


According to the documentation
https://gcc.gnu.org/wiki/Atomic/GCCMM/UnalignedPolicy alignment of both "obj1"
and "obj2.ao" should be 8 bytes.
But in the above test case alignment of obj2.ao is 4 bytes.

The bug is found on Solaris x86 -m32

[Bug c/65146] alignment of _Atomic structure member is not correct

[joseph at codesourcery dot com]

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

--- Comment #1 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
On Fri, 20 Feb 2015, alexey.lapshin at oracle dot com wrote:

> Alignment of single _Atomic object match with documentation :
> https://gcc.gnu.org/wiki/Atomic/GCCMM/UnalignedPolicy . 

That's not documentation.  Implementation differences from early plans are 
not a bug.  Differences in alignment are only bugs if they mean atomic 
operations *fail to be atomic*.

[Bug c/65146] alignment of _Atomic structure member is not correct

[alexey.lapshin at oracle dot com]

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

--- Comment #2 from Alexey Lapshin <alexey.lapshin at oracle dot com> ---
if alignment of atomic object less then it`s size then it could not be
lock-free on x86 32. If that object would split across cache lines then the
operation would not be atomic.

At the same time compiler reports for this 8-bytes size object:

__atomic_always_lock_free(sizeof(obj2.ao), &obj2.ao) TRUE 
atomic_is_lock_free( &obj2.ao ) TRUE

so it looks like lock-free atomic operation on this 4-bytes aligned data object
could fail to be atomic.

[Bug c/65146] alignment of _Atomic structure member is not correct

[joseph at codesourcery dot com]

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

--- Comment #3 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
It's quite possible there are x86 struct bugs, cf. 
<https://gcc.gnu.org/ml/gcc-patches/2013-11/msg02840.html> and 
<https://gcc.gnu.org/ml/gcc-patches/2013-11/msg02862.html>.

[Bug target/65146] alignment of _Atomic structure member is not correct

[jason at gcc dot gnu.org]

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

--- Comment #20 from Jason Merrill <jason at gcc dot gnu.org> ---
This issue came up in the GCC/LLVM compatibility discussion today.

[Bug target/65146] alignment of _Atomic structure member is not correct

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |uros at gcc dot gnu.org

--- Comment #21 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, are we talking about something like (no -Wpsabi here)
--- gcc/config/i386/i386.c.jj   2020-08-24 10:00:01.321258451 +0200
+++ gcc/config/i386/i386.c      2020-08-26 17:54:39.089468108 +0200
@@ -16487,7 +16487,11 @@ iamcu_alignment (tree type, int align)

   /* Intel MCU psABI specifies scalar types > 4 bytes aligned to 4
      bytes.  */
-  mode = TYPE_MODE (strip_array_types (type));
+  type = strip_array_types (type);
+  if (TYPE_ATOMIC (type))
+    return align;
+
+  mode = TYPE_MODE (type);
   switch (GET_MODE_CLASS (mode))
     {
     case MODE_INT:
@@ -16757,7 +16761,7 @@ ix86_minimum_alignment (tree exp, machin
   /* Don't do dynamic stack realignment for long long objects with
      -mpreferred-stack-boundary=2.  */
   if ((mode == DImode || (type && TYPE_MODE (type) == DImode))
-      && (!type || !TYPE_USER_ALIGN (type))
+      && (!type || (!TYPE_USER_ALIGN (type) && !TYPE_ATOMIC (type)))
       && (!decl || !DECL_USER_ALIGN (decl)))
     {
       gcc_checking_assert (!TARGET_STV);
@@ -20293,7 +20297,10 @@ x86_field_alignment (tree type, int comp
     return computed;
   if (TARGET_IAMCU)
     return iamcu_alignment (type, computed);
-  mode = TYPE_MODE (strip_array_types (type));
+  type = strip_array_types (type);
+  if (TYPE_ATOMIC (type))
+    return computed;
+  mode = TYPE_MODE (type);
   if (mode == DFmode || mode == DCmode
       || GET_MODE_CLASS (mode) == MODE_INT
       || GET_MODE_CLASS (mode) == MODE_COMPLEX_INT)
No idea whether we need the ix86_minimum_alignment change and about IAMCU I'll
defer to H.J., but the last hunk fixes something like #c1 or
struct A { char a; _Atomic long long b; };
struct B { char a; _Atomic double b; };
int a = __builtin_offsetof (struct A, b);
int b = __builtin_offsetof (struct B, b);
with -m32 for me.

[Bug target/65146] alignment of _Atomic structure member is not correct

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #22 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 49131
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49131&action=edit
gcc11-pr65146.patch

More complete patch.

[Bug target/65146] alignment of _Atomic structure member is not correct

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |https://gitlab.com/x86-psAB
                   |                            |Is/i386-ABI/-/issues/1

--- Comment #23 from H.J. Lu <hjl.tools at gmail dot com> ---
I created:

https://gitlab.com/x86-psABIs/i386-ABI/-/issues/1

[Bug target/65146] alignment of _Atomic structure member is not correct

[cvs-commit at gcc dot gnu.org]

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

--- Comment #24 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:04df5e7de2f3dd652a9cddc1c9adfbdf45947ae6

commit r11-2909-g04df5e7de2f3dd652a9cddc1c9adfbdf45947ae6
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Aug 27 18:44:40 2020 +0200

    ia32: Fix alignment of _Atomic fields [PR65146]

    For _Atomic fields, lowering the alignment of long long or double etc.
    fields on ia32 is undesirable, because then one really can't perform atomic
    operations on those using cmpxchg8b.

    The following patch stops lowering the alignment in fields for _Atomic
    types (the x86_field_alignment change) and for -mpreferred-stack-boundary=2
    also ensures we don't misalign _Atomic long long etc. automatic variables
    (the ix86_{local,minimum}_alignment changes).
    Not sure about iamcu_alignment change, I know next to nothing about IA MCU,
    but unless it doesn't have cmpxchg8b instruction, it would surprise me if
we
    don't want to do it as well.
    clang apparently doesn't lower the field alignment for _Atomic.

    2020-08-27  Jakub Jelinek  <jakub@redhat.com>

            PR target/65146
            * config/i386/i386.c (iamcu_alignment): Don't decrease alignment
            for TYPE_ATOMIC types.
            (ix86_local_alignment): Likewise.
            (ix86_minimum_alignment): Likewise.
            (x86_field_alignment): Likewise, and emit a -Wpsabi diagnostic
            for it.

            * gcc.target/i386/pr65146.c: New test.

[Bug target/65146] alignment of _Atomic structure member is not correct

[peter at cordes dot ca]

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

--- Comment #25 from Peter Cordes <peter at cordes dot ca> ---
(In reply to CVS Commits from comment #24)
> The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:
> 
> https://gcc.gnu.org/g:04df5e7de2f3dd652a9cddc1c9adfbdf45947ae6
> 
> commit r11-2909-g04df5e7de2f3dd652a9cddc1c9adfbdf45947ae6
> Author: Jakub Jelinek <jakub@redhat.com>
> Date:   Thu Aug 27 18:44:40 2020 +0200
> 
>     ia32: Fix alignment of _Atomic fields [PR65146]
>     
>     For _Atomic fields, lowering the alignment of long long or double etc.
>     fields on ia32 is undesirable, because then one really can't perform
> atomic
>     operations on those using cmpxchg8b.


Just for the record, the description of this bugfix incorrectly mentioned
cmpxchg8b being a problem.  lock cmpxchg8b is *always* atomic, even if that
means the CPU has to take a bus lock (disastrously expensive affecting all
cores system-wide) instead of just delaying MESI response for one line
exclusively owned in this core's private cache (aka cache lock).

The correctness problem is __atomic_load_n / __atomic_store_n compiling to
actual 8-byte pure loads / pure stores using SSE2 movq, SSE1 movlps, or x87
fild/fistp (bouncing through the stack), such as

  movq  %xmm0, (%eax)

That's where correctness depends on Intel and AMD's atomicity guarantees which
are conditional on alignment.

(And if AVX is supported, same deal for 16-byte load/store.  Although we can
and should use movaps for that, which bakes alignment checking into the
instruction.  Intel did recently document that CPUs with AVX guarantee
atomicity of 16-byte aligned loads/stores, retroactive to all CPUs with AVX. 
It's about time, but yay.)

>     Not sure about iamcu_alignment change, I know next to nothing about IA
> MCU,
>     but unless it doesn't have cmpxchg8b instruction, it would surprise me
> if we
>     don't want to do it as well.


I had to google iamcu.  Apparently it's Pentium-like, but only has soft-FP (so
I assume no MMX or SSE as well as no x87).

If that leaves it no way to do 8-byte load/store except (lock) cmpxchg8b, that
may mean there's no need for alignment, unless cache-line-split lock is still a
performance issue.  If it's guaranteed unicore as well, we can even omit the
lock prefix and cmpxchg8b will still be an atomic RMW (or load or store) wrt.
interrupts.  (And being unicore would likely mean much less system-wide
overhead for a split lock.)

[Bug target/65146] alignment of _Atomic structure member is not correct

[egallager at gcc dot gnu.org]

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

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|https://gitlab.com/x86-psAB |
                   |Is/i386-ABI/-/issues/1      |
           See Also|                            |https://gitlab.com/x86-psAB
                   |                            |Is/i386-ABI/-/issues/1
                 CC|                            |egallager at gcc dot gnu.org

--- Comment #26 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to H.J. Lu from comment #23)
> I created:
> 
> https://gitlab.com/x86-psABIs/i386-ABI/-/issues/1

Let's move that to the "See Also" field, now that that's possible.