[PATCH] D142123: [clang-tidy] Add check to suggest use of #pragma once

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D142123#4066460 , @aaron.ballman 
wrote:

> In D142123#4066447 , @lebedev.ri 
> wrote:
>
>> In D142123#4066351 , @njames93 
>> wrote:
>>
>>> Given the fact its non-standard, it has caveats and peoples eagerness to 
>>> blindly enable all checks (or all checks from a module). 
>>> I feel this check would likely cause more harm than good.
>>
>> Shall we remove abseil checks?
>> Shall we remove libc++-specific checks?
>> Shall we remove webkit checks?
>> Shall we remove backwards-compatibility checks?
>>
>> Nothing is ever useful for everyone. Much like `-Weverything`,
>> enabling all checks comes with an explcit caveat that
>> one needs to disable the checks that are not applicable for the codebase.
>>
>> +1 to having this check.
>
> We're not asking for it to be useful to everyone; we are asking for 
> justification for the current proposed form because there are problems with 
> what's proposed. We're trying to figure out what the correct approach is 
> (fwiw, I'd be opposed to what's proposed as-is but would be fine with a 
> tweaked proposal).

Thanks!


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

https://reviews.llvm.org/D142123

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

[PATCH] D142123: [clang-tidy] Add check to suggest use of #pragma once

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D142123#4066351 , @njames93 wrote:

> Given the fact its non-standard, it has caveats and peoples eagerness to 
> blindly enable all checks (or all checks from a module). 
> I feel this check would likely cause more harm than good.

Shall we remove abseil checks?
Shall we remove libc++-specific checks?
Shall we remove webkit checks?
Shall we remove backwards-compatibility checks?

Nothing is ever useful for everyone. Much like `-Weverything`,
enabling all checks comes with an explcit caveat that
one needs to disable the checks that are not applicable for the codebase.

+1 to having this check.


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

https://reviews.llvm.org/D142123

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

[PATCH] D136651: [Clang] Give Clang the ability to use a shared stat cache

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D136651#4063818 , @friss wrote:

> In D136651#4063632 , @lebedev.ri 
> wrote:
>
>> In D136651#4063597 , @friss wrote:
>>
>>> In D136651#4060071 , @lebedev.ri 
>>> wrote:
>>>
 Docs still missing.
>>>
>>> Sorry @lebedev.ri I missed your earlier comment about this. What format of 
>>> doc would you like to see? A more elaborate comment in the tool's source or 
>>> something else?
>>
>> Right now it's impossible to discover that tool unless you already know it 
>> exists (or it's mentioned in some xcode doc, i guess)
>> It should have it's own description in it's `--help`, and ideally an `.rst` 
>> documentation page in `docs`.
>
> I have no issue adding a more detailed help text. The documentation page 
> seems somewhat overkill, but I guess I could be convinced otherwise. You need 
> a fairly special build environment for this to make a difference and I'm not 
> sure it's broadly applicable. You also need good build system integration to 
> make it practical.

... and all of that knowledge seems to be missing from the current docs, which 
is why i'm asking in the first place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136651

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

[PATCH] D136651: [Clang] Give Clang the ability to use a shared stat cache

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D136651#4063597 , @friss wrote:

> In D136651#4060071 , @lebedev.ri 
> wrote:
>
>> Docs still missing.
>
> Sorry @lebedev.ri I missed your earlier comment about this. What format of 
> doc would you like to see? A more elaborate comment in the tool's source or 
> something else?

Right now it's impossible to discover that tool unless you already know it 
exists (or it's mentioned in some xcode doc, i guess)
It should have it's own description in it's `--help`, and ideally an `.rst` 
documentation page in `docs`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136651

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

[PATCH] D142033: [OpenCL] Always add nounwind attribute for OpenCL

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142033

___
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/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

[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

[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();
+
+// CHECK-NO-EXCEP

[PATCH] D136651: [Clang] Give Clang the ability to use a shared stat cache

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Docs still missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136651

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

[PATCH] D141899: [IR][X86] Remove X86AMX type in LLVM IR instead of target extension

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D141899#4058173 , @LuoYuanke wrote:

> @zixuan-wu, changing x86_amx would break our internal code. May I know the 
> motivation to change the type?

I just want to point out that generally "causes [too much] churn downstream"
is not relevant concern upstream, as downstreams are considered to be on their 
own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141899

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

[PATCH] D141581: [clang] Make clangBasic and clangDriver depend on LLVMTargetParser.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Why is it not sufficient to link to `RISCVTargetParserTableGen`, but is 
sufficient to link to `LLVMTargetParser`?
Does `RISCVTargetParserTableGen` itself not link to `LLVMTargetParser`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141581

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

@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

[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] D105498: [clang] Remove assumption about SourceLocation alignment.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105498

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

[PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.

What's the status here? Should this be abandoned?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[PATCH] D43153: [clang] Implement P0692 "Access Checking on Specializations"

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rC Clang

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

https://reviews.llvm.org/D43153

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

[PATCH] D134588: [clang-tidy] Fix bugprone-exception-escape warn on noexcept calls

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134588

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

[PATCH] D130209: [clang-tidy][NFC] Update tests to specify CheckOptions using new syntax

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130209

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

[PATCH] D97204: [RFC] Clang 64-bit source locations

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added subscribers: steakhal, StephenFan.
Herald added a reviewer: NoQ.
Herald added a reviewer: njames93.

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97204

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

[PATCH] D105494: [clang] Introduce a union inside ProgramPoint.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.
Herald added a reviewer: NoQ.

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105494

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

[PATCH] D121629: clang: also check alloc_alignment claims in return

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121629

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

[PATCH] D115118: [clang-tidy] Assume that `noexcept` functions won't throw anything in `bugprone-exception-escape` check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.
Herald added a reviewer: njames93.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115118

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

[PATCH] D97361: [clang-tidy] Add readability-redundant-using check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added subscribers: carlosgalvezp, StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97361

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

[PATCH] D105495: [clang] Make negative getLocWithOffset widening-safe.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added subscribers: steakhal, StephenFan.
Herald added a reviewer: NoQ.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105495

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

[PATCH] D105497: [clang] Serialize source locations as 64-bit in PCH.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105497

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

[PATCH] D90448: [clang] Add type check for explicit instantiation of static data members

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90448

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

[PATCH] D79113: Revert "Remove false positive in AvoidNonConstGlobalVariables."

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added subscribers: carlosgalvezp, StephenFan.
Herald added a reviewer: njames93.
Herald added projects: clang-tools-extra, All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79113

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

[PATCH] D85097: [Sema] add warning for comparisons like 'x<=y<=z'

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097

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

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79636

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

[PATCH] D78028: move shebangs from python2 to python3

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added subscribers: cfe-commits, llvm-commits, Moerafaat, zero9178, 
Enna1, bzcheeseman, sdasgup3, carlosgalvezp, wenzhicui, wrengr, cota, 
StephenFan, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, 
stephenneuendorffer, aartbik, arichardson.
Herald added a reviewer: NoQ.
Herald added a reviewer: njames93.
Herald added projects: MLIR, LLVM, clang-tools-extra, All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


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

https://reviews.llvm.org/D78028

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

[PATCH] D125272: [clang] Add -fcheck-new support

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125272

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

[PATCH] D135658: demangle OptFunction trace names

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: StephenFan.

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135658

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

[PATCH] D119271: clang: emit allocalign to LLVM for alloc_align attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119271

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

[PATCH] D115848: tidy-llvm

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added a subscriber: StephenFan.
Herald added a reviewer: njames93.
Herald added a project: All.

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115848

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

[PATCH] D96223: [clang-tidy] Simplify static assert check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.
Herald added subscribers: carlosgalvezp, StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96223

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

[PATCH] D99565: [X86] Support replacing aligned vector moves with unaligned moves when avx is enabled.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99565

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

[PATCH] D92001: [ubsan] Fix crash on __builtin_assume_aligned

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


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

https://reviews.llvm.org/D92001

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

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added subscribers: carlosgalvezp, StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229

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

[PATCH] D72239: [clang-tidy] new opencl recursion not supported check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added subscribers: Naghasan, StephenFan.
Herald added a reviewer: njames93.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D72239

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

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added subscribers: carlosgalvezp, StephenFan.
Herald added a reviewer: njames93.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71707

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

[PATCH] D63089: [clang] Warn on implicit boolean casts in more contexts (PR34180)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63089

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

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added subscribers: carlosgalvezp, StephenFan.
Herald added a reviewer: njames93.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


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

https://reviews.llvm.org/D59859

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

[PATCH] D58811: [clang] Fix misuses of char width to char align

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58811

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

[PATCH] D63929: [clang-tidy] - Introduce abseil-prefixed-thread-annotations check.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.
Herald added subscribers: carlosgalvezp, StephenFan, mgehre.
Herald added a reviewer: njames93.
Herald added projects: clang-tools-extra, All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D63929

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

[PATCH] D50852: [clang-tidy] abseil-auto-make-unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision now requires review to proceed.
Herald added subscribers: StephenFan, mgehre.
Herald added a reviewer: njames93.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D50852

___
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] D141561: [clang] True `noexcept` (`-fstrict-noexcept` language dialect)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: aaron.ballman, rjmccall, efriedma, jcranmer, 
jyknight, rsmith.
lebedev.ri added a project: LLVM.
Herald added subscribers: StephenFan, dschuff.
Herald added a project: All.
lebedev.ri requested review of this revision.
Herald added subscribers: MaskRay, aheejin.
Herald added a project: clang.

This tentatively implements the following RFC:
https://discourse.llvm.org/t/rfc-clang-true-noexcept-aka-defaults-are-often-wrong-hardcoded-defaults-are-always-wrong/67629

The idea is that when the opt-in is specified,
we turn all EH Terminate scopes into EH UB scopes,
and don't emit any `std::terminate()` calls,
which is the user-facing change.

This probably needs better docs,
and might be missing some pieces.

We might be able to do more to omit more destructor calls.

This is missing UBSan bits, those will be in D137381 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141561

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCleanup.cpp
  clang/lib/CodeGen/CGCleanup.h
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGenCXX/exception-escape-as-ub-cleanups.cpp
  clang/test/CodeGenCXX/exception-escape.cpp

Index: clang/test/CodeGenCXX/exception-escape.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/exception-escape.cpp
@@ -0,0 +1,632 @@
+// 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 -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,CHECK-NORMAL-NOEXCEPT
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu -fcxx-exceptions -fexceptions -fstrict-noexcept | FileCheck %s --check-prefixes=CHECK-ALL,CHECK-EXCEPTIONS,CHECK-STRICT-NOEXCEPT
+
+void will_throw(int line = __builtin_LINE());
+void might_throw(int line = __builtin_LINE());
+void will_not_throw(int line = __builtin_LINE()) noexcept;
+
+//.
+// CHECK-ALL: @_ZTIi = external constant ptr
+//.
+// CHECK-NO-EXCEPTIONS: Function Attrs: mustprogress noinline nounwind optnone
+// CHECK-NO-EXCEPTIONS-LABEL: define {{[^@]+}}@_Z45exception_escape_is_program_termination_or_ubi
+// CHECK-NO-EXCEPTIONS-SAME: (i32 noundef [[X:%.*]]) #[[ATTR0:[0-9]+]] {
+// CHECK-NO-EXCEPTIONS-NEXT:  entry:
+// CHECK-NO-EXCEPTIONS-NEXT:[[X_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NO-EXCEPTIONS-NEXT:store i32 [[X]], ptr [[X_ADDR]], align 4
+// CHECK-NO-EXCEPTIONS-NEXT:[[TMP0:%.*]] = load i32, ptr [[X_ADDR]], align 4
+// CHECK-NO-EXCEPTIONS-NEXT:[[CMP:%.*]] = icmp eq i32 [[TMP0]], 2
+// CHECK-NO-EXCEPTIONS-NEXT:br i1 [[CMP]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
+// CHECK-NO-EXCEPTIONS:   if.then:
+// CHECK-NO-EXCEPTIONS-NEXT:call void @_Z10will_throwi(i32 noundef 100)
+// CHECK-NO-EXCEPTIONS-NEXT:br label [[IF_END]]
+// CHECK-NO-EXCEPTIONS:   if.end:
+// CHECK-NO-EXCEPTIONS-NEXT:call void @_Z14will_not_throwi(i32 noundef 200) #[[ATTR3:[0-9]+]]
+// CHECK-NO-EXCEPTIONS-NEXT:[[TMP1:%.*]] = load i32, ptr [[X_ADDR]], align 4
+// CHECK-NO-EXCEPTIONS-NEXT:[[CMP1:%.*]] = icmp eq i32 [[TMP1]], 3
+// CHECK-NO-EXCEPTIONS-NEXT:br i1 [[CMP1]], label [[IF_THEN2:%.*]], label [[IF_END3:%.*]]
+// CHECK-NO-EXCEPTIONS:   if.then2:
+// CHECK-NO-EXCEPTIONS-NEXT:call void @_Z10will_throwi(i32 noundef 300)
+// CHECK-NO-EXCEPTIONS-NEXT:br label [[IF_END3]]
+// CHECK-NO-EXCEPTIONS:   if.end3:
+// CHECK-NO-EXCEPTIONS-NEXT:[[TMP2:%.*]] = load i32, ptr [[X_ADDR]], align 4
+// CHECK-NO-EXCEPTIONS-NEXT:[[CMP4:%.*]] = icmp eq i32 [[TMP2]], 4
+// CHECK-NO-EXCEPTIONS-NEXT:br i1 [[CMP4]], label [[IF_THEN5:%.*]], label [[IF_END6:%.*]]
+// CHECK-NO-EXCEPTIONS:   if.then5:
+// CHECK-NO-EXCEPTIONS-NEXT:call void @_Z14will_not_throwi(i32 noundef 400) #[[ATTR3]]
+// CHECK-NO-EXCEPTIONS-NEXT:call void @_Z10will_throwi(i32 noundef 500)
+// CHECK-NO-EXCEPTIONS-NEXT:br label [[IF_END6]]
+// CHECK-NO-EXCEPTIONS:   if.end6:
+// CHECK-NO-EXCEPTIONS-NEXT:[[TMP3:%.*]] = load i32, ptr [[X_ADDR]], align 4
+// CHECK-NO-EXCEPTIONS-NEXT:[[CMP7:%.*]] = icmp eq i32 [[TMP3]], 5
+// CHECK-NO-EXCEPTIONS-NEXT:br i1 [[CMP7]], label [[IF_THEN8:%.*]], label [[IF_END9:%.*]]
+// CHECK-NO-EXCEPTIONS:   if.then8:
+// CHECK-NO-EXCEPTIONS-NEXT:call void @_Z10will_throwi(i32 noundef 900)
+// CHEC

[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:s

[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

[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

[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}}
+}
+
+[[cl

[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] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D137381#4038716 , @MaskRay wrote:

> I am unfamiliar with Clang codegen so I am just commenting about what a user 
> may feel about this feature.
>
> `compiler-rt/test/ubsan/TestCases/Misc/exception-escape.cpp` cannot 
> demonstrate the value of the new UBSan check `-fsanitize=exception-escape`, 
> as the executable will fail without the option (Clang emits `call` instead of 
> `invoke` for a nounwind function call, and there is no LSDA information, 
> libgcc/libunwind will call terminate; it's trivial to get more diagnostic 
> with a SIBABRT signal handler).
> To demonstrate the value, `footgun` and its caller need to be in different 
> TUs and the caller does not know `footgun` is nounwind.
> In such a setup, I think people likely care more about 
> `-fno-exceptions`/`-fexceptions` exception propagation instead of 
> `__attribute__((pure))` and find `-fsanitize=exception-escape` not do what it 
> may advertise.
>
> The more common `-fno-exceptions`/`-fexceptions` exception propagation 
> dilemma can be detected today with `-fno-asynchronous-unwind-tables`.

Thank you for taking a look. I'm afraid this review comment is not based on 
reality, so i'm forced to ignore it.
The whole point of the clang change is to *NOT* lower such calls that we "know" 
will not unwind to a `call`, but into an `invoke`, with sanitization in the 
`landingpad`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

___
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: Func

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

___
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] D136651: [Clang] Give Clang the ability to use a shared stat cache

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

As someone who has no idea what this does, this seems to be missing docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136651

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

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.

In D140467#4010675 , @pengfei wrote:

>> If it exists it must be tested.
>> Every piece of code generation needs to be tested.
>
> Let me show you the number:
>
>   $ grep -rho '__builtin_ia32\w\+' clang/test/CodeGen | sort|uniq |wc -l
>   337
>   $ grep -rho '_mm512_\w\+' clang/test/CodeGen | sort|uniq |wc -l
>   2304
>
> Note most of `__builtin_ia32` tests are negative ones and `_mm512_` is less 
> than 1/3 of the total x86 intrinsics. Two orders of magnitude!
>
> I took a look at the tests. The positive tests come from 
> clang/test/CodeGen/builtins-x86.c. Other targets don't have a big test either
> `wc -l clang/test/CodeGen/builtins-*`
>
>> These are not testing use of these builtins correctly
>
> As I have explained, users are not suggested to use these builtins given we 
> have provided the more stable, well documented corresponding intrinsics. The 
> only case user has to use it is the intrinsic is missing. In that case, we do 
> need test case for it.
>
> In a word, as titled, it is NFC from the perspective of intrinsics. So I 
> think we don't need test cases for them.
>
>> This test amounts to a builtin header test for immintrin.h. This should move 
>> to clang/test/Headers and replaced with a builtin test directly checking the 
>> builtin handling
>
> Not get your point. But currently no builtin tests under 
> `clang/test/Headers/`.

I agree with @arsenm. At least for clang irgen, we should have good test 
coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140467

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

[PATCH] D140281: [X86] Rename CMPCCXADD intrinsics.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D140281#4006650 , @FreddyYe wrote:

> In D140281#4004707 , @lebedev.ri 
> wrote:
>
>> This patch says what it does, but not why it does what it does.
>
> Sorry for not mention. This patch is mainly to align with other intrinsics to 
> follow single leading "_" style. Gcc and intrinsic guide website will also 
> apply this change.

(please adjust the description to say as much)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140281

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

[PATCH] D140281: [X86] Rename CMPCCXADD intrinsics.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

This patch says what it does, but not why it does what it does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140281

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

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[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/D137381/new/

https://reviews.llvm.org/D137381

___
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] D139717: Revert "[Driver] Remove Joined -X"

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

This is missing a test, like the original commit mentioned.
Why can you not just use the the split variant, `-X clang ...`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139717

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

[PATCH] D139647: [opt] Disincentivize new tests from using old pass syntax

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

@aeubanks Thank you for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139647

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

[PATCH] D139006: [UpdateTestChecks] Match define for labels

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a subscriber: nikic.
lebedev.ri added a comment.

In D139006#3981326 , @sebastian-ne 
wrote:

> I guess this is fine to merge. I’ll leave it for a day in case someone has 
> more comments.

I guess, we could be pedantic and post an RFC to disscuss.
But this really is a bug, and the alternative solutions would only hide it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139006

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

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

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



Comment at: clang/test/CodeGen/windows-seh-EHa-CppCondiTemps.cpp:3
 
+// FIXME: this check appears to be miscompiled?
+// XFAIL: *

tentzen wrote:
> lebedev.ri wrote:
> > efriedma wrote:
> > > lebedev.ri wrote:
> > > > tentzen wrote:
> > > > > lebedev.ri wrote:
> > > > > > This test broke once we always started adding (outermost) UB scope 
> > > > > > for nounwind functions.
> > > > > > I don't quite get what is going wrong. It could be a bug in SEH 
> > > > > > handling.
> > > > > > Can someone who has some idea about that code take a look and 
> > > > > > suggest a fix?
> > > > > > @tentzen ?
> > > > > By definition, non-unwind function I think is for Synchronous EH. So 
> > > > > this Sanitizer check should exclude Asynchronous EH functions, those 
> > > > > with option -fasync-exceptions.
> > > > >  
> > > > I do not understand.
> > > > If the function can unwind, then why is it marked `nounwind`?
> > > > This kind of thing is exactly what i was afraid of with those SEH 
> > > > patches.
> > > clang should not be marking functions "nounwind" in -fasync-exceptions 
> > > mode; if it is, I'd consider that a bug.  (I assume someone just forgot 
> > > to add a check to some code that adds nounwind.)
> > My thoughts precisely. @tentzen please fix :)
> This is copied from LLVM reference manual:
> 
> nounwind
> This function attribute indicates that the function never raises an 
> exception. If the function does raise an exception, its runtime behavior is 
> undefined. 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.
> 
> So I think nounwind only implies no synchronous/software unwind, not HW traps 
> etc.
Ok, good point.
But i'm still not quite sure why the test is getting miscompiled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

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

[PATCH] D139197: [clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D139197#3977370 , @carlosgalvezp 
wrote:

> @lebedev.ri If you are happy with the patch, would you mind approving it? Or 
> do you have additional comments that should be addressed? The patch does not 
> need to be approved by the Code Owner.

No further comments. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139197

___
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] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

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



Comment at: clang/test/CodeGen/windows-seh-EHa-CppCondiTemps.cpp:3
 
+// FIXME: this check appears to be miscompiled?
+// XFAIL: *

efriedma wrote:
> lebedev.ri wrote:
> > tentzen wrote:
> > > lebedev.ri wrote:
> > > > This test broke once we always started adding (outermost) UB scope for 
> > > > nounwind functions.
> > > > I don't quite get what is going wrong. It could be a bug in SEH 
> > > > handling.
> > > > Can someone who has some idea about that code take a look and suggest a 
> > > > fix?
> > > > @tentzen ?
> > > By definition, non-unwind function I think is for Synchronous EH. So this 
> > > Sanitizer check should exclude Asynchronous EH functions, those with 
> > > option -fasync-exceptions.
> > >  
> > I do not understand.
> > If the function can unwind, then why is it marked `nounwind`?
> > This kind of thing is exactly what i was afraid of with those SEH patches.
> clang should not be marking functions "nounwind" in -fasync-exceptions mode; 
> if it is, I'd consider that a bug.  (I assume someone just forgot to add a 
> check to some code that adds nounwind.)
My thoughts precisely. @tentzen please fix :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

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

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/test/CodeGen/windows-seh-EHa-CppCondiTemps.cpp:3
 
+// FIXME: this check appears to be miscompiled?
+// XFAIL: *

tentzen wrote:
> lebedev.ri wrote:
> > This test broke once we always started adding (outermost) UB scope for 
> > nounwind functions.
> > I don't quite get what is going wrong. It could be a bug in SEH handling.
> > Can someone who has some idea about that code take a look and suggest a fix?
> > @tentzen ?
> By definition, non-unwind function I think is for Synchronous EH. So this 
> Sanitizer check should exclude Asynchronous EH functions, those with option 
> -fasync-exceptions.
>  
I do not understand.
If the function can unwind, then why is it marked `nounwind`?
This kind of thing is exactly what i was afraid of with those SEH patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

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

[PATCH] D139267: Supporting tbaa.struct metadata generation for bitfields

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added reviewers: wsmoses, jdoerfert.
lebedev.ri added a comment.

Certainly needs tests.
I'm not sure if anyone actually knows this area of LLVM, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139267

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

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a subscriber: tentzen.
lebedev.ri added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:5402-5411
+if (SanOpts.has(SanitizerKind::ExceptionEscape) &&
+ExceptionEscapeUBLastInvokeSrcLoc) {
+  llvm::Constant *CheckSourceLocation = EmitCheckSourceLocation(Loc);
+  Builder.CreateStore(
+  CheckSourceLocation,
+  Address(ExceptionEscapeUBLastInvokeSrcLoc,
+  CheckSourceLocation->getType(),

lebedev.ri wrote:
> lebedev.ri wrote:
> > rjmccall wrote:
> > > lebedev.ri wrote:
> > > > lebedev.ri wrote:
> > > > > @rjmccall
> > > > > So there are two issues:
> > > > > 1. `getInvokeDest()` isn't necessarily called just before creating an 
> > > > > `invoke`, see e.g. `-EHa` handling in 
> > > > > `CodeGenFunction::PopCleanupBlock()`
> > > > > 2. Even if we ignore that, we need to do this for *every* `invoke`, 
> > > > > not just those going to the our UB landing pad, consider: 
> > > > > https://godbolt.org/z/qTeKor41a <- the invoke leads to a normal 
> > > > > landing pad, yet we immediately rethrow the just-caught exception, 
> > > > > and now end up in the UB landing pad.
> > > > > 
> > > > > So i'm not really seeing an alternative path here?
> > > > @rjmccall do you agree with my reasoning here?
> > > > Perhaps there is some other solution that i'm not seeing,
> > > > other than not having the source location?
> > > I think there are four interesting questions posed by your example.
> > > 
> > > The first is whether we should make an effort to report the original 
> > > source location for the call to `maybe_throws` instead of the rethrow 
> > > location.  To me, the answer is no, and the main reason is that the user 
> > > is explicitly acknowledging the possibility of an exception and is 
> > > (probably) trying to handle it.  That means that the proximate source of 
> > > programmer error is now that their attempt to handle it failed, not that 
> > > an exception was raised in the first place.  That might seem weird for 
> > > your example specifically, 
> > > 
> > > The second is what source location we should report for invokes that 
> > > don't have an obvious source location.  The answer is that I think we 
> > > should always try to have a source location, even if it might be an 
> > > imperfect one.  Synthesized code almost always has a source location that 
> > > triggered the synthesis.  Cleanups are typically associated with some 
> > > variable or temporary that itself should have a source location.  Even 
> > > things like `__cxa_end_catch` are associated with the `catch` clause.  
> > > Hopefully UBSan  has a flexible enough schema in how source locations are 
> > > expressed to it for us to communicate these things down to the runtime, 
> > > like "synthesized code at Foo.cpp:18" or "cleanup for variable at 
> > > Bar.cpp:107".
> > > 
> > > The third is the more general question of whether we need to store a 
> > > source location before every `invoke`.  To ensure that this variable is 
> > > meaningfully initialized, we need to store a source location before every 
> > > invoke that can lead to a UB scope.  But the variable is stateful, so 
> > > it's equally important that we not emit stores that might overwrite the 
> > > source location that we actually want to report.  For example, we want 
> > > the source location reported in your example to be the source location 
> > > for the `throw;`, not something associated with the `__cxa_end_catch` 
> > > cleanup.  This basically boils down to not doing stores within a landing 
> > > pad before it enters a `catch` handler.  Fortunately, landing pad code 
> > > like that should always be in a terminate scope, so I think you can just 
> > > look at the current EH stack and see whether the landing pad can possibly 
> > > reach a UB scope.  (You might need some save-and-restore logic around 
> > > `@finally` blocks in ObjC.)
> > > 
> > > The final question is whether we need to emit cleanups on paths that lead 
> > > to UB scopes at all.  In the simplest cases, I think we don't.  The 
> > > standard says that it's implementation-specified whether the stack is 
> > > unwound on an exception path that leads to termination.  That rule 
> > > doesn't technically apply here, but only because we're dealing with a 
> > > language extension that makes unwinding UB in the first place.  Since we 
> > > have room to choose the semantics, the standard gives us pretty strong 
> > > cover to be just as aggressive about skipping cleanups in contexts where 
> > > throwing is UB as it is in contexts where throwing terminates.  That, in 
> > > turn, means that the landing pad in these contexts will usually just be a 
> > > UB scope with no cleanups in it.  So even if we did need to worry about 
> > > cleanups overwriting the source location, it usually won't be possible 
> > > because we won't be running cleanups.  In fact, if there's a catch-all or 
> > > a ter

[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] D135658: demangle OptFunction trace names

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Old pass manager is dead. There is no point in making improvements to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135658

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

[PATCH] D139197: [clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

> This shouldn't be a blocker for this patch though, it's just removing 
> functionality that belongs in another check. As said, the check is 1 day old 
> so it's unlikely anyone is relying on it yet :)

SGTM in general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139197

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

[PATCH] D139197: Do not warn about redundant static in misc-use-anonymous-namespace

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I was commenting because we already have a similar diagnostic in clang-tidy 15:

  namespace {
static void foo();
static void bar(){}
void foo() {}
static int x;
  }

  $ clang-tidy-15  -checks=\*,-llvmlibc-* /tmp/test.cpp 
  Error while trying to load a compilation database:
  Could not auto-detect compilation database for file "/tmp/test.cpp"
  No compilation database found in /tmp or any parent directory
  fixed-compilation-database: Error while opening fixed database: No such file 
or directory
  json-compilation-database: Error while opening JSON database: No such file or 
directory
  Running without flags.
  5 warnings generated.
  /tmp/test.cpp:3:17: warning: 'bar' is a static definition in anonymous 
namespace; static is redundant here 
[readability-static-definition-in-anonymous-namespace]
  static void bar(){}
  ~~~ ^
  /tmp/test.cpp:5:16: warning: variable 'x' is non-const and globally 
accessible, consider making it const 
[cppcoreguidelines-avoid-non-const-global-variables]
  static int x;
 ^
  /tmp/test.cpp:5:16: warning: variable name 'x' is too short, expected at 
least 3 characters [readability-identifier-length]
  /tmp/test.cpp:5:16: warning: 'x' is a static definition in anonymous 
namespace; static is redundant here 
[readability-static-definition-in-anonymous-namespace]
  static int x;
  ~~~^
  /tmp/test.cpp:6:3: warning: anonymous namespace not terminated with a closing 
comment [llvm-namespace-comment]
}
^
  // namespace
  /tmp/test.cpp:1:13: note: anonymous namespace starts here
namespace {
  ^

that for some reason only fires on definitions, not declarations.
I thought this was changing that old check.
I think it may be good to not have duplicate coverage,
but do we need a new check given that there is one already?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139197

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

[PATCH] D139197: Do not warn about redundant static in misc-use-anonymous-namespace

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

This patch should not land before that one does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139197

___
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

[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]+]] {
-// CHECK-

[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] D139017: Changed the intentional spelling errors back to the original, made more slight grammar corrections

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139017

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

[PATCH] D139006: [UpdateTestChecks] Match define for labels

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D139006#3960466 , @mtrofin wrote:

> LGTM, but I'd wait for Johannes to review it, too (because of 
> e67f6477fd1ed29acbeddf8482c25d8db826912f 
> . I for 
> one don't quite follow the reasoning there wrt adding the `else`)

That commit explicitly links to https://reviews.llvm.org/D68819,
after reading that thread, it is obvious that 
rGe67f6477fd1ed29acbeddf8482c25d8db826912f 

was made prevent the churn we are about to expirience.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139006

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

[PATCH] D139006: [UpdateTestChecks] Match define for labels

[Roman Lebedev via Phabricator via cfe-commits]

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

LGTM.
I do think this is an important fix, not the least because i've also just hit 
that,
but i do want to explicitly call out that this will cause a massive test case 
churn,
so i do want a second reviewer here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139006

___
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;// exp

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:5402-5411
+if (SanOpts.has(SanitizerKind::ExceptionEscape) &&
+ExceptionEscapeUBLastInvokeSrcLoc) {
+  llvm::Constant *CheckSourceLocation = EmitCheckSourceLocation(Loc);
+  Builder.CreateStore(
+  CheckSourceLocation,
+  Address(ExceptionEscapeUBLastInvokeSrcLoc,
+  CheckSourceLocation->getType(),

lebedev.ri wrote:
> rjmccall wrote:
> > lebedev.ri wrote:
> > > lebedev.ri wrote:
> > > > @rjmccall
> > > > So there are two issues:
> > > > 1. `getInvokeDest()` isn't necessarily called just before creating an 
> > > > `invoke`, see e.g. `-EHa` handling in 
> > > > `CodeGenFunction::PopCleanupBlock()`
> > > > 2. Even if we ignore that, we need to do this for *every* `invoke`, not 
> > > > just those going to the our UB landing pad, consider: 
> > > > https://godbolt.org/z/qTeKor41a <- the invoke leads to a normal landing 
> > > > pad, yet we immediately rethrow the just-caught exception, and now end 
> > > > up in the UB landing pad.
> > > > 
> > > > So i'm not really seeing an alternative path here?
> > > @rjmccall do you agree with my reasoning here?
> > > Perhaps there is some other solution that i'm not seeing,
> > > other than not having the source location?
> > I think there are four interesting questions posed by your example.
> > 
> > The first is whether we should make an effort to report the original source 
> > location for the call to `maybe_throws` instead of the rethrow location.  
> > To me, the answer is no, and the main reason is that the user is explicitly 
> > acknowledging the possibility of an exception and is (probably) trying to 
> > handle it.  That means that the proximate source of programmer error is now 
> > that their attempt to handle it failed, not that an exception was raised in 
> > the first place.  That might seem weird for your example specifically, 
> > 
> > The second is what source location we should report for invokes that don't 
> > have an obvious source location.  The answer is that I think we should 
> > always try to have a source location, even if it might be an imperfect one. 
> >  Synthesized code almost always has a source location that triggered the 
> > synthesis.  Cleanups are typically associated with some variable or 
> > temporary that itself should have a source location.  Even things like 
> > `__cxa_end_catch` are associated with the `catch` clause.  Hopefully UBSan  
> > has a flexible enough schema in how source locations are expressed to it 
> > for us to communicate these things down to the runtime, like "synthesized 
> > code at Foo.cpp:18" or "cleanup for variable at Bar.cpp:107".
> > 
> > The third is the more general question of whether we need to store a source 
> > location before every `invoke`.  To ensure that this variable is 
> > meaningfully initialized, we need to store a source location before every 
> > invoke that can lead to a UB scope.  But the variable is stateful, so it's 
> > equally important that we not emit stores that might overwrite the source 
> > location that we actually want to report.  For example, we want the source 
> > location reported in your example to be the source location for the 
> > `throw;`, not something associated with the `__cxa_end_catch` cleanup.  
> > This basically boils down to not doing stores within a landing pad before 
> > it enters a `catch` handler.  Fortunately, landing pad code like that 
> > should always be in a terminate scope, so I think you can just look at the 
> > current EH stack and see whether the landing pad can possibly reach a UB 
> > scope.  (You might need some save-and-restore logic around `@finally` 
> > blocks in ObjC.)
> > 
> > The final question is whether we need to emit cleanups on paths that lead 
> > to UB scopes at all.  In the simplest cases, I think we don't.  The 
> > standard says that it's implementation-specified whether the stack is 
> > unwound on an exception path that leads to termination.  That rule doesn't 
> > technically apply here, but only because we're dealing with a language 
> > extension that makes unwinding UB in the first place.  Since we have room 
> > to choose the semantics, the standard gives us pretty strong cover to be 
> > just as aggressive about skipping cleanups in contexts where throwing is UB 
> > as it is in contexts where throwing terminates.  That, in turn, means that 
> > the landing pad in these contexts will usually just be a UB scope with no 
> > cleanups in it.  So even if we did need to worry about cleanups overwriting 
> > the source location, it usually won't be possible because we won't be 
> > running cleanups.  In fact, if there's a catch-all or a terminate scope, 
> > the UB scope won't be reachable, so the only situation where we have to 
> > worry about cleanups being run before we reach a UB scope is if there are 
> > cleanups inside a *partial* catch clause

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a reviewer: efriedma.
lebedev.ri added a comment.

@rjmccall thank you for taking a look!

In D137381#3955100 , @rjmccall wrote:

> In D137381#3953178 , @lebedev.ri 
> wrote:
>
>> ping. We need to get this going, reminder, this was caught because it caused 
>> a visible miscompilation.
>
> I'm sorry, I caught COVID-19 during the LLVM conference and have not had a 
> lot of energy recently,

I'm sorry.

> plus last week was the Thanksgiving holiday in the US.



> Please don't describe this as a miscompilation.  It may not match what you 
> want as a user, but what the the compiler is doing is not incorrect.

Right.

> What you're doing is adding a mitigation to try to protect against a source 
> of UB that recently affected you, which is commendable but doesn't have the 
> same level of urgency.

I'm mainly worried that there are likely other "miscompilations" in the wild.
Also, after this, we might want to reevaluate which clang attribute does what,




Comment at: clang/lib/CodeGen/CGCall.cpp:5402-5411
+if (SanOpts.has(SanitizerKind::ExceptionEscape) &&
+ExceptionEscapeUBLastInvokeSrcLoc) {
+  llvm::Constant *CheckSourceLocation = EmitCheckSourceLocation(Loc);
+  Builder.CreateStore(
+  CheckSourceLocation,
+  Address(ExceptionEscapeUBLastInvokeSrcLoc,
+  CheckSourceLocation->getType(),

rjmccall wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > @rjmccall
> > > So there are two issues:
> > > 1. `getInvokeDest()` isn't necessarily called just before creating an 
> > > `invoke`, see e.g. `-EHa` handling in `CodeGenFunction::PopCleanupBlock()`
> > > 2. Even if we ignore that, we need to do this for *every* `invoke`, not 
> > > just those going to the our UB landing pad, consider: 
> > > https://godbolt.org/z/qTeKor41a <- the invoke leads to a normal landing 
> > > pad, yet we immediately rethrow the just-caught exception, and now end up 
> > > in the UB landing pad.
> > > 
> > > So i'm not really seeing an alternative path here?
> > @rjmccall do you agree with my reasoning here?
> > Perhaps there is some other solution that i'm not seeing,
> > other than not having the source location?
> I think there are four interesting questions posed by your example.
> 
> The first is whether we should make an effort to report the original source 
> location for the call to `maybe_throws` instead of the rethrow location.  To 
> me, the answer is no, and the main reason is that the user is explicitly 
> acknowledging the possibility of an exception and is (probably) trying to 
> handle it.  That means that the proximate source of programmer error is now 
> that their attempt to handle it failed, not that an exception was raised in 
> the first place.  That might seem weird for your example specifically, 
> 
> The second is what source location we should report for invokes that don't 
> have an obvious source location.  The answer is that I think we should always 
> try to have a source location, even if it might be an imperfect one.  
> Synthesized code almost always has a source location that triggered the 
> synthesis.  Cleanups are typically associated with some variable or temporary 
> that itself should have a source location.  Even things like 
> `__cxa_end_catch` are associated with the `catch` clause.  Hopefully UBSan  
> has a flexible enough schema in how source locations are expressed to it for 
> us to communicate these things down to the runtime, like "synthesized code at 
> Foo.cpp:18" or "cleanup for variable at Bar.cpp:107".
> 
> The third is the more general question of whether we need to store a source 
> location before every `invoke`.  To ensure that this variable is meaningfully 
> initialized, we need to store a source location before every invoke that can 
> lead to a UB scope.  But the variable is stateful, so it's equally important 
> that we not emit stores that might overwrite the source location that we 
> actually want to report.  For example, we want the source location reported 
> in your example to be the source location for the `throw;`, not something 
> associated with the `__cxa_end_catch` cleanup.  This basically boils down to 
> not doing stores within a landing pad before it enters a `catch` handler.  
> Fortunately, landing pad code like that should always be in a terminate 
> scope, so I think you can just look at the current EH stack and see whether 
> the landing pad can possibly reach a UB scope.  (You might need some 
> save-and-restore logic around `@finally` blocks in ObjC.)
> 
> The final question is whether we need to emit cleanups on paths that lead to 
> UB scopes at all.  In the simplest cases, I think we don't.  The standard 
> says that it's implementation-specified whether the stack is unwound on an 
> exception path that leads to termination.  That rule doesn't technically 
> appl

[PATCH] D138836: [UpdateTestChecks] Fix `update_test_checks.py` to add "unused" prefixes

[Roman Lebedev via Phabricator via cfe-commits]

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

Anyways, assuming you tested that this works, LG.
Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138836

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

[PATCH] D138836: [UpdateTestChecks] Fix `update_test_checks.py` to add "unused" prefixes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D138836#3954850 , @mtrofin wrote:

> In D138836#3954782 , @lebedev.ri 
> wrote:
>
>> Thanks!
>> There's also the one for MCA, but this situation basically never happens for 
>> those tests.
>
> Yup, checked that and the mir one. The latter seems to be implementing its 
> own "add_checks", so would leave it as-is (plus, I'm guessing, by their 
> nature, mir tests wouldn't get into this type of scenario anyway, IIUC).

mca!=mir


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138836

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

[PATCH] D138836: [UpdateTestChecks] Fix `update_test_checks.py` to add "unused" prefixes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thanks!
There's also the one for MCA, but this situation basically never happens for 
those tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138836

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

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

ping. We need to get this going, reminder, this was caught because it caused a 
visible miscompilation.
@rjmccall are you satisfied with the way we emit srcloc info?
@aaron.ballman thoughts on the docs changes?

One unanswered question i have, is if we can get the exception's underlying C++ 
type as a string,
and print it. Also, is it useful to pass `Exn` into the handler? Can we do 
anything useful with it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

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

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

ping




Comment at: clang/lib/CodeGen/CGCall.cpp:5402-5411
+if (SanOpts.has(SanitizerKind::ExceptionEscape) &&
+ExceptionEscapeUBLastInvokeSrcLoc) {
+  llvm::Constant *CheckSourceLocation = EmitCheckSourceLocation(Loc);
+  Builder.CreateStore(
+  CheckSourceLocation,
+  Address(ExceptionEscapeUBLastInvokeSrcLoc,
+  CheckSourceLocation->getType(),

lebedev.ri wrote:
> @rjmccall
> So there are two issues:
> 1. `getInvokeDest()` isn't necessarily called just before creating an 
> `invoke`, see e.g. `-EHa` handling in `CodeGenFunction::PopCleanupBlock()`
> 2. Even if we ignore that, we need to do this for *every* `invoke`, not just 
> those going to the our UB landing pad, consider: 
> https://godbolt.org/z/qTeKor41a <- the invoke leads to a normal landing pad, 
> yet we immediately rethrow the just-caught exception, and now end up in the 
> UB landing pad.
> 
> So i'm not really seeing an alternative path here?
@rjmccall do you agree with my reasoning here?
Perhaps there is some other solution that i'm not seeing,
other than not having the source location?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

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

[PATCH] D136806: [Pipelines] Introduce SROA after (final, full) loop unrolling

[Roman Lebedev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8adfa29706e5: [Pipelines] Introduce SROA after (final, 
run-time) loop unrolling (authored by lebedev.ri).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136806

Files:
  clang/test/CodeGen/cleanup-destslot-simple.c
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-lto-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
  llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling-2.ll
  llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling.ll
  llvm/test/Transforms/PhaseOrdering/single-iteration-loop-sroa.ll

Index: llvm/test/Transforms/PhaseOrdering/single-iteration-loop-sroa.ll
===
--- llvm/test/Transforms/PhaseOrdering/single-iteration-loop-sroa.ll
+++ llvm/test/Transforms/PhaseOrdering/single-iteration-loop-sroa.ll
@@ -13,17 +13,17 @@
 ; CHECK-NEXT:store i16 [[TMP0:%.*]], ptr [[DATA]], align 2
 ; CHECK-NEXT:br label [[BB6_I_I:%.*]]
 ; CHECK:   bb6.i.i:
-; CHECK-NEXT:[[ITER_SROA_0_07_I_I:%.*]] = phi i64 [ [[TMP2:%.*]], [[BB6_I_I]] ], [ 0, [[START:%.*]] ]
+; CHECK-NEXT:[[ITER_SROA_0_07_I_I:%.*]] = phi i64 [ [[TMP1:%.*]], [[BB6_I_I]] ], [ 0, [[START:%.*]] ]
 ; CHECK-NEXT:[[_40_I_I:%.*]] = sub nsw i64 0, [[ITER_SROA_0_07_I_I]]
-; CHECK-NEXT:[[TMP2]] = add nuw nsw i64 [[ITER_SROA_0_07_I_I]], 1
+; CHECK-NEXT:[[TMP1]] = add nuw nsw i64 [[ITER_SROA_0_07_I_I]], 1
 ; CHECK-NEXT:[[_34_I_I:%.*]] = getelementptr inbounds [0 x i8], ptr [[DATA]], i64 0, i64 [[ITER_SROA_0_07_I_I]]
-; CHECK-NEXT:[[TMP1:%.*]] = getelementptr [0 x i8], ptr [[DATA]], i64 0, i64 [[_40_I_I]]
-; CHECK-NEXT:[[_39_I_I:%.*]] = getelementptr i8, ptr [[TMP1:%.*]], i64 1
+; CHECK-NEXT:[[TMP2:%.*]] = getelementptr [0 x i8], ptr [[DATA]], i64 0, i64 [[_40_I_I]]
+; CHECK-NEXT:[[_39_I_I:%.*]] = getelementptr i8, ptr [[TMP2]], i64 1
 ; CHECK-NEXT:[[TMP_0_COPYLOAD_I_I_I_I:%.*]] = load i8, ptr [[_34_I_I]], align 1
 ; CHECK-NEXT:[[TMP2_0_COPYLOAD_I_I_I_I:%.*]] = load i8, ptr [[_39_I_I]], align 1
 ; CHECK-NEXT:store i8 [[TMP2_0_COPYLOAD_I_I_I_I]], ptr [[_34_I_I]], align 1
 ; CHECK-NEXT:store i8 [[TMP_0_COPYLOAD_I_I_I_I]], ptr [[_39_I_I]], align 1
-; CHECK-NEXT:[[EXITCOND_NOT_I_I:%.*]] = icmp eq i64 [[TMP2]], [[X:%.*]]
+; CHECK-NEXT:[[EXITCOND_NOT_I_I:%.*]] = icmp eq i64 [[TMP1]], [[X:%.*]]
 ; CHECK-NEXT:br i1 [[EXITCOND_NOT_I_I]], label [[EXIT:%.*]], label [[BB6_I_I]]
 ; CHECK:   exit:
 ; CHECK-NEXT:[[DOTSROA_0_0_COPYLOAD:%.*]] = load i16, ptr [[DATA]], align 2
Index: llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling.ll
===
--- llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling.ll
+++ llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling.ll
@@ -13,38 +13,10 @@
 define void @wibble(ptr %arg) personality ptr null {
 ; CHECK-LABEL: @wibble(
 ; CHECK-NEXT:  bb:
-; CHECK-NEXT:[[I1:%.*]] = alloca [[T1:%.*]], align 16
 ; CHECK-NEXT:[[I10_3_I_PRE:%.*]] = load i8, ptr [[ARG:%.*]], align 1
-; CHECK-NEXT:[[VECTOR_RECUR_INIT:%.*]] = insertelement <4 x i8> poison, i8 [[I10_3_I_PRE]], i64 3
-; CHECK-NEXT:[[TMP0:%.*]] = getelementptr [64 x i8], ptr [[ARG]], i64 0, i64 1
-; CHECK-NEXT:[[WIDE_LOAD:%.*]] = load <4 x i8>, ptr [[TMP0]], align 1
-; CHECK-NEXT:[[TMP1:%.*]] = shufflevector <4 x i8> [[VECTOR_RECUR_INIT]], <4 x i8> [[WIDE_LOAD]], <4 x i32> 
-; CHECK-NEXT:[[TMP2:%.*]] = or <4 x i8> [[TMP1]], 
-; CHECK-NEXT:[[TMP3:%.*]] = zext <4 x i8> [[TMP2]] to <4 x i32>
-; CHECK-NEXT:store <4 x i32> [[TMP3]], ptr [[I1]], align 16
-; CHECK-NEXT:[[TMP4:%.*]] = getelementptr inbounds [16 x i32], ptr [[I1]], i64 0, i64 4
-; CHECK-NEXT:[[TMP5:%.*]] = getelementptr [64 x i8], ptr [[ARG]], i64 0, i64 5
-; CHECK-NEXT:[[WIDE_LOAD_1:%.*]] = load <4 x i8>, ptr [[TMP5]], align 1
-; CHECK-NEXT:[[TMP6:%.*]] = shufflevector <4 x i8> [[WIDE_LOAD]], <4 x i8> [[WIDE_LOAD_1]], <4 x i32> 
-; CHECK-NEXT:[[TMP7:%.*]] = or <4 x i8> [[TMP6]], 
-; CHECK-NEXT:[[TMP8:%.*]] = zext <4 x i8> [[TMP7]] to <4 x i32>
-; CHECK-NEXT:store <4 x i32> [[TMP8]], ptr [[TMP4]], align 16
-; CHECK-NEXT:[[TMP9:%.*]] = getelementptr inbounds [16 x i32], ptr [[I1]], i64 0, i64 8
-; CHECK-NEXT:[[TMP10:%.*]] = getelementptr [64 x i8], ptr [[ARG]], i64 0, i64 9
-; CHECK-NEXT:[[WIDE_LOAD_2:%.*]] = load <4 x i8>, ptr [[TMP10]], align 1
-; CHECK-NEXT:[[TMP11:%.*]] = shufflevector <4 x

[PATCH] D136806: [Pipelines] Introduce SROA after (final, full) loop unrolling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D136806#3934445 , @asbirlea wrote:

> Green light perf-wise.
> I cannot comment on whether the position is "the right" one though. I'm 
> deferring to the other reviewers.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136806

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

[PATCH] D136806: [Pipelines] Introduce SROA after (final, full) loop unrolling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D136806#3932369 , @asbirlea wrote:

> As far as performance, this looks fine. Not seeing measurable gains.

Thank you for checking!
Is the evaluation still ongoing, or is this the green light to go ahead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136806

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

[PATCH] D136806: [Pipelines] Introduce SROA after (final, full) loop unrolling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 475925.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
Herald added a subscriber: pcwang-thead.

Adjust full LTO pipeline, for symmetry with non-full LTO pipeline.
Looks like there is test coverage shortage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136806

Files:
  clang/test/CodeGen/cleanup-destslot-simple.c
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-lto-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
  llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling-2.ll
  llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling.ll
  llvm/test/Transforms/PhaseOrdering/single-iteration-loop-sroa.ll

Index: llvm/test/Transforms/PhaseOrdering/single-iteration-loop-sroa.ll
===
--- llvm/test/Transforms/PhaseOrdering/single-iteration-loop-sroa.ll
+++ llvm/test/Transforms/PhaseOrdering/single-iteration-loop-sroa.ll
@@ -13,17 +13,17 @@
 ; CHECK-NEXT:store i16 [[TMP0:%.*]], ptr [[DATA]], align 2
 ; CHECK-NEXT:br label [[BB6_I_I:%.*]]
 ; CHECK:   bb6.i.i:
-; CHECK-NEXT:[[ITER_SROA_0_07_I_I:%.*]] = phi i64 [ [[TMP2:%.*]], [[BB6_I_I]] ], [ 0, [[START:%.*]] ]
+; CHECK-NEXT:[[ITER_SROA_0_07_I_I:%.*]] = phi i64 [ [[TMP1:%.*]], [[BB6_I_I]] ], [ 0, [[START:%.*]] ]
 ; CHECK-NEXT:[[_40_I_I:%.*]] = sub nsw i64 0, [[ITER_SROA_0_07_I_I]]
-; CHECK-NEXT:[[TMP2]] = add nuw nsw i64 [[ITER_SROA_0_07_I_I]], 1
+; CHECK-NEXT:[[TMP1]] = add nuw nsw i64 [[ITER_SROA_0_07_I_I]], 1
 ; CHECK-NEXT:[[_34_I_I:%.*]] = getelementptr inbounds [0 x i8], ptr [[DATA]], i64 0, i64 [[ITER_SROA_0_07_I_I]]
-; CHECK-NEXT:[[TMP1:%.*]] = getelementptr [0 x i8], ptr [[DATA]], i64 0, i64 [[_40_I_I]]
-; CHECK-NEXT:[[_39_I_I:%.*]] = getelementptr i8, ptr [[TMP1:%.*]], i64 1
+; CHECK-NEXT:[[TMP2:%.*]] = getelementptr [0 x i8], ptr [[DATA]], i64 0, i64 [[_40_I_I]]
+; CHECK-NEXT:[[_39_I_I:%.*]] = getelementptr i8, ptr [[TMP2]], i64 1
 ; CHECK-NEXT:[[TMP_0_COPYLOAD_I_I_I_I:%.*]] = load i8, ptr [[_34_I_I]], align 1
 ; CHECK-NEXT:[[TMP2_0_COPYLOAD_I_I_I_I:%.*]] = load i8, ptr [[_39_I_I]], align 1
 ; CHECK-NEXT:store i8 [[TMP2_0_COPYLOAD_I_I_I_I]], ptr [[_34_I_I]], align 1
 ; CHECK-NEXT:store i8 [[TMP_0_COPYLOAD_I_I_I_I]], ptr [[_39_I_I]], align 1
-; CHECK-NEXT:[[EXITCOND_NOT_I_I:%.*]] = icmp eq i64 [[TMP2]], [[X:%.*]]
+; CHECK-NEXT:[[EXITCOND_NOT_I_I:%.*]] = icmp eq i64 [[TMP1]], [[X:%.*]]
 ; CHECK-NEXT:br i1 [[EXITCOND_NOT_I_I]], label [[EXIT:%.*]], label [[BB6_I_I]]
 ; CHECK:   exit:
 ; CHECK-NEXT:[[DOTSROA_0_0_COPYLOAD:%.*]] = load i16, ptr [[DATA]], align 2
Index: llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling.ll
===
--- llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling.ll
+++ llvm/test/Transforms/PhaseOrdering/X86/SROA-after-final-loop-unrolling.ll
@@ -13,38 +13,10 @@
 define void @wibble(ptr %arg) personality ptr null {
 ; CHECK-LABEL: @wibble(
 ; CHECK-NEXT:  bb:
-; CHECK-NEXT:[[I1:%.*]] = alloca [[T1:%.*]], align 16
 ; CHECK-NEXT:[[I10_3_I_PRE:%.*]] = load i8, ptr [[ARG:%.*]], align 1
-; CHECK-NEXT:[[VECTOR_RECUR_INIT:%.*]] = insertelement <4 x i8> poison, i8 [[I10_3_I_PRE]], i64 3
-; CHECK-NEXT:[[TMP0:%.*]] = getelementptr [64 x i8], ptr [[ARG]], i64 0, i64 1
-; CHECK-NEXT:[[WIDE_LOAD:%.*]] = load <4 x i8>, ptr [[TMP0]], align 1
-; CHECK-NEXT:[[TMP1:%.*]] = shufflevector <4 x i8> [[VECTOR_RECUR_INIT]], <4 x i8> [[WIDE_LOAD]], <4 x i32> 
-; CHECK-NEXT:[[TMP2:%.*]] = or <4 x i8> [[TMP1]], 
-; CHECK-NEXT:[[TMP3:%.*]] = zext <4 x i8> [[TMP2]] to <4 x i32>
-; CHECK-NEXT:store <4 x i32> [[TMP3]], ptr [[I1]], align 16
-; CHECK-NEXT:[[TMP4:%.*]] = getelementptr inbounds [16 x i32], ptr [[I1]], i64 0, i64 4
-; CHECK-NEXT:[[TMP5:%.*]] = getelementptr [64 x i8], ptr [[ARG]], i64 0, i64 5
-; CHECK-NEXT:[[WIDE_LOAD_1:%.*]] = load <4 x i8>, ptr [[TMP5]], align 1
-; CHECK-NEXT:[[TMP6:%.*]] = shufflevector <4 x i8> [[WIDE_LOAD]], <4 x i8> [[WIDE_LOAD_1]], <4 x i32> 
-; CHECK-NEXT:[[TMP7:%.*]] = or <4 x i8> [[TMP6]], 
-; CHECK-NEXT:[[TMP8:%.*]] = zext <4 x i8> [[TMP7]] to <4 x i32>
-; CHECK-NEXT:store <4 x i32> [[TMP8]], ptr [[TMP4]], align 16
-; CHECK-NEXT:[[TMP9:%.*]] = getelementptr inbounds [16 x i32], ptr [[I1]], i64 0, i64 8
-; CHECK-NEXT:[[TMP10:%.*]] = getelementptr [64 x i8], ptr [[ARG]], i64 0, i64 9
-; CHECK-NEXT:[[WIDE_LOAD_2:%.*]] = load <4 x i8>, ptr [[TMP10]], align 1
-; CHECK-NEXT:[[TMP11:%.*]] 

[PATCH] D136806: [Pipelines] Introduce SROA after (final, full) loop unrolling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D136806#3931860 , @spatel wrote:

> LGTM based on the timing, results, and alternatives discussed

Thank you for the review.

> There's some testing in progress according to previous comments, so best to 
> wait for that to finish in case it turns up anything new.




Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1119
+  if (IsFullLTO) {
+FPM.addPass(SROAPass());
+  }

spatel wrote:
> Is there a reason to put this down here vs. tacking it on the end of the 
> previous IsFullLTO block? 
> If LoopUnroll is the reason for adding SROA, then mention that specifically 
> in the comment?
> 
> IIRC, all of the FullLTO predicates in this set of passes were questionable 
> (see TODO comment above this function). They just accumulated because the 
> code was duplicated and diverged over time.
> Is there a reason to put this down here vs. tacking it on the end of the 
> previous IsFullLTO block?

I don't have any particular reason for this. Will change.

> If LoopUnroll is the reason for adding SROA, then mention that specifically 
> in the comment?

Will do.

> IIRC, all of the FullLTO predicates in this set of passes were questionable 
> (see TODO comment above this function). They just accumulated because the 
> code was duplicated and diverged over time.

Yeah, i remember all that. This is indeed ugly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136806

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

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/CGCall.cpp:5402-5411
+if (SanOpts.has(SanitizerKind::ExceptionEscape) &&
+ExceptionEscapeUBLastInvokeSrcLoc) {
+  llvm::Constant *CheckSourceLocation = EmitCheckSourceLocation(Loc);
+  Builder.CreateStore(
+  CheckSourceLocation,
+  Address(ExceptionEscapeUBLastInvokeSrcLoc,
+  CheckSourceLocation->getType(),

@rjmccall
So there are two issues:
1. `getInvokeDest()` isn't necessarily called just before creating an `invoke`, 
see e.g. `-EHa` handling in `CodeGenFunction::PopCleanupBlock()`
2. Even if we ignore that, we need to do this for *every* `invoke`, not just 
those going to the our UB landing pad, consider: 
https://godbolt.org/z/qTeKor41a <- the invoke leads to a normal landing pad, 
yet we immediately rethrow the just-caught exception, and now end up in the UB 
landing pad.

So i'm not really seeing an alternative path here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

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

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 475894.
lebedev.ri added a comment.

Update tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCleanup.cpp
  clang/lib/CodeGen/CGCleanup.h
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/CodeGenCXX/catch-exception-escape.cpp
  clang/test/Driver/fsanitize.c
  compiler-rt/lib/ubsan/ubsan_checks.inc
  compiler-rt/lib/ubsan/ubsan_handlers.cpp
  compiler-rt/lib/ubsan/ubsan_handlers.h
  compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
  compiler-rt/test/ubsan/TestCases/Misc/exception-escape.cpp
  compiler-rt/test/ubsan_minimal/TestCases/exception-escape.cpp

Index: compiler-rt/test/ubsan_minimal/TestCases/exception-escape.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan_minimal/TestCases/exception-escape.cpp
@@ -0,0 +1,48 @@
+// RUN: %clangxx -fsanitize=exception-escape %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --implicit-check-not="exception-escape" --check-prefixes=CHECK-ALL,CHECK-ONE
+// RUN: not --crash %run %t a 2>&1 | FileCheck %s --implicit-check-not="exception-escape" --check-prefixes=CHECK-ALL,CHECK-TWO
+// RUN: not --crash %run %t a b 2>&1 | FileCheck %s --implicit-check-not="exception-escape" --check-prefixes=CHECK-ALL,CHECK-THREE
+
+#include 
+#include 
+
+void thrower() { throw 0; }
+
+void nonthrower() noexcept {}
+
+// pure functions are generally side-effect free,
+// so we need to be smart to defeat optimizer
+// from completely dropping the call to the function...
+
+void *__attribute__((pure)) footgun(int x) {
+  if (x == 2)
+thrower();
+  if (x == 3)
+thrower();
+  nonthrower();
+  fprintf(stderr, "SUCCESS\n");
+  return malloc(1);
+}
+
+// CHECK-ALL: TEST
+
+// CHECK-ONE: SUCCESS
+// CHECK-TWO: exception-escape
+// CHECK-THREE: exception-escape
+
+int main(int argc, char **argv) {
+  bool status = 0;
+  void *mem = nullptr;
+
+  fprintf(stderr, "TEST\n");
+
+  try {
+mem = footgun(argc);
+status = !(mem != 0);
+  } catch (...) {
+  }
+
+  fprintf(stderr, "escaping: %p\n", mem);
+  free(mem);
+  return status;
+}
Index: compiler-rt/test/ubsan/TestCases/Misc/exception-escape.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/exception-escape.cpp
@@ -0,0 +1,104 @@
+// RUN: %clangxx -fsanitize=exception-escape %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --implicit-check-not="error:" --check-prefixes=CHECK-ALL,CHECK-ONE
+// RUN: not %run %t 2 2>&1 | FileCheck %s --implicit-check-not="error:" --check-prefixes=CHECK-ALL,CHECK-TWO
+// RUN: not %run %t 2 3 2>&1 | FileCheck %s --implicit-check-not="error:" --check-prefixes=CHECK-ALL,CHECK-THREE
+// RUN: %run %t 2 3 4 2>&1 | FileCheck %s --implicit-check-not="error:" --check-prefixes=CHECK-ALL,CHECK-FOUR
+// RUN: not %run %t 2 3 4 5 2>&1 | FileCheck %s --implicit-check-not="error:" --check-prefixes=CHECK-ALL,CHECK-FIVE
+// RUN: not %run %t 2 3 4 5 6 2>&1 | FileCheck %s --implicit-check-not="error:" --check-prefixes=CHECK-ALL,CHECK-SIX
+// RUN: not %run %t 2 3 4 5 6 6 2>&1 | FileCheck %s --implicit-check-not="error:" --check-prefixes=CHECK-ALL,CHECK-SEVEN
+
+#include 
+#include 
+
+void thrower() { throw 0; }
+
+void maybe_throws() {}
+
+void nonthrower() noexcept {}
+
+// pure functions are generally side-effect free,
+// so we need to be smart to defeat optimizer
+// from completely dropping the call to the function...
+
+void *__attribute__((pure)) footgun(int x) {
+  if (x == 2)
+#line 100
+thrower();
+
+  if (x == 3)
+#line 200
+thrower();
+
+  nonthrower();
+
+  if (x == 4) {
+try {
+#line 300
+  thrower();
+} catch (...) {
+}
+  }
+
+  if (x == 5) {
+try {
+#line 400
+  thrower();
+} catch (...) {
+#line 500
+  throw;
+}
+  }
+
+  if (x == 6) {
+try {
+#line 600
+  thrower();
+} catch (...) {
+#line 700
+  maybe_throws();
+#line 800
+  throw;
+}
+  }
+
+  if (x == 7) {
+try {
+#line 900
+  thrower();
+} catch (...) {
+#line 1000
+  thrower();
+#line 1100
+  throw;
+}
+  }
+
+  fprintf(stderr, "SUCCESS\n"); // Should be here, not in `main()`.
+  return malloc(1);
+}
+
+// CHECK-ALL: TEST
+
+// CHECK-ONE: SUCCESS
+// CHECK-TWO: exception-escape.cpp:100:5: runtime error: exception escapes out of function that should not throw exception
+// CHECK-THREE: exception-escape.cpp:200:5: runtime error: exception escapes out of function that should not throw exception
+// CHECK-FOUR: SUCCESS
+// 

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

[Roman Lebedev via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/CodeGenFunction.h:2028
+// FIXME: what about FuncletPads?
+if (llvm::BasicBlock *InvokeBB = Builder.GetInsertBlock();
+SanOpts.has(SanitizerKind::ExceptionEscape) &&

rjmccall wrote:
> The purpose of `getInvokeDestImpl` is to outline the slow path, and this 
> definitely belongs in the slow path.
The problem is that we aggressively cache all the EH stuff.
I will take a second look, maybe this can be done less intrusively.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381

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