[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D106030#3018754 , @aeubanks wrote:

> Would it be ok to split this into two attributes, dontcall-warn and 
> dontcall-error? So we rely less on the Clang AST when determining if a 
> dontcall call should be emitted as a warning or an error.

Doesn't matter to me; go for it.  `BackendConsumer::DontCallDiagHandler` is 
what looks at the AST.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Would it be ok to split this into two attributes, dontcall-warn and 
dontcall-error? So we rely less on the Clang AST when determining if a dontcall 
call should be emitted as a warning or an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Fixup: https://reviews.llvm.org/D108718


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

I'm getting test failures for the newly added 
llvm/test/CodeGen/X86/attr-dontcall.ll, thought there's not much info other than

> Exit Code: 1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG846e562dcc6a: [Clang] add support for error+warning fn attrs 
(authored by nickdesaulniers).

Changed prior to commit:
  https://reviews.llvm.org/D106030?vs=366717=368678#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll
  llvm/test/ThinLTO/X86/dontcall.ll

Index: llvm/test/ThinLTO/X86/dontcall.ll
===
--- /dev/null
+++ llvm/test/ThinLTO/X86/dontcall.ll
@@ -0,0 +1,33 @@
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: not llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps 2>&1 \
+; RUN:   -r=%t/a.bc,callee,px \
+; RUN:   -r=%t/b.bc,callee,x  \
+; RUN:   -r=%t/b.bc,caller,px
+
+; TODO: As part of LTO, we check that types match, but *we don't yet check that
+; attributes match!!! What should happen if we remove "dontcall" from the
+; definition or declaration of @callee?
+
+; CHECK: call to callee marked "dontcall"
+
+;--- a.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @callee() "dontcall" noinline {
+  ret i32 42
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @callee() "dontcall"
+
+define i32 @caller() {
+entry:
+  %0 = call i32 @callee()
+  ret i32 %0
+}
Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7952,6 +7953,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -75,6 +75,7 @@
 #include 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

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

Clang parts LGTM, thank you for this!




Comment at: clang/test/Sema/attr-error.c:31-32
+
+__attribute__((error("foo"))) int bad5(void);   // expected-error {{'error' 
and 'warning' attributes are not compatible}}
+__attribute__((warning("foo"))) int bad5(void); // expected-note {{conflicting 
attribute is here}}
+

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > I think the diagnostic order is backwards here. The first declaration 
> > > > is where I'd expect the note and the second declaration is where I'd 
> > > > expect the error. (The idea is: the first declaration adds an attribute 
> > > > to the decl, so the redeclaration is what introduces the conflict and 
> > > > so that's where the error should live.) As an example: 
> > > > https://godbolt.org/z/bjGTWxYvh
> > > I'm not sure how to reconcile this. Looking at `bad4` above (different 
> > > case than `bad5`), the diagnostic we get is:
> > > ```
> > > /tmp/y.c:1:30: error: 'warning' and 'error' attributes are not compatible
> > > __attribute__((error("foo"), warning("foo")))
> > >  ^
> > > /tmp/y.c:1:16: note: conflicting attribute is here
> > > __attribute__((error("foo"), warning("foo")))
> > >^
> > > 1 error generated.
> > > ```
> > > which looks correct to me. If I flip the order these are diagnosed in, 
> > > that fixes `bad5` but if I flip around the ordering:
> > > ```
> > > --- a/clang/lib/Sema/SemaDeclAttr.cpp
> > > +++ b/clang/lib/Sema/SemaDeclAttr.cpp
> > > -  Diag(CI.getLoc(), diag::err_attributes_are_not_compatible) << CI 
> > > << EA;
> > > -  Diag(EA->getLocation(), diag::note_conflicting_attribute);
> > > +  Diag(EA->getLocation(), diag::err_attributes_are_not_compatible)
> > > +  << CI << EA;
> > > +  Diag(CI.getLoc(), diag::note_conflicting_attribute);
> > > ```
> > > Then `bad4` doesn't look quite correct.
> > > ```
> > > /tmp/y.c:1:16: error: 'warning' and 'error' attributes are not compatible
> > > __attribute__((error("foo"), warning("foo")))
> > >^
> > > /tmp/y.c:1:30: note: conflicting attribute is here
> > > __attribute__((error("foo"), warning("foo")))
> > >  ^
> > > ```
> > > Perhaps in the callers of `handleErrorAttr` I'm confusing which `Decl` is 
> > > the "new" vs the "old?"
> > > which looks correct to me.
> > 
> > That also looks correct to me; the second attribute is the diagnostic and 
> > the first attribute is the note.
> > 
> > > Perhaps in the callers of handleErrorAttr I'm confusing which Decl is the 
> > > "new" vs the "old?"
> > 
> > Huh, this is rather strange. The logic you're using is basically the same 
> > as `checkAttrMutualExclusion()`, just with extra work to figure out which 
> > kind of attribute you're dealing with. I'm not seeing what's causing the 
> > order to be different when I reason my way through the code. Perhaps in the 
> > `bad4()` case, are we somehow processing the attributes in reverse order?
> > 
> > FWIW, at the end of the day, we can live with the diagnostic in either 
> > situation, even if they're a bit strange. I think the merge behavior (two 
> > different decls) is more important to get right because that's going to be 
> > the far more common scenario to see the diagnostic in. If it turns out that 
> > it's a systemic issue (or just gnarly to figure out), it won't block this 
> > review. We can always improve the behavior in a follow-up.
> I suspect that `DiagnoseMutualExclusions` (via tablegen) would be able to 
> catch this, had we distinct `Attr`s for warning and error; example tablegen 
> rule: `def : MutualExclusions<[Hot, Cold]>;`.
I suspect you're correct. Perhaps the logic in ClangAttrEmitter.cpp is already 
accounting for some oddity here that we're not. The way the generated mutual 
exclusions look are:
```
  if (const auto *Second = dyn_cast(A)) {
if (const auto *First = D->getAttr()) {
  S.Diag(First->getLocation(), diag::err_attributes_are_not_compatible) << 
First << Second;
  S.Diag(Second->getLocation(), diag::note_conflicting_attribute);
  return false;
}
return true;
  }
```
Compared with your code, this is different. In your code, `CI` is analogous to 
`Second` above, and `EA` is analogous to `First`. You diagnose at the location 
of `First` but pass in `<< Second (CI) << First (EA)` as the arguments.
```
ErrorAttr *Sema::mergeErrorAttr(Decl *D, const AttributeCommonInfo ,
StringRef NewUserDiagnostic) {
  if (const auto *EA = D->getAttr()) {
std::string NewAttr = CI.getNormalizedFullName();
assert((NewAttr == "error" || NewAttr == "warning") &&
   "unexpected normalized full name");
bool Match = (EA->isError() && NewAttr == "error") ||
 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/test/Sema/attr-error.c:31-32
+
+__attribute__((error("foo"))) int bad5(void);   // expected-error {{'error' 
and 'warning' attributes are not compatible}}
+__attribute__((warning("foo"))) int bad5(void); // expected-note {{conflicting 
attribute is here}}
+

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > I think the diagnostic order is backwards here. The first declaration is 
> > > where I'd expect the note and the second declaration is where I'd expect 
> > > the error. (The idea is: the first declaration adds an attribute to the 
> > > decl, so the redeclaration is what introduces the conflict and so that's 
> > > where the error should live.) As an example: 
> > > https://godbolt.org/z/bjGTWxYvh
> > I'm not sure how to reconcile this. Looking at `bad4` above (different case 
> > than `bad5`), the diagnostic we get is:
> > ```
> > /tmp/y.c:1:30: error: 'warning' and 'error' attributes are not compatible
> > __attribute__((error("foo"), warning("foo")))
> >  ^
> > /tmp/y.c:1:16: note: conflicting attribute is here
> > __attribute__((error("foo"), warning("foo")))
> >^
> > 1 error generated.
> > ```
> > which looks correct to me. If I flip the order these are diagnosed in, that 
> > fixes `bad5` but if I flip around the ordering:
> > ```
> > --- a/clang/lib/Sema/SemaDeclAttr.cpp
> > +++ b/clang/lib/Sema/SemaDeclAttr.cpp
> > -  Diag(CI.getLoc(), diag::err_attributes_are_not_compatible) << CI << 
> > EA;
> > -  Diag(EA->getLocation(), diag::note_conflicting_attribute);
> > +  Diag(EA->getLocation(), diag::err_attributes_are_not_compatible)
> > +  << CI << EA;
> > +  Diag(CI.getLoc(), diag::note_conflicting_attribute);
> > ```
> > Then `bad4` doesn't look quite correct.
> > ```
> > /tmp/y.c:1:16: error: 'warning' and 'error' attributes are not compatible
> > __attribute__((error("foo"), warning("foo")))
> >^
> > /tmp/y.c:1:30: note: conflicting attribute is here
> > __attribute__((error("foo"), warning("foo")))
> >  ^
> > ```
> > Perhaps in the callers of `handleErrorAttr` I'm confusing which `Decl` is 
> > the "new" vs the "old?"
> > which looks correct to me.
> 
> That also looks correct to me; the second attribute is the diagnostic and the 
> first attribute is the note.
> 
> > Perhaps in the callers of handleErrorAttr I'm confusing which Decl is the 
> > "new" vs the "old?"
> 
> Huh, this is rather strange. The logic you're using is basically the same as 
> `checkAttrMutualExclusion()`, just with extra work to figure out which kind 
> of attribute you're dealing with. I'm not seeing what's causing the order to 
> be different when I reason my way through the code. Perhaps in the `bad4()` 
> case, are we somehow processing the attributes in reverse order?
> 
> FWIW, at the end of the day, we can live with the diagnostic in either 
> situation, even if they're a bit strange. I think the merge behavior (two 
> different decls) is more important to get right because that's going to be 
> the far more common scenario to see the diagnostic in. If it turns out that 
> it's a systemic issue (or just gnarly to figure out), it won't block this 
> review. We can always improve the behavior in a follow-up.
I suspect that `DiagnoseMutualExclusions` (via tablegen) would be able to catch 
this, had we distinct `Attr`s for warning and error; example tablegen rule: 
`def : MutualExclusions<[Hot, Cold]>;`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 366717.
nickdesaulniers added a comment.

- reorder diag vs note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Misc/serialized-diags-driver.c
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll
  llvm/test/ThinLTO/X86/dontcall.ll

Index: llvm/test/ThinLTO/X86/dontcall.ll
===
--- /dev/null
+++ llvm/test/ThinLTO/X86/dontcall.ll
@@ -0,0 +1,33 @@
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: not llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps 2>&1 \
+; RUN:   -r=%t/a.bc,callee,px \
+; RUN:   -r=%t/b.bc,callee,x  \
+; RUN:   -r=%t/b.bc,caller,px
+
+; TODO: As part of LTO, we check that types match, but *we don't yet check that
+; attributes match!!! What should happen if we remove "dontcall" from the
+; definition or declaration of @callee?
+
+; CHECK: call to callee marked "dontcall"
+
+;--- a.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @callee() "dontcall" noinline {
+  ret i32 42
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @callee() "dontcall"
+
+define i32 @caller() {
+entry:
+  %0 = call i32 @callee()
+  ret i32 %0
+}
Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7945,6 +7946,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -75,6 +75,7 @@
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
+#include 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Sema/attr-error.c:31-32
+
+__attribute__((error("foo"))) int bad5(void);   // expected-error {{'error' 
and 'warning' attributes are not compatible}}
+__attribute__((warning("foo"))) int bad5(void); // expected-note {{conflicting 
attribute is here}}
+

nickdesaulniers wrote:
> aaron.ballman wrote:
> > I think the diagnostic order is backwards here. The first declaration is 
> > where I'd expect the note and the second declaration is where I'd expect 
> > the error. (The idea is: the first declaration adds an attribute to the 
> > decl, so the redeclaration is what introduces the conflict and so that's 
> > where the error should live.) As an example: https://godbolt.org/z/bjGTWxYvh
> I'm not sure how to reconcile this. Looking at `bad4` above (different case 
> than `bad5`), the diagnostic we get is:
> ```
> /tmp/y.c:1:30: error: 'warning' and 'error' attributes are not compatible
> __attribute__((error("foo"), warning("foo")))
>  ^
> /tmp/y.c:1:16: note: conflicting attribute is here
> __attribute__((error("foo"), warning("foo")))
>^
> 1 error generated.
> ```
> which looks correct to me. If I flip the order these are diagnosed in, that 
> fixes `bad5` but if I flip around the ordering:
> ```
> --- a/clang/lib/Sema/SemaDeclAttr.cpp
> +++ b/clang/lib/Sema/SemaDeclAttr.cpp
> -  Diag(CI.getLoc(), diag::err_attributes_are_not_compatible) << CI << EA;
> -  Diag(EA->getLocation(), diag::note_conflicting_attribute);
> +  Diag(EA->getLocation(), diag::err_attributes_are_not_compatible)
> +  << CI << EA;
> +  Diag(CI.getLoc(), diag::note_conflicting_attribute);
> ```
> Then `bad4` doesn't look quite correct.
> ```
> /tmp/y.c:1:16: error: 'warning' and 'error' attributes are not compatible
> __attribute__((error("foo"), warning("foo")))
>^
> /tmp/y.c:1:30: note: conflicting attribute is here
> __attribute__((error("foo"), warning("foo")))
>  ^
> ```
> Perhaps in the callers of `handleErrorAttr` I'm confusing which `Decl` is the 
> "new" vs the "old?"
> which looks correct to me.

That also looks correct to me; the second attribute is the diagnostic and the 
first attribute is the note.

> Perhaps in the callers of handleErrorAttr I'm confusing which Decl is the 
> "new" vs the "old?"

Huh, this is rather strange. The logic you're using is basically the same as 
`checkAttrMutualExclusion()`, just with extra work to figure out which kind of 
attribute you're dealing with. I'm not seeing what's causing the order to be 
different when I reason my way through the code. Perhaps in the `bad4()` case, 
are we somehow processing the attributes in reverse order?

FWIW, at the end of the day, we can live with the diagnostic in either 
situation, even if they're a bit strange. I think the merge behavior (two 
different decls) is more important to get right because that's going to be the 
far more common scenario to see the diagnostic in. If it turns out that it's a 
systemic issue (or just gnarly to figure out), it won't block this review. We 
can always improve the behavior in a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/test/Sema/attr-error.c:31-32
+
+__attribute__((error("foo"))) int bad5(void);   // expected-error {{'error' 
and 'warning' attributes are not compatible}}
+__attribute__((warning("foo"))) int bad5(void); // expected-note {{conflicting 
attribute is here}}
+

aaron.ballman wrote:
> I think the diagnostic order is backwards here. The first declaration is 
> where I'd expect the note and the second declaration is where I'd expect the 
> error. (The idea is: the first declaration adds an attribute to the decl, so 
> the redeclaration is what introduces the conflict and so that's where the 
> error should live.) As an example: https://godbolt.org/z/bjGTWxYvh
I'm not sure how to reconcile this. Looking at `bad4` above (different case 
than `bad5`), the diagnostic we get is:
```
/tmp/y.c:1:30: error: 'warning' and 'error' attributes are not compatible
__attribute__((error("foo"), warning("foo")))
 ^
