[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.



Comment at: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl:20
 // Test that Attr.Const from OpenCLBuiltins.td is lowered to a readnone 
attribute.
+// FIXME: we don't, though.
 // CHECK-LABEL: @test_const_attr

svenvh wrote:
> lebedev.ri wrote:
> > I've looked, and i really don't understand how D64319 works.
> > It seems like the AST is then serialized into a header?
> > Because just adding a new attribute without spelling does not solve the 
> > issue.
> We're not setting the `NoUnwind` attribute for OpenCL (yet).  The following 
> quick-and-dirty patch appears to fix this test for your patch (but will cause 
> other tests to fail).  If you think it's time to add `NoUnwind` now, I can 
> try putting up a review for that.
> 
> ```
> diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
> index 276d91fa2758..1ea3c11fbe03 100644
> --- a/clang/lib/CodeGen/CGCall.cpp
> +++ b/clang/lib/CodeGen/CGCall.cpp
> @@ -1972,7 +1972,7 @@ void 
> CodeGenModule::getDefaultFunctionAttributes(StringRef Name,
>// TODO: NoUnwind attribute should be added for other GPU modes OpenCL, 
> HIP,
>// SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
>// code.
> -  if (getLangOpts().CUDA && getLangOpts().CUDAIsDevice) {
> +  if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) || 
> getLangOpts().OpenCL) {
>  // Exceptions aren't supported in CUDA device code.
>  FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
>}
> ```
Yeah, that would be my guess as to how you'd fix this it, yes.
I'd suggest posting such a patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added inline comments.



Comment at: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl:20
 // Test that Attr.Const from OpenCLBuiltins.td is lowered to a readnone 
attribute.
+// FIXME: we don't, though.
 // CHECK-LABEL: @test_const_attr

lebedev.ri wrote:
> I've looked, and i really don't understand how D64319 works.
> It seems like the AST is then serialized into a header?
> Because just adding a new attribute without spelling does not solve the issue.
We're not setting the `NoUnwind` attribute for OpenCL (yet).  The following 
quick-and-dirty patch appears to fix this test for your patch (but will cause 
other tests to fail).  If you think it's time to add `NoUnwind` now, I can try 
putting up a review for that.

```
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 276d91fa2758..1ea3c11fbe03 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -1972,7 +1972,7 @@ void 
CodeGenModule::getDefaultFunctionAttributes(StringRef Name,
   // TODO: NoUnwind attribute should be added for other GPU modes OpenCL, HIP,
   // SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
   // code.
-  if (getLangOpts().CUDA && getLangOpts().CUDAIsDevice) {
+  if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) || 
getLangOpts().OpenCL) {
 // Exceptions aren't supported in CUDA device code.
 FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
   }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Ok, so. I looked really hard, and essentially i'm not sure we can just change 
those attributes from implying `readonly`/`readnone`.
Things kinda just fall apart




Comment at: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl:20
 // Test that Attr.Const from OpenCLBuiltins.td is lowered to a readnone 
attribute.
+// FIXME: we don't, though.
 // CHECK-LABEL: @test_const_attr

I've looked, and i really don't understand how D64319 works.
It seems like the AST is then serialized into a header?
Because just adding a new attribute without spelling does not solve the issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D138958#4060633 , @lebedev.ri 
wrote:

> Ok, if we must not unconditionally emit the memory attributes, then let's not.
> Please stamp? :)

FWIW, I don't think we should land any of this until after the Clang 16 branch 
has happened; I think we need more time for this to bake before we release it 
to the wild.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 489975.
lebedev.ri added subscribers: Anastasia, arsenm.
lebedev.ri added a comment.

Ok, if we must not unconditionally emit the memory attributes, then let's not.
Please stamp? :)

@Anastasia @venvh @arsenm This breaks opencl headers in a way i do not 
understand how to undo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

Files:
  clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/function-attributes.c
  clang/test/CodeGen/pragma-weak.c
  clang/test/CodeGen/struct-passing.c
  clang/test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp
  clang/test/CodeGenCXX/pr58798.cpp
  clang/test/CodeGenCXX/pure-const-attrs-and-ir-memory-attributes.cpp
  clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
  clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
@@ -9,12 +9,12 @@
   int i;
   ~B_ShouldDiag() noexcept(true) {} //no disg, no throw stmt
 };
-struct R_ShouldDiag : A_ShouldDiag {
+struct R_ShouldDiag_NoThrow : A_ShouldDiag {
   B_ShouldDiag b;
-  ~R_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
+  ~R_ShouldDiag_NoThrow() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
 throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
-  __attribute__((nothrow)) R_ShouldDiag() {// expected-note {{function declared non-throwing here}}
+  __attribute__((nothrow)) R_ShouldDiag_NoThrow() {// expected-note {{function declared non-throwing here}}
 throw 1;// expected-warning {{has a non-throwing exception specification but}}
   }
   void __attribute__((nothrow)) SomeThrow() {// expected-note {{function declared non-throwing here}}
@@ -240,7 +240,7 @@
   }
 }
 // As seen in p34973, this should not throw the warning.  If there is an active
-// exception, catch(...) catches everything. 
+// exception, catch(...) catches everything.
 void o_ShouldNotDiag() noexcept {
   try {
 throw;
Index: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
===
--- clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
+++ clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
@@ -17,16 +17,18 @@
 }
 
 // Test that Attr.Const from OpenCLBuiltins.td is lowered to a readnone attribute.
+// FIXME: we don't, though.
 // CHECK-LABEL: @test_const_attr
-// CHECK: call spir_func i32 @_Z3maxii({{.*}}) [[ATTR_CONST:#[0-9]]]
+// CHECK: call spir_func i32 @_Z3maxii({{.*}}) [[ATTR_CONST_OR_PURE:#[0-9]]]
 // CHECK: ret
 int test_const_attr(int a) {
   return max(a, 2);
 }
 
 // Test that Attr.Pure from OpenCLBuiltins.td is lowered to a readonly attribute.
+// FIXME: we don't, though.
 // CHECK-LABEL: @test_pure_attr
-// CHECK: call spir_func <4 x float> @_Z11read_imagef{{.*}} [[ATTR_PURE:#[0-9]]]
+// CHECK: call spir_func <4 x float> @_Z11read_imagef{{.*}} [[ATTR_CONST_OR_PURE]]
 // CHECK: ret
 kernel void test_pure_attr(read_only image1d_t img) {
   float4 resf = read_imagef(img, 42);
@@ -48,7 +50,5 @@
   float res = fract(a, b);
 }
 
-// CHECK: attributes [[ATTR_CONST]] =
-// CHECK-SAME: memory(none)
-// CHECK: attributes [[ATTR_PURE]] =
-// CHECK-SAME: memory(read)
+// CHECK: attributes [[ATTR_CONST_OR_PURE]] =
+// CHECK-NOT: memory(none)
Index: clang/test/CodeGenCXX/pure-const-attrs-and-ir-memory-attributes.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pure-const-attrs-and-ir-memory-attributes.cpp
@@ -0,0 +1,193 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-attributes --check-globals
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu   | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-NO-EXCEPTIONS
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu -fcxx-exceptions  | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-NO-EXCEPTIONS
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu -fcxx-exceptions -fexceptions | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-EXCEPTIONS
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu -fcxx-exceptions -fexceptions | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-EXCEPTIONS
+
+void opaque_callee();
+
+// 

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

@jdoerfert can you please clarify, do you believe we are fine as-is, or need to 
change attributes we emit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

The issue is the function call, not the stores.
Stores are valid as long as they are to local memory (stack or allocated by the 
function).

Because of the call, you can't tag it as `memory(none)`. But you may be able to 
tag it with inaccessiblemem if the call is also marked as such. Otherwise, no.

Performance wise it shouldn't matter much. How many functions are tagged with 
pure/const in source code? So here I would lean towards correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added subscribers: nlopes, regehr.
lebedev.ri added a comment.

In D138958#4054903 , @jdoerfert wrote:

> In D138958#4045567 , @efriedma 
> wrote:
>
>> From an IR semantics standpoint, I'm not sure the `memory(none)` marking is 
>> right if the function throws an exception.  LangRef doesn't explicitly 
>> exclude the possibility, but take the following:
>>
>>   void f() { throw 1; }
>>
>> It gets lowered to:
>>
>>   define dso_local void @_Z1fv() local_unnamed_addr #0 {
>>   entry:
>> %exception = tail call ptr @__cxa_allocate_exception(i64 4) #1
>> store i32 1, ptr %exception, align 16, !tbaa !5
>> tail call void @__cxa_throw(ptr nonnull %exception, ptr nonnull @_ZTIi, 
>> ptr null) #2
>> unreachable
>>   }
>
> If you mark this one as `readonly/none` we might, at some point, see the 
> mismatch in the body and declare it UB. 
> From the outside, it should almost be fine since it's not-`nounwind` and the 
> memory that is accessed is freshly allocated.
> However, it still would be a problem waiting to happen to have escaping 
> memory being written in a `readonly/none` function.
>
> That said, I think we anyway want a `memory` category to express all accesses 
> are to freshly allocated memory that may escape the function. This is a 
> common pattern.
> So `memory(allocated, inaccessiblemem)` would express that the allocation 
> does access some inaccesible memory and the function will also access the 
> newly allocated memory.
> Its arguably not as good as `readnone` but I'm not sure how to get there. 
> The best idea I have off the top of my head is a dedicated `exception` 
> category for `memory` such that it won't interfere with anything but other 
> exceptions, which it already does due to `unwind`.
> Thus, `pure/const` -> `memory(exception).

Thank you for commenting! Is there a feeling that just dropping the attributes,
until they can be reasonably represented, hinders optimizations too much,
and we must retain the ongoing miscompile?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D138958#4045567 , @efriedma wrote:

> From an IR semantics standpoint, I'm not sure the `memory(none)` marking is 
> right if the function throws an exception.  LangRef doesn't explicitly 
> exclude the possibility, but take the following:
>
>   void f() { throw 1; }
>
> It gets lowered to:
>
>   define dso_local void @_Z1fv() local_unnamed_addr #0 {
>   entry:
> %exception = tail call ptr @__cxa_allocate_exception(i64 4) #1
> store i32 1, ptr %exception, align 16, !tbaa !5
> tail call void @__cxa_throw(ptr nonnull %exception, ptr nonnull @_ZTIi, 
> ptr null) #2
> unreachable
>   }

