[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

[James Y Knight via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfddc4e41164e: Correct handling of the throw() 
exception specifier in C++17. (authored by jyknight).

Changed prior to commit:
  https://reviews.llvm.org/D113517?vs=385971=386326#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113517/new/

https://reviews.llvm.org/D113517

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/test/CXX/except/except.spec/p9-dynamic.cpp


Index: clang/test/CXX/except/except.spec/p9-dynamic.cpp
===
--- clang/test/CXX/except/except.spec/p9-dynamic.cpp
+++ clang/test/CXX/except/except.spec/p9-dynamic.cpp
@@ -1,12 +1,26 @@
-// 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();
+}
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -477,11 +477,11 @@
 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())
@@ -521,6 +521,10 @@
 /*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();
   }
 }
 
@@ -580,10 +584,8 @@
 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())
@@ -599,6 +601,10 @@
 EHFilterScope  = cast(*EHStack.begin());
 emitFilterDispatchBlock(*this, filterScope);
 EHStack.popFilter();
+  } else if (Proto->canThrow() == CT_Cannot &&
+  /* possible empty when under async exceptions */
+ !EHStack.empty()) {
+EHStack.popTerminate();
   }
 }
 


Index: clang/test/CXX/except/except.spec/p9-dynamic.cpp
===
--- clang/test/CXX/except/except.spec/p9-dynamic.cpp
+++ clang/test/CXX/except/except.spec/p9-dynamic.cpp
@@ -1,12 +1,26 @@
-// 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: 

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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.




Comment at: clang/lib/CodeGen/CGException.cpp:480-487
+  // In C++17 and later, 'throw()' aka EST_DynamicNone is treated the same way
+  // as noexcept. In earlier standards, it is handled separately, below.
+  if ((getLangOpts().CPlusPlus17 || EST != EST_DynamicNone) &&
+  Proto->canThrow() == CT_Cannot) {
 // noexcept functions are simple terminate scopes.
 if (!getLangOpts().EHAsynch) // -EHa: HW exception still can occur
   EHStack.pushTerminate();

rsmith wrote:
> 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`.
Agreed, that's better. I'll make that change and submit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113517/new/

https://reviews.llvm.org/D113517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

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.




Comment at: clang/lib/CodeGen/CGException.cpp:480-487
+  // In C++17 and later, 'throw()' aka EST_DynamicNone is treated the same way
+  // as noexcept. In earlier standards, it is handled separately, below.
+  if ((getLangOpts().CPlusPlus17 || EST != EST_DynamicNone) &&
+  Proto->canThrow() == CT_Cannot) {
 // noexcept functions are simple terminate scopes.
 if (!getLangOpts().EHAsynch) // -EHa: HW exception still can occur
   EHStack.pushTerminate();

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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113517/new/

https://reviews.llvm.org/D113517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

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())) {


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113517/new/

https://reviews.llvm.org/D113517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113517/new/

https://reviews.llvm.org/D113517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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. Do we generate better code if we call `terminate` 
rather than `unexpected`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113517/new/

https://reviews.llvm.org/D113517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added a reviewer: aaron.ballman.
jyknight requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113517

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/test/CXX/except/except.spec/p9-dynamic.cpp


Index: clang/test/CXX/except/except.spec/p9-dynamic.cpp
===
--- clang/test/CXX/except/except.spec/p9-dynamic.cpp
+++ clang/test/CXX/except/except.spec/p9-dynamic.cpp
@@ -1,12 +1,26 @@
-// 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();
+}
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -477,7 +477,10 @@
 return;
 
   ExceptionSpecificationType EST = Proto->getExceptionSpecType();
-  if (isNoexceptExceptionSpec(EST) && Proto->canThrow() == CT_Cannot) {
+  // In C++17 and later, 'throw()' aka EST_DynamicNone is treated the same way
+  // as noexcept. In earlier standards, it is handled separately, below.
+  if ((getLangOpts().CPlusPlus17 || EST != EST_DynamicNone) &&
+  Proto->canThrow() == CT_Cannot) {
 // noexcept functions are simple terminate scopes.
 if (!getLangOpts().EHAsynch) // -EHa: HW exception still can occur
   EHStack.pushTerminate();
@@ -580,7 +583,8 @@
 return;
 
   ExceptionSpecificationType EST = Proto->getExceptionSpecType();
-  if (isNoexceptExceptionSpec(EST) && Proto->canThrow() == CT_Cannot &&
+  if ((getLangOpts().CPlusPlus17 || EST != EST_DynamicNone) &&
+  Proto->canThrow() == CT_Cannot &&
   !EHStack.empty() /* possible empty when under async exceptions */) {
 EHStack.popTerminate();
   } else if (EST == EST_Dynamic || EST == EST_DynamicNone) {


Index: clang/test/CXX/except/except.spec/p9-dynamic.cpp
===
--- clang/test/CXX/except/except.spec/p9-dynamic.cpp
+++ clang/test/CXX/except/except.spec/p9-dynamic.cpp
@@ -1,12 +1,26 @@
-// 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();
+}
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp