llvm / llvm-project

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

[clang] Value-initialisation of type with `noexcept(false)` trivial default constructor incorrectly potentially-throwing #70545

Open wreien opened 10 months ago

wreien commented 10 months ago
struct S {
  S() noexcept(false) = default;
};

static_assert(noexcept(S()));
static_assert(__is_nothrow_constructible(S));

Clang incorrectly fails these two static assertions.

For the former, S() is value-initialization, and so by https://eel.is/c++draft/dcl.init#general-9.1 it is zero-initialised without default-initialization, and the default constructor (being trivial) is not called. As such, because the default constructor is not implicitly invoked, it does not meet the requirements for https://eel.is/c++draft/except.spec#6.2 and the assertion should pass.

For the latter, https://eel.is/c++draft/meta.unary.prop#9 checks effectively S s(); (as not-a-function-declaration) not S s;, and thus should check whether value-initialization is potentially-throwing, as above.

llvmbot commented 10 months ago

@llvm/issue-subscribers-clang-frontend

Author: Nathaniel Shead (wreien)

``` struct S { S() noexcept(false) = default; }; static_assert(noexcept(S())); static_assert(__is_nothrow_constructible(S)); ``` Clang incorrectly fails these two static assertions. For the former, `S()` is value-initialization, and so by https://eel.is/c++draft/dcl.init#general-9.1 it is zero-initialised without default-initialization, and the default constructor (being trivial) is not called. As such, because the default constructor is not implicitly invoked, it does not meet the requirements for https://eel.is/c++draft/except.spec#6.2 and the assertion should pass. For the latter, https://eel.is/c++draft/meta.unary.prop#9 checks effectively `S s();` (as not-a-function-declaration) not `S s;`, and thus should check whether value-initialization is potentially-throwing, as above.
llvmbot commented 10 months ago

@llvm/issue-subscribers-c-1

Author: Nathaniel Shead (wreien)

``` struct S { S() noexcept(false) = default; }; static_assert(noexcept(S())); static_assert(__is_nothrow_constructible(S)); ``` Clang incorrectly fails these two static assertions. For the former, `S()` is value-initialization, and so by https://eel.is/c++draft/dcl.init#general-9.1 it is zero-initialised without default-initialization, and the default constructor (being trivial) is not called. As such, because the default constructor is not implicitly invoked, it does not meet the requirements for https://eel.is/c++draft/except.spec#6.2 and the assertion should pass. For the latter, https://eel.is/c++draft/meta.unary.prop#9 checks effectively `S s();` (as not-a-function-declaration) not `S s;`, and thus should check whether value-initialization is potentially-throwing, as above.
shafik commented 10 months ago

So it looks like in Sema::canThrow(...):

https://github.com/llvm/llvm-project/blob/ebc2c4bde3ec0d37d1bcb224e7ffcc18ac4e2620/clang/lib/Sema/SemaExceptionSpec.cpp#L1189-L1191

it does not take into account that it is marked zeroing.

CC @zygoloid

shafik commented 10 months ago

So it looks like in Sema::canThrow(...):

https://github.com/llvm/llvm-project/blob/ebc2c4bde3ec0d37d1bcb224e7ffcc18ac4e2620/clang/lib/Sema/SemaExceptionSpec.cpp#L1189-L1191

it does not take into account that it is marked zeroing.

CC @zygoloid

plausible fix:

if (CE->requiresZeroInitialization() && CE->getConstructor()->isTrivial() && CE->getConstructor()->isDefaultConstructor())
      return CT_Cannot;

I ran check-clang and the only tests that fail are identical to this test case for the noexcept part of the issue.

https://github.com/llvm/llvm-project/blob/0d1da7c37f64bcca9b57396af26b7cf4136aefa7/clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.default/p2.cpp#L80

and

https://github.com/llvm/llvm-project/blob/0d1da7c37f64bcca9b57396af26b7cf4136aefa7/clang/test/CXX/drs/dr17xx.cpp#L156

and

https://github.com/llvm/llvm-project/blob/0d1da7c37f64bcca9b57396af26b7cf4136aefa7/clang/test/CXX/drs/dr17xx.cpp#L160

philnik777 commented 10 months ago

I 100% disagree with this. I don't know whether it is the case that the standard currently requires this behaviour, but if I write struct S { S() noexcept(false) = default; }; I obviously want is_nothrow_default_constructible to be false. If this is required by the standard, IMO this is a language defect and we should file a CWG issue instead. This should either be ill-formed or do what a user would expect.

shafik commented 10 months ago

I 100% disagree with this. I don't know whether it is the case that the standard currently requires this behaviour, but if I write struct S { S() noexcept(false) = default; }; I obviously want is_nothrow_default_constructible to be false. If this is required by the standard, IMO this is a language defect and we should file a CWG issue instead. This should either be ill-formed or do what a user would expect.

The other implementations disagree with clang on except MSVC on the second case: https://godbolt.org/z/ed9bK48sv

The reading of the wording seems correct and I could not find a defect report but I would like to hear from Richard on this.

cor3ntin commented 10 months ago

I imagine we could warn here. Something like "noexcept on the default constructor of an aggregate has no effect", something like that.

philnik777 commented 10 months ago

@shafik They're not exactly consistent with themselves in lots of cases: https://godbolt.org/z/xnovcrxK6

zygoloid commented 10 months ago

This seems worth filing as a core issue, to get this discussed and to point out the vendor inconsistency demonstrated by @philnik777's compiler explorer link.

(Personally, I think the "value initialization doesn't call a trivial default constructor" guarantee ought to never change program semantics -- it's really just an optimization that for whatever reason the language rules specify rather than leaving to as-if / QoI -- and any case where it causes visibly different program semantics, such as here, ought to be treated as a bug in the language rules. But CWG might not agree.)

shafik commented 10 months ago

I sent a query to core and we will see what the response is: https://lists.isocpp.org/core/current/15016.php

shafik commented 10 months ago

This now CWG2820, it should be live I am guessing after Kona sometime.

frederick-vs-ja commented 4 months ago

CWG2820 has been accepted as a DR, so the original issue is not a bug now. But something seems wrong with dtors...