llvm / llvm-project

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

[rejects-valid] aggregate initialization of classes with immovable members are broken #95035

Open ericniebler opened 1 month ago

ericniebler commented 1 month ago

The following well-formed C++20 code is rejected by clang (all version tested from 13 through trunk):

template <class T>
struct single : T {};

template <class A, class B>
struct pair {
  A a;
  B b;
};

struct immovable {
  immovable() = default;
  immovable(immovable&&) = delete;
};

struct S{};

int main() {
  // error: call to deleted constructor of 'immovable'
  [[maybe_unused]]
  auto a = single<single<immovable>>{single<immovable>{immovable{}}};

  // error: no viable conversion from 'pair<int, immovable>' to 'int'
  [[maybe_unused]]
  auto b = single<pair<int, immovable>>{pair<int, immovable>{42, immovable{}}};

  // error: initializer for aggregate with no elements requires explicit braces
  [[maybe_unused]]
  auto c = single<pair<S, immovable>>{pair<S, immovable>{S{}, immovable{}}};
}

Other compilers get this right. https://godbolt.org/z/rorxx1jK9

llvmbot commented 1 month ago

@llvm/issue-subscribers-clang-frontend

Author: Eric Niebler (ericniebler)

The following well-formed C++20 code is rejected by clang (all version tested from 13 through trunk): ```c++ template <class T> struct single : T {}; template <class A, class B> struct pair { A a; B b; }; struct immovable { immovable() = default; immovable(immovable&&) = delete; }; struct S{}; int main() { // error: call to deleted constructor of 'immovable' [[maybe_unused]] auto a = single<single<immovable>>{single<immovable>{immovable{}}}; // error: no viable conversion from 'pair<int, immovable>' to 'int' [[maybe_unused]] auto b = single<pair<int, immovable>>{pair<int, immovable>{42, immovable{}}}; // error: initializer for aggregate with no elements requires explicit braces [[maybe_unused]] auto c = single<pair<S, immovable>>{pair<S, immovable>{S{}, immovable{}}}; } ``` Other compilers get this right. https://godbolt.org/z/rorxx1jK9
llvmbot commented 1 month ago

@llvm/issue-subscribers-c-20

Author: Eric Niebler (ericniebler)

The following well-formed C++20 code is rejected by clang (all version tested from 13 through trunk): ```c++ template <class T> struct single : T {}; template <class A, class B> struct pair { A a; B b; }; struct immovable { immovable() = default; immovable(immovable&&) = delete; }; struct S{}; int main() { // error: call to deleted constructor of 'immovable' [[maybe_unused]] auto a = single<single<immovable>>{single<immovable>{immovable{}}}; // error: no viable conversion from 'pair<int, immovable>' to 'int' [[maybe_unused]] auto b = single<pair<int, immovable>>{pair<int, immovable>{42, immovable{}}}; // error: initializer for aggregate with no elements requires explicit braces [[maybe_unused]] auto c = single<pair<S, immovable>>{pair<S, immovable>{S{}, immovable{}}}; } ``` Other compilers get this right. https://godbolt.org/z/rorxx1jK9
zygoloid commented 1 month ago

In general, in-place initialization of base classes is not possible, because they may overlap earlier fields. It's not really clear what the language rule ought to be here, but IIRC clang conservatively doesn't perform in-place initialization for any base class rather than trying something more subtle to distinguish the broken cases from the working ones.

ericniebler commented 1 month ago

i don't follow. can you give me an example of a broken case?

zygoloid commented 1 month ago

Here's an example of a case that would be miscompiled if the base class were initialized in-place, for which both Clang and GCC perform a copy.

It looks like GCC performs the same extra copy as Clang when initializing a base class from a function call, but has the guaranteed copy elision behavior when performing aggregate initialization of the base class (and I'd imagine in other non-function-call initialization scenarios). GCC's behavior here seems pretty reasonable. Would be good to also get the language rule adjusted to match :) (IIRC there's a core issue on this already.)

[Edit: fixed example]

ericniebler commented 1 month ago

i'm totally confused. all i see is valid code that should compile and run just fine. what are you meaning when you say "in-place initialization"? where in the standard does it say that compilers should reject this:

template <class T>
struct single : T {};

struct immovable {
  immovable() = default;
  immovable(immovable&&) = delete;
};

single<immovable> a{immovable{}};

i'm dumbfounded that this can be anything other than valid code.

zygoloid commented 1 month ago

It doesn't say that in the standard. The point of the example is that if we initialize in place for such an example then (due to ABI decisions) we would miscompile in some cases. So we don't do that -- the standard rules don't quite work. If we add this to your example:

immovable f();
single<immovable> b{f()};

then GCC rejects too, for the same reason.

shafik commented 1 month ago

(IIRC there's a core issue on this already.)

I was not able to find a core issue for this, although I may be using the right keywords. The only open empty base class issue I found was this one: https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2736

zygoloid commented 1 month ago

This is CWG2403. See also reflector thread starting https://lists.isocpp.org/core/2018/02/4003.php

zygoloid commented 1 month ago

For now I suggest we follow the approach described towards the end of the reflector thread: instead of forcing a constructor call for all base class initialization, only force a constructor call if the initializer is function-call-like: either a prvalue function call, or a conditional expression one of whose results is function-call-like, looking through the usual transparent things (parentheses, right-hand side of a comma). Don't force a constructor call if the initializer is already a constructor call or aggregate initialization or similar.

zygoloid commented 1 month ago

For whoever looks into this: https://github.com/llvm/llvm-project/blob/7cff05ada05e87408966d56b4c1675033187ff5c/clang/lib/Sema/SemaInit.cpp#L4255 is the place to start.