llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.09k stars 11.59k forks source link

Clang rejects deleted defaulted noexcept default constructor #32897

Open BillyONeal opened 7 years ago

BillyONeal commented 7 years ago
Bugzilla Link 33550
Version trunk
OS All
CC @CaseyCarter,@DougGregor,@Ivan171,@ojeda,@zygoloid

Extended Description

Consider the following:

template<class _Ty>
struct atomic {

    constexpr atomic(_Ty _Val) throw() // TRANSITION, VSO#174686
        : _My_val(_Val)
        {   // construct from _Val, initialization is not an atomic operation
        }

    atomic() _NOEXCEPT = default;
    atomic(const atomic&) = delete;
    atomic& operator=(const atomic&) = delete;
    atomic& operator=(const atomic&) volatile = delete;

    _Ty _My_val;
};

struct non_default_ctor_able {
    non_default_ctor_able() = delete;
    explicit non_default_ctor_able(int) {}
    non_default_ctor_able(const non_default_ctor_able&) = default;
    non_default_ctor_able& operator=(const non_default_ctor_able&) = default;
    ~non_default_ctor_able() = default;
};

int main() {
    atomic<non_default_ctor_able> atm{non_default_ctor_able{42}};
    (void)atm;
};

C1XX, EDG, and GCC are all happy with this. Clang rejects ( https://wandbox.org/permlink/b40YJn7QwRprgdTp ) , saying:

prog.cc:9:2: error: exception specification of explicitly defaulted default constructor does not match the calculated one
        atomic() noexcept = default;
        ^
prog.cc:26:35: note: in instantiation of template class 'atomic<non_default_ctor_able>' requested here
    atomic<non_default_ctor_able> atm{non_default_ctor_able{42}};
                                  ^
1 error generated.
ojeda commented 5 years ago

The wording changed again for C++20 (but they should still be deleted).

    struct S {
        ~S() noexcept(false) = default;
    };

Testing on godbolt: gcc, icc, msvc accept it (and they all seem to actually delete the special member functions); clang still rejects it.

https://godbolt.org/z/SXKzat

BillyONeal commented 6 years ago

unsigned char data[suitable_atomic_size];

That moves the problem to the constexpr atomic(T initial_value) constructor though. That constexpr ctor effectively requires that atomic<T> really contain a T, barring "compiler magic". Rather than ask for compiler magic that would have surprising behavior in any case, the right thing to do is fix the spec.

contains a copy of the bytes of a T, that can be updated and extracted atomically to reconstitute a T object

That is the model; you certainly can't form a T* that points to the T inside. In our implementation all the actual manipulation of the contents (except for that initial value constructor) goes through the _InterlockedXxx intrinsics so we're operating on the bytes, not on the T.

But the "= default" on the default constructor must go

Interestingly the =default thing doesn't even have a requirement that the resulting atomic is trivially default constructible; and if T is not a hardware atomic size our implementation is not (since the spinlock needs to be zeroed).

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

A "fixed" implementation of atomic is unimplementable :)

Well, depending on what you mean by "fixed" :)

I'm thinking of something like:

template<typename T>
struct atomic {
  // size and alignment adjusted suitably to support atomic access on the target
  alignas(suitable_atomic_align<T>)
  unsigned char data[suitable_atomic_size<T>];

public:
  atomic() noexcept = default;
  // ...
};

Fundamentally, my model for atomic<T> is not that it contains a T, but that it contains a copy of the bytes of a T, that can be updated and extracted atomically to reconstitute a T object. (There's no such thing as an "atomic T object", so all we can have here is the object representation of one.)

Allowing this atomic container of object representation to be uninitialized seems to not be completely crazy, but it's certainly surprising behavior for the atomic default constructor. (I do think LWG 2334 is a good change. But the = default on the default constructor must go if we make that change -- at least when T's default constructor is not trivial -- and removing the =default in that case would also work around the CWG 1778 issue.)

BillyONeal commented 6 years ago

A fixed implementation of atomic would not encounter this Clang bug :)

A "fixed" implementation of atomic is unimplementable :)

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

DR1778 appears to be unimplementable. See http://lists.isocpp.org/core/2018/01/3741.php

CaseyCarter commented 6 years ago

The atomic default constructor is specified as leaving the object uninitialized

That particular abomination is the subject LWG 2334. Let's simply pretend that the class in the sample is named foo instead of atomic for now ;)

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

Confirmed.

However, isn't that definition of atomic wrong? The atomic default constructor is specified as leaving the object uninitialized, and this implementation will try to run the default constructor. Example:

struct Foo {
  Foo() noexcept { std::cout << "this text should not appear"; }
}
std::atomic<Foo> foo;

... will incorrectly print out text with this implementation. A fixed implementation of atomic would not encounter this Clang bug :)

CaseyCarter commented 6 years ago

This is CWG 1778 (http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1778). As a heads up, this will affect users of clang + MSVC's standard library parallel algorithms in the not-too-distant future. There may not be many of them, but we will point them at this bug ;)