/tmp/y.c:1:16: note: conflicting attribute is here
__attribute__((error("foo"), warning("foo")))
   ^
1 error generated.
```
which looks correct to me. If I flip the order these are diagnosed in, that 
fixes `bad5` but if I flip around the ordering:
```
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
-  Diag(CI.getLoc(), diag::err_attributes_are_not_compatible) << CI << EA;
-  Diag(EA->getLocation(), diag::note_conflicting_attribute);
+  Diag(EA->getLocation(), diag::err_attributes_are_not_compatible)
+  << CI << EA;
+  Diag(CI.getLoc(), diag::note_conflicting_attribute);
```
Then `bad4` doesn't look quite correct.
```
/tmp/y.c:1:16: error: 'warning' and 'error' attributes are not compatible
__attribute__((error("foo"), warning("foo")))
   ^
/tmp/y.c:1:30: note: conflicting attribute is here
__attribute__((error("foo"), warning("foo")))
 ^
```
Perhaps in the callers of `handleErrorAttr` I'm confusing which `Decl` is the 
"new" vs the "old?"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Sema/attr-error.c:31-32
+
+__attribute__((error("foo"))) int bad5(void);   // expected-error {{'error' 
and 'warning' attributes are not compatible}}
+__attribute__((warning("foo"))) int bad5(void); // expected-note {{conflicting 
attribute is here}}
+

I think the diagnostic order is backwards here. The first declaration is where 
I'd expect the note and the second declaration is where I'd expect the error. 
(The idea is: the first declaration adds an attribute to the decl, so the 
redeclaration is what introduces the conflict and so that's where the error 
should live.) As an example: https://godbolt.org/z/bjGTWxYvh


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 366356.
nickdesaulniers added a comment.

- remove unnecessary parens


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Misc/serialized-diags-driver.c
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll
  llvm/test/ThinLTO/X86/dontcall.ll

Index: llvm/test/ThinLTO/X86/dontcall.ll
===
--- /dev/null
+++ llvm/test/ThinLTO/X86/dontcall.ll
@@ -0,0 +1,33 @@
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: not llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps 2>&1 \
+; RUN:   -r=%t/a.bc,callee,px \
+; RUN:   -r=%t/b.bc,callee,x  \
+; RUN:   -r=%t/b.bc,caller,px
+
+; TODO: As part of LTO, we check that types match, but *we don't yet check that
+; attributes match!!! What should happen if we remove "dontcall" from the
+; definition or declaration of @callee?
+
+; CHECK: call to callee marked "dontcall"
+
+;--- a.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @callee() "dontcall" noinline {
+  ret i32 42
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @callee() "dontcall"
+
+define i32 @caller() {
+entry:
+  %0 = call i32 @callee()
+  ret i32 %0
+}
Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7950,6 +7951,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -75,6 +75,7 @@
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
+#include 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:3368
+  Diag(Old->getLocation(), diag::note_previous_declaration);
+  New->dropAttr();
+}

Should be dropping `ErrorAttr`.



Comment at: clang/test/Sema/attr-error.c:26
+
+__attribute__((error("foo"), warning("foo"))) // expected-error {{'warning' 
and 'error' attributes are not compatible}}
+int

aaron.ballman wrote:
> Can you also add a test for:
> ```
> __attribute__((error("foo"))) int bad5(void); // expected-note {{conflicting 
> attribute is here}}
> __attribute__((warning("foo"))) int bad5(void); // expected-error {{'warning' 
> and 'error' attributes are not compatible}}
> ```
Ah, that exposes the case you discussed above:
> I think you need to follow the attribute merge pattern used in SemaDecl.cpp.

So I did a relatively larger refactor to support that. Doing so required me to 
make this an InheritableAttr. PTAL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 366355.
nickdesaulniers marked 4 inline comments as done.
nickdesaulniers added a comment.

- fix tests on windows, let diag eng format decl, drop correct attr, refactor 
to handle merging as InheritableAttr


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Misc/serialized-diags-driver.c
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll
  llvm/test/ThinLTO/X86/dontcall.ll

Index: llvm/test/ThinLTO/X86/dontcall.ll
===
--- /dev/null
+++ llvm/test/ThinLTO/X86/dontcall.ll
@@ -0,0 +1,33 @@
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: not llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps 2>&1 \
+; RUN:   -r=%t/a.bc,callee,px \
+; RUN:   -r=%t/b.bc,callee,x  \
+; RUN:   -r=%t/b.bc,caller,px
+
+; TODO: As part of LTO, we check that types match, but *we don't yet check that
+; attributes match!!! What should happen if we remove "dontcall" from the
+; definition or declaration of @callee?
+
+; CHECK: call to callee marked "dontcall"
+
+;--- a.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @callee() "dontcall" noinline {
+  ret i32 42
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @callee() "dontcall"
+
+define i32 @caller() {
+entry:
+  %0 = call i32 @callee()
+  ret i32 %0
+}
Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7950,6 +7951,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+: diag::warn_fe_backend_warning_attr)
+<< FD->getDeclName() << EA->getUserDiagnostic();
+  }

The diagnostics engine knows how to properly format a `NamedDecl *` directly 
(and this will take care of properly quoting the name in the diagnostics as 
well).



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:964-965
+bool match =
+(EA->isError() && NewAttrSpellingIndex < ErrorAttr::GNU_warning) ||
+(EA->isWarning() && NewAttrSpellingIndex >= ErrorAttr::GNU_warning);
+if (!match) {

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > It's a bit tricky for me to tell from the patch -- are the enumerators 
> > > > in the correct order that this logic still works for when the [[]] 
> > > > spelling is used?
> > > For reference, the generated `enum Spelling` looks like:
> > > ```
> > >  3611 public: 
> > >   
> > > 
> > >  3612   enum Spelling {   
> > >   
> > > 
> > >  3613 GNU_error = 0,  
> > >   
> > > 
> > >  3614 CXX11_gnu_error = 1,
> > >   
> > > 
> > >  3615 C2x_gnu_error = 2,  
> > >   
> > > 
> > >  3616 GNU_warning = 3,
> > >   
> > > 
> > >  3617 CXX11_gnu_warning = 4,  
> > >   
> > > 
> > >  3618 C2x_gnu_warning = 5,
> > >   
> > > 
> > >  3619   SpellingNotCalculated = 15
> > >   
> > > 
> > >  3620 
> > >   
> > > 
> > >  3621   };
> > > ```
> > > while using `getAttributeSpellingListIndex` rather than 
> > > `getNormalizedFullName` allows us to avoid a pair of string comparisons 
> > > in favor of a pair of `unsigned` comparisons, we now have an issue 
> > > because there are technically six spellings. (I guess it's not clear to 
> > > me what's `CXX11_gnu_*` vs `C2x_gnu_*`?)  We could be explicit against 
> > > checking all six rather than two comparisons.
> > > 
> > > Or I can use local variables with more helpful identifiers like:
> > > 
> > > ```
> > > bool NewAttrIsError = NewAttrSpellingIndex < ErrorAttr::GNU_warning;
> > > bool NewAttrIsWarning = NewAttrSpellingIndex >= ErrorAttr::GNU_warning;
> > > bool Match = (EA->isError() && NewAttrIsError) || (EA->isWarning() && 
> > > NewAttrIsWarning);
> > > ```
> > > 
> > > WDYT?
> > > (I guess it's not clear to me what's CXX11_gnu_* vs C2x_gnu_*?)
> > 
> > A bit of a historical thing. C2x `[[]]` and C++11 `[[]]` are modeled as 
> > different spellings. This allows us to specify attributes with a `[[]]` 
> > spelling in only one of the languages without having to do an awkward dance 
> > involving language options. e.g., it makes it easier to support an 
> > attribute spelled `__attribute__((foo))` and `[[vendor::foo]]` in C2x and 
> > spelled `[[foo]]` in C++. it picks up the `gnu` bit in the spelling by 
> > virtue of being an attribute with a `GCC` spelling -- IIRC, we needed to 
> > distinguish between GCC and Clang there for some reason I no longer recall.
> > 
> > > WDYT?
> > 
> > I think the current approach is reasonable, but I think the previous 
> > approach (doing the string compare) was more readable. If you have a 
> > preference for using the string compare approach as it originally was, I'd 
> > be fine with that now that I see how my suggestion is playing out in 
> > practice. If you'd prefer to keep the current approach, I'd also be fine 
> > with that. Thank you for 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:964-965
+bool match =
+(EA->isError() && NewAttrSpellingIndex < ErrorAttr::GNU_warning) ||
+(EA->isWarning() && NewAttrSpellingIndex >= ErrorAttr::GNU_warning);
+if (!match) {

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > It's a bit tricky for me to tell from the patch -- are the enumerators in 
> > > the correct order that this logic still works for when the [[]] spelling 
> > > is used?
> > For reference, the generated `enum Spelling` looks like:
> > ```
> >  3611 public:   
> > 
> > 
> >  3612   enum Spelling { 
> > 
> > 
> >  3613 GNU_error = 0,
> > 
> > 
> >  3614 CXX11_gnu_error = 1,  
> > 
> > 
> >  3615 C2x_gnu_error = 2,
> > 
> > 
> >  3616 GNU_warning = 3,  
> > 
> > 
> >  3617 CXX11_gnu_warning = 4,
> > 
> > 
> >  3618 C2x_gnu_warning = 5,  
> > 
> > 
> >  3619   SpellingNotCalculated = 15  
> > 
> > 
> >  3620   
> > 
> > 
> >  3621   };
> > ```
> > while using `getAttributeSpellingListIndex` rather than 
> > `getNormalizedFullName` allows us to avoid a pair of string comparisons in 
> > favor of a pair of `unsigned` comparisons, we now have an issue because 
> > there are technically six spellings. (I guess it's not clear to me what's 
> > `CXX11_gnu_*` vs `C2x_gnu_*`?)  We could be explicit against checking all 
> > six rather than two comparisons.
> > 
> > Or I can use local variables with more helpful identifiers like:
> > 
> > ```
> > bool NewAttrIsError = NewAttrSpellingIndex < ErrorAttr::GNU_warning;
> > bool NewAttrIsWarning = NewAttrSpellingIndex >= ErrorAttr::GNU_warning;
> > bool Match = (EA->isError() && NewAttrIsError) || (EA->isWarning() && 
> > NewAttrIsWarning);
> > ```
> > 
> > WDYT?
> > (I guess it's not clear to me what's CXX11_gnu_* vs C2x_gnu_*?)
> 
> A bit of a historical thing. C2x `[[]]` and C++11 `[[]]` are modeled as 
> different spellings. This allows us to specify attributes with a `[[]]` 
> spelling in only one of the languages without having to do an awkward dance 
> involving language options. e.g., it makes it easier to support an attribute 
> spelled `__attribute__((foo))` and `[[vendor::foo]]` in C2x and spelled 
> `[[foo]]` in C++. it picks up the `gnu` bit in the spelling by virtue of 
> being an attribute with a `GCC` spelling -- IIRC, we needed to distinguish 
> between GCC and Clang there for some reason I no longer recall.
> 
> > WDYT?
> 
> I think the current approach is reasonable, but I think the previous approach 
> (doing the string compare) was more readable. If you have a preference for 
> using the string compare approach as it originally was, I'd be fine with that 
> now that I see how my suggestion is playing out in practice. If you'd prefer 
> to keep the current approach, I'd also be fine with that. Thank you for 
> trying this out, sorry for the hassle!
> now that I see how my suggestion is playing out in practice
> Thank you for trying this out, sorry for the hassle!

Ah, that's ok.  [[ https://www.youtube.com/watch?v=aPVLyB0Yc6I | Sometimes you 
eat the bar, sometimes the bar eats you. ]]  I've done that myself already in 
this patch when playing with llvm::Optional (and C++ default parameters).

Changed back to string comparison now.  I don't expect any of this code to be 
hot, ever.  Marking as done.



Comment at: 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 366050.
nickdesaulniers marked 4 inline comments as done.
nickdesaulniers added a comment.

- combine c++ w/ c test
- use -verify= rather than -D
- remove REQUIRES
- go back to string comparison


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Misc/serialized-diags-driver.c
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll
  llvm/test/ThinLTO/X86/dontcall.ll

Index: llvm/test/ThinLTO/X86/dontcall.ll
===
--- /dev/null
+++ llvm/test/ThinLTO/X86/dontcall.ll
@@ -0,0 +1,33 @@
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: not llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps 2>&1 \
+; RUN:   -r=%t/a.bc,callee,px \
+; RUN:   -r=%t/b.bc,callee,x  \
+; RUN:   -r=%t/b.bc,caller,px
+
+; TODO: As part of LTO, we check that types match, but *we don't yet check that
+; attributes match!!! What should happen if we remove "dontcall" from the
+; definition or declaration of @callee?
+
+; CHECK: call to callee marked "dontcall"
+
+;--- a.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @callee() "dontcall" noinline {
+  ret i32 42
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @callee() "dontcall"
+
+define i32 @caller() {
+entry:
+  %0 = call i32 @callee()
+  ret i32 %0
+}
Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7950,6 +7951,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -75,6 +75,7 @@
 #include 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:964-965
+bool match =
+(EA->isError() && NewAttrSpellingIndex < ErrorAttr::GNU_warning) ||
+(EA->isWarning() && NewAttrSpellingIndex >= ErrorAttr::GNU_warning);
+if (!match) {

nickdesaulniers wrote:
> aaron.ballman wrote:
> > It's a bit tricky for me to tell from the patch -- are the enumerators in 
> > the correct order that this logic still works for when the [[]] spelling is 
> > used?
> For reference, the generated `enum Spelling` looks like:
> ```
>  3611 public: 
>   
> 
>  3612   enum Spelling {   
>   
> 
>  3613 GNU_error = 0,  
>   
> 
>  3614 CXX11_gnu_error = 1,
>   
> 
>  3615 C2x_gnu_error = 2,  
>   
> 
>  3616 GNU_warning = 3,
>   
> 
>  3617 CXX11_gnu_warning = 4,  
>   
> 
>  3618 C2x_gnu_warning = 5,
>   
> 
>  3619   SpellingNotCalculated = 15
>   
> 
>  3620 
>   
> 
>  3621   };
> ```
> while using `getAttributeSpellingListIndex` rather than 
> `getNormalizedFullName` allows us to avoid a pair of string comparisons in 
> favor of a pair of `unsigned` comparisons, we now have an issue because there 
> are technically six spellings. (I guess it's not clear to me what's 
> `CXX11_gnu_*` vs `C2x_gnu_*`?)  We could be explicit against checking all six 
> rather than two comparisons.
> 
> Or I can use local variables with more helpful identifiers like:
> 
> ```
> bool NewAttrIsError = NewAttrSpellingIndex < ErrorAttr::GNU_warning;
> bool NewAttrIsWarning = NewAttrSpellingIndex >= ErrorAttr::GNU_warning;
> bool Match = (EA->isError() && NewAttrIsError) || (EA->isWarning() && 
> NewAttrIsWarning);
> ```
> 
> WDYT?
> (I guess it's not clear to me what's CXX11_gnu_* vs C2x_gnu_*?)

A bit of a historical thing. C2x `[[]]` and C++11 `[[]]` are modeled as 
different spellings. This allows us to specify attributes with a `[[]]` 
spelling in only one of the languages without having to do an awkward dance 
involving language options. e.g., it makes it easier to support an attribute 
spelled `__attribute__((foo))` and `[[vendor::foo]]` in C2x and spelled 
`[[foo]]` in C++. it picks up the `gnu` bit in the spelling by virtue of being 
an attribute with a `GCC` spelling -- IIRC, we needed to distinguish between 
GCC and Clang there for some reason I no longer recall.

> WDYT?

I think the current approach is reasonable, but I think the previous approach 
(doing the string compare) was more readable. If you have a preference for 
using the string compare approach as it originally was, I'd be fine with that 
now that I see how my suggestion is playing out in practice. If you'd prefer to 
keep the current approach, I'd also be fine with that. Thank you for trying 
this out, sorry for the hassle!



Comment at: clang/test/Frontend/backend-attribute-error-warning-optimize.c:1
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -O2 -verify -emit-codegen-only %s

Out of curiosity, why is this required? I would have assumed this would be 
backend-independent functionality?

Also, I have no idea where these tests should live. They're not really frontend 
tests, it's more about the integration between the frontend and the backend. We 
do have `clang/test/Integration` but there's not a lot of content there to be 
able to say "oh yeah, this is testing the same sort of stuff". No idea if other 
reviewers have opinions on this.



Comment at: 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11172
 
+def err_attribute_removed_on_redeclaration : Error<
+  "'%0' attribute removed from redeclared variable">;

aaron.ballman wrote:
> This diagnostic is a bit confusing to me -- should this be a warning about 
> the attribute being ignored in this case, rather than an error? 
> Alternatively, should this be re-worded to say that the attribute must appear 
> on the first declaration? If the latter, we have some diagnostics that could 
> maybe be combined into using a `%select` for this: 
> `err_noreturn_missing_on_first_decl` and `err_internal_linkage_redeclaration` 
> are awfully similar to the same thing.
Rebased onto D107613; reusing `diag::err_attribute_missing_on_first_decl` now. 
done.



Comment at: clang/lib/Sema/SemaDecl.cpp:2982
 
+  // If redeclaration removes error or warning attribute...
+  if (auto *EA = New->getAttr())

aaron.ballman wrote:
> The comment doesn't seem to match the code -- this is when the redeclaration 
> *adds* the attribute. And don't we want to silently accept that if the 
> redeclaration adds the same annotation with the same message? e.g.,
> ```
> __attribute__((error("derp"))) void func();
> __attribute__((error("derp"))) void func(); // fine
> __attribute__((error("derp"))) void func() {} // fine
> ```
Rebased onto D107613, so this block was moved from `Sema::mergeDeclAttributes` 
(which should be for `Attributes` on any subclass of `Decl`, which is too 
generic for a `FunctionDecl` only `Attribute`) to `Sema::MergeFunctionDecl` 
(which is only for `FunctionDecl` `Attribute`s).

I had a similar test case already in clang/test/Sema/attr-error.c testing 
`__attribute__((error("foo"), error("foo"))) int good1(void);`, but I will add 
a test to that file for redeclarations with matching attributes. done.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:955
+
+  if (const auto *EA = D->getAttr()) {
+unsigned NewAttrSpellingIndex = AL.getAttributeSpellingListIndex();

aaron.ballman wrote:
> This logic will catch the case where the attributes are processed in a group 
> (e.g., two attributes in one specifier or two attributes on the same 
> declaration) but it won't handle the redeclaration merging case. I think you 
> need to follow the attribute merge pattern used in SemaDecl.cpp.
I'm not sure that I need to change anything here; redeclarations of 
`ErrorAttr`s are checked in `Sema::MergeFunctionDecl`.

But when I was playing around with something else here earlier this week, I 
noticed a pre-existing inconsistency (or maybe just a difference, described 
below) of `handle*Attr` functionns vs `merge*Attr` functions.

`mergeDeclAttribute` in `clang/lib/Sema/SemaDecl.cpp` has a large `if`/`else` 
chain for new attrs, which calls `merge*Attr` methods of `Sema`, but it doesn't 
give your a reference to any "`Old`"/pre-existing attr that may be in conflict. 
 A comment in `mergeDeclAttribute` seems to confirm this.

>> // This function copies an attribute Attr from a previous declaration to the 
>>  
>>   
>> // new declaration D if the new declaration doesn't itself have that 
>> attribute
>>   
>> // yet or if that attribute allows duplicates.

Because we do not allow this `Attribute` to be added on subsequent `Decl`s, I 
_do not think_ we need to follow the attribute merge pattern used in 
SemaDecl.cpp.  But I could be wrong; please triple check.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:964-965
+bool match =
+(EA->isError() && NewAttrSpellingIndex < ErrorAttr::GNU_warning) ||
+(EA->isWarning() && NewAttrSpellingIndex >= ErrorAttr::GNU_warning);
+if (!match) {

aaron.ballman wrote:
> It's a bit tricky for me to tell from the patch -- are the enumerators in the 
> correct order that this logic still works for when the [[]] spelling is used?
For reference, the generated `enum Spelling` looks like:
```
 3611 public:   


 3612   enum Spelling { 


 3613 GNU_error = 0,


 3614 CXX11_gnu_error = 1,  


 3615 C2x_gnu_error = 2, 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 364892.
nickdesaulniers marked 8 inline comments as done.
nickdesaulniers added a comment.

- rebase on D107613 , reuse 
diag::err_attribute_missing_on_first_decl
- test matching redeclarations
- remove getSpelling method call for diag
- capitalize Match
- convert an assert to a static_assert
- reword diagnostic


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Frontend/backend-attribute-error-warning.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Misc/serialized-diags-driver.c
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll
  llvm/test/ThinLTO/X86/dontcall.ll

Index: llvm/test/ThinLTO/X86/dontcall.ll
===
--- /dev/null
+++ llvm/test/ThinLTO/X86/dontcall.ll
@@ -0,0 +1,33 @@
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: not llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps 2>&1 \
+; RUN:   -r=%t/a.bc,callee,px \
+; RUN:   -r=%t/b.bc,callee,x  \
+; RUN:   -r=%t/b.bc,caller,px
+
+; TODO: As part of LTO, we check that types match, but *we don't yet check that
+; attributes match!!! What should happen if we remove "dontcall" from the
+; definition or declaration of @callee?
+
+; CHECK: call to callee marked "dontcall"
+
+;--- a.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @callee() "dontcall" noinline {
+  ret i32 42
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @callee() "dontcall"
+
+define i32 @caller() {
+entry:
+  %0 = call i32 @callee()
+  ret i32 %0
+}
Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7948,6 +7949,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:76
+def err_fe_backend_error_attr :
+  Error<"call to %0 declared with attribute error: %1">, BackendInfo;
+def warn_fe_backend_warning_attr :





Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:78
+def warn_fe_backend_warning_attr :
+  Warning<"call to %0 declared with attribute warning: %1">, BackendInfo,
+  InGroup;





Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > nickdesaulniers wrote:
> > > > > aaron.ballman wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > GCC doesn't seem to support this spelling -- do they have a 
> > > > > > > > different one we should reuse?
> > > > > > > I think these diagnostics don't have corresponding flags in GCC, 
> > > > > > > so they cannot be disabled.
> > > > > > > 
> > > > > > > Without adding this, clang/test/Misc/warning-flags.c would fail, 
> > > > > > > because I was adding a new unnamed warning 
> > > > > > > `warn_fe_backend_user_diagnostic`.  Perhaps I should not be 
> > > > > > > adding this line here, and doing something else?
> > > > > > > 
> > > > > > > That test says we shouldn't be adding new warnings not controlled 
> > > > > > > by flags, but that's very much my intention.
> > > > > > > 
> > > > > > > Though now `-Wno-user-diagnostic` doesn't produce a 
> > > > > > > `-Wunknown-warning-option` diagnostic...
> > > > > > Ah! I think the warning attribute should be controllable via a 
> > > > > > diagnostic flag (we should always be able to disable warnings with 
> > > > > > some sort of flag) and I don't think the error attribute should be 
> > > > > > controllable (an error is an error and should stop translation, 
> > > > > > same as with `#error`).
> > > > > > 
> > > > > > Normally, I'd say "let's use the same flag that controls 
> > > > > > `#warning`" but...
> > > > > > ```
> > > > > > def PoundWarning : DiagGroup<"#warnings">;
> > > > > > ```
> > > > > > That's... not exactly an obvious flag for the warning attribute. So 
> > > > > > I would probably name it `warning-attributes` similar to how we 
> > > > > > have `deprecated-attributes` already.
> > > > > Done, now `-Wno-warning-attributes` doesn't produce 
> > > > > `-Wunknown-warning-option`, but also doesn't disable the diagnostic.  
> > > > > Was there something else I needed to add?
> > > > huh, that sounds suspicious -- you don't need to do anything special 
> > > > for `-Wno-foo` handling, that should be automatically supported via 
> > > > tablegen. I'm not certain why you're not seeing 
> > > > `-Wno-warning-attributes` silencing the warnings.
> > > Ah! Because I was testing `__attribute__((error("")))` not 
> > > `__attribute__((warning("")))`. `-Wno-warning-attributes` only affects 
> > > the latter, not the former.  I should add a test for 
> > > `-Wno-warning-attributes`! Is this something I also need to add 
> > > documentation for?
> > Given that this behavior surprised you, I certainly wouldn't oppose 
> > mentioning it in the documentation, but I also don't think it's strictly 
> > required. That said, I do agree about adding some additional test coverage 
> > for when the warning is disabled.
> > 
> > Just to double-check: do you think this functionality needs an "escape 
> > hatch" for the error case? e.g., should the `error` attribute generate a 
> > warning diagnostic that defaults to being an error, so we have 
> > `-Wno-warning-attributes` to turn off `warning` attribute diagnostics and 
> > `-Wno-error-attributes` to turn off `error` attribute diagnostics?
> Ah, GCC also controls this via `-Wattribute-warning`; let me rename my 
> implementation.
> 
> > do you think this functionality needs an "escape hatch" for the error case?
> 
> No.  If users don't want an error, then they should prefer 
> `__attribute__((warning("")))` to `__attribute__((error("")))`.
Okay, that's fine by me. If we find a need to give API consumers the ability to 
ignore the `error` attribute, we can always add one later.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11172
 
+def err_attribute_removed_on_redeclaration : Error<
+  "'%0' attribute removed from redeclared variable">;

This diagnostic is a bit confusing to me -- should this be a warning about the 
attribute being ignored in this case, rather than an error? Alternatively, 
should this be re-worded to say that the attribute must appear on the first 
declaration? If the latter, we have some diagnostics that could maybe be 
combined into using a `%select` for this: 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > nickdesaulniers wrote:
> > > > aaron.ballman wrote:
> > > > > nickdesaulniers wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > GCC doesn't seem to support this spelling -- do they have a 
> > > > > > > different one we should reuse?
> > > > > > I think these diagnostics don't have corresponding flags in GCC, so 
> > > > > > they cannot be disabled.
> > > > > > 
> > > > > > Without adding this, clang/test/Misc/warning-flags.c would fail, 
> > > > > > because I was adding a new unnamed warning 
> > > > > > `warn_fe_backend_user_diagnostic`.  Perhaps I should not be adding 
> > > > > > this line here, and doing something else?
> > > > > > 
> > > > > > That test says we shouldn't be adding new warnings not controlled 
> > > > > > by flags, but that's very much my intention.
> > > > > > 
> > > > > > Though now `-Wno-user-diagnostic` doesn't produce a 
> > > > > > `-Wunknown-warning-option` diagnostic...
> > > > > Ah! I think the warning attribute should be controllable via a 
> > > > > diagnostic flag (we should always be able to disable warnings with 
> > > > > some sort of flag) and I don't think the error attribute should be 
> > > > > controllable (an error is an error and should stop translation, same 
> > > > > as with `#error`).
> > > > > 
> > > > > Normally, I'd say "let's use the same flag that controls `#warning`" 
> > > > > but...
> > > > > ```
> > > > > def PoundWarning : DiagGroup<"#warnings">;
> > > > > ```
> > > > > That's... not exactly an obvious flag for the warning attribute. So I 
> > > > > would probably name it `warning-attributes` similar to how we have 
> > > > > `deprecated-attributes` already.
> > > > Done, now `-Wno-warning-attributes` doesn't produce 
> > > > `-Wunknown-warning-option`, but also doesn't disable the diagnostic.  
> > > > Was there something else I needed to add?
> > > huh, that sounds suspicious -- you don't need to do anything special for 
> > > `-Wno-foo` handling, that should be automatically supported via tablegen. 
> > > I'm not certain why you're not seeing `-Wno-warning-attributes` silencing 
> > > the warnings.
> > Ah! Because I was testing `__attribute__((error("")))` not 
> > `__attribute__((warning("")))`. `-Wno-warning-attributes` only affects the 
> > latter, not the former.  I should add a test for `-Wno-warning-attributes`! 
> > Is this something I also need to add documentation for?
> Given that this behavior surprised you, I certainly wouldn't oppose 
> mentioning it in the documentation, but I also don't think it's strictly 
> required. That said, I do agree about adding some additional test coverage 
> for when the warning is disabled.
> 
> Just to double-check: do you think this functionality needs an "escape hatch" 
> for the error case? e.g., should the `error` attribute generate a warning 
> diagnostic that defaults to being an error, so we have 
> `-Wno-warning-attributes` to turn off `warning` attribute diagnostics and 
> `-Wno-error-attributes` to turn off `error` attribute diagnostics?
Ah, GCC also controls this via `-Wattribute-warning`; let me rename my 
implementation.

> do you think this functionality needs an "escape hatch" for the error case?

No.  If users don't want an error, then they should prefer 
`__attribute__((warning("")))` to `__attribute__((error("")))`.



Comment at: llvm/include/llvm/IR/DiagnosticInfo.h:1079
+public:
+  // TODO: make LocCookie optional
+  DiagnosticInfoDontCall(StringRef CalleeName, unsigned LocCookie)

nickdesaulniers wrote:
> TODO: play with llvm::Optional
Ok, that was fun, but I didn't find it made construction sites of 
`DiagnosticInfoDontCall` nicer.  This is also how `DiagnosticInfoInlineAsm` 
works, which is where I got the idea for `!srcloc` from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 364280.
nickdesaulniers marked 5 inline comments as done.
nickdesaulniers edited the summary of this revision.
nickdesaulniers added a comment.

- remove stale todos, change warning to -Wwarning-attributes to match GCC, 
update docs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Frontend/backend-attribute-error-warning.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Misc/serialized-diags-driver.c
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll
  llvm/test/ThinLTO/X86/dontcall.ll

Index: llvm/test/ThinLTO/X86/dontcall.ll
===
--- /dev/null
+++ llvm/test/ThinLTO/X86/dontcall.ll
@@ -0,0 +1,33 @@
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: not llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps 2>&1 \
+; RUN:   -r=%t/a.bc,callee,px \
+; RUN:   -r=%t/b.bc,callee,x  \
+; RUN:   -r=%t/b.bc,caller,px
+
+; TODO: As part of LTO, we check that types match, but *we don't yet check that
+; attributes match!!! What should happen if we remove "dontcall" from the
+; definition or declaration of @callee?
+
+; CHECK: call to callee marked "dontcall"
+
+;--- a.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @callee() "dontcall" noinline {
+  ret i32 42
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @callee() "dontcall"
+
+define i32 @caller() {
+entry:
+  %0 = call i32 @callee()
+  ret i32 %0
+}
Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7918,6 +7919,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:6057
+assertions that depend on optimizations, while providing diagnostics
+pointing to precise locations of the call site in the source.
+  }];

nickdesaulniers wrote:
> @aaron.ballman anything I should add to the documentation blurb?
I think the prose is generally fine, but it might be useful to show some 
trivial example usages to set expectations. e.g.,
```
__attribute__((warning("oh no"))) void unused_func() {} // Not diagnosed 
because it's never called
__attribute__((warning("oh no"))) void used_func() {}
__attribute__((error("oh no"))) int unevaluated_func() { return 0; }
__attribute__((error("oh no"))) constexpr int constexpr_func() { return 12; }

void use() {
  used_func(); // Warning
  sizeof(unevaluated_func()); // No error, function never called
  int array[constexpr_func()]; // No error, function never called
}
```
Feel free to use better/different/more examples, btw. I'm not tied to mine.



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > nickdesaulniers wrote:
> > > > > aaron.ballman wrote:
> > > > > > GCC doesn't seem to support this spelling -- do they have a 
> > > > > > different one we should reuse?
> > > > > I think these diagnostics don't have corresponding flags in GCC, so 
> > > > > they cannot be disabled.
> > > > > 
> > > > > Without adding this, clang/test/Misc/warning-flags.c would fail, 
> > > > > because I was adding a new unnamed warning 
> > > > > `warn_fe_backend_user_diagnostic`.  Perhaps I should not be adding 
> > > > > this line here, and doing something else?
> > > > > 
> > > > > That test says we shouldn't be adding new warnings not controlled by 
> > > > > flags, but that's very much my intention.
> > > > > 
> > > > > Though now `-Wno-user-diagnostic` doesn't produce a 
> > > > > `-Wunknown-warning-option` diagnostic...
> > > > Ah! I think the warning attribute should be controllable via a 
> > > > diagnostic flag (we should always be able to disable warnings with some 
> > > > sort of flag) and I don't think the error attribute should be 
> > > > controllable (an error is an error and should stop translation, same as 
> > > > with `#error`).
> > > > 
> > > > Normally, I'd say "let's use the same flag that controls `#warning`" 
> > > > but...
> > > > ```
> > > > def PoundWarning : DiagGroup<"#warnings">;
> > > > ```
> > > > That's... not exactly an obvious flag for the warning attribute. So I 
> > > > would probably name it `warning-attributes` similar to how we have 
> > > > `deprecated-attributes` already.
> > > Done, now `-Wno-warning-attributes` doesn't produce 
> > > `-Wunknown-warning-option`, but also doesn't disable the diagnostic.  Was 
> > > there something else I needed to add?
> > huh, that sounds suspicious -- you don't need to do anything special for 
> > `-Wno-foo` handling, that should be automatically supported via tablegen. 
> > I'm not certain why you're not seeing `-Wno-warning-attributes` silencing 
> > the warnings.
> Ah! Because I was testing `__attribute__((error("")))` not 
> `__attribute__((warning("")))`. `-Wno-warning-attributes` only affects the 
> latter, not the former.  I should add a test for `-Wno-warning-attributes`! 
> Is this something I also need to add documentation for?
Given that this behavior surprised you, I certainly wouldn't oppose mentioning 
it in the documentation, but I also don't think it's strictly required. That 
said, I do agree about adding some additional test coverage for when the 
warning is disabled.

Just to double-check: do you think this functionality needs an "escape hatch" 
for the error case? e.g., should the `error` attribute generate a warning 
diagnostic that defaults to being an error, so we have 
`-Wno-warning-attributes` to turn off `warning` attribute diagnostics and 
`-Wno-error-attributes` to turn off `error` attribute diagnostics?



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > nickdesaulniers wrote:
> > > > > nickdesaulniers wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > nickdesaulniers wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > I think the usability in this situation is a concern -- only 
> > > > > > > > > having a function name but no source location information is 
> > > > > > > > > pretty tricky. Do you have a sense for how often we would hit 
> > > > > > > > > this case?
> > > > > > > > I don't think we ever encounter it when building the kernel 
> 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:6057
+assertions that depend on optimizations, while providing diagnostics
+pointing to precise locations of the call site in the source.
+  }];

