gcc vs clang for non-power-2 atomic structures

gcc vs clang for non-power-2 atomic structures

[Jim Wilson]

We got a change request for the RISC-V psABI to define the atomic
structure size and alignment.  And looking at this, it turned out that
gcc and clang are implementing this differently.  Consider this
testcase

rohan:2274$ cat tmp.c
#include <stdio.h>
struct s { int a; int b; int c;};
int
main(void)
{
  printf("size=%ld align=%ld\n", sizeof (struct s), _Alignof(struct s));
  printf("size=%ld align=%ld\n", sizeof (_Atomic (struct s)),
_Alignof(_Atomic (struct s)));
  return 0;
}
rohan:2275$ gcc tmp.c
rohan:2276$ ./a.out
size=12 align=4
size=12 align=4
rohan:2277$ clang tmp.c
rohan:2278$ ./a.out
size=12 align=4
size=16 align=16
rohan:2279$

This is with an x86 compiler.  I get the same result with a RISC-V
compiler.  This is an ABI incompatibility between gcc and clang.  gcc
has code in build_qualified_type in tree.c that sets alignment for
power-of-2 structs to the same size integer alignment, but we don't
change alignment for non-power-of-2 structs.  Clang is padding the
size of non-power-of-2 structs to the next power-of-2 and giving them
that alignment.

Unfortunately, I don't know who to contact on the clang side, but we
need to have a discussion here, and we probably need to fix one of the
compilers to match the other one, as we should not have ABI
incompatibilities like this between gcc and clang.

The original RISC-V bug report is at
    https://github.com/riscv/riscv-elf-psabi-doc/pull/112
There is a pointer to a gist with a larger testcase with RISC-V results.

Jim

Re: gcc vs clang for non-power-2 atomic structures

[Iain Sandoe]

Hi Jim,

> On 23 Aug 2019, at 00:56, Jim Wilson <jimw@sifive.com> wrote:
> 
> We got a change request for the RISC-V psABI to define the atomic
> structure size and alignment.  And looking at this, it turned out that
> gcc and clang are implementing this differently.  Consider this
> testcase
> 
> rohan:2274$ cat tmp.c
> #include <stdio.h>
> struct s { int a; int b; int c;};
> int
> main(void)
> {
>  printf("size=%ld align=%ld\n", sizeof (struct s), _Alignof(struct s));
>  printf("size=%ld align=%ld\n", sizeof (_Atomic (struct s)),
> _Alignof(_Atomic (struct s)));
>  return 0;
> }
> rohan:2275$ gcc tmp.c
> rohan:2276$ ./a.out
> size=12 align=4
> size=12 align=4
> rohan:2277$ clang tmp.c
> rohan:2278$ ./a.out
> size=12 align=4
> size=16 align=16
> rohan:2279$
> 
> This is with an x86 compiler.  

A search for _Atomic in the latest (x86_64) psABI document shows no results.

> I get the same result with a RISC-V
> compiler.  This is an ABI incompatibility between gcc and clang.  gcc
> has code in build_qualified_type in tree.c that sets alignment for
> power-of-2 structs to the same size integer alignment, but we don't
> change alignment for non-power-of-2 structs.  Clang is padding the
> size of non-power-of-2 structs to the next power-of-2 and giving them
> that alignment.

If the psABI makes no statement about what _Atomic should do, AFAIK
the compiler is free to do something different (for the same entity) from 
the non-_Atomic version. 

e.g from 6.2.5 of n2176 (C18 draft)

	• 27  Further, there is the _Atomic qualifier. The presence of the _Atomic qualifier designates an atomic type. The size, representation, and alignment of an atomic type need not be the same as those of the corresponding unqualified type. Therefore, this Standard explicitly uses the phrase “atomic, qualified or unqualified type” whenever the atomic version of a type is permitted along with the other qualified versions of a type. The phrase “qualified or unqualified type”, without specific mention of atomic, does not include the atomic types.

So the hole (in both cases) to be plugged is to add specification for _Atomic
variants to the psABI…. 

… of course, it makes sense for the psABI maintainers to discuss with
the compiler “vendors” what makes the best choice.

(and it’s probably a significant piece of work to figure out all the interactions
with __attribute__ modifiers)

> Unfortunately, I don't know who to contact on the clang side, but we
> need to have a discussion here, and we probably need to fix one of the
> compilers to match the other one, as we should not have ABI
> incompatibilities like this between gcc and clang.

The equivalent of “MAINTAINERS” in the LLVM sources for backends is 
“llvm/CODE_OWNERS.TXT” which says that Alex Bradbury is the code
owner for the RISC-V backend.

HTH,
Iain

> The original RISC-V bug report is at
>    https://github.com/riscv/riscv-elf-psabi-doc/pull/112
> There is a pointer to a gist with a larger testcase with RISC-V results.
> 
> Jim

Re: gcc vs clang for non-power-2 atomic structures

[Jonathan Wakely]

On Fri, 23 Aug 2019 at 08:21, Iain Sandoe <idsandoe@googlemail.com> wrote:
>
> Hi Jim,
>
> > On 23 Aug 2019, at 00:56, Jim Wilson <jimw@sifive.com> wrote:
> >
> > We got a change request for the RISC-V psABI to define the atomic
> > structure size and alignment.  And looking at this, it turned out that
> > gcc and clang are implementing this differently.  Consider this
> > testcase
> >
> > rohan:2274$ cat tmp.c
> > #include <stdio.h>
> > struct s { int a; int b; int c;};
> > int
> > main(void)
> > {
> >  printf("size=%ld align=%ld\n", sizeof (struct s), _Alignof(struct s));
> >  printf("size=%ld align=%ld\n", sizeof (_Atomic (struct s)),
> > _Alignof(_Atomic (struct s)));
> >  return 0;
> > }
> > rohan:2275$ gcc tmp.c
> > rohan:2276$ ./a.out
> > size=12 align=4
> > size=12 align=4
> > rohan:2277$ clang tmp.c
> > rohan:2278$ ./a.out
> > size=12 align=4
> > size=16 align=16
> > rohan:2279$
> >
> > This is with an x86 compiler.
>
> A search for _Atomic in the latest (x86_64) psABI document shows no results.

See https://groups.google.com/forum/#!topic/ia32-abi/Tlu6Hs-ohPY and
the various GCC bugs linked to from that thread.

This is a big can of worms, and GCC needs fixing (but probably not
until the ABI group decide something).


> > I get the same result with a RISC-V
> > compiler.  This is an ABI incompatibility between gcc and clang.  gcc
> > has code in build_qualified_type in tree.c that sets alignment for
> > power-of-2 structs to the same size integer alignment, but we don't
> > change alignment for non-power-of-2 structs.  Clang is padding the
> > size of non-power-of-2 structs to the next power-of-2 and giving them
> > that alignment.
>
> If the psABI makes no statement about what _Atomic should do, AFAIK
> the compiler is free to do something different (for the same entity) from
> the non-_Atomic version.

Right, but GCC and Clang should agree. So it needs to be in the psABI.


> e.g from 6.2.5 of n2176 (C18 draft)
>
>         • 27  Further, there is the _Atomic qualifier. The presence of the _Atomic qualifier designates an atomic type. The size, representation, and alignment of an atomic type need not be the same as those of the corresponding unqualified type. Therefore, this Standard explicitly uses the phrase “atomic, qualified or unqualified type” whenever the atomic version of a type is permitted along with the other qualified versions of a type. The phrase “qualified or unqualified type”, without specific mention of atomic, does not include the atomic types.
>
> So the hole (in both cases) to be plugged is to add specification for _Atomic
> variants to the psABI….
>
> … of course, it makes sense for the psABI maintainers to discuss with
> the compiler “vendors” what makes the best choice.
>
> (and it’s probably a significant piece of work to figure out all the interactions
> with __attribute__ modifiers)
>
> > Unfortunately, I don't know who to contact on the clang side, but we
> > need to have a discussion here, and we probably need to fix one of the
> > compilers to match the other one, as we should not have ABI
> > incompatibilities like this between gcc and clang.
>
> The equivalent of “MAINTAINERS” in the LLVM sources for backends is
> “llvm/CODE_OWNERS.TXT” which says that Alex Bradbury is the code
> owner for the RISC-V backend.

