[Sema] Correct nothrow inherited by noexcept

[erichkeane]

Summary

As reported in https://bugs.llvm.org/show_bug.cgi?id=33235,
a noexcept function was unable to inherit from a nothrow defaulted
constructor. Attribute "nothrow" is supposed to be semantically
identical to noexcept, and in fact, a number of other places in the
code treat them identically.

This patch simply checks the RecordDecl for the correct attribute in
the case where no other exception specifier was set.

[STL_MSFT]

This sounds right to me and the test looks good. (I'll let an actual compiler dev sign off)

[aaron.ballman]

I'm not certain we have the semantics of __declspec(nothrow) exactly correct -- a simple test shows that __attribute__((nothrow)), __declspec(nothrow), and noexcept(true) are subtly different.

When I run this with MSVC 2017, the terminate handler is not called and the application continues rather than terminates, but Clang calls the terminate handler.

#include <exception>
#include <iostream>

__declspec(nothrow) void f() { throw 1; }

int main() {
  std::set_terminate([]() {
    std::cout << "terminate called" << std::endl;
    std::abort();
  });

  f();
}

However, in this case terminate is called using GCC 6.3 and Clang.

#include <exception>
#include <iostream>
 
__attribute__((nothrow)) void f() { throw 1; }
 
int main() {
  std::set_terminate([]() {
    std::cout << "terminate called" << std::endl;
    std::abort();
  });
 
  f();
}

For your test case: GCC accepts (with attribute), MSVC accepts (with __declspec), and EDG rejects Derived() noexcept = default because of incompatible exception specs. I think it's probably reasonable for us to accept despite the slight semantic differences.

@STL_MSFT -- do you think __declspec(nothrow) calling the terminate handler in Clang is a bug?

lib/Sema/SemaDeclCXX.cpp

170 ↗

(On Diff #116443)

Elide the braces.

test/SemaCXX/nothrow-as-noexcept-ctor.cpp

3 ↗

(On Diff #116443)

You can remove the spurious newline from the test.

[STL_MSFT]

do you think __declspec(nothrow) calling the terminate handler in Clang is a bug?

It's certainly a behavior difference with potential performance impact, although I don't think it can be viewed as a bug, strictly speaking. MSVC treats __declspec(nothrow) as an optimization request, with undefined behavior if it's violated. Termination is definitely one of the possible results of undefined behavior.

We've recently had to slightly rethink our EH strategy in light of C++17's addition of noexcept into the type system, and the removal of dynamic exception specifications (and the change in semantics to throw()). MSVC's /EHsc makes this extra fun. If you're interested, I can put you in contact with the compiler dev who recently made those changes.

[aaron.ballman]

In D38209#880553, @STL_MSFT wrote:

do you think __declspec(nothrow) calling the terminate handler in Clang is a bug?

It's certainly a behavior difference with potential performance impact, although I don't think it can be viewed as a bug, strictly speaking. MSVC treats __declspec(nothrow) as an optimization request, with undefined behavior if it's violated. Termination is definitely one of the possible results of undefined behavior.

We've recently had to slightly rethink our EH strategy in light of C++17's addition of noexcept into the type system, and the removal of dynamic exception specifications (and the change in semantics to throw()). MSVC's /EHsc makes this extra fun. If you're interested, I can put you in contact with the compiler dev who recently made those changes.

That might be helpful. I'm mostly interested in whether __declspec(nothrow) is intended to be part of the type system in the same way noexcept specifiers are.

@erichkeane -- can you see whether __attribute__((nothrow)) is part of the type system as far as GCC is concerned (in C++17 mode, assuming GCC has implemented that functionality already)?

[erichkeane]

In D38209#880559, @aaron.ballman wrote:

In D38209#880553, @STL_MSFT wrote:

do you think __declspec(nothrow) calling the terminate handler in Clang is a bug?

It's certainly a behavior difference with potential performance impact, although I don't think it can be viewed as a bug, strictly speaking. MSVC treats __declspec(nothrow) as an optimization request, with undefined behavior if it's violated. Termination is definitely one of the possible results of undefined behavior.

We've recently had to slightly rethink our EH strategy in light of C++17's addition of noexcept into the type system, and the removal of dynamic exception specifications (and the change in semantics to throw()). MSVC's /EHsc makes this extra fun. If you're interested, I can put you in contact with the compiler dev who recently made those changes.

That might be helpful. I'm mostly interested in whether __declspec(nothrow) is intended to be part of the type system in the same way noexcept specifiers are.

@erichkeane -- can you see whether __attribute__((nothrow)) is part of the type system as far as GCC is concerned (in C++17 mode, assuming GCC has implemented that functionality already)?

I just did some playing around with godbolt, and it seems that attribute-nothrow does NOT modify the mangled name in GCC C++17 mode (when done as a template-parameter to a 'call' function), so it seems to me that it is NOT part of the type system in this regard.

Interestingly, however, GCC allows the first exampel in the test (Base w/ attribute, derrived with noexcept) with or WITHOUT the attribute. It seems to just completely lack this check even in trunk.

[erichkeane]

2 small changes @aaron.ballman requested.

[aaron.ballman]

This is somewhat complicated in that noexcept, throw(), and attribute(())/__declspec no throw are all synonyms for one another except for the type system effect differences between attribute and __declspec, but I think our behavior here is reasonable.

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

if (!Proto)
     return;
 
   ExceptionSpecificationType EST = Proto->getExceptionSpecType();
 
   // If we have a throw-all spec at this point, ignore the function.
   if (ComputedEST == EST_None)
     return;
 
+  if (EST == EST_None && Method->hasAttr<NoThrowAttr>())
+    EST = EST_BasicNoexcept;
+
   switch(EST) {
   // If this function can throw any exceptions, make a note of that.
   case EST_MSAny:
   case EST_None:
     ClearExceptions();
     ComputedEST = EST;
     return;
   // FIXME: If the call to this decl is using any of its default arguments, we

cfe/trunk/test/SemaCXX/nothrow-as-noexcept-ctor.cpp

+// RUN: %clang_cc1 %s -fcxx-exceptions -fsyntax-only -Wexceptions -verify -std=c++14
+
+// expected-no-diagnostics
+struct Base {
+  __attribute__((nothrow)) Base() {}
+};
+
+struct Derived : Base {
+  Derived() noexcept = default;
+};
+
+struct Base2 {
+   Base2() noexcept {}
+};
+
+struct Derived2 : Base2 {
+  __attribute__((nothrow)) Derived2() = default;
+};
+
+struct Base3 {
+  __attribute__((nothrow)) Base3() {}
+};
+
+struct Derived3 : Base3 {
+  __attribute__((nothrow)) Derived3() = default;
+};