@aaron.ballman anything I should add to the documentation blurb?



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > nickdesaulniers wrote:
> > > > aaron.ballman wrote:
> > > > > GCC doesn't seem to support this spelling -- do they have a different 
> > > > > one we should reuse?
> > > > I think these diagnostics don't have corresponding flags in GCC, so 
> > > > they cannot be disabled.
> > > > 
> > > > Without adding this, clang/test/Misc/warning-flags.c would fail, 
> > > > because I was adding a new unnamed warning 
> > > > `warn_fe_backend_user_diagnostic`.  Perhaps I should not be adding this 
> > > > line here, and doing something else?
> > > > 
> > > > That test says we shouldn't be adding new warnings not controlled by 
> > > > flags, but that's very much my intention.
> > > > 
> > > > Though now `-Wno-user-diagnostic` doesn't produce a 
> > > > `-Wunknown-warning-option` diagnostic...
> > > Ah! I think the warning attribute should be controllable via a diagnostic 
> > > flag (we should always be able to disable warnings with some sort of 
> > > flag) and I don't think the error attribute should be controllable (an 
> > > error is an error and should stop translation, same as with `#error`).
> > > 
> > > Normally, I'd say "let's use the same flag that controls `#warning`" 
> > > but...
> > > ```
> > > def PoundWarning : DiagGroup<"#warnings">;
> > > ```
> > > That's... not exactly an obvious flag for the warning attribute. So I 
> > > would probably name it `warning-attributes` similar to how we have 
> > > `deprecated-attributes` already.
> > Done, now `-Wno-warning-attributes` doesn't produce 
> > `-Wunknown-warning-option`, but also doesn't disable the diagnostic.  Was 
> > there something else I needed to add?
> huh, that sounds suspicious -- you don't need to do anything special for 
> `-Wno-foo` handling, that should be automatically supported via tablegen. I'm 
> not certain why you're not seeing `-Wno-warning-attributes` silencing the 
> warnings.
Ah! Because I was testing `__attribute__((error("")))` not 
`__attribute__((warning("")))`. `-Wno-warning-attributes` only affects the 
latter, not the former.  I should add a test for `-Wno-warning-attributes`! Is 
this something I also need to add documentation for?



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > nickdesaulniers wrote:
> > > > nickdesaulniers wrote:
> > > > > aaron.ballman wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > I think the usability in this situation is a concern -- only 
> > > > > > > > having a function name but no source location information is 
> > > > > > > > pretty tricky. Do you have a sense for how often we would hit 
> > > > > > > > this case?
> > > > > > > I don't think we ever encounter it when building the kernel 
> > > > > > > sources. Perhaps I should make this an assertion on 
> > > > > > > `LocCookie.isValid()` instead?
> > > > > > > 
> > > > > > > While clang should always attach a `srcloc` MetaData node, other 
> > > > > > > frontend might not, so it's optional.  But this TU is strictly 
> > > > > > > Clang.
> > > > > > I thought backend optimization passes would drop source location 
> > > > > > information with some regularity, so I'm a bit hesitant to go with 
> > > > > > an assertion unless it turns out I'm wrong there. However, if the 
> > > > > > backend shouldn't ever lose *this* source location information, 
> > > > > > then I think it's good to make it an assertion.
> > > > > Perhaps debug info, but this SourceLocation is squirreled away in a 
> > > > > Metadata node attached to the call site.
> > > > > 
> > > > > Ah, but via further testing of @arsenm 's point ("what about indirect 
> > > > > calls?"), this case is tricky:
> > > > > ```
> > > > > // clang -O2 foo.c
> > > > > __attribute__((error("oh no foo")))
> > > > > void foo(void);
> > > > > 
> > > > > void (*bar)(void);
> > > > > 
> > > > > void baz(void) {
> > > > >   bar = foo;
> > > > >   bar();
> > > > > }
> > > > > ```
> > > > > since we did not emit the Metadata node on the call, but then later 
> > > > > during optimizations we replaced the call to `bar` with a call to 
> > > > > `foo`. At that point, the front end did not emit a Metadata node with 
> > > > > source location information, 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3819
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > ObjC methods as well?
> > > I guess I can add them, but I'm unfamiliar with the language. If there's 
> > > no GNU implementation of an ObjC compiler, do we need to worry about GNU 
> > > C extensions in ObjC?
> > There's nothing platform-specific about the semantics of these attributes, 
> > so GNU or not doesn't really matter. I'm fine holding off on ObjC methods 
> > until a user requests support for it, but given how these are generally 
> > useful attributes, I think it would make sense to support ObjC up front 
> > unless there's a burden from supporting it.
> Looking at the generated IR for ObjC method calls, I don't think we can 
> support them (easily).
> 
> It looks like method calls in ObjC are lowered to direct calls to 
> `objc_msg_lookup`+indirect CALLs, IIUC.  This BackendDiagnostic relies on 
> observing direct calls during instruction selection. (I suspect ObjC supports 
> runtime modification of classes?) So during instruction selection, we can't 
> know that we're making a method call to a particular method (easily; maybe 
> there's helpers for this though?).  We don't seem to remove the indirection 
> even with optimizations enabled (making me think that methods can be swapped 
> at runtime).
> 
> We could try to diagnose in the frontend, but this very very quickly falls 
> apart if there's any control flow involved.  ie. maybe we could diagnose:
> ```
> void foo(void) { dontcall(); }
> ```
> but this whole BackendDiagnostic relies on the backend to run optimizations; 
> the frontend couldn't diagnose:
> ```
> void foo(void) { if (myRuntimeAssertion) dontcall(); }
> ```
> which is the predominate use case for this fn attr.
> 
> That said, the patch as is works for functions (not ObjC methods) in 
> objective-c mode already; would you like a test for that?
> 
> Marking as Done, please reopen this thread if I'm mistaken in this analysis.
> That said, the patch as is works for functions (not ObjC methods) in 
> objective-c mode already; would you like a test for that?

