Correct handling of the 'throw()' exception specifier in C++17.

[jyknight]

Summary

Per C++17 [except.spec], 'throw()' has become equivalent to
'noexcept', and should therefore call std::terminate, not
std::unexpected.

[rsmith]

What's the motivation for this change? I believe the current behavior is still conforming: set_unexpected is no longer (officially) part of the standard library (though it still exists as a zombie name), and the default unexpected handler calls terminate, so calling unexpected rather than terminate should have the same effect, except in non-conforming programs that call std::set_unexpected anyway (and for such programs, calling unexpected seems like the behavior the programmer would expect). Do we generate better code if we call terminate rather than unexpected?

[jyknight]

In D113517#3120030, @rsmith wrote:

What's the motivation for this change? I believe the current behavior is still conforming: set_unexpected is no longer (officially) part of the standard library (though it still exists as a zombie name), and the default unexpected handler calls terminate, so calling unexpected rather than terminate should have the same effect, except in non-conforming programs that call std::set_unexpected anyway (and for such programs, calling unexpected seems like the behavior the programmer would expect). Do we generate better code if we call terminate rather than unexpected?

Today: no, we don't.

But, I'm planning to propose further changes to improve noexcept codegen in Clang -- which is currently quite bad, compared to what it should be, because it's effectively following the same rules as legacy throw(). This change allows those future optimizations to apply to throw() as well, in C++17 mode, which is the desirable outcome.

[• Quuxplusone]

Peanut gallery says: It seems like Proto->canThrow() is already returning the correct answer for C++17-and-later: a function declared throw() cannot throw. So from the POV of C++17-and-later, this could be a simple patch:

