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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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



================
Comment at: clang/include/clang/Basic/Attr.td:4133
+def NoUnwind : DeclOrStmtAttr {
+  let Spellings = [Clang<"nounwind", /*allowInC =*/0>];
+  let Accessors = [Accessor<"isClangNoUnwind", [CXX11<"clang", "nounwind">]>];
----------------
Should this be allowed in C (where structured exception handling is still a thing)?


================
Comment at: clang/include/clang/Basic/Attr.td:4138
+                             "functions and statements">;
+  let SimpleHandler = 1;
+}
----------------
I would have guessed we'd want to help the user out in a case like: `[[clang::nounwind]] void func() noexcept(false);`, given that this stuff can creep in via macros?


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



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



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



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


================
Comment at: clang/include/clang/Basic/AttrDocs.td:598
 If a statement is marked ``nomerge`` and contains call expressions, those call
-expressions inside the statement will not be merged during optimization. This 
+expressions inside the statement will not be merged during optimization. This
 attribute can be used to prevent the optimizer from obscuring the source
----------------
Looks like a whole pile of unrelated whitespace-only changes crept in; those can be backed out.


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



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


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:252-257
+  CallExprFinder CEF(S, St);
+  if (!CEF.foundCallExpr()) {
+    S.Diag(St->getBeginLoc(), diag::warn_attribute_ignored_no_calls_in_stmt)
+        << A;
+    return nullptr;
+  }
----------------
Oh great! But you're missing test coverage for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958