No thanks, I think the existing C coverage handles that.

> Marking as Done, please reopen this thread if I'm mistaken in this analysis.

Thanks for checking this out, I think it's a good reason to say "let's deal 
with this later (if ever)".



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > aaron.ballman wrote:
> > > > > nickdesaulniers wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > I think the usability in this situation is a concern -- only 
> > > > > > > having a function name but no source location information is 
> > > > > > > pretty tricky. Do you have a sense for how often we would hit 
> > > > > > > this case?
> > > > > > I don't think we ever encounter it when building the kernel 
> > > > > > sources. Perhaps I should make this an assertion on 
> > > > > > `LocCookie.isValid()` instead?
> > > > > > 
> > > > > > While clang should always attach a `srcloc` MetaData node, other 
> > > > > > frontend might not, so it's optional.  But this TU is strictly 
> > > > > > Clang.
> > > > > I thought backend optimization passes would drop source location 
> > > > > information with some regularity, so I'm a bit hesitant to go with an 
> > > > > assertion unless it turns out I'm wrong there. However, if the 
> > > > > backend shouldn't ever lose *this* source location information, then 
> > > > > I think it's good to make it an assertion.
> > > > Perhaps debug info, but this SourceLocation is squirreled away in a 
> > > > Metadata node attached to the call site.
> > > > 
> > > > Ah, but via further testing of @arsenm 's point ("what about indirect 
> > > > calls?"), this case is tricky:
> > > > ```
> > > > // clang -O2 foo.c
> > > > __attribute__((error("oh no foo")))
> > > > void foo(void);
> > > > 
> > > > void (*bar)(void);
> > > > 
> > > > void baz(void) {
> > > >   bar = foo;
> > > >   bar();
> > > > }
> > > > ```
> > > > since we did not emit the Metadata node on the call, but then later 
> > > > during optimizations we replaced the call to `bar` with a call to 
> > > > `foo`. At that point, the front end did not emit a Metadata node with 
> > > > source location information, so we can't reconstruct the call site 
> > > > properly for the diagnostic. Hmm...I'll need to think more about this 
> > > > case.
> > > One thing I was thinking of to support @arsenm's case:
> > > 
> > > We probably generally could support diagnosing indirect 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3819
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > ObjC methods as well?
> > I guess I can add them, but I'm unfamiliar with the language. If there's no 
> > GNU implementation of an ObjC compiler, do we need to worry about GNU C 
> > extensions in ObjC?
> There's nothing platform-specific about the semantics of these attributes, so 
> GNU or not doesn't really matter. I'm fine holding off on ObjC methods until 
> a user requests support for it, but given how these are generally useful 
> attributes, I think it would make sense to support ObjC up front unless 
> there's a burden from supporting it.
Looking at the generated IR for ObjC method calls, I don't think we can support 
them (easily).

