[cfe-dev] libc++: help diagnosing the following std::atomic compile error

[Howard Hinnant]

On May 21, 2013, at 9:04 AM, David Chisnall <David.Chisnall at cl.cam.ac.uk> wrote:

> On 20 May 2013, at 22:49, Richard Smith <richard at metafoo.co.uk> wrote:
> 
>> struct T { char c[3]; };
>> static_assert(sizeof(std::atomic<T>) == 3 && alignof(std::atomic<T>) == 1, "under libstdc++");
>> static_assert(sizeof(std::atomic<T>) == 4 && alignof(std::atomic<T>) == 4, "under libc++");
> 
> This looks like an ABI issue, but I don't imagine that we'll encounter any ABIs where the libstdc++ answer should be correct for alignof.  It's debatable whether the sizeof answer is correct, as the last byte should be padding, but as the intention of C11 _Atomic() and C++11 std::atomic<> is to be compiled down (for <word size types) to single atomic load / store operations (or short load-linked, store-conditional sequences) without exposing racy issues libstdc++ answer looks wrong (I know of no architectures which can do 3-byte atomic ops, and with alignof(1) this could end up spanning two cache lines and be impossible to operate on atomically without a mutex in anything except Haswell or BlueGene/Q).
> 
> This is my concern with the GCC atomic builtins, and why I am not happy with clang supporting them or encouraging their use.  There is explicitly no guarantee in C11 that T and _Atomic(T) have the same ABI and it was expected that they would not, because efficient atomic operations have stricter constraints (much stricter on some architectures, a bit stricter on x86).  It is almost impossible to use the GCC builtins correctly in a situation where the ABIs differ for the atomic and non-atomic types.
> 
> David
> 

I'm getting {3, 1}, not {4, 4} with both the current <atomic>, and the one I'm testing.  That is on both x86_64, and i386, Mountain Lion.


> On May 21, 2013, at 9:11 AM, David Chisnall <David.Chisnall at cl.cam.ac.uk> wrote:
> 
>> 
>> On 21 May 2013, at 01:55, Howard Hinnant <hhinnant at apple.com> wrote:
>> 
>>> I've enclosed a proposed patch for fixing <atomic>.  This patch changes us from using _Atomic(T) for the stored type, to just T, but aligned to _Atomic(T).  
>> 
>> This may still end up with the std::atomic<> types lacking padding in structures that makes it possible to act on them correctly.  For example, what do you get when you do:
>> 
>> struct {
>> 	std::atomic<char> x;
>> 	char y;
>> };
>> 
>> y should be sufficiently after x that they can be operated on independently (at least from an ISA perspective - if they're in the same cache line then most RISCy CPUs will do a bit of redundant work anyway).  Even though the alignment of y is 1, we (typically) should be adding 3 bytes of padding after x.
> 

I should stress that I have no particular attachment to this approach.  From Richard's suggestions:


On May 20, 2013, at 5:20 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> I think there are basically two long-term options and one short-term workaround:
> 
> 1) Use T rather than _Atomic(T), manually ensure that std::atomic<T> has the right alignment, and use the __atomic_* builtins instead of the __c11_atomic_* builtins (this is the approach which libstdc++ takes), or
> 2) Get someone to implement initialization support for _Atomic(T), where T is a class type where the selected copy/move constructor is trivial. I don't have time to work on this right now, I'm afraid.
> 
> Workaround: Use __c11_atomic_init instead of initializing the member in the mem-initializer-list, for now at least. This has the disadvantage that you can't mark the constructor as 'constexpr', but you could restrict this to the case of non-scalar T to avoid regressing.

I picked the one that I had the skills to do.  It is fine with me if we abandon 1), and go for 2).

Howard