If you mark this one as `readonly/none` we might, at some point, see the 
mismatch in the body and declare it UB. 
From the outside, it should almost be fine since it's not-`nounwind` and the 
memory that is accessed is freshly allocated.
However, it still would be a problem waiting to happen to have escaping memory 
being written in a `readonly/none` function.

That said, I think we anyway want a `memory` category to express all accesses 
are to freshly allocated memory that may escape the function. This is a common 
pattern.
So `memory(allocated, inaccessiblemem)` would express that the allocation does 
access some inaccesible memory and the function will also access the newly 
allocated memory.
Its arguably not as good as `readnone` but I'm not sure how to get there. 
The best idea I have off the top of my head is a dedicated `exception` category 
for `memory` such that it won't interfere with anything but other exceptions, 
which it already does due to `unwind`.
Thus, `pure/const` -> `memory(exception).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I'm not trying to argue what the `pure`/`const` attribute in C/C++ is supposed 
to mean.  I agree with your interpretation of the gcc 
documentation/implementation.  I'm saying that there's a mismatch between the 
gcc `pure`/`const` and the LLVM IR `memory(read)`/`memory(none)`, and therefore 
you can't unconditionally lower `const` to `memory(none)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D138958#4045567 , @efriedma wrote:

> From an IR semantics standpoint, I'm not sure the `memory(none)` marking is 
> right if the function throws an exception.

Then `memory(read)` would be wrong, either.
A bit of a hot take: it's a Schrodinger's exception in quantum state.
What i mean by that, is that e.g. `pure` attribute is documented as

  and will not cause any observable side-effects other than
  returning a value. They may read memory, but can not modify memory in a way
  that would either affect observable state or their outcome on subsequent runs.

So if it does unwind, that's fine, but if you *observed* that (not by catching 
an exception),
then you're in UB lands, because *you*, not the function, violated it's 
contract.

That might be too hot of a take, but it does seem like the EH internal details 
aren't really *that* observable...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Eli Friedman via Phabricator via cfe-commits]

efriedma added subscribers: nikic, efriedma.
efriedma added reviewers: nikic, jdoerfert.
efriedma added a comment.

From an IR semantics standpoint, I'm not sure the `memory(none)` marking is 
right if the function throws an exception.  LangRef doesn't explicitly exclude 
the possibility, but take the following:

  void f() { throw 1; }

It gets lowered to:

  define dso_local void @_Z1fv() local_unnamed_addr #0 {
  entry:
%exception = tail call ptr @__cxa_allocate_exception(i64 4) #1
store i32 1, ptr %exception, align 16, !tbaa !5
tail call void @__cxa_throw(ptr nonnull %exception, ptr nonnull @_ZTIi, ptr 
null) #2
unreachable
  }