It looks like method calls in ObjC are lowered to direct calls to 
`objc_msg_lookup`+indirect CALLs, IIUC.  This BackendDiagnostic relies on 
observing direct calls during instruction selection. (I suspect ObjC supports 
runtime modification of classes?) So during instruction selection, we can't 
know that we're making a method call to a particular method (easily; maybe 
there's helpers for this though?).  We don't seem to remove the indirection 
even with optimizations enabled (making me think that methods can be swapped at 
runtime).

We could try to diagnose in the frontend, but this very very quickly falls 
apart if there's any control flow involved.  ie. maybe we could diagnose:
```
void foo(void) { dontcall(); }
```
but this whole BackendDiagnostic relies on the backend to run optimizations; 
the frontend couldn't diagnose:
```
void foo(void) { if (myRuntimeAssertion) dontcall(); }
```
which is the predominate use case for this fn attr.

That said, the patch as is works for functions (not ObjC methods) in 
objective-c mode already; would you like a test for that?

Marking as Done, please reopen this thread if I'm mistaken in this analysis.



Comment at: clang/include/clang/Basic/AttrDocs.td:6026
+depend on optimizations, while providing diagnostics pointing to precise
+locations of the call site in the source.
+  }];

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > nickdesaulniers wrote:
> > > > aaron.ballman wrote:
> > > > > I think the documentation for these should probably be combined into 
> > > > > one documentation blob; the only difference in behavior is whether 
> > > > > the diagnostic is a warning or an attribute.
> > > > > 
> > > > > I think the documentation needs to go into more details about how 
> > > > > this attribute works in practice. For example, what should I expect 
> > > > > from code like:
> > > > > ```
> > > > > struct Base {
> > > > >   __attribute__((warning("derp"))) virtual void foo();
> > > > > };
> > > > > 
> > > > > struct Derived : Base {
> > > > >   void foo() override; // Does calling this also warn?
> > > > > };
> > > > > 
> > > > > __attribute__((error("DERP!"))) void func() { // external function 
> > > > > symbol!
> > > > >   func(); // Does this diagnose?
> > > > > }
> > > > > ```
> > > > > I suppose another interesting question given the design is to use the 
> > > > > optimizer is: what about LTO? Say I have:
> > > > > ```
> > > > > // TU1.c
> > > > > __attribute__((error("Derp Derp Derp"))) void func(void);
> > > > > 
> > > > > // TU.c
> > > > > extern void func(void);
> > > > > void blerp(void) { func(); }
> > > > > ```
> > > > > What should happen and does LTO change the answer?
> > > > > I think the documentation for these should probably be combined into 
> > > > > one documentation blob
> > > > 
> > > > I'm having trouble sharing the documentation block between the two. 
> > > > I've tried declaring a `code` or trying to inherit from a base class, 
> > > > but I can't seem to get either to build.  What's the preferred method 
> > > > to do so here?
> > > > 
> > > > >   void foo() override; // Does calling this also warn?
> > > > 
> > > > Virtual methods calls do not diagnose, in g++ or my implementation 
> > > > here, regardless of optimization level.
> > > > 
> > > > >   func(); // Does this diagnose?
> > > > 
> > > > Yes, unless the infinite recursion is removed via optimization; both in 
> > > > gcc and my implementation here.
> > > > 
> > > > > What should happen and does LTO change the answer?
> > > > 
> > > > This reminds me of having non-matching declarations for the same 
> > > > symbol.  I'm not sure we should codify what should happen in such a 
> > > > case.  garbage in, garbage out. Or do we merge definitions as part of 
> > > > LTO?
> > > > What's the preferred method to do so here?
> > > 
> > > Write the `Documentation` subclass once in 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 363888.
nickdesaulniers marked 8 inline comments as done.
nickdesaulniers edited the summary of this revision.
nickdesaulniers added a comment.
Herald added subscribers: ormris, steven_wu.

- LTO test, diagnose inheritability


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Frontend/backend-attribute-error-warning.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll
  llvm/test/ThinLTO/X86/dontcall.ll

Index: llvm/test/ThinLTO/X86/dontcall.ll
===
--- /dev/null
+++ llvm/test/ThinLTO/X86/dontcall.ll
@@ -0,0 +1,33 @@
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: not llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps 2>&1 \
+; RUN:   -r=%t/a.bc,callee,px \
+; RUN:   -r=%t/b.bc,callee,x  \
+; RUN:   -r=%t/b.bc,caller,px
+
+; TODO: As part of LTO, we check that types match, but *we don't yet check that
+; attributes match!!! What should happen if we remove "dontcall" from the
+; definition or declaration of @callee?
+
+; CHECK: call to callee marked "dontcall"
+
+;--- a.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @callee() "dontcall" noinline {
+  ret i32 42
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @callee() "dontcall"
+
+define i32 @caller() {
+entry:
+  %0 = call i32 @callee()
+  ret i32 %0
+}
Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7918,6 +7919,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
===
--- 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+  let Spellings = [GCC<"error">];

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > I think this should be an inheritable attribute (same below) so that 
> > > > redeclarations get the marking as well.
> > > > 
> > > > However, this does make for a bit of a novel situation with regards to 
> > > > other attributes. The typical behavior for function attributes is:
> > > > ```
> > > > void func(void);
> > > > 
> > > > void use1(void) {
> > > >   func(); // Func is not marked yet, no attribute behavior
> > > > }
> > > > 
> > > > void func(void) __attribute__((whatever)));
> > > > 
> > > > void use2(void) {
> > > >   func(); // Func is marked, attribute does something
> > > > }
> > > > 
> > > > void func(void) {} // Func is still marked because the attribute is 
> > > > inheritted
> > > > 
> > > > void use3(void) {
> > > >   func(); // Func is marked, attribute does something
> > > > ```
> > > > but because all of the interesting work is happening in the backend, I 
> > > > believe the unmarked use will still act as though the attribute was 
> > > > marked.
> > > Changing this def to:
> > > ```
> > > -def Error : Attr {
> > > +def Error : InheritableAttr {
> > > ```
> > > doesn't seem to make your test case work; is there some other method I 
> > > should be using to find the re-declaration and check the attributes 
> > > against that?
> > That's what I meant by this being novel -- I don't think those test cases 
> > will work with this design (having the backend decide whether to diagnose), 
> > and so this attribute will behave somewhat inconsistently with other 
> > function attributes. This may cause confusion for users or for tooling. I 
> > think for users, we can document the behavior and that will be fine. For 
> > tooling, we may have to get more creative (such as applying the attribute 
> > to all redeclarations of the same function), but perhaps we can hold off on 
> > that until we see if tooling runs into a problem in practice.
> I'm having a very difficult time getting inheritable attr's to work correctly.
> 
> For example:
> ```
> void foo(void);
> 
> void x0(void) { foo(); }
> 
> __attribute__((error("oh no foo")))
> void foo(void);
> 
> void x1(void) { foo(); }
> 
> void foo(void);
> 
> void x2(void) { foo(); }
> ```
> 
> I can get all `Decl`s of `foo` to have the attribute (via `Inherit` in the 
> ast dump), but the first call to `foo()` doesn't diagnose; at that point we 
> haven't encountered a `Decl` that has the attribute.  So it's hard to match 
> GCC's behavior of warning on all three.  Is there something I should be doing 
> differently in `CodeGenModule::SetFunctionAttributes` to check the "final 
> decl" rather than the current/latest `Decl`?
We spoke about this a bit off-list and I think we're going to try requiring 
writing the attribute on the first declaration. I think that might give the 
most intuitive behavior for this attribute.



Comment at: clang/include/clang/Basic/AttrDocs.td:6026
+depend on optimizations, while providing diagnostics pointing to precise
+locations of the call site in the source.
+  }];

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > I think the documentation for these should probably be combined into 
> > > > one documentation blob; the only difference in behavior is whether the 
> > > > diagnostic is a warning or an attribute.
> > > > 
> > > > I think the documentation needs to go into more details about how this 
> > > > attribute works in practice. For example, what should I expect from 
> > > > code like:
> > > > ```
> > > > struct Base {
> > > >   __attribute__((warning("derp"))) virtual void foo();
> > > > };
> > > > 
> > > > struct Derived : Base {
> > > >   void foo() override; // Does calling this also warn?
> > > > };
> > > > 
> > > > __attribute__((error("DERP!"))) void func() { // external function 
> > > > symbol!
> > > >   func(); // Does this diagnose?
> > > > }
> > > > ```
> > > > I suppose another interesting question given the design is to use the 
> > > > optimizer is: what about LTO? Say I have:
> > > > ```
> > > > // TU1.c
> > > > __attribute__((error("Derp Derp Derp"))) void func(void);
> > > > 
> > > > // TU.c
> > > > extern void func(void);
> > > > void blerp(void) { func(); }
> > > > ```
> > > > What should happen and does LTO change the answer?
> > > > I think the documentation for these should probably be combined into 
> > > > one documentation blob
> > > 
> > > I'm having trouble sharing the documentation block between the two. I've 
> > > tried declaring a `code` or trying to inherit from a base class, but I 
> > > can't seem to get either to build.  What's the preferred method to do so 
> > > here?
> > > 
> > > >   void 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+  let Spellings = [GCC<"error">];

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > I think this should be an inheritable attribute (same below) so that 
> > > redeclarations get the marking as well.
> > > 
> > > However, this does make for a bit of a novel situation with regards to 
> > > other attributes. The typical behavior for function attributes is:
> > > ```
> > > void func(void);
> > > 
> > > void use1(void) {
> > >   func(); // Func is not marked yet, no attribute behavior
> > > }
> > > 
> > > void func(void) __attribute__((whatever)));
> > > 
> > > void use2(void) {
> > >   func(); // Func is marked, attribute does something
> > > }
> > > 
> > > void func(void) {} // Func is still marked because the attribute is 
> > > inheritted
> > > 
> > > void use3(void) {
> > >   func(); // Func is marked, attribute does something
> > > ```
> > > but because all of the interesting work is happening in the backend, I 
> > > believe the unmarked use will still act as though the attribute was 
> > > marked.
> > Changing this def to:
> > ```
> > -def Error : Attr {
> > +def Error : InheritableAttr {
> > ```
> > doesn't seem to make your test case work; is there some other method I 
> > should be using to find the re-declaration and check the attributes against 
> > that?
> That's what I meant by this being novel -- I don't think those test cases 
> will work with this design (having the backend decide whether to diagnose), 
> and so this attribute will behave somewhat inconsistently with other function 
> attributes. This may cause confusion for users or for tooling. I think for 
> users, we can document the behavior and that will be fine. For tooling, we 
> may have to get more creative (such as applying the attribute to all 
> redeclarations of the same function), but perhaps we can hold off on that 
> until we see if tooling runs into a problem in practice.
I'm having a very difficult time getting inheritable attr's to work correctly.

For example:
```
void foo(void);

void x0(void) { foo(); }

__attribute__((error("oh no foo")))
void foo(void);

void x1(void) { foo(); }

void foo(void);

void x2(void) { foo(); }
```