Tim Northover and JF Bastien at Apple should probably be involved too.

IMO GCC is broken, and the longer we take to fix it the more painful
it will be for users as there will be more code affected by the
change.

Re: gcc vs clang for non-power-2 atomic structures

[Iain Sandoe]

> On 23 Aug 2019, at 10:35, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> 
> On Fri, 23 Aug 2019 at 08:21, Iain Sandoe <idsandoe@googlemail.com> wrote:
>> 
>> Hi Jim,
>> 
>>> On 23 Aug 2019, at 00:56, Jim Wilson <jimw@sifive.com> wrote:
>>> 
>>> We got a change request for the RISC-V psABI to define the atomic
>>> structure size and alignment.  And looking at this, it turned out that
>>> gcc and clang are implementing this differently.  Consider this
>>> testcase
>>> 
>>> rohan:2274$ cat tmp.c
>>> #include <stdio.h>
>>> struct s { int a; int b; int c;};
>>> int
>>> main(void)
>>> {
>>> printf("size=%ld align=%ld\n", sizeof (struct s), _Alignof(struct s));
>>> printf("size=%ld align=%ld\n", sizeof (_Atomic (struct s)),
>>> _Alignof(_Atomic (struct s)));
>>> return 0;
>>> }
>>> rohan:2275$ gcc tmp.c
>>> rohan:2276$ ./a.out
>>> size=12 align=4
>>> size=12 align=4
>>> rohan:2277$ clang tmp.c
>>> rohan:2278$ ./a.out
>>> size=12 align=4
>>> size=16 align=16
>>> rohan:2279$
>>> 
>>> This is with an x86 compiler.
>> 
>> A search for _Atomic in the latest (x86_64) psABI document shows no results.
> 
> See https://groups.google.com/forum/#!topic/ia32-abi/Tlu6Hs-ohPY and
> the various GCC bugs linked to from that thread.
> 
> This is a big can of worms, and GCC needs fixing (but probably not
> until the ABI group decide something).

I agree about the size of the can of worms.
However, there doesn’t seem to be any actual issue filed on:
https://github.com/hjl-tools/x86-psABI

Would it help if someone did?

>>> I get the same result with a RISC-V
>>> compiler.  This is an ABI incompatibility between gcc and clang.  gcc
>>> has code in build_qualified_type in tree.c that sets alignment for
>>> power-of-2 structs to the same size integer alignment, but we don't
>>> change alignment for non-power-of-2 structs.  Clang is padding the
>>> size of non-power-of-2 structs to the next power-of-2 and giving them
>>> that alignment.
>> 
>> If the psABI makes no statement about what _Atomic should do, AFAIK
>> the compiler is free to do something different (for the same entity) from
>> the non-_Atomic version.
> 
> Right, but GCC and Clang should agree. So it needs to be in the psABI.

absolutely, it’s the psABI that’s lacking here - the compilers (as commented
by Richard Smith in a referenced thread) should not be making ABI up…

>> e.g from 6.2.5 of n2176 (C18 draft)
>> 
>>        • 27  Further, there is the _Atomic qualifier. The presence of the _Atomic qualifier designates an atomic type. The size, representation, and alignment of an atomic type need not be the same as those of the corresponding unqualified type. Therefore, this Standard explicitly uses the phrase “atomic, qualified or unqualified type” whenever the atomic version of a type is permitted along with the other qualified versions of a type. The phrase “qualified or unqualified type”, without specific mention of atomic, does not include the atomic types.
>> 
>> So the hole (in both cases) to be plugged is to add specification for _Atomic
>> variants to the psABI….
>> 
>> … of course, it makes sense for the psABI maintainers to discuss with
>> the compiler “vendors” what makes the best choice.
>> 
>> (and it’s probably a significant piece of work to figure out all the interactions
>> with __attribute__ modifiers)
>> 
>>> Unfortunately, I don't know who to contact on the clang side, but we
>>> need to have a discussion here, and we probably need to fix one of the
>>> compilers to match the other one, as we should not have ABI
>>> incompatibilities like this between gcc and clang.
>> 
>> The equivalent of “MAINTAINERS” in the LLVM sources for backends is
>> “llvm/CODE_OWNERS.TXT” which says that Alex Bradbury is the code
>> owner for the RISC-V backend.
> 
> Tim Northover and JF Bastien at Apple should probably be involved too.
> 
> IMO GCC is broken, and the longer we take to fix it the more painful
> it will be for users as there will be more code affected by the
> change.

I suspect that (even if this is not a solution chosen elsewhere), for Darwin,
there will have to be a target hook to control this since I can’t change the
ABI retrospectively for the systems already released.  That is, GCC is emitting
broken code on those platforms anyway (since the platform ABI is determined
by what clang/llvm produces).

Iain

Re: gcc vs clang for non-power-2 atomic structures

[Jonathan Wakely]

On Fri, 23 Aug 2019, 11:13 Iain Sandoe, <idsandoe@googlemail.com> wrote:
>
>
> > On 23 Aug 2019, at 10:35, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> > On Fri, 23 Aug 2019 at 08:21, Iain Sandoe <idsandoe@googlemail.com> wrote:
> >>
> >> Hi Jim,
> >>
> >>> On 23 Aug 2019, at 00:56, Jim Wilson <jimw@sifive.com> wrote:
> >>>
> >>> We got a change request for the RISC-V psABI to define the atomic
> >>> structure size and alignment.  And looking at this, it turned out that
> >>> gcc and clang are implementing this differently.  Consider this
> >>> testcase
> >>>
> >>> rohan:2274$ cat tmp.c
> >>> #include <stdio.h>
> >>> struct s { int a; int b; int c;};
> >>> int
> >>> main(void)
> >>> {
> >>> printf("size=%ld align=%ld\n", sizeof (struct s), _Alignof(struct s));
> >>> printf("size=%ld align=%ld\n", sizeof (_Atomic (struct s)),
> >>> _Alignof(_Atomic (struct s)));
> >>> return 0;
> >>> }
> >>> rohan:2275$ gcc tmp.c
> >>> rohan:2276$ ./a.out
> >>> size=12 align=4
> >>> size=12 align=4
> >>> rohan:2277$ clang tmp.c
> >>> rohan:2278$ ./a.out
> >>> size=12 align=4
> >>> size=16 align=16
> >>> rohan:2279$
> >>>
> >>> This is with an x86 compiler.
> >>
> >> A search for _Atomic in the latest (x86_64) psABI document shows no results.
> >
> > See https://groups.google.com/forum/#!topic/ia32-abi/Tlu6Hs-ohPY and
> > the various GCC bugs linked to from that thread.
> >
> > This is a big can of worms, and GCC needs fixing (but probably not
> > until the ABI group decide something).
>
> I agree about the size of the can of worms.
> However, there doesn’t seem to be any actual issue filed on:
> https://github.com/hjl-tools/x86-psABI
>
> Would it help if someone did?


Is that for IA-32 as well, or only x86-64 and x32? I thought the
Google group was the right place for IA-32 issues, and that would then
get picked up by the x86-64 psABI too. But I don't know. It would be
helpful if these things weren't spread around over so many different
locations with no actual fixes happening.

Just to confuse me further, the wiki at that GitHub repo says to go to gitlab!

"All x86-64 psABI changes should be sent to
https://gitlab.com/x86-psABIs/x86-64-ABI."

It feels like nobody cares about fixing this.

Re: gcc vs clang for non-power-2 atomic structures

[Joseph Myers]

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

On Fri, 23 Aug 2019, Iain Sandoe wrote:

> absolutely, it’s the psABI that’s lacking here - the compilers (as commented
> by Richard Smith in a referenced thread) should not be making ABI up…

With over 50 target architectures supported in GCC, most of which probably 
don't have anyone maintaining a psABI for them, you don't get support for 
new language features that require an ABI without making some reasonable 
default choice that allows the features to work everywhere and then 
letting architecture maintainers liaise with ABI maintainers in the case 
where such exist.

(ABIs for atomics have the further tricky issue that there can be multiple 
choices for how to map the memory model onto a given architecture; see 
<https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html>.  So it's not 
just a matter of type sizes and alignment.)

There *is* a clear GCC bug (bug 65146) in the specific case of _Atomic 
long long / _Atomic double in structures on 32-bit x86; those need to be 
forced to 8-byte alignment when atomic as they are outside structures.  
Size / alignment for _Atomic versions of types whose size isn't (2, 4, 8, 
16) bytes is another matter; the GCC default (don't change size / 
alignment when making atomic) seems perfectly reasonable, but where psABIs 
specify something we do of course need to follow it (and the choice may be 
OS-specific, not just processor-specific, where the ABI is defined by the 
default compiler for some OS).

Note: it's likely some front-end code, and stdatomic.h, might have to 
change to handle the possibility of atomic types being larger than 
non-atomic, as those end up using type-generic atomic load / store 
built-in functions, and those certainly expect pointers to arguments of 
the same size (when one argument is the atomic type and one non-atomic).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: gcc vs clang for non-power-2 atomic structures

[Jim Wilson]

I was pointed at
    https://bugs.llvm.org/show_bug.cgi?id=26462
for the LLVM discussion of this problem.

Another issue here is that we should have ABI testing for atomic.
For instance, gcc/testsuite/gcc.dg/compat has no atomic testcases.
Likewise g++.dg/compat.

Jim

Re: gcc vs clang for non-power-2 atomic structures

[Iain Sandoe]

Hi Joseph,

> On 23 Aug 2019, at 17:14, Joseph Myers <joseph@codesourcery.com> wrote:
> 
> On Fri, 23 Aug 2019, Iain Sandoe wrote:
> 
>> absolutely, it’s the psABI that’s lacking here - the compilers (as commented
>> by Richard Smith in a referenced thread) should not be making ABI up…
> 
> With over 50 target architectures supported in GCC, most of which probably 
> don't have anyone maintaining a psABI for them, you don't get support for 
> new language features that require an ABI without making some reasonable 
> default choice that allows the features to work everywhere and then 
> letting architecture maintainers liaise with ABI maintainers in the case 
> where such exist.

yes. That’s perfectly reasonable
However, it’s more than a little disappointing that X86, for which I would hope
that the psABI _was_ considered supported, remains silent on the issue so long
after it arose (I guess the interested parties with $ need to sponsor some work
 to update it).

> (ABIs for atomics have the further tricky issue that there can be multiple 
> choices for how to map the memory model onto a given architecture; see 
> <https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html>.  So it's not 
> just a matter of type sizes and alignment.)

Indeed, I have tangled a bit with that trying to adapt libatomic to be better
behaved on Darwin.

> but where psABIs 
> specify something we do of course need to follow it (and the choice may be 
> OS-specific, not just processor-specific, where the ABI is defined by the 
> default compiler for some OS).

agreed .. it seems highly likely for X86 as things stand - since there’s a
bunch of things already out there with different ABIs baked in.
> 
> Note: it's likely some front-end code, and stdatomic.h, might have to 
> change to handle the possibility of atomic types being larger than 
> non-atomic, as those end up using type-generic atomic load / store 
> built-in functions, and those certainly expect pointers to arguments of 
> the same size (when one argument is the atomic type and one non-atomic).

It seems to me that whatever might be chosen for the definitive psABI / platform
(i.e. arch + OS + version) going forward, we will need to support what has
been emitted in the past.

So a recommendation for suitable FE hooks  (and preferably a way to make
the C11 atomic match the std::atomic, even if this is “only” a QoI issue),
would be worth addressing.

thanks
Iain

> 
> -- 
> Joseph S. Myers
> joseph@codesourcery.com