So we have a function that essentially makes a heap allocation, store a value 
to it, then throws it.  And throwing the exception then makes other changes to 
the exception handling state which are visible to the caller.  Given these 
details are exposed in LLVM IR, saying that this is `memory(none)` seems a bit 
dubious.  (I don't have a testcase that breaks off the top of my head, but I 
suspect it's possible to write such a testcase.)  Given that, if you're going 
to drop the nounwind attribute, I suspect it also makes sense to also drop the 
`memory(none)` attribute.

The AddPotentialArgAccess() helper exists to handle a similar sort of 
situation: a function is marked `pure` in C, but the return value is returned 
indirectly, so it's not actually pure from the perspective of LLVM IR.  To fix 
that, we adjust the IR attributes to something more appropriate.

(Adding @nikic and @jdoerfert for additional perspective on the IR attributes.)

-

See also https://github.com/llvm/llvm-project/issues/36098 , which is also 
about a mismatch between the meaning of the C `pure` attribute and the LLVM IR 
`memory(none)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 488343.
lebedev.ri marked an inline comment as done.
lebedev.ri edited the summary of this revision.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

Files:
  clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/function-attributes.c
  clang/test/CodeGen/struct-passing.c
  clang/test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp
  clang/test/CodeGenCXX/pr58798.cpp
  clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
@@ -9,12 +9,12 @@
   int i;
   ~B_ShouldDiag() noexcept(true) {} //no disg, no throw stmt
 };
-struct R_ShouldDiag : A_ShouldDiag {
+struct R_ShouldDiag_NoThrow : A_ShouldDiag {
   B_ShouldDiag b;
-  ~R_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
+  ~R_ShouldDiag_NoThrow() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
 throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
-  __attribute__((nothrow)) R_ShouldDiag() {// expected-note {{function declared non-throwing here}}
+  __attribute__((nothrow)) R_ShouldDiag_NoThrow() {// expected-note {{function declared non-throwing here}}
 throw 1;// expected-warning {{has a non-throwing exception specification but}}
   }
   void __attribute__((nothrow)) SomeThrow() {// expected-note {{function declared non-throwing here}}
@@ -240,7 +240,7 @@
   }
 }
 // As seen in p34973, this should not throw the warning.  If there is an active
-// exception, catch(...) catches everything. 
+// exception, catch(...) catches everything.
 void o_ShouldNotDiag() noexcept {
   try {
 throw;
Index: clang/test/CodeGenCXX/pr58798.cpp
===
--- clang/test/CodeGenCXX/pr58798.cpp
+++ clang/test/CodeGenCXX/pr58798.cpp
@@ -1,18 +1,18 @@
 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-attributes
 // RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu -fexceptions -fcxx-exceptions | FileCheck %s
 
-// CHECK: Function Attrs: mustprogress noinline nounwind optnone willreturn memory(read)
+// CHECK: Function Attrs: mustprogress noinline optnone willreturn memory(read)
 // CHECK-LABEL: define {{[^@]+}}@_Z54early_caller_of_callee_with_clang_attr_with_clang_attri
 // CHECK-SAME: (i32 noundef [[A:%.*]]) #[[ATTR0:[0-9]+]] {
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:[[A_ADDR:%.*]] = alloca i32, align 4
 // CHECK-NEXT:store i32 [[A]], ptr [[A_ADDR]], align 4
 // CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
-// CHECK-NEXT:[[CALL:%.*]] = call noundef i32 @_Z22callee_with_clang_attri(i32 noundef [[TMP0]]) #[[ATTR3:[0-9]+]]
+// CHECK-NEXT:[[CALL:%.*]] = call noundef i32 @_Z22callee_with_clang_attri(i32 noundef [[TMP0]]) #[[ATTR4:[0-9]+]]
 // CHECK-NEXT:ret i32 [[CALL]]
 //
 
-// CHECK: Function Attrs: mustprogress noinline nounwind optnone willreturn memory(read)
+// CHECK: Function Attrs: mustprogress noinline optnone willreturn memory(read)
 // CHECK-LABEL: define {{[^@]+}}@_Z22callee_with_clang_attri
 // CHECK-SAME: (i32 noundef [[A:%.*]]) #[[ATTR0]] {
 // CHECK-NEXT:  entry:
@@ -22,9 +22,9 @@
 // CHECK-NEXT:[[TOBOOL:%.*]] = icmp ne i32 [[TMP0]], 0
 // CHECK-NEXT:br i1 [[TOBOOL]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
 // CHECK:   if.then:
-// CHECK-NEXT:[[EXCEPTION:%.*]] = call ptr @__cxa_allocate_exception(i64 4) #[[ATTR4:[0-9]+]]
+// CHECK-NEXT:[[EXCEPTION:%.*]] = call ptr @__cxa_allocate_exception(i64 4) #[[ATTR5:[0-9]+]]
 // CHECK-NEXT:store i32 42, ptr [[EXCEPTION]], align 16
-// CHECK-NEXT:call void @__cxa_throw(ptr [[EXCEPTION]], ptr @_ZTIi, ptr null) #[[ATTR5:[0-9]+]]
+// CHECK-NEXT:call void @__cxa_throw(ptr [[EXCEPTION]], ptr @_ZTIi, ptr null) #[[ATTR6:[0-9]+]]
 // CHECK-NEXT:unreachable
 // CHECK:   if.end:
 // CHECK-NEXT:ret i32 24
@@ -32,23 +32,31 @@
 
 // CHECK: Function Attrs: mustprogress noinline nounwind optnone
 // CHECK-LABEL: define {{[^@]+}}@_Z52early_caller_of_callee_with_clang_attr_with_cxx_attri
-// CHECK-SAME: (i32 noundef [[A:%.*]]) #[[ATTR1:[0-9]+]] {
+// CHECK-SAME: (i32 noundef [[A:%.*]]) #[[ATTR1:[0-9]+]] personality ptr @__gxx_personality_v0 {
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:[[A_ADDR:%.*]] = alloca i32, align 4
 // CHECK-NEXT:

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:4138
+ "functions and statements">;
+  let SimpleHandler = 1;
+}

lebedev.ri wrote:
> aaron.ballman wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > lebedev.ri wrote:
> > > > > aaron.ballman wrote:
> > > > > > I would have guessed we'd want to help the user out in a case like: 
> > > > > > `[[clang::nounwind]] void func() noexcept(false);`, given that this 
> > > > > > stuff can creep in via macros?
> > > > > Could you please clarify, what do you mean by "help the user" in this 
> > > > > case?
> > > > > Given
> > > > > ```
> > > > > [[clang::nounwind]] void func() noexcept(false);
> > > > > 
> > > > > void qqq();
> > > > > 
> > > > > void foo() {
> > > > >   try {
> > > > > func();
> > > > >   } catch (...) {
> > > > > qqq();
> > > > >   }
> > > > > }
> > > > > ```
> > > > > we already end up with
> > > > > ```
> > > > > ; Function Attrs: mustprogress noinline nounwind optnone uwtable
> > > > > define dso_local void @_Z3foov() #0 {
> > > > > entry:
> > > > >   call void @_Z4funcv() #2
> > > > >   ret void
> > > > > }
> > > > > 
> > > > > ; Function Attrs: nounwind
> > > > > declare void @_Z4funcv() #1
> > > > > 
> > > > > attributes #0 = { mustprogress noinline nounwind optnone uwtable 
> > > > > "frame-pointer"="all" "min-legal-vector-width"="0" 
> > > > > "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
> > > > > "target-cpu"="x86-64" 
> > > > > "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" 
> > > > > "tune-cpu"="generic" }
> > > > > attributes #1 = { nounwind "frame-pointer"="all" 
> > > > > "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
> > > > > "target-cpu"="x86-64" 
> > > > > "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" 
> > > > > "tune-cpu"="generic" }
> > > > > attributes #2 = { nounwind }
> > > > > ```
> > > > > and if i drop `[[clang::nounwind]]`, `landingpad` is back.
> > > > > Could you please clarify, what do you mean by "help the user" in this 
> > > > > case?
> > > > 
> > > > The user has said "this function throws exceptions" (`noexcept(false)`) 
> > > > and it's also said "this function never unwinds to its caller" (the 
> > > > attribute) and these statements are in conflict with one another and 
> > > > likely signify user confusion. I would have expected a warning 
> > > > diagnostic here.
> > > Ah. This is effectively intentional. This attribute, effectively,
> > > specifies the behavior in case the exception does get thrown, as being UB.
> > > If we diagnose that case, should we also disallow the stmt case?
> > > ```
> > > void i_will_throw() noexcept(false);
> > > 
> > > void foo() {
> > >   [[clang::nounwind]] i_will_throw(); // Should this be diagnosed?
> > > }
> > > ```
> > > Presumably not, because that immediately limits usefulness of the 
> > > attribute.
> > > 
> > > What we *DO* want, is diagnostics (and more importantly, a sanitizer!)
> > > in the case the exception *does* reach this UB boundary.
> > Sorry if this is a dumb question, but is the goal of this attribute 
> > basically to insert UB where there is well-defined behavior per spec? 
> > Throwing an exception that escapes from a function marked `noexcept(false)` 
> > is guaranteed to call `std::terminate()`: 
> > http://eel.is/c++draft/except#spec-5
> > Sorry if this is a dumb question, but is the goal of this attribute 
> > basically to insert UB where there is well-defined behavior per spec? 
> 
> Precisely! I was under impression i did call that out in the RFC/description, 
> but guess not explicitly enough?
> This is consistent with the existing (accidental) behavior of `const`/`pure` 
> attirbutes.
@lebedev.ri and I (and a few others) discussed this attribute on IRC and we're 
thinking of trying a different approach. Instead of exposing this attribute 
(which is pretty expert-only and seems likely to cause unintentional UB as a 
result), it might be worth exploring adding a `-f` flag to treat non-throwing 
exception specifications as implying `nounwind`. This is a language dialect and 
we usually avoid those, but we already have a significantly greater dialect in 
this same space with the `-fno-exceptions` functionality. We think the perf 
gains for the flag are likely to be worth supporting the dialect.

Next steps here are to rip out the nounwind attribute bits so we can move on 
the const and pure changes, and to start an RFC about the potential new flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:4138
+ "functions and statements">;
+  let SimpleHandler = 1;
+}

aaron.ballman wrote:
> lebedev.ri wrote:
> > aaron.ballman wrote:
> > > lebedev.ri wrote:
> > > > aaron.ballman wrote:
> > > > > I would have guessed we'd want to help the user out in a case like: 
> > > > > `[[clang::nounwind]] void func() noexcept(false);`, given that this 
> > > > > stuff can creep in via macros?
> > > > Could you please clarify, what do you mean by "help the user" in this 
> > > > case?
> > > > Given
> > > > ```
> > > > [[clang::nounwind]] void func() noexcept(false);
> > > > 
> > > > void qqq();
> > > > 
> > > > void foo() {
> > > >   try {
> > > > func();
> > > >   } catch (...) {
> > > > qqq();
> > > >   }
> > > > }
> > > > ```
> > > > we already end up with
> > > > ```
> > > > ; Function Attrs: mustprogress noinline nounwind optnone uwtable
> > > > define dso_local void @_Z3foov() #0 {
> > > > entry:
> > > >   call void @_Z4funcv() #2
> > > >   ret void
> > > > }
> > > > 
> > > > ; Function Attrs: nounwind
> > > > declare void @_Z4funcv() #1
> > > > 
> > > > attributes #0 = { mustprogress noinline nounwind optnone uwtable 
> > > > "frame-pointer"="all" "min-legal-vector-width"="0" 
> > > > "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
> > > > "target-cpu"="x86-64" 
> > > > "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" 
> > > > "tune-cpu"="generic" }
> > > > attributes #1 = { nounwind "frame-pointer"="all" 
> > > > "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
> > > > "target-cpu"="x86-64" 
> > > > "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" 
> > > > "tune-cpu"="generic" }
> > > > attributes #2 = { nounwind }
> > > > ```
> > > > and if i drop `[[clang::nounwind]]`, `landingpad` is back.
> > > > Could you please clarify, what do you mean by "help the user" in this 
> > > > case?
> > > 
> > > The user has said "this function throws exceptions" (`noexcept(false)`) 
> > > and it's also said "this function never unwinds to its caller" (the 
> > > attribute) and these statements are in conflict with one another and 
> > > likely signify user confusion. I would have expected a warning diagnostic 
> > > here.
> > Ah. This is effectively intentional. This attribute, effectively,
> > specifies the behavior in case the exception does get thrown, as being UB.
> > If we diagnose that case, should we also disallow the stmt case?
> > ```
> > void i_will_throw() noexcept(false);
> > 
> > void foo() {
> >   [[clang::nounwind]] i_will_throw(); // Should this be diagnosed?
> > }
> > ```
> > Presumably not, because that immediately limits usefulness of the attribute.
> > 
> > What we *DO* want, is diagnostics (and more importantly, a sanitizer!)
> > in the case the exception *does* reach this UB boundary.
> Sorry if this is a dumb question, but is the goal of this attribute basically 
> to insert UB where there is well-defined behavior per spec? Throwing an 
> exception that escapes from a function marked `noexcept(false)` is guaranteed 
> to call `std::terminate()`: http://eel.is/c++draft/except#spec-5
> Sorry if this is a dumb question, but is the goal of this attribute basically 
> to insert UB where there is well-defined behavior per spec? 

Precisely! I was under impression i did call that out in the RFC/description, 
but guess not explicitly enough?
This is consistent with the existing (accidental) behavior of `const`/`pure` 
attirbutes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:4138
+ "functions and statements">;
+  let SimpleHandler = 1;
+}

lebedev.ri wrote:
> aaron.ballman wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > I would have guessed we'd want to help the user out in a case like: 
> > > > `[[clang::nounwind]] void func() noexcept(false);`, given that this 
> > > > stuff can creep in via macros?
> > > Could you please clarify, what do you mean by "help the user" in this 
> > > case?
> > > Given
> > > ```
> > > [[clang::nounwind]] void func() noexcept(false);
> > > 
> > > void qqq();
> > > 
> > > void foo() {
> > >   try {
> > > func();
> > >   } catch (...) {
> > > qqq();
> > >   }
> > > }
> > > ```
> > > we already end up with
> > > ```
> > > ; Function Attrs: mustprogress noinline nounwind optnone uwtable
> > > define dso_local void @_Z3foov() #0 {
> > > entry:
> > >   call void @_Z4funcv() #2
> > >   ret void
> > > }
> > > 
> > > ; Function Attrs: nounwind
> > > declare void @_Z4funcv() #1
> > > 
> > > attributes #0 = { mustprogress noinline nounwind optnone uwtable 
> > > "frame-pointer"="all" "min-legal-vector-width"="0" 
> > > "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
> > > "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" 
> > > "tune-cpu"="generic" }
> > > attributes #1 = { nounwind "frame-pointer"="all" 
> > > "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
> > > "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" 
> > > "tune-cpu"="generic" }
> > > attributes #2 = { nounwind }
> > > ```
> > > and if i drop `[[clang::nounwind]]`, `landingpad` is back.
> > > Could you please clarify, what do you mean by "help the user" in this 
> > > case?
> > 
> > The user has said "this function throws exceptions" (`noexcept(false)`) and 
> > it's also said "this function never unwinds to its caller" (the attribute) 
> > and these statements are in conflict with one another and likely signify 
> > user confusion. I would have expected a warning diagnostic here.
> Ah. This is effectively intentional. This attribute, effectively,
> specifies the behavior in case the exception does get thrown, as being UB.
> If we diagnose that case, should we also disallow the stmt case?
> ```
> void i_will_throw() noexcept(false);
> 
> void foo() {
>   [[clang::nounwind]] i_will_throw(); // Should this be diagnosed?
> }
> ```
> Presumably not, because that immediately limits usefulness of the attribute.
> 
> What we *DO* want, is diagnostics (and more importantly, a sanitizer!)
> in the case the exception *does* reach this UB boundary.
Sorry if this is a dumb question, but is the goal of this attribute basically 
to insert UB where there is well-defined behavior per spec? Throwing an 
exception that escapes from a function marked `noexcept(false)` is guaranteed 
to call `std::terminate()`: http://eel.is/c++draft/except#spec-5


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:4138
+ "functions and statements">;
+  let SimpleHandler = 1;
+}

aaron.ballman wrote:
> lebedev.ri wrote:
> > aaron.ballman wrote:
> > > I would have guessed we'd want to help the user out in a case like: 
> > > `[[clang::nounwind]] void func() noexcept(false);`, given that this stuff 
> > > can creep in via macros?
> > Could you please clarify, what do you mean by "help the user" in this case?
> > Given
> > ```
> > [[clang::nounwind]] void func() noexcept(false);
> > 
> > void qqq();
> > 
> > void foo() {
> >   try {
> > func();
> >   } catch (...) {
> > qqq();
> >   }
> > }
> > ```
> > we already end up with
> > ```
> > ; Function Attrs: mustprogress noinline nounwind optnone uwtable
> > define dso_local void @_Z3foov() #0 {
> > entry:
> >   call void @_Z4funcv() #2
> >   ret void
> > }
> > 
> > ; Function Attrs: nounwind
> > declare void @_Z4funcv() #1
> > 
> > attributes #0 = { mustprogress noinline nounwind optnone uwtable 
> > "frame-pointer"="all" "min-legal-vector-width"="0" 
> > "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
> > "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" 
> > "tune-cpu"="generic" }
> > attributes #1 = { nounwind "frame-pointer"="all" "no-trapping-math"="true" 
> > "stack-protector-buffer-size"="8" "target-cpu"="x86-64" 
> > "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
> > attributes #2 = { nounwind }
> > ```
> > and if i drop `[[clang::nounwind]]`, `landingpad` is back.
> > Could you please clarify, what do you mean by "help the user" in this case?
> 
> The user has said "this function throws exceptions" (`noexcept(false)`) and 
> it's also said "this function never unwinds to its caller" (the attribute) 
> and these statements are in conflict with one another and likely signify user 
> confusion. I would have expected a warning diagnostic here.
Ah. This is effectively intentional. This attribute, effectively,
specifies the behavior in case the exception does get thrown, as being UB.
If we diagnose that case, should we also disallow the stmt case?
```
void i_will_throw() noexcept(false);