- if (isNoexceptExceptionSpec(EST) && Proto->canThrow() == CT_Cannot) {
+ if (Proto->canThrow() == CT_Cannot) {

and the only reason we can't do this is because C++14-and-earlier needs a different behavior.

Meanwhile, it looks as if every other use of isNoexceptExceptionSpec (or its inverse isDynamicExceptionSpec) is related to pretty-printing the exception specification, and isn't used for semantics at all.
Perhaps a cleaner way to preserve the current special case for C++14-and-earlier is to add a "language version" parameter to canThrow, and/or introduce an isEssentiallyNoexcept() helper:

- if (isNoexceptExceptionSpec(EST) && Proto->canThrow() == CT_Cannot) {
+ if (Proto->isEssentiallyNoexcept(getLangOpts())) {

[rsmith]

In D113517#3121455, @jyknight wrote:

This change allows those future optimizations to apply to throw() as well, in C++17 mode, which is the desirable outcome.

I see. It seems inconsistent that throw(X) would still call unexpected but that throw() would call terminate, but I suppose in the throw() case there is really not much interesting that an unexpected_handler can do other than (take some logging action and) terminate the program -- in particular, it can't do exception translation. So maybe the inconsistency is not a big deal, and it's more important to get the better code generation, especially given how rare throw(X) is compared to throw(). OK, I think I'm convinced that this is the best direction.

clang/lib/CodeGen/CGException.cpp

479–484

Maybe the logic would be clearer if we swap the terminate and unexpected cases:

if  (EST == EST_Dynamic || (!getLangOpts().CPlusPlus17 && EST == EST_DynamicNone)) {
  // Prepare to call unexpected
} else if (Proto->canThrow() == CT_Cannot) {
  // Prepare to call terminate
}

This would keep the syntactic checks of EST separated from the semantic checks of canThrow.

[jyknight]

In D113517#3122217, @rsmith wrote:

In D113517#3121455, @jyknight wrote:

This change allows those future optimizations to apply to throw() as well, in C++17 mode, which is the desirable outcome.

I see. It seems inconsistent that throw(X) would still call unexpected but that throw() would call terminate, but I suppose in the throw() case there is really not much interesting that an unexpected_handler can do other than (take some logging action and) terminate the program -- in particular, it can't do exception translation. So maybe the inconsistency is not a big deal, and it's more important to get the better code generation, especially given how rare throw(X) is compared to throw(). OK, I think I'm convinced that this is the best direction.

Well, "throw(X)" is not even permitted by C++17. In Clang, we allow that it only if you turn off the default-error warning option. Given that, I'm not too worried about it keeping the legacy behavior, while "throw()" gets the new behavior.

clang/lib/CodeGen/CGException.cpp

479–484

Agreed, that's better. I'll make that change and submit.

clang/lib/CodeGen/CGException.cpp

if (const CapturedDecl* CD = dyn_cast_or_null<CapturedDecl>(D)) {
         EHStack.pushTerminate();
     }
     return;
   }
   const FunctionProtoType *Proto = FD->getType()->getAs<FunctionProtoType>();
   if (!Proto)
     return;
 
   ExceptionSpecificationType EST = Proto->getExceptionSpecType();
-  if (isNoexceptExceptionSpec(EST) && Proto->canThrow() == CT_Cannot) {
-    // noexcept functions are simple terminate scopes.
-    if (!getLangOpts().EHAsynch) // -EHa: HW exception still can occur
-      EHStack.pushTerminate();
-  } else if (EST == EST_Dynamic || EST == EST_DynamicNone) {
+  // In C++17 and later, 'throw()' aka EST_DynamicNone is treated the same way
+  // as noexcept. In earlier standards, it is handled in this block, along with
+  // 'throw(X...)'.
+  if (EST == EST_Dynamic ||
+      (EST == EST_DynamicNone && !getLangOpts().CPlusPlus17)) {
     // TODO: Revisit exception specifications for the MS ABI.  There is a way to
     // encode these in an object file but MSVC doesn't do anything with it.
     if (getTarget().getCXXABI().isMicrosoft())
       return;
     // In Wasm EH we currently treat 'throw()' in the same way as 'noexcept'. In
     // case of throw with types, we ignore it and print a warning for now.
     // TODO Correctly handle exception specification in Wasm EH
     if (CGM.getLangOpts().hasWasmExceptions()) {
if (EST == EST_Dynamic ||
 
     for (unsigned I = 0; I != NumExceptions; ++I) {
       QualType Ty = Proto->getExceptionType(I);
       QualType ExceptType = Ty.getNonReferenceType().getUnqualifiedType();
       llvm::Value *EHType = CGM.GetAddrOfRTTIDescriptor(ExceptType,
                                                         /*ForEH=*/true);
       Filter->setFilter(I, EHType);
     }
+  } else if (Proto->canThrow() == CT_Cannot) {
+    // noexcept functions are simple terminate scopes.
+    if (!getLangOpts().EHAsynch) // -EHa: HW exception still can occur
+      EHStack.pushTerminate();
   }
 }
 
 /// Emit the dispatch block for a filter scope if necessary.
 static void emitFilterDispatchBlock(CodeGenFunction &CGF,
                                     EHFilterScope &filterScope) {
   llvm::BasicBlock *dispatchBlock = filterScope.getCachedEHDispatchBlock();
   if (!dispatchBlock) return;
if (!FD) {
     }
     return;
   }
   const FunctionProtoType *Proto = FD->getType()->getAs<FunctionProtoType>();
   if (!Proto)
     return;
 
   ExceptionSpecificationType EST = Proto->getExceptionSpecType();
-  if (isNoexceptExceptionSpec(EST) && Proto->canThrow() == CT_Cannot &&
-      !EHStack.empty() /* possible empty when under async exceptions */) {
-    EHStack.popTerminate();
-  } else if (EST == EST_Dynamic || EST == EST_DynamicNone) {
+  if (EST == EST_Dynamic ||
+      (EST == EST_DynamicNone && !getLangOpts().CPlusPlus17)) {
     // TODO: Revisit exception specifications for the MS ABI.  There is a way to
     // encode these in an object file but MSVC doesn't do anything with it.
     if (getTarget().getCXXABI().isMicrosoft())
       return;
     // In wasm we currently treat 'throw()' in the same way as 'noexcept'. In
     // case of throw with types, we ignore it and print a warning for now.
     // TODO Correctly handle exception specification in wasm
     if (CGM.getLangOpts().hasWasmExceptions()) {
       if (EST == EST_DynamicNone)
         EHStack.popTerminate();
       return;
     }
     EHFilterScope &filterScope = cast<EHFilterScope>(*EHStack.begin());
     emitFilterDispatchBlock(*this, filterScope);
     EHStack.popFilter();
+  } else if (Proto->canThrow() == CT_Cannot &&
+              /* possible empty when under async exceptions */
+             !EHStack.empty()) {
+    EHStack.popTerminate();
   }
 }
 
 void CodeGenFunction::EmitCXXTryStmt(const CXXTryStmt &S) {
   EnterCXXTryStmt(S);
   EmitStmt(S.getTryBlock());
   ExitCXXTryStmt(S);
 }

clang/test/CXX/except/except.spec/p9-dynamic.cpp

-// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - -fcxx-exceptions -fexceptions | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - -fcxx-exceptions -fexceptions | FileCheck %s --check-prefixes=CHECK,CHECK-PRE17
+// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -std=c++17 -Wno-dynamic-exception-spec -emit-llvm -o - -fcxx-exceptions -fexceptions | FileCheck %s --check-prefixes=CHECK,CHECK-17
 
 void external();
 
+// CHECK-LABEL: _Z6targetv(
+// CHECK: invoke void @_Z8externalv()
+// CHECK:      landingpad { i8*, i32 }
+// CHECK-NEXT:   filter [1 x i8*] [i8* bitcast (i8** @_ZTIi to i8*)]
+// CHECK:      call void @__cxa_call_unexpected
 void target() throw(int)
 {
-  // CHECK: invoke void @_Z8externalv()
   external();
 }
-// CHECK:      landingpad { i8*, i32 }
-// CHECK-NEXT:   filter [1 x i8*] [i8* bitcast (i8** @_ZTIi to i8*)]
-// CHECK:      call void @__cxa_call_unexpected
+
+// CHECK-LABEL: _Z7target2v(
+// CHECK: invoke void @_Z8externalv()
+// CHECK:            landingpad { i8*, i32 }
+// CHECK-PRE17-NEXT:   filter [0 x i8*] zeroinitializer
+// CHECK-17-NEXT:      catch i8* null
+// CHECK-PRE17:      call void @__cxa_call_unexpected
+// CHECK-17:         call void @__clang_call_terminate
+void target2() throw()
+{
+  external();
+}