I can get all `Decl`s of `foo` to have the attribute (via `Inherit` in the ast 
dump), but the first call to `foo()` doesn't diagnose; at that point we haven't 
encountered a `Decl` that has the attribute.  So it's hard to match GCC's 
behavior of warning on all three.  Is there something I should be doing 
differently in `CodeGenModule::SetFunctionAttributes` to check the "final decl" 
rather than the current/latest `Decl`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > I think the usability in this situation is a concern -- only having a 
> > > > function name but no source location information is pretty tricky. Do 
> > > > you have a sense for how often we would hit this case?
> > > I don't think we ever encounter it when building the kernel sources. 
> > > Perhaps I should make this an assertion on `LocCookie.isValid()` instead?
> > > 
> > > While clang should always attach a `srcloc` MetaData node, other frontend 
> > > might not, so it's optional.  But this TU is strictly Clang.
> > I thought backend optimization passes would drop source location 
> > information with some regularity, so I'm a bit hesitant to go with an 
> > assertion unless it turns out I'm wrong there. However, if the backend 
> > shouldn't ever lose *this* source location information, then I think it's 
> > good to make it an assertion.
> Perhaps debug info, but this SourceLocation is squirreled away in a Metadata 
> node attached to the call site.
> 
> Ah, but via further testing of @arsenm 's point ("what about indirect 
> calls?"), this case is tricky:
> ```
> // clang -O2 foo.c
> __attribute__((error("oh no foo")))
> void foo(void);
> 
> void (*bar)(void);
> 
> void baz(void) {
>   bar = foo;
>   bar();
> }
> ```
> since we did not emit the Metadata node on the call, but then later during 
> optimizations we replaced the call to `bar` with a call to `foo`. At that 
> point, the front end did not emit a Metadata node with source location 
> information, so we can't reconstruct the call site properly for the 
> diagnostic. Hmm...I'll need to think more about this case.
One thing I was thinking of to support @arsenm's case:

We probably generally could support diagnosing indirect calls if `-g` was used 
to emit `DILocation` MD nodes.  I'd need to change this up from a custom 
`!srcloc` node with one `unsigned` that is a `SourceLocation` to use the the 
`DILocation` tuple of `line`, `column`, and `scope` then change the frontend's 
`BackendDiagnosticConsumer` to reinstantiate a `SourceLocation` from those (if 
possible).

The only thing is: is it bad if we can only diagnose with `-g` for indirect 
calls?

Otherwise I don't see how we can support diagnosing indirect calls. GCC can.  
We'd perhaps need to emit `!srcloc` or `DILocation` nodes for EVERY indirect 
call from the frontend, and add logic to merge these or something during 
optimizations when we turn indirect calls into direct calls.  And I'm sure that 
would cause lots of churn in the LLVM source tree / in the tests. I would 
create a parent revision to precommit that.  But I think that's maybe too 
overkill?

The current behavior is that we don't diagnose indirect calls.  I'm happy to 
add documentation, or create child or parent revisions trying to tackle that, 
but I think if we can't provide accurate debug info, it's perhaps better not to 
diagnose at all.

I could change that so that we still emit the diagnostic, but don't show 
_where_ the callsite was in the sources as part of the diagnostic.  But I think 
it's perhaps better to simply not warn in that case when we can't provide line 
info.

But this can go either way, so I appreciate others' guidance and thoughts here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:6026
+depend on optimizations, while providing diagnostics pointing to precise
+locations of the call site in the source.
+  }];

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > I think the documentation for these should probably be combined into one 
> > > documentation blob; the only difference in behavior is whether the 
> > > diagnostic is a warning or an attribute.
> > > 
> > > I think the documentation needs to go into more details about how this 
> > > attribute works in practice. For example, what should I expect from code 
> > > like:
> > > ```
> > > struct Base {
> > >   __attribute__((warning("derp"))) virtual void foo();
> > > };
> > > 
> > > struct Derived : Base {
> > >   void foo() override; // Does calling this also warn?
> > > };
> > > 
> > > __attribute__((error("DERP!"))) void func() { // external function symbol!
> > >   func(); // Does this diagnose?
> > > }
> > > ```
> > > I suppose another interesting question given the design is to use the 
> > > optimizer is: what about LTO? Say I have:
> > > ```
> > > // TU1.c
> > > __attribute__((error("Derp Derp Derp"))) void func(void);
> > > 
> > > // TU.c
> > > extern void func(void);
> > > void blerp(void) { func(); }
> > > ```
> > > What should happen and does LTO change the answer?
> > > I think the documentation for these should probably be combined into one 
> > > documentation blob
> > 
> > I'm having trouble sharing the documentation block between the two. I've 
> > tried declaring a `code` or trying to inherit from a base class, but I 
> > can't seem to get either to build.  What's the preferred method to do so 
> > here?
> > 
> > >   void foo() override; // Does calling this also warn?
> > 
> > Virtual methods calls do not diagnose, in g++ or my implementation here, 
> > regardless of optimization level.
> > 
> > >   func(); // Does this diagnose?
> > 
> > Yes, unless the infinite recursion is removed via optimization; both in gcc 
> > and my implementation here.
> > 
> > > What should happen and does LTO change the answer?
> > 
> > This reminds me of having non-matching declarations for the same symbol.  
> > I'm not sure we should codify what should happen in such a case.  garbage 
> > in, garbage out. Or do we merge definitions as part of LTO?
> > What's the preferred method to do so here?
> 
> Write the `Documentation` subclass once in AttrDocs.td and refer to it in 
> both attributes in Attr.td (IIRC, this may require adding a `let Heading = 
> "whatever"` in AttrDocs.td). However, I think there should only be one 
> attribute in Attr.td and so I think the documentation sharing issue will fall 
> out naturally from there.
> 
> > Virtual methods calls do not diagnose, in g++ or my implementation here, 
> > regardless of optimization level.
> 
> Assuming the base method is marked and the derived is not, if this means you 
> don't warn on `SomeDerived->foo()`, then I think that makes sense but if that 
> means you don't warn on `SomeBase->foo()`, then I think there's an issue.
> 
> > This reminds me of having non-matching declarations for the same symbol. 
> > I'm not sure we should codify what should happen in such a case. garbage 
> > in, garbage out. Or do we merge definitions as part of LTO?
> 
> I guess I don't see this as garbage in so much as trying to verify that the 
> behavior under this patch isn't unreasonable. Naively, I would expect no 
> diagnostic from that case because I wouldn't expect the compiler to be able 
> to "see" the annotation in the other TU. But I have no idea if that's 
> actually the behavior.
> What should happen and does LTO change the answer?
> But I have no idea if that's actually the behavior.

Well for that input, the TU1.c's declaration is not used and the declaration in 
TU.c does not match.  So it's malformed input IMO, just as if in a non-lto case 
declarations didn't match between TUs.  So that is the actual behavior, but I'm 
not sure whether documenting what we have for bad input ossifies the behavior?  
I'd be much more open for adding a test for LTO for well formed input (ie. 
declarations match).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 363581.
nickdesaulniers marked 2 inline comments as done.
nickdesaulniers edited the summary of this revision.
nickdesaulniers added a comment.

- reusing existing Diag/Notes, remove test comment, add test for indirect call


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Frontend/backend-attribute-error-warning.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll

Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7918,6 +7919,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -75,6 +75,7 @@
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/GlobalValue.h"
@@ -1151,6 +1152,16 @@
   CLI.setCallee(RetTy, FuncTy, CI->getCalledOperand(), std::move(Args), *CI)
   .setTailCall(IsTailCall);
 
+  if (const Function *F = CI->getCalledFunction())
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = CI->getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  F->getContext().diagnose(D);
+}
+
   return lowerCallTo(CLI);
 }
 
Index: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
===
--- llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -47,6 +47,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:375-378
+def warn_changed_error_warning_attr_text : Warning<
+  "duplicate attribute changes text from %0 to %1">,
+  InGroup;
+

FWIW, elsewhere we typically use `warn_duplicate_attribute` and 
`note_previous_attribute` as a pair. It probably makes sense to move those to 
be common diagnostics instead of adding a new one with a new warning group.



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 

nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > GCC doesn't seem to support this spelling -- do they have a different 
> > > > one we should reuse?
> > > I think these diagnostics don't have corresponding flags in GCC, so they 
> > > cannot be disabled.
> > > 
> > > Without adding this, clang/test/Misc/warning-flags.c would fail, because 
> > > I was adding a new unnamed warning `warn_fe_backend_user_diagnostic`.  
> > > Perhaps I should not be adding this line here, and doing something else?
> > > 
> > > That test says we shouldn't be adding new warnings not controlled by 
> > > flags, but that's very much my intention.
> > > 
> > > Though now `-Wno-user-diagnostic` doesn't produce a 
> > > `-Wunknown-warning-option` diagnostic...
> > Ah! I think the warning attribute should be controllable via a diagnostic 
> > flag (we should always be able to disable warnings with some sort of flag) 
> > and I don't think the error attribute should be controllable (an error is 
> > an error and should stop translation, same as with `#error`).
> > 
> > Normally, I'd say "let's use the same flag that controls `#warning`" but...
> > ```
> > def PoundWarning : DiagGroup<"#warnings">;
> > ```
> > That's... not exactly an obvious flag for the warning attribute. So I would 
> > probably name it `warning-attributes` similar to how we have 
> > `deprecated-attributes` already.
> Done, now `-Wno-warning-attributes` doesn't produce 
> `-Wunknown-warning-option`, but also doesn't disable the diagnostic.  Was 
> there something else I needed to add?
huh, that sounds suspicious -- you don't need to do anything special for 
`-Wno-foo` handling, that should be automatically supported via tablegen. I'm 
not certain why you're not seeing `-Wno-warning-attributes` silencing the 
warnings.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:949-960
+static void handleErrorAttr(Sema , Decl *D, const ParsedAttr ) {
+  StringRef Str;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
+return;
+  D->addAttr(::new (S.Context) ErrorAttr(S.Context, AL, Str));
+}
+static void handleWarningAttr(Sema , Decl *D, const ParsedAttr ) {

nickdesaulniers wrote:
> aaron.ballman wrote:
> > Pretty sure you can get rid of the manual handlers and just use 
> > `SimpleHandler = 1` in Attr.td, unless we need to add extra diagnostic 
> > logic (which we might need to do for attribute merging).
> It seems that the use of `Args` in the tablegen definitions is incompatible 
> with `SimpleHander`:
> ```
> In file included from 
> /android0/llvm-project/clang/lib/Sema/ParsedAttr.cpp:108:
> tools/clang/include/clang/Sema/AttrParsedAttrImpl.inc:4039:32: error: no 
> matching constructor for initialization of 'clang::ErrorAttr'
>   D->addAttr(::new (S.Context) ErrorAttr(S.Context, Attr));
>^ ~~~
> tools/clang/include/clang/AST/Attrs.inc:3601:7: note: candidate constructor 
> (the implicit copy constructor) not viable: requires 1 argument, but 2 were 
> provided
> class ErrorAttr : public Attr {
>   ^
> tools/clang/include/clang/AST/Attrs.inc:3601:7: note: candidate constructor 
> (the implicit move constructor) not viable: requires 1 argument, but 2 were 
> provided
> tools/clang/include/clang/AST/Attrs.inc:3624:3: note: candidate constructor 
> not viable: requires 3 arguments, but 2 were provided
> ```
> where the 3rd argument would be the `llvm::StringRef UserDiagnostic`.
Oh yeah, derp, that's my mistake. Also, it's moot given that we want to check 
merging logic anyway (often the handler will call the merge function to 
generate the semantic attribute to add to the declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/test/Frontend/backend-attribute-error-warning.c:29
+  duplicate_errors(); // expected-error {{call to 'duplicate_errors' declared 
with attribute error: two}}
+  // TODO: why is this a warning not an error
+  duplicate_warnings(); // expected-warning {{call to 'duplicate_warnings' 
declared with attribute warning: two}}

because the callee has a `warning` attribute! duh...TODO: delete comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+  let Spellings = [GCC<"warning">];

aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > Given that the only functional difference between these attributes is the 
> > > diagnostic level, I sort of wonder whether we should have one semantic 
> > > attribute with two spellings (one for warning, one for error) and an 
> > > accessor field to distinguish which is which.
> > > 
> > > Another interesting question is: are these attributes mutually exclusive? 
> > > Can they be duplicated (perhaps across declarations)? What happens if the 
> > > messages differ? (We may need some attribute merging logic to catch these 
> > > situations.)
> > > I sort of wonder whether we should have one semantic attribute with two 
> > > spellings 
> > 
> > We'd need to be able to distinguish between the `Spellings` at runtime, I 
> > think. It looks like those are private to `ParsedAttrInfo`s, so I'm not 
> > sure how we could check that.
> > 
> > I think we'd want to somehow check the spelling in the code I added to 
> > `clang/lib/Sema/SemaDeclAttr.cpp`? Or is there a different approach that 
> > might work better?
> > 
> > > are these attributes mutually exclusive?
> > 
> > ```
> > __attribute__((error("err"),warning("warn"),error("err2")))
> > void foo(void);
> > 
> > void x1(void) { foo(); }
> > ```
> > in GCC produces a warning with message "warn" and error with message 
> > "err2".  In my current implementation, we error once with message "err".  
> > So I probably should check for multiple instances of the attribute, and use 
> > the message from the latest instance.  Oh, but I'm just calling 
> > `getUserDiagnostic` which was a table-gen'd getter; how do I even specify 
> > which instance of the attribute when there are multiple?  For example:
> > ```
> > __attribute__((error("err"),error("err2")))
> > ```
> > calls to `getUserDiagnostic` produce `err`...
> > 
> > > We may need some attribute merging logic to catch these situations.
> > 
> > So we don't currently have anything for that? What happens with 
> > `__attribute__(alias("")))` when multiple are given? ex. 
> > `__attribute__(alias("bar"),alias("foo"))`
> > We'd need to be able to distinguish between the Spellings at runtime, I 
> > think. It looks like those are private to ParsedAttrInfos, so I'm not sure 
> > how we could check that.
> 
> That's the "accessor" thing I was talking about.
> ```
> def Diagnostic : InheritableAttr {
>   let Spellings = [GCC<"warning">, GCC<"error">];
>   ...
>   let Accessors = [Accessor<"isError", [GCC<"error">]>,
> Accessor<"isWarning", [GCC<"warning">]>];
>   ...
> }
> ```
> Would let you do:
> ```
> DiagnosticAttr *DA = ...;
> if (DA->isError()) ...
> ...
> if (DA->isWarning()) ...
> ```
> 
> > So I probably should check for multiple instances of the attribute, and use 
> > the message from the latest instance.
> 
> I don't think we should follow GCC's lead and produce both a warning and an 
> error diagnostic. I think if the attributes are consistent (both error or 
> both warning), then we should warn if the text is different but use the last 
> attribute's argument as the message (if the text is the same, we should not 
> diagnose at all); if the attributes are inconsistent, I think we should 
> probably give an error rather than a warning, but a case could be made to 
> warn and let the last attribute "win". I consider different arguments or 
> different attributes to be an indication of programmer confusion that's worth 
> the user dealing with.
> 
> > So we don't currently have anything for that? 
> 
> We do have a bunch of things for this. See `mergeDeclAttribute()` in 
> SemaDecl.cpp for the typical code pattern.
> I don't think we should follow GCC's lead and produce both a warning and an 
> error diagnostic. I think if the attributes are consistent (both error or 
> both warning), then we should warn if the text is different but use the last 
> attribute's argument as the message (if the text is the same, we should not 
> diagnose at all); if the attributes are inconsistent, I think we should 
> probably give an error rather than a warning, but a case could be made to 
> warn and let the last attribute "win". I consider different arguments or 
> different attributes to be an indication of programmer confusion that's worth 
> the user dealing with.

GCC's behavior is basically the same as our `ArgList::getLastArg()` (but for 
attributes); the warning and error attributes aren't mutually exclusive, and we 
simply accept the last instance's string as the message to display. We'd need 
to then check for the presence of both.

Ah, also, it seems that `mergeDeclAttribute` is useful for inheritable 
attributes, example test case:
```
void foo(void);

void x0(void) { foo(); }

__attribute__((error("oh no foo")))
void foo(void);

void x1(void) { 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 360324.
nickdesaulniers marked 4 inline comments as done.
nickdesaulniers edited the summary of this revision.
nickdesaulniers added a comment.

- merge ErrorAttr with WarningAttr
- fix dumb short circuit bug
- rename diag group
- handle multiple instances of the attribute on one fn

Note: I'll be offline the next few days; still plenty of TODOs to work out, but
leaving this in the review queue for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Frontend/backend-attribute-error-warning.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll

Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7918,6 +7919,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -75,6 +75,7 @@
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/GlobalValue.h"
@@ -1151,6 +1152,16 @@
   CLI.setCallee(RetTy, FuncTy, CI->getCalledOperand(), std::move(Args), *CI)
   .setTailCall(IsTailCall);
 
+  if (const Function *F = CI->getCalledFunction())
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = CI->getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  F->getContext().diagnose(D);
+}
+
   return lowerCallTo(CLI);
 }
 
Index: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
===
--- llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+  let Spellings = [GCC<"error">];

nickdesaulniers wrote:
> aaron.ballman wrote:
> > I think this should be an inheritable attribute (same below) so that 
> > redeclarations get the marking as well.
> > 
> > However, this does make for a bit of a novel situation with regards to 
> > other attributes. The typical behavior for function attributes is:
> > ```
> > void func(void);
> > 
> > void use1(void) {
> >   func(); // Func is not marked yet, no attribute behavior
> > }
> > 
> > void func(void) __attribute__((whatever)));
> > 
> > void use2(void) {
> >   func(); // Func is marked, attribute does something
> > }
> > 
> > void func(void) {} // Func is still marked because the attribute is 
> > inheritted
> > 
> > void use3(void) {
> >   func(); // Func is marked, attribute does something
> > ```
> > but because all of the interesting work is happening in the backend, I 
> > believe the unmarked use will still act as though the attribute was marked.
> Changing this def to:
> ```
> -def Error : Attr {
> +def Error : InheritableAttr {
> ```
> doesn't seem to make your test case work; is there some other method I should 
> be using to find the re-declaration and check the attributes against that?
That's what I meant by this being novel -- I don't think those test cases will 
work with this design (having the backend decide whether to diagnose), and so 
this attribute will behave somewhat inconsistently with other function 
attributes. This may cause confusion for users or for tooling. I think for 
users, we can document the behavior and that will be fine. For tooling, we may 
have to get more creative (such as applying the attribute to all redeclarations 
of the same function), but perhaps we can hold off on that until we see if 
tooling runs into a problem in practice.



Comment at: clang/include/clang/Basic/Attr.td:3819
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];

nickdesaulniers wrote:
> aaron.ballman wrote:
> > ObjC methods as well?
> I guess I can add them, but I'm unfamiliar with the language. If there's no 
> GNU implementation of an ObjC compiler, do we need to worry about GNU C 
> extensions in ObjC?
There's nothing platform-specific about the semantics of these attributes, so 
GNU or not doesn't really matter. I'm fine holding off on ObjC methods until a 
user requests support for it, but given how these are generally useful 
attributes, I think it would make sense to support ObjC up front unless there's 
a burden from supporting it.



Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+  let Spellings = [GCC<"warning">];

nickdesaulniers wrote:
> aaron.ballman wrote:
> > Given that the only functional difference between these attributes is the 
> > diagnostic level, I sort of wonder whether we should have one semantic 
> > attribute with two spellings (one for warning, one for error) and an 
> > accessor field to distinguish which is which.
> > 
> > Another interesting question is: are these attributes mutually exclusive? 
> > Can they be duplicated (perhaps across declarations)? What happens if the 
> > messages differ? (We may need some attribute merging logic to catch these 
> > situations.)
> > I sort of wonder whether we should have one semantic attribute with two 
> > spellings 
> 
> We'd need to be able to distinguish between the `Spellings` at runtime, I 
> think. It looks like those are private to `ParsedAttrInfo`s, so I'm not sure 
> how we could check that.
> 
> I think we'd want to somehow check the spelling in the code I added to 
> `clang/lib/Sema/SemaDeclAttr.cpp`? Or is there a different approach that 
> might work better?
> 
> > are these attributes mutually exclusive?
> 
> ```
> __attribute__((error("err"),warning("warn"),error("err2")))
> void foo(void);
> 
> void x1(void) { foo(); }
> ```
> in GCC produces a warning with message "warn" and error with message "err2".  
> In my current implementation, we error once with message "err".  So I 
> probably should check for multiple instances of the attribute, and use the 
> message from the latest instance.  Oh, but I'm just calling 
> `getUserDiagnostic` which was a table-gen'd getter; how do I even specify 
> which instance of the attribute when there are multiple?  For example:
> ```
> __attribute__((error("err"),error("err2")))
> ```
> calls to `getUserDiagnostic` produce `err`...
> 
> > We may need some attribute merging logic to catch these situations.
> 
> So we don't currently have anything for that? What happens with 
> `__attribute__(alias("")))` when multiple are given? ex. 
> `__attribute__(alias("bar"),alias("foo"))`
> We'd need to be able to distinguish between the Spellings at runtime, I 
> 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+  let Spellings = [GCC<"error">];

aaron.ballman wrote:
> I think this should be an inheritable attribute (same below) so that 
> redeclarations get the marking as well.
> 
> However, this does make for a bit of a novel situation with regards to other 
> attributes. The typical behavior for function attributes is:
> ```
> void func(void);
> 
> void use1(void) {
>   func(); // Func is not marked yet, no attribute behavior
> }
> 
> void func(void) __attribute__((whatever)));
> 
> void use2(void) {
>   func(); // Func is marked, attribute does something
> }
> 
> void func(void) {} // Func is still marked because the attribute is inheritted
> 
> void use3(void) {
>   func(); // Func is marked, attribute does something
> ```
> but because all of the interesting work is happening in the backend, I 
> believe the unmarked use will still act as though the attribute was marked.
Changing this def to:
```
-def Error : Attr {
+def Error : InheritableAttr {
```
doesn't seem to make your test case work; is there some other method I should 
be using to find the re-declaration and check the attributes against that?



Comment at: clang/include/clang/Basic/Attr.td:3819
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];

aaron.ballman wrote:
> ObjC methods as well?
I guess I can add them, but I'm unfamiliar with the language. If there's no GNU 
implementation of an ObjC compiler, do we need to worry about GNU C extensions 
in ObjC?



Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+  let Spellings = [GCC<"warning">];

aaron.ballman wrote:
> Given that the only functional difference between these attributes is the 
> diagnostic level, I sort of wonder whether we should have one semantic 
> attribute with two spellings (one for warning, one for error) and an accessor 
> field to distinguish which is which.
> 
> Another interesting question is: are these attributes mutually exclusive? Can 
> they be duplicated (perhaps across declarations)? What happens if the 
> messages differ? (We may need some attribute merging logic to catch these 
> situations.)
> I sort of wonder whether we should have one semantic attribute with two 
> spellings 

We'd need to be able to distinguish between the `Spellings` at runtime, I 
think. It looks like those are private to `ParsedAttrInfo`s, so I'm not sure 
how we could check that.

I think we'd want to somehow check the spelling in the code I added to 
`clang/lib/Sema/SemaDeclAttr.cpp`? Or is there a different approach that might 
work better?

> are these attributes mutually exclusive?

```
__attribute__((error("err"),warning("warn"),error("err2")))
void foo(void);

void x1(void) { foo(); }
```
in GCC produces a warning with message "warn" and error with message "err2".  
In my current implementation, we error once with message "err".  So I probably 
should check for multiple instances of the attribute, and use the message from 
the latest instance.  Oh, but I'm just calling `getUserDiagnostic` which was a 
table-gen'd getter; how do I even specify which instance of the attribute when 
there are multiple?  For example:
```
__attribute__((error("err"),error("err2")))
```
calls to `getUserDiagnostic` produce `err`...

> We may need some attribute merging logic to catch these situations.

So we don't currently have anything for that? What happens with 
`__attribute__(alias("")))` when multiple are given? ex. 
`__attribute__(alias("bar"),alias("foo"))`



Comment at: clang/include/clang/Basic/AttrDocs.td:6026
+depend on optimizations, while providing diagnostics pointing to precise
+locations of the call site in the source.
+  }];

aaron.ballman wrote:
> I think the documentation for these should probably be combined into one 
> documentation blob; the only difference in behavior is whether the diagnostic 
> is a warning or an attribute.
> 
> I think the documentation needs to go into more details about how this 
> attribute works in practice. For example, what should I expect from code like:
> ```
> struct Base {
>   __attribute__((warning("derp"))) virtual void foo();
> };
> 
> struct Derived : Base {
>   void foo() override; // Does calling this also warn?
> };
> 
> __attribute__((error("DERP!"))) void func() { // external function symbol!
>   func(); // Does this diagnose?
> }
> ```
> I suppose another interesting question given the design is to use the 
> optimizer is: what about LTO? Say I have:
> ```
> // TU1.c
> __attribute__((error("Derp Derp Derp"))) void func(void);
> 
> // TU.c
> extern void func(void);
> void blerp(void) { func(); }
> ```
> What should happen and does LTO change the answer?
> I think the documentation 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 359912.
nickdesaulniers marked 5 inline comments as done.
nickdesaulniers edited the summary of this revision.
nickdesaulniers added a comment.
Herald added a subscriber: pengfei.

- change IR Attr to dontcall
- check during ISel(s)
- rename td diag
- handle operator int


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Frontend/backend-attribute-error-warning.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/X86/attr-dontcall.ll

Index: llvm/test/CodeGen/X86/attr-dontcall.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/attr-dontcall.ll
@@ -0,0 +1,11 @@
+; RUN: not llc -global-isel=0 -fast-isel=0 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=0 -fast-isel=1 -stop-after=finalize-isel %s 2>&1 | FileCheck %s
+; RUN: not llc -global-isel=1 -fast-isel=0 -stop-after=irtranslator %s 2>&1 | FileCheck %s
+
+declare void @foo() "dontcall"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall"
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+}
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -69,6 +69,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/InlineAsm.h"
@@ -7918,6 +7919,15 @@
   }
 
   if (Function *F = I.getCalledFunction()) {
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = I.getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  DAG.getContext()->diagnose(D);
+}
+
 if (F->isDeclaration()) {
   // Is this an LLVM intrinsic or a target-specific intrinsic?
   unsigned IID = F->getIntrinsicID();
Index: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -75,6 +75,7 @@
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/GlobalValue.h"
@@ -1151,6 +1152,16 @@
   CLI.setCallee(RetTy, FuncTy, CI->getCalledOperand(), std::move(Args), *CI)
   .setTailCall(IsTailCall);
 
+  if (const Function *F = CI->getCalledFunction())
+if (F->hasFnAttribute("dontcall")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = CI->getMetadata("srcloc"))
+LocCookie =
+mdconst::extract(MD->getOperand(0))->getZExtValue();
+  DiagnosticInfoDontCall D(F->getName(), LocCookie);
+  F->getContext().diagnose(D);
+}
+
   return lowerCallTo(CLI);
 }
 
Index: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
===
--- llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -47,6 +47,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+  let Spellings = [GCC<"error">];

I think this should be an inheritable attribute (same below) so that 
redeclarations get the marking as well.

However, this does make for a bit of a novel situation with regards to other 
attributes. The typical behavior for function attributes is:
```
void func(void);

void use1(void) {
  func(); // Func is not marked yet, no attribute behavior
}

void func(void) __attribute__((whatever)));

void use2(void) {
  func(); // Func is marked, attribute does something
}

void func(void) {} // Func is still marked because the attribute is inheritted

void use3(void) {
  func(); // Func is marked, attribute does something
```
but because all of the interesting work is happening in the backend, I believe 
the unmarked use will still act as though the attribute was marked.



Comment at: clang/include/clang/Basic/Attr.td:3819
+  let Args = [StringArgument<"UserDiagnostic">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [ErrorAttrDocs];

ObjC methods as well?



Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+  let Spellings = [GCC<"warning">];

Given that the only functional difference between these attributes is the 
diagnostic level, I sort of wonder whether we should have one semantic 
attribute with two spellings (one for warning, one for error) and an accessor 
field to distinguish which is which.

Another interesting question is: are these attributes mutually exclusive? Can 
they be duplicated (perhaps across declarations)? What happens if the messages 
differ? (We may need some attribute merging logic to catch these situations.)



Comment at: clang/include/clang/Basic/AttrDocs.td:6026
+depend on optimizations, while providing diagnostics pointing to precise
+locations of the call site in the source.
+  }];

I think the documentation for these should probably be combined into one 
documentation blob; the only difference in behavior is whether the diagnostic 
is a warning or an attribute.

I think the documentation needs to go into more details about how this 
attribute works in practice. For example, what should I expect from code like:
```
struct Base {
  __attribute__((warning("derp"))) virtual void foo();
};

struct Derived : Base {
  void foo() override; // Does calling this also warn?
};

__attribute__((error("DERP!"))) void func() { // external function symbol!
  func(); // Does this diagnose?
}
```
I suppose another interesting question given the design is to use the optimizer 
is: what about LTO? Say I have:
```
// TU1.c
__attribute__((error("Derp Derp Derp"))) void func(void);

// TU.c
extern void func(void);
void blerp(void) { func(); }
```
What should happen and does LTO change the answer?



Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:75
 
+def err_fe_backend_user_diagnostic :
+  Error<"call to %0 declared with attribute error: %1">, BackendInfo;

Should probably give these names that relate to the attribute. How about 
`err_fe_backend_error_attr` (and `warn_fe_backend_warn_attr`) or something 
along those lines?



Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:78
+def warn_fe_backend_user_diagnostic :
+  Warning<"call to %0 declared with attribute warning: %1">, BackendInfo, 
InGroup;
+

80-col limit



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 

GCC doesn't seem to support this spelling -- do they have a different one we 
should reuse?



Comment at: clang/lib/CodeGen/CGCall.cpp:5308-5310
+  if (TargetDecl)
+if (TargetDecl->hasAttr() ||
+TargetDecl->hasAttr()) {





Comment at: clang/lib/CodeGen/CodeGenAction.cpp:776
+
+  CalleeName = FD->getName();
+}

This isn't really safe -- not all functions have names that can be retrieved 
this way. We should be sure to add some test coverage for functions without 
"names" (like `operator int()`).

I think it's better to call `getNameForDiagnostic()` here (unless the 
diagnostic printer does that already for a `DeclarationName`, in which case 
`getDeclName()` would be better.



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}

I think the usability in this situation is a concern -- only having a function 
name but no source location information is pretty tricky. Do you have a sense 
for how often we would hit this case?



Comment at: 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D106030#2878897 , @arsenm wrote:

> Adding something to the IR for the sole purpose of producing a diagnostic 
> feels really weird. I'm not sure I see why the frontend can't see this 
> attribute and directly warn

I interpreted this differently than @efriedma or @george.burgess.iv I think.  
Rephrasing the question as:

> Do we really need to keep the diagnostic itself in the IR?

The answer is no; we can keep it strictly in the frontend, and reconstitute 
there; we have access to the FunctionDecl which has the attribute.  So I can 
rework this instead of `"user-diagnostic"="oh no"` IR Fn Attr, to be just 
`dontcall` (in IR) then have the frontend check the FunctionDecl for the text 
of the user provided diagnostic.

But if the question was more so:

> Why do we need this?

Then I think @efriedma and @george.burgess.iv provided adequate reasoning. 
Perhaps I can reuse some of their thoughts in the commit message / patch 
description?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> Adding something to the IR for the sole purpose of producing a diagnostic 
> feels really weird. I'm not sure I see why the frontend can't see this 
> attribute and directly warn

To add a bit more clarification, the goal of this attribute is specifically to 
emit diagnostics after optimizations such as inlining have taken place. 
`diagnose_if` is clang's hammer if we want frontend diagnostics: 
https://godbolt.org/z/jbzbqEzbG . `diagnose_if` is a bit more flexible than 
"the condition must be an ICE per the standard," but AIUI the kernel has code 
like what Eli mentioned, which clang currently can't work with in the frontend 
(since it requires inlining, etc etc).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

In D106030#2878897 , @arsenm wrote:

> Adding something to the IR for the sole purpose of producing a diagnostic 
> feels really weird. I'm not sure I see why the frontend can't see this 
> attribute and directly warn

In C++, you can do trickery with template arguments to trigger compile errors, 
for example, the following:

  #include 
  template struct dependent_false : std::false_type {};
  template inline void f() {
if constexpr (x == 0) {
  /* do something */
} else if constexpr (x == 1) {
  /* do something else */
} else {
  static_assert(dependent_false() && "Invalid argument");
}
  }
  void g() { f<1>(); }

Now suppose you're an enterprising Linux kernel developer, and you decide you 
want this in C.  You don't really care about portability to compilers other 
than gcc, or compiling at any optimization level less than -O2, so instead of 
using a real language feature, you decide to depend on a combination of 
unintentional compiler semantics and linker diagnostics.  So you write 
something like the following:

  #define BUILD_BUG() undefined_symbol()
  extern void undefined_symbol();
  
  inline void f(int x) {
if (x == 0) {
  /* do something */
} else if (x == 1) {
  /* do something else */
} else {
  BUILD_BUG();
}
  }
  void g() { f(1); }

Then, after you're doing this for a while, you ask the gcc developers for a 
simple feature: an attribute you can apply to undefined_symbol that makes the 
compiler print the error message at compile-time instead of link-time.

---

Actually, in clang, you could probably use inline asm like the following to 
accomplish roughly the same thing, assuming you aren't building with 
-fno-integrated-as:

  #define BUILD_BUG() asm(".err \"BUILD_BUG\"")
  static inline void f(int x) {
if (x == 0) {
  /* do something */
} else if (x == 1) {
  /* do something else */
} else {
  BUILD_BUG();
}
  }
  void g() { f(2); }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

Adding something to the IR for the sole purpose of producing a diagnostic feels 
really weird. I'm not sure I see why the frontend can't see this attribute and 
directly warn


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 358814.
nickdesaulniers added a comment.

- remove accidentally committed logging statement


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Frontend/backend-attribute-error-warning.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/Generic/attr-user-diagnostic.ll

Index: llvm/test/CodeGen/Generic/attr-user-diagnostic.ll
===
--- /dev/null
+++ llvm/test/CodeGen/Generic/attr-user-diagnostic.ll
@@ -0,0 +1,9 @@
+; RUN: not llc -stop-after=pre-isel-intrinsic-lowering %s 2>&1 | FileCheck %s
+
+declare void @foo() "user-diagnostic"="oh no"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo: oh no
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoUser::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << ": " << getMsgStr();
+}
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -14,6 +14,7 @@
 #include "llvm/CodeGen/PreISelIntrinsicLowering.h"
 #include "llvm/Analysis/ObjCARCInstKind.h"
 #include "llvm/CodeGen/Passes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instructions.h"
@@ -204,6 +205,25 @@
   Changed |= lowerObjCCall(F, "objc_sync_exit");
   break;
 }
+// TODO: is this the best pass for this?
+for (BasicBlock  : F) {
+  for (Instruction  : BB) {
+if (CallBase *CB = dyn_cast()) {
+  if (Function *Callee = CB->getCalledFunction()) {
+if (Callee->hasFnAttribute("user-diagnostic")) {
+  unsigned LocCookie = 0;
+  if (MDNode *MD = CB->getMetadata("srcloc"))
+LocCookie = mdconst::extract(MD->getOperand(0))
+->getZExtValue();
+  DiagnosticInfoUser DU(
+  Callee->getFnAttribute("user-diagnostic").getValueAsString(),
+  Callee->getName(), LocCookie);
+  F.getContext().diagnose(DU);
+}
+  }
+}
+  }
+}
   }
   return Changed;
 }
Index: llvm/include/llvm/IR/DiagnosticInfo.h
===
--- llvm/include/llvm/IR/DiagnosticInfo.h
+++ llvm/include/llvm/IR/DiagnosticInfo.h
@@ -79,6 +79,7 @@
   DK_PGOProfile,
   DK_Unsupported,
   DK_SrcMgr,
+  DK_UserDiagnostic,
   DK_FirstPluginKind // Must be last value to work with
  // getNextAvailablePluginDiagnosticKind
 };
@@ -1070,6 +1071,25 @@
   }
 };
 
+class DiagnosticInfoUser : public DiagnosticInfo {
+  StringRef MsgStr;
+  StringRef CalleeName;
+  unsigned LocCookie;
+
+public:
+  DiagnosticInfoUser(StringRef MsgStr, StringRef CalleeName, unsigned LocCookie,
+ DiagnosticSeverity Severity = DS_Error)
+  : DiagnosticInfo(DK_UserDiagnostic, Severity), MsgStr(MsgStr),
+CalleeName(CalleeName), LocCookie(LocCookie) {}
+  StringRef getMsgStr() const { return MsgStr; }
+  StringRef getFunctionName() const { return CalleeName; }
+  unsigned getLocCookie() const { return LocCookie; }
+  void print(DiagnosticPrinter ) const override;
+  static bool classof(const DiagnosticInfo *DI) {
+return DI->getKind() == DK_UserDiagnostic;
+  }
+};
+
 } // end namespace llvm
 
 #endif // LLVM_IR_DIAGNOSTICINFO_H
Index: llvm/docs/LangRef.rst
===
--- llvm/docs/LangRef.rst
+++ llvm/docs/LangRef.rst
@@ -2037,6 +2037,12 @@
 show that no exceptions passes by it. This is normally the case for
 the ELF x86-64 abi, but it can be disabled for 

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp:208
 }
+// TODO: is this the best pass for this?
+for (BasicBlock  : F) {

Why not just do in in codegen in IRTranslator/SelectionDAGBuilder?



Comment at: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp:212
+if (CallBase *CB = dyn_cast()) {
+  if (Function *Callee = CB->getCalledFunction()) {
+if (Callee->hasFnAttribute("user-diagnostic")) {

What's the expected behavior if the function is called indirectly?



Comment at: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp:219
+  DiagnosticInfoUser DU(
+  Callee->getFnAttribute("user-diagnostic").getValueAsString(),
+  Callee->getName(), LocCookie);

Just do one getFnAttribute instead of pre-checking it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers created this revision.
nickdesaulniers added reviewers: rsmith, aaron.ballman, craig.topper, efriedma, 
lebedev.ri, jdoerfert, arsenm.
Herald added subscribers: dexonsmith, hiraditya.
nickdesaulniers requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, wdng.
Herald added projects: clang, LLVM.

Add support for the GNU C style __attribute__((error(""))) and
__attribute__((warning(""))). These attributes are meant to be put on
definitions of functions whom should not be called.

They are frequently used to provide compile time diagnostics similar to
_Static_assert, but which may rely on non-ICE conditions (ie. relying on
compiler optimizations). While users may instead simply call undefined
functions in such cases to get a linkage failure from the linker, these
provide a much more ergonomic and actionable diagnostic to users and do
so at compile time rather than at link time.

These are used throughout the Linux kernel in its implementation of
BUILD_BUG and BUILD_BUG_ON macros. These macros generally cannot be
converted to use _Static_assert because many of the parameters are not
ICEs. The Linux kernel still needs to be modified to make use of these
when building with Clang; I have a patch that does so I will send once
this feature is landed.

To do so, we create a new IR level Function attribute,
"user-diagnostic"="" which contains the string parameter from source
language function attributes (both error and warning boil down to one IR
Fn Attr). Then, similar to calls to inline asm, we attach a !srcloc
Metadata node to call sites of such attributed callees.

The backend diagnoses these before instruction selection, while we still
know that a call is a call (vs say a JMP that's a tail call) in an arch
agnostic manner.

The frontend then reconstructs the SourceLocation from that Metadata,
and determines whether to emit an error or warning based on the callee's
attribute.

Some things to iron out TODO:

- Is user-diagnostic the best identifier for the new fn attr? Maybe 
"diagnose-if-called"? I also use `UserDiagnostic` throughout much of this patch 
for C++ class names; surely there's something better?
- When is the best time for the backend to produce such diagnostics? If we wait 
until post-ISEL, then we need to diagnose instructions which may be JMPs, not 
just CALLs.

Link: https://bugs.llvm.org/show_bug.cgi?id=16428
Link: https://github.com/ClangBuiltLinux/linux/issues/1173


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106030

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-error.c
  clang/test/CodeGen/attr-warning.c
  clang/test/Frontend/backend-attribute-error-warning-optimize.c
  clang/test/Frontend/backend-attribute-error-warning.c
  clang/test/Frontend/backend-attribute-error-warning.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-error.c
  clang/test/Sema/attr-warning.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/test/CodeGen/Generic/attr-user-diagnostic.ll

Index: llvm/test/CodeGen/Generic/attr-user-diagnostic.ll
===
--- /dev/null
+++ llvm/test/CodeGen/Generic/attr-user-diagnostic.ll
@@ -0,0 +1,9 @@
+; RUN: not llc -stop-after=pre-isel-intrinsic-lowering %s 2>&1 | FileCheck %s
+
+declare void @foo() "user-diagnostic"="oh no"
+define void @bar() {
+  call void @foo()
+  ret void
+}
+
+; CHECK: error: call to foo: oh no
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -401,3 +401,7 @@
 
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
+
+void DiagnosticInfoUser::print(DiagnosticPrinter ) const {
+  DP << "call to " << getFunctionName() << ": " << getMsgStr();
+}
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -14,6 +14,7 @@
 #include "llvm/CodeGen/PreISelIntrinsicLowering.h"
 #include "llvm/Analysis/ObjCARCInstKind.h"
 #include "llvm/CodeGen/Passes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instructions.h"
@@ -204,6 +205,25 @@
   Changed |= lowerObjCCall(F, "objc_sync_exit");
   break;
 }
+// TODO: is this the best pass for this?
+