void foo() {
  [[clang::nounwind]] i_will_throw(); // Should this be diagnosed?
}
```
Presumably not, because that immediately limits usefulness of the attribute.

What we *DO* want, is diagnostics (and more importantly, a sanitizer!)
in the case the exception *does* reach this UB boundary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:4133
+def NoUnwind : DeclOrStmtAttr {
+  let Spellings = [Clang<"nounwind", /*allowInC =*/0>];
+  let Accessors = [Accessor<"isClangNoUnwind", [CXX11<"clang", "nounwind">]>];

lebedev.ri wrote:
> aaron.ballman wrote:
> > Should this be allowed in C (where structured exception handling is still a 
> > thing)?
> The only two thing about structured exceptions i know
> is that it's abbreviation and that it's a MSVC thing. 
> I don't know anything about how "exceptions" work there,
> and therefore i do not want to touch it.
> 
> https://llvm.org/docs/LangRef.html notes:
> ```
> nounwind
> This function attribute indicates that the function never raises an 
> exception. <...>.
> However, functions marked nounwind may still trap or generate asynchronous 
> exceptions.
> Exception handling schemes that are recognized by LLVM to handle asynchronous 
> exceptions,
> such as SEH, will still provide their implementation defined semantics.
> ```
Okay, let's leave it as-is for now and we can relax the restriction later if we 
find a need.



Comment at: clang/include/clang/Basic/Attr.td:4138
+ "functions and statements">;
+  let SimpleHandler = 1;
+}

lebedev.ri wrote:
> aaron.ballman wrote:
> > I would have guessed we'd want to help the user out in a case like: 
> > `[[clang::nounwind]] void func() noexcept(false);`, given that this stuff 
> > can creep in via macros?
> Could you please clarify, what do you mean by "help the user" in this case?
> Given
> ```
> [[clang::nounwind]] void func() noexcept(false);
> 
> void qqq();
> 
> void foo() {
>   try {
> func();
>   } catch (...) {
> qqq();
>   }
> }
> ```
> we already end up with
> ```
> ; Function Attrs: mustprogress noinline nounwind optnone uwtable
> define dso_local void @_Z3foov() #0 {
> entry:
>   call void @_Z4funcv() #2
>   ret void
> }
> 
> ; Function Attrs: nounwind
> declare void @_Z4funcv() #1
> 
> attributes #0 = { mustprogress noinline nounwind optnone uwtable 
> "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" 
> "stack-protector-buffer-size"="8" "target-cpu"="x86-64" 
> "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
> attributes #1 = { nounwind "frame-pointer"="all" "no-trapping-math"="true" 
> "stack-protector-buffer-size"="8" "target-cpu"="x86-64" 
> "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
> attributes #2 = { nounwind }
> ```
> and if i drop `[[clang::nounwind]]`, `landingpad` is back.
> Could you please clarify, what do you mean by "help the user" in this case?

The user has said "this function throws exceptions" (`noexcept(false)`) and 
it's also said "this function never unwinds to its caller" (the attribute) and 
these statements are in conflict with one another and likely signify user 
confusion. I would have expected a warning diagnostic here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:4133
+def NoUnwind : DeclOrStmtAttr {
+  let Spellings = [Clang<"nounwind", /*allowInC =*/0>];
+  let Accessors = [Accessor<"isClangNoUnwind", [CXX11<"clang", "nounwind">]>];

aaron.ballman wrote:
> Should this be allowed in C (where structured exception handling is still a 
> thing)?
The only two thing about structured exceptions i know
is that it's abbreviation and that it's a MSVC thing. 
I don't know anything about how "exceptions" work there,
and therefore i do not want to touch it.

https://llvm.org/docs/LangRef.html notes:
```
nounwind
This function attribute indicates that the function never raises an exception. 
<...>.
However, functions marked nounwind may still trap or generate asynchronous 
exceptions.
Exception handling schemes that are recognized by LLVM to handle asynchronous 
exceptions,
such as SEH, will still provide their implementation defined semantics.
```



Comment at: clang/include/clang/Basic/Attr.td:4138
+ "functions and statements">;
+  let SimpleHandler = 1;
+}

aaron.ballman wrote:
> I would have guessed we'd want to help the user out in a case like: 
> `[[clang::nounwind]] void func() noexcept(false);`, given that this stuff can 
> creep in via macros?
Could you please clarify, what do you mean by "help the user" in this case?
Given
```
[[clang::nounwind]] void func() noexcept(false);

void qqq();

void foo() {
  try {
func();
  } catch (...) {
qqq();
  }
}
```
we already end up with
```
; Function Attrs: mustprogress noinline nounwind optnone uwtable
define dso_local void @_Z3foov() #0 {
entry:
  call void @_Z4funcv() #2
  ret void
}

; Function Attrs: nounwind
declare void @_Z4funcv() #1

attributes #0 = { mustprogress noinline nounwind optnone uwtable 
"frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" 
"stack-protector-buffer-size"="8" "target-cpu"="x86-64" 
"target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #1 = { nounwind "frame-pointer"="all" "no-trapping-math"="true" 
"stack-protector-buffer-size"="8" "target-cpu"="x86-64" 
"target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #2 = { nounwind }
```
and if i drop `[[clang::nounwind]]`, `landingpad` is back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 487933.
lebedev.ri marked 10 inline comments as done.
lebedev.ri added a comment.

@aaron.ballman thank you for taking a look!
Addressed all review notes other than the SEH question and the simplehandler 
one.
(i'll deal with whitespace changes when committing, they should just be fixed 
upstream)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

Files:
  clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/Builtins.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/function-attributes.c
  clang/test/CodeGen/struct-passing.c
  clang/test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp
  clang/test/CodeGenCXX/exception-escape-RAII-codegen.cpp
  clang/test/CodeGenCXX/exception-escape-codegen.cpp
  clang/test/CodeGenCXX/pr58798.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-nounwind.cpp
  clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
@@ -9,12 +9,12 @@
   int i;
   ~B_ShouldDiag() noexcept(true) {} //no disg, no throw stmt
 };
-struct R_ShouldDiag : A_ShouldDiag {
+struct R_ShouldDiag_NoThrow : A_ShouldDiag {
   B_ShouldDiag b;
-  ~R_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
+  ~R_ShouldDiag_NoThrow() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
 throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
-  __attribute__((nothrow)) R_ShouldDiag() {// expected-note {{function declared non-throwing here}}
+  __attribute__((nothrow)) R_ShouldDiag_NoThrow() {// expected-note {{function declared non-throwing here}}
 throw 1;// expected-warning {{has a non-throwing exception specification but}}
   }
   void __attribute__((nothrow)) SomeThrow() {// expected-note {{function declared non-throwing here}}
@@ -24,6 +24,18 @@
throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
+struct R_ShouldDiag_NoUnwind : A_ShouldDiag {
+  B_ShouldDiag b;
+  ~R_ShouldDiag_NoUnwind() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
+throw 1; // expected-warning {{has a non-throwing exception specification but}}
+  }
+  __attribute__((nounwind)) R_ShouldDiag_NoUnwind() {// expected-note {{function declared non-throwing here}}
+throw 1;// expected-warning {{has a non-throwing exception specification but}}
+  }
+  void __attribute__((nounwind)) SomeThrow() {// expected-note {{function declared non-throwing here}}
+   throw 1; // expected-warning {{has a non-throwing exception specification but}}
+  }
+};
 
 struct M_ShouldNotDiag {
   B_ShouldDiag b;
@@ -240,7 +252,7 @@
   }
 }
 // As seen in p34973, this should not throw the warning.  If there is an active
-// exception, catch(...) catches everything. 
+// exception, catch(...) catches everything.
 void o_ShouldNotDiag() noexcept {
   try {
 throw;
Index: clang/test/Sema/attr-nounwind.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-nounwind.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fexceptions -fcxx-exceptions %s
+
+int bar();
+
+[[clang::nounwind]] void nounwind_fn(void) { }
+
+void foo() {
+  [[clang::nounwind]] bar();
+  [[clang::nounwind(0)]] bar(); // expected-error {{'nounwind' attribute takes no arguments}}
+  int x;
+  [[clang::nounwind]] x = 0; // expected-warning {{'nounwind' attribute is ignored because there exists no call expression inside the statement}}
+  [[clang::nounwind]] { asm("nop"); } // expected-warning {{'nounwind' attribute is ignored because there exists no call expression inside the statement}}
+  [[clang::nounwind]] label: x = 1; // expected-warning {{'nounwind' attribute only applies to functions and statements}}
+
+  [[clang::nounwind]] nounwind_fn();
+
+  __attribute__((nounwind)) bar(); // expected-warning {{attribute is ignored on this statement as it only applies to functions; use '[[clang::nounwind]]' on statements}}
+}
+

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Precommit CI seems to have found relevant issues that need to be addressed.




Comment at: clang/include/clang/Basic/Attr.td:4133
+def NoUnwind : DeclOrStmtAttr {
+  let Spellings = [Clang<"nounwind", /*allowInC =*/0>];
+  let Accessors = [Accessor<"isClangNoUnwind", [CXX11<"clang", "nounwind">]>];

Should this be allowed in C (where structured exception handling is still a 
thing)?



Comment at: clang/include/clang/Basic/Attr.td:4138
+ "functions and statements">;
+  let SimpleHandler = 1;
+}

I would have guessed we'd want to help the user out in a case like: 
`[[clang::nounwind]] void func() noexcept(false);`, given that this stuff can 
creep in via macros?



Comment at: clang/include/clang/Basic/AttrDocs.td:549
+Note: this attribute does not define any behaviour regarding exceptions,
+so functions annotated with this exception are allowed to throw exceptions,
+and doing so is neither UB nor will cause immediate program termination,





Comment at: clang/include/clang/Basic/AttrDocs.td:564
+or exibiting UB), and will not cause any observable side-effects other than
+returning a value. They can not write to memory, unlike functions declared
+with ``pure`` attribute, but may still read memory that is immutable between





Comment at: clang/include/clang/Basic/AttrDocs.td:569
+Note: this attribute does not define any behaviour regarding exceptions,
+so functions annotated with this exception are allowed to throw exceptions,
+and doing so is neither UB nor will cause immediate program termination,





Comment at: clang/include/clang/Basic/AttrDocs.td:582
+
+If a statement is marked with a ``[[clang::noinline]]`` attribute
+and contains call expressions, those call expressions inside the statement

Presumably?



Comment at: clang/include/clang/Basic/AttrDocs.td:598
 If a statement is marked ``nomerge`` and contains call expressions, those call
-expressions inside the statement will not be merged during optimization. This 
+expressions inside the statement will not be merged during optimization. This
 attribute can be used to prevent the optimizer from obscuring the source

Looks like a whole pile of unrelated whitespace-only changes crept in; those 
can be backed out.



Comment at: clang/lib/CodeGen/CGCall.cpp:2210-2212
+if (TargetDecl->hasAttr()) {
+  FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
+}





Comment at: clang/lib/Sema/SemaStmtAttr.cpp:246-248
+  if (!NIA.isClangNoUnwind()) {
+S.Diag(St->getBeginLoc(), diag::warn_function_attribute_ignored_in_stmt)
+<< "[[clang::nounwind]]";

Test coverage?



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:252-257
+  CallExprFinder CEF(S, St);
+  if (!CEF.foundCallExpr()) {
+S.Diag(St->getBeginLoc(), diag::warn_attribute_ignored_no_calls_in_stmt)
+<< A;
+return nullptr;
+  }

Oh great! But you're missing test coverage for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:565
+returning a value. They can not write to memory, but may read memory that is
+immutable between invocations of the function.
+

lebedev.ri wrote:
> erichkeane wrote:
> > A quick sentence somewhere saying HOW this is a 'stronger' version of pure 
> > would be a good addition here.
> Hopefully this is sufficient?
I think that clarifies it for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:565
+returning a value. They can not write to memory, but may read memory that is
+immutable between invocations of the function.
+

erichkeane wrote:
> A quick sentence somewhere saying HOW this is a 'stronger' version of pure 
> would be a good addition here.
Hopefully this is sufficient?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 487548.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

In D138958#4037526 , @erichkeane 
wrote:

> 2 quick nits, otherwise  LFTM.

@erichkeane thank you for the review!
@aaron.ballman would you like to stamp this too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

Files:
  clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/Builtins.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/function-attributes.c
  clang/test/CodeGen/struct-passing.c
  clang/test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp
  clang/test/CodeGenCXX/exception-escape-RAII-codegen.cpp
  clang/test/CodeGenCXX/exception-escape-codegen.cpp
  clang/test/CodeGenCXX/pr58798.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
@@ -9,12 +9,12 @@
   int i;
   ~B_ShouldDiag() noexcept(true) {} //no disg, no throw stmt
 };
-struct R_ShouldDiag : A_ShouldDiag {
+struct R_ShouldDiag_NoThrow : A_ShouldDiag {
   B_ShouldDiag b;
-  ~R_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
+  ~R_ShouldDiag_NoThrow() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
 throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
-  __attribute__((nothrow)) R_ShouldDiag() {// expected-note {{function declared non-throwing here}}
+  __attribute__((nothrow)) R_ShouldDiag_NoThrow() {// expected-note {{function declared non-throwing here}}
 throw 1;// expected-warning {{has a non-throwing exception specification but}}
   }
   void __attribute__((nothrow)) SomeThrow() {// expected-note {{function declared non-throwing here}}
@@ -24,6 +24,18 @@
throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
+struct R_ShouldDiag_NoUnwind : A_ShouldDiag {
+  B_ShouldDiag b;
+  ~R_ShouldDiag_NoUnwind() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
+throw 1; // expected-warning {{has a non-throwing exception specification but}}
+  }
+  __attribute__((nounwind)) R_ShouldDiag_NoUnwind() {// expected-note {{function declared non-throwing here}}
+throw 1;// expected-warning {{has a non-throwing exception specification but}}
+  }
+  void __attribute__((nounwind)) SomeThrow() {// expected-note {{function declared non-throwing here}}
+   throw 1; // expected-warning {{has a non-throwing exception specification but}}
+  }
+};
 
 struct M_ShouldNotDiag {
   B_ShouldDiag b;
@@ -240,7 +252,7 @@
   }
 }
 // As seen in p34973, this should not throw the warning.  If there is an active
-// exception, catch(...) catches everything. 
+// exception, catch(...) catches everything.
 void o_ShouldNotDiag() noexcept {
   try {
 throw;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -115,6 +115,7 @@
 // CHECK-NEXT: NoStackProtector (SubjectMatchRule_function)
 // CHECK-NEXT: NoThreadSafetyAnalysis (SubjectMatchRule_function)
 // CHECK-NEXT: NoThrow (SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: NoUnwind (SubjectMatchRule_function)
 // CHECK-NEXT: NoUwtable (SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: NotTailCalled (SubjectMatchRule_function)
 // CHECK-NEXT: OSConsumed (SubjectMatchRule_variable_is_parameter)
Index: clang/test/CodeGenCXX/pr58798.cpp
===
--- clang/test/CodeGenCXX/pr58798.cpp
+++ /dev/null
@@ -1,186 +0,0 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-attributes
-// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu -fexceptions -fcxx-exceptions | FileCheck %s
-
-// CHECK: 

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Erich Keane via Phabricator via cfe-commits]

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

2 quick nits, otherwise  LFTM.




Comment at: clang/include/clang/Basic/AttrDocs.td:558
+  let Content = [{
+A stronger version of ``__attribute__((pure))`` attribute.
+





Comment at: clang/include/clang/Basic/AttrDocs.td:565
+returning a value. They can not write to memory, but may read memory that is
+immutable between invocations of the function.
+

A quick sentence somewhere saying HOW this is a 'stronger' version of pure 
would be a good addition here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.
Herald added a subscriber: StephenFan.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Last(?) ping of the year?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

(ping, maybe)

In D138958#3968118 , @lebedev.ri 
wrote:

> I've been thinking, and it seems to me that explicitly opting into
> `__attribute__((nounwind))`-provided semantics should override
> the `__attribute__((nothrow))`/`noexcept` semantics.
> So looks like i should look into exception specification handling.

I've looked, and unless told otherwise, it seems like we
shouldn't add new exception specification for this attribute,
but special-case the attribute where it matters,
but i may be wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.

I've been thinking, and it seems to me that explicitly opting into
`__attribute__((nounwind))`-provided semantics should override
the `__attribute__((nothrow))`/`noexcept` semantics.
So looks like i should look into exception specification handling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.



Comment at: clang/lib/Analysis/CFG.cpp:2725
   NoReturn = true;
-if (FD->hasAttr())
+if (FD->hasAttr() || FD->hasAttr())
   AddEHEdge = false;

erichkeane wrote:
> lebedev.ri wrote:
> > erichkeane wrote:
> > > I find myself thinking we should probably have a function in FunctionDecl 
> > > that tests for the various states of the function, rather than keep 
> > > checking the attribute's presence.  
> > Yes, we have a huge spaghetti code spread through clang
> > that tries to answer the same two question:
> > 1. i'm a caller, if i call this function, might it throw?
> > 1. i'm a callee, what should i do with exceptions that try to unwind out of 
> > me?
> > 
> > I don't know how to improve that, and i don't think just moving this into 
> > FunctionDecl would help.
> > 
> `FunctionDecl::WontThrow` and `FunctionDecl::ShouldUnwind` ? 
As noted in the RFC, there are 3 possible behaviors on unwind.
If we want to improve interface, we should account for all of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'll take a look at rewording the docs if no one else does.  I should hopefully 
have time next week, the rest of the patch is perhaps more important at the 
moment.




Comment at: clang/docs/ReleaseNotes.rst:841
   operator.
-- ``clang_Cursor_getNumTemplateArguments``, 
``clang_Cursor_getTemplateArgumentKind``, 
-  ``clang_Cursor_getTemplateArgumentType``, 
``clang_Cursor_getTemplateArgumentValue`` and 
+- ``clang_Cursor_getNumTemplateArguments``, 
``clang_Cursor_getTemplateArgumentKind``,
+  ``clang_Cursor_getTemplateArgumentType``, 
``clang_Cursor_getTemplateArgumentValue`` and

lebedev.ri wrote:
> erichkeane wrote:
> > unrelated changes here?
> Having whitespaces before newline is bad :/
> This is editor doing so on file save,
> and my git complains otherwise.
Yep, i had to turn that off in my editor at one point because I did this!  I 
just pushed d5fc931ba to remove that WS, so this will disappear in your  next 
rebase.



Comment at: clang/lib/Analysis/CFG.cpp:2725
   NoReturn = true;
-if (FD->hasAttr())
+if (FD->hasAttr() || FD->hasAttr())
   AddEHEdge = false;

lebedev.ri wrote:
> erichkeane wrote:
> > I find myself thinking we should probably have a function in FunctionDecl 
> > that tests for the various states of the function, rather than keep 
> > checking the attribute's presence.  
> Yes, we have a huge spaghetti code spread through clang
> that tries to answer the same two question:
> 1. i'm a caller, if i call this function, might it throw?
> 1. i'm a callee, what should i do with exceptions that try to unwind out of 
> me?
> 
> I don't know how to improve that, and i don't think just moving this into 
> FunctionDecl would help.
> 
`FunctionDecl::WontThrow` and `FunctionDecl::ShouldUnwind` ? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 479413.
lebedev.ri marked 5 inline comments as done.
lebedev.ri added a comment.

@erichkeane thank you for taking a look!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

Files:
  clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/Builtins.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/function-attributes.c
  clang/test/CodeGen/struct-passing.c
  clang/test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp
  clang/test/CodeGenCXX/exception-escape-RAII-codegen.cpp
  clang/test/CodeGenCXX/exception-escape-codegen.cpp
  clang/test/CodeGenCXX/pr58798.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
@@ -9,12 +9,12 @@
   int i;
   ~B_ShouldDiag() noexcept(true) {} //no disg, no throw stmt
 };
-struct R_ShouldDiag : A_ShouldDiag {
+struct R_ShouldDiag_NoThrow : A_ShouldDiag {
   B_ShouldDiag b;
-  ~R_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
+  ~R_ShouldDiag_NoThrow() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
 throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
-  __attribute__((nothrow)) R_ShouldDiag() {// expected-note {{function declared non-throwing here}}
+  __attribute__((nothrow)) R_ShouldDiag_NoThrow() {// expected-note {{function declared non-throwing here}}
 throw 1;// expected-warning {{has a non-throwing exception specification but}}
   }
   void __attribute__((nothrow)) SomeThrow() {// expected-note {{function declared non-throwing here}}
@@ -24,6 +24,18 @@
throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
+struct R_ShouldDiag_NoUnwind : A_ShouldDiag {
+  B_ShouldDiag b;
+  ~R_ShouldDiag_NoUnwind() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
+throw 1; // expected-warning {{has a non-throwing exception specification but}}
+  }
+  __attribute__((nounwind)) R_ShouldDiag_NoUnwind() {// expected-note {{function declared non-throwing here}}
+throw 1;// expected-warning {{has a non-throwing exception specification but}}
+  }
+  void __attribute__((nounwind)) SomeThrow() {// expected-note {{function declared non-throwing here}}
+   throw 1; // expected-warning {{has a non-throwing exception specification but}}
+  }
+};
 
 struct M_ShouldNotDiag {
   B_ShouldDiag b;
@@ -240,7 +252,7 @@
   }
 }
 // As seen in p34973, this should not throw the warning.  If there is an active
-// exception, catch(...) catches everything. 
+// exception, catch(...) catches everything.
 void o_ShouldNotDiag() noexcept {
   try {
 throw;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -115,6 +115,7 @@
 // CHECK-NEXT: NoStackProtector (SubjectMatchRule_function)
 // CHECK-NEXT: NoThreadSafetyAnalysis (SubjectMatchRule_function)
 // CHECK-NEXT: NoThrow (SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: NoUnwind (SubjectMatchRule_function)
 // CHECK-NEXT: NoUwtable (SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: NotTailCalled (SubjectMatchRule_function)
 // CHECK-NEXT: OSConsumed (SubjectMatchRule_variable_is_parameter)
Index: clang/test/CodeGenCXX/pr58798.cpp
===
--- clang/test/CodeGenCXX/pr58798.cpp
+++ /dev/null
@@ -1,186 +0,0 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --check-attributes
-// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu -fexceptions -fcxx-exceptions | FileCheck %s
-
-// CHECK: Function Attrs: mustprogress noinline nounwind optnone willreturn memory(read)
-// CHECK-LABEL: define {{[^@]+}}@_Z54early_caller_of_callee_with_clang_attr_with_clang_attri
-// CHECK-SAME: (i32 noundef [[A:%.*]]) #[[ATTR0:[0-9]+]] {
-// 

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:841
   operator.
-- ``clang_Cursor_getNumTemplateArguments``, 
``clang_Cursor_getTemplateArgumentKind``, 
-  ``clang_Cursor_getTemplateArgumentType``, 
``clang_Cursor_getTemplateArgumentValue`` and 
+- ``clang_Cursor_getNumTemplateArguments``, 
``clang_Cursor_getTemplateArgumentKind``,
+  ``clang_Cursor_getTemplateArgumentType``, 
``clang_Cursor_getTemplateArgumentValue`` and

erichkeane wrote:
> unrelated changes here?
Having whitespaces before newline is bad :/
This is editor doing so on file save,
and my git complains otherwise.



Comment at: clang/include/clang/Basic/AttrDocs.td:560
+
+A function declared with ``__attribute__((const))`` attribute can not have
+infinite loops (i.e. they must either terminate or return to the caller,

erichkeane wrote:
> Similar changes to above.  This attribute is more of an 'assertion' by the 
> developer that they promise they don't do these things (and we will UB 
> otherwise), so I think they need to be written from that perspective.
> 
> I might suggest making an attempt to reword these docs.
> 
> ALSO, since these attributes can be spelled `clang::pure` and `clang::const` 
> (IIRC?), I'd suggest we make the documents spelling-agnostic other than in 
> direct code examples.
> I might suggest making an attempt to reword these docs.

This is my best attempt :)
If you can propose a better wording,
please feel free to do so.



Comment at: clang/lib/Analysis/CFG.cpp:2725
   NoReturn = true;
-if (FD->hasAttr())
+if (FD->hasAttr() || FD->hasAttr())
   AddEHEdge = false;

erichkeane wrote:
> I find myself thinking we should probably have a function in FunctionDecl 
> that tests for the various states of the function, rather than keep checking 
> the attribute's presence.  
Yes, we have a huge spaghetti code spread through clang
that tries to answer the same two question:
1. i'm a caller, if i call this function, might it throw?
1. i'm a callee, what should i do with exceptions that try to unwind out of me?

I don't know how to improve that, and i don't think just moving this into 
FunctionDecl would help.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:841
   operator.
-- ``clang_Cursor_getNumTemplateArguments``, 
``clang_Cursor_getTemplateArgumentKind``, 
-  ``clang_Cursor_getTemplateArgumentType``, 
``clang_Cursor_getTemplateArgumentValue`` and 
+- ``clang_Cursor_getNumTemplateArguments``, 
``clang_Cursor_getTemplateArgumentKind``,
+  ``clang_Cursor_getTemplateArgumentType``, 
``clang_Cursor_getTemplateArgumentValue`` and

unrelated changes here?



Comment at: clang/include/clang/Basic/AttrDocs.td:541
+  let Content = [{
+A function declared with ``__attribute__((pure))`` attribute can not have
+infinite loops (i.e. they must either terminate or return to the caller,





Comment at: clang/include/clang/Basic/AttrDocs.td:544
+be it either via a normal return, or by throwing an exception and unwinding
+or exibiting UB), and can not cause any observable side-effects other than
+returning a value. They may read memory, but can not modify memory in a way





Comment at: clang/include/clang/Basic/AttrDocs.td:560
+
+A function declared with ``__attribute__((const))`` attribute can not have
+infinite loops (i.e. they must either terminate or return to the caller,

Similar changes to above.  This attribute is more of an 'assertion' by the 
developer that they promise they don't do these things (and we will UB 
otherwise), so I think they need to be written from that perspective.

I might suggest making an attempt to reword these docs.

ALSO, since these attributes can be spelled `clang::pure` and `clang::const` 
(IIRC?), I'd suggest we make the documents spelling-agnostic other than in 
direct code examples.



Comment at: clang/lib/Analysis/CFG.cpp:2725
   NoReturn = true;
-if (FD->hasAttr())
+if (FD->hasAttr() || FD->hasAttr())
   AddEHEdge = false;

I find myself thinking we should probably have a function in FunctionDecl that 
tests for the various states of the function, rather than keep checking the 
attribute's presence.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: aaron.ballman, erichkeane, rjmccall.
lebedev.ri added a project: LLVM.
Herald added subscribers: carlosgalvezp, jdoerfert.
Herald added a reviewer: NoQ.
Herald added a reviewer: njames93.
Herald added a project: All.
lebedev.ri requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

This is an implementation of the following RFC:
https://discourse.llvm.org/t/rfc-better-ux-for-clangs-unwind-affecting-attributes/66890

In C++, there are 3 possible behaviors when the
exception escapes out an function that can not unwind:

1. exception propagates into function's caller
2. defined behavior of immediate program termination
3. the wild UB case, behavior is undefined.

Let's look at obvious examples:

1. exception propagates into caller, is caught there, and all is good: 
https://godbolt.org/z/MbTW9rofn
2. exception can not exit `noexcept` function, program is terminated: 
https://godbolt.org/z/ffeaPz1dK

Now, the third case, the wild UB case, is the most interesting one.
There are 3 clang/gcc attributes that are relevant here, let's look at them:

1. `__attribute__((pure))`: https://godbolt.org/z/PY3KrETb7, there the fun 
begins. In clang, we get UB, in gcc we get "exception propagates into 
function's caller"
2. `__attribute__((const))`: https://godbolt.org/z/ozxoW16e9, same as 
`__attribute__((pure))`
3. `__attribute__((nothrow))`: https://godbolt.org/z/YMf4sTcfa, the behavior is 
consistently defined as immediate program termination. I do not understand why 
it was defined as such, in the sense of how is that different from plain 
`noexcept`, but at least we are consistent.

Now, there are 3 problems:

1. Our modelling of `__attribute__((const))`/`__attribute__((pure))` differs 
from that of GCC, we add UB.
2. We can not ask for `__attribute__((const))`/`__attribute__((pure))` 
behavior, without it acting as exception barrier.
3. We can not separately ask for the exception propagation to be UB. This would 
be a handy optimization tool, especially given how brittle our IRGen for the 
case 2. (program termination) is.

Therefore, this patch does two things:

1. Match GCC's implementation-defined behavior on 
`__attribute__((pure))`/`__attribute__((const))`
  - they should not cause UB on exception escape, nor should they cause 
immediate program termination, exceptions should be free to escape into their 
caller.
2. Introduce `__attribute__((nounwind))`, which would be lowered into LLVM IR's 
`nounwind` attribute, and if an exception escapes out of an function marked 
with such an attribute, wild UB happens.

Please note, while currently such an UB is indeed not sanitized,
i have a patch in progress to handle it:
https://reviews.llvm.org/D137381,
so we would not be introducing something that is impossible to deal with.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138958

Files:
  clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/Builtins.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/function-attributes.c
  clang/test/CodeGen/struct-passing.c
  clang/test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp
  clang/test/CodeGenCXX/exception-escape-RAII-codegen.cpp
  clang/test/CodeGenCXX/exception-escape-codegen.cpp
  clang/test/CodeGenCXX/pr58798.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-func.cpp
@@ -9,12 +9,12 @@
   int i;
   ~B_ShouldDiag() noexcept(true) {} //no disg, no throw stmt
 };
-struct R_ShouldDiag : A_ShouldDiag {
+struct R_ShouldDiag_NoThrow : A_ShouldDiag {
   B_ShouldDiag b;
-  ~R_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
+  ~R_ShouldDiag_NoThrow() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
 throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
-  __attribute__((nothrow)) R_ShouldDiag() {// expected-note {{function declared non-throwing here}}
+  __attribute__((nothrow)) R_ShouldDiag_NoThrow() {// expected-note {{function declared non-throwing here}}
 throw 1;//