Page MenuHomePhabricator

[Sema] Correct nothrow inherited by noexcept
ClosedPublic

Authored by erichkeane on Sep 22 2017, 5:13 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane created this revision.Sep 22 2017, 5:13 PM
STL_MSFT edited edge metadata.Sep 22 2017, 5:40 PM

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

aaron.ballman edited edge metadata.Sep 25 2017, 5:51 AM

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.

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.

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)?

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 marked 2 inline comments as done.

2 small changes @aaron.ballman requested.

aaron.ballman accepted this revision.Sep 28 2017, 10:50 AM

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.

This revision is now accepted and ready to land.Sep 28 2017, 10:50 AM
This revision was automatically updated to reflect the committed changes.