Reland [clang] Rework dontcall attributes

To avoid using the AST when emitting diagnostics, split the "dontcall"
attribute into "dontcall-warn" and "dontcall-error", and also add the
frontend attribute value as the LLVM attribute value. This gives us all
the information to report diagnostics we need from within the IR (aside
from access to the original source).

One downside is we directly use LLVM's demangler rather than using the
existing Clang diagnostic pretty printing of symbols.

Previous revisions didn't properly declare the new dependencies.

Reviewed By: nickdesaulniers

Differential Revision: https://reviews.llvm.org/D110364

clang/lib/CodeGen/CMakeLists.txt

@@ -5,6 +5,7 @@ set(LLVM_LINK_COMPONENTS
   Core
   Coroutines
   Coverage
+  Demangle
   Extensions
   FrontendOpenMP
   IPO

clang/lib/CodeGen/CodeGenAction.cpp

@@ -27,6 +27,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
+#include "llvm/Demangle/Demangle.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
@@ -760,30 +761,18 @@ void BackendConsumer::OptimizationFailureHandler(
 }
 
 void BackendConsumer::DontCallDiagHandler(const DiagnosticInfoDontCall &D) {
-  if (const Decl *DE = Gen->GetDeclForMangledName(D.getFunctionName()))
-    if (const auto *FD = dyn_cast<FunctionDecl>(DE)) {
-      assert(FD->hasAttr<ErrorAttr>() &&
-             "expected error or warning function attribute");
+  SourceLocation LocCookie =
+      SourceLocation::getFromRawEncoding(D.getLocCookie());
 
-      if (const auto *EA = FD->getAttr<ErrorAttr>()) {
-        assert((EA->isError() || EA->isWarning()) &&
-               "ErrorAttr neither error or warning");
+  // FIXME: we can't yet diagnose indirect calls. When/if we can, we
+  // should instead assert that LocCookie.isValid().
+  if (!LocCookie.isValid())
+    return;
 
-        SourceLocation LocCookie =
-            SourceLocation::getFromRawEncoding(D.getLocCookie());
-
-        // FIXME: we can't yet diagnose indirect calls. When/if we can, we
-        // should instead assert that LocCookie.isValid().
-        if (!LocCookie.isValid())
-          return;
-
-        Diags.Report(LocCookie, EA->isError()
-                                    ? diag::err_fe_backend_error_attr
-                                    : diag::warn_fe_backend_warning_attr)
-            << FD << EA->getUserDiagnostic();
-      }
-    }
-  // TODO: assert if DE or FD were nullptr?
+  Diags.Report(LocCookie, D.getSeverity() == DiagnosticSeverity::DS_Error
+                              ? diag::err_fe_backend_error_attr
+                              : diag::warn_fe_backend_warning_attr)
+      << llvm::demangle(D.getFunctionName().str()) << D.getNote();
 }
 
 /// This function is invoked when the backend needs

clang/lib/CodeGen/CodeGenModule.cpp

@@ -2138,8 +2138,12 @@ void CodeGenModule::SetFunctionAttributes(GlobalDecl GD, llvm::Function *F,
   else if (const auto *SA = FD->getAttr<SectionAttr>())
      F->setSection(SA->getName());
 
-  if (FD->hasAttr<ErrorAttr>())
-    F->addFnAttr("dontcall");
+  if (const auto *EA = FD->getAttr<ErrorAttr>()) {
+    if (EA->isError())
+      F->addFnAttr("dontcall-error", EA->getUserDiagnostic());
+    else if (EA->isWarning())
+      F->addFnAttr("dontcall-warn", EA->getUserDiagnostic());
+  }
 
   // If we plan on emitting this inline builtin, we can't treat it as a builtin.
   if (FD->isInlineBuiltinDeclaration()) {

clang/test/CodeGen/attr-error.c

@@ -7,5 +7,5 @@ void bar(void) {
 
 // CHECK: call void @foo(), !srcloc [[SRCLOC:![0-9]+]]
 // CHECK: declare{{.*}} void @foo() [[ATTR:#[0-9]+]]
-// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall"
+// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall-error"="oh no"
 // CHECK: [[SRCLOC]] = !{i32 {{[0-9]+}}}

clang/test/CodeGen/attr-warning.c

@@ -7,5 +7,5 @@ void bar(void) {
 
 // CHECK: call void @foo(), !srcloc [[SRCLOC:![0-9]+]]
 // CHECK: declare{{.*}} void @foo() [[ATTR:#[0-9]+]]
-// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall"
+// CHECK: attributes [[ATTR]] = {{{.*}}"dontcall-warn"="oh no"
 // CHECK: [[SRCLOC]] = !{i32 {{[0-9]+}}}

clang/test/Frontend/backend-attribute-error-warning-optimize.c

@@ -8,7 +8,7 @@ int x(void) {
   return 8 % 2 == 1;
 }
 void baz(void) {
-  foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+  foo(); // expected-error {{call to foo declared with 'error' attribute: oh no foo}}
   if (x())
     bar();
 }

clang/test/Frontend/backend-attribute-error-warning.c

@@ -1,7 +1,5 @@
 // RUN: %clang_cc1 -verify=expected,enabled -emit-codegen-only %s
-// RUN: %clang_cc1 -verify=expected,enabled -emit-codegen-only %s -x c++
 // RUN: %clang_cc1 -verify -emit-codegen-only -Wno-attribute-warning %s
-// RUN: %clang_cc1 -verify -emit-codegen-only -Wno-attribute-warning %s -x c++
 
 __attribute__((error("oh no foo"))) void foo(void);
 
@@ -24,38 +22,12 @@ void                                            // expected-note@-1 {{previous a
 duplicate_warnings(void);
 
 void baz(void) {
-  foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+  foo(); // expected-error {{call to foo declared with 'error' attribute: oh no foo}}
   if (x())
-    bar(); // expected-error {{call to 'bar' declared with 'error' attribute: oh no bar}}
+    bar(); // expected-error {{call to bar declared with 'error' attribute: oh no bar}}
 
-  quux();                     // enabled-warning {{call to 'quux' declared with 'warning' attribute: oh no quux}}
-  __compiletime_assert_455(); // expected-error {{call to '__compiletime_assert_455' declared with 'error' attribute: demangle me}}
-  duplicate_errors();         // expected-error {{call to 'duplicate_errors' declared with 'error' attribute: two}}
-  duplicate_warnings();       // enabled-warning {{call to 'duplicate_warnings' declared with 'warning' attribute: two}}
+  quux();                     // enabled-warning {{call to quux declared with 'warning' attribute: oh no quux}}
+  __compiletime_assert_455(); // expected-error {{call to __compiletime_assert_455 declared with 'error' attribute: demangle me}}
+  duplicate_errors();         // expected-error {{call to duplicate_errors declared with 'error' attribute: two}}
+  duplicate_warnings();       // enabled-warning {{call to duplicate_warnings declared with 'warning' attribute: two}}
 }
-
-#ifdef __cplusplus
-template <typename T>
-__attribute__((error("demangle me, too")))
-T
-nocall(T t);
-
-struct Widget {
-  __attribute__((warning("don't call me!")))
-  operator int() { return 42; }
-};
-
-void baz_cpp(void) {
-  foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
-  if (x())
-    bar(); // expected-error {{call to 'bar' declared with 'error' attribute: oh no bar}}
-  quux();  // enabled-warning {{call to 'quux' declared with 'warning' attribute: oh no quux}}
-
-  // Test that we demangle correctly in the diagnostic for C++.
-  __compiletime_assert_455(); // expected-error {{call to '__compiletime_assert_455' declared with 'error' attribute: demangle me}}
-  nocall<int>(42);            // expected-error {{call to 'nocall<int>' declared with 'error' attribute: demangle me, too}}
-
-  Widget W;
-  int w = W; // enabled-warning {{'operator int' declared with 'warning' attribute: don't call me!}}
-}
-#endif

clang/test/Frontend/backend-attribute-error-warning.cpp

@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -verify=expected,enabled -emit-codegen-only %s
+// RUN: %clang_cc1 -verify -emit-codegen-only -Wno-attribute-warning %s
+
+__attribute__((error("oh no foo"))) void foo(void);
+
+__attribute__((error("oh no bar"))) void bar(void);
+
+int x(void) {
+  return 8 % 2 == 1;
+}
+
+__attribute__((warning("oh no quux"))) void quux(void);
+
+__attribute__((error("demangle me"))) void __compiletime_assert_455(void);
+
+__attribute__((error("one"), error("two"))) // expected-warning {{attribute 'error' is already applied with different arguments}}
+void                                        // expected-note@-1 {{previous attribute is here}}
+duplicate_errors(void);
+
+__attribute__((warning("one"), warning("two"))) // expected-warning {{attribute 'warning' is already applied with different arguments}}
+void                                            // expected-note@-1 {{previous attribute is here}}
+duplicate_warnings(void);
+
+void baz(void) {
+  foo(); // expected-error {{call to foo() declared with 'error' attribute: oh no foo}}
+  if (x())
+    bar(); // expected-error {{call to bar() declared with 'error' attribute: oh no bar}}
+
+  quux();                     // enabled-warning {{call to quux() declared with 'warning' attribute: oh no quux}}
+  __compiletime_assert_455(); // expected-error {{call to __compiletime_assert_455() declared with 'error' attribute: demangle me}}
+  duplicate_errors();         // expected-error {{call to duplicate_errors() declared with 'error' attribute: two}}
+  duplicate_warnings();       // enabled-warning {{call to duplicate_warnings() declared with 'warning' attribute: two}}
+}
+
+#ifdef __cplusplus
+template <typename T>
+__attribute__((error("demangle me, too")))
+T
+nocall(T t);
+
+struct Widget {
+  __attribute__((warning("don't call me!")))
+  operator int() { return 42; }
+};
+
+void baz_cpp(void) {
+  foo(); // expected-error {{call to foo() declared with 'error' attribute: oh no foo}}
+  if (x())
+    bar(); // expected-error {{call to bar() declared with 'error' attribute: oh no bar}}
+  quux();  // enabled-warning {{call to quux() declared with 'warning' attribute: oh no quux}}
+
+  // Test that we demangle correctly in the diagnostic for C++.
+  __compiletime_assert_455(); // expected-error {{call to __compiletime_assert_455() declared with 'error' attribute: demangle me}}
+  nocall<int>(42);            // expected-error {{call to int nocall<int>(int) declared with 'error' attribute: demangle me, too}}
+
+  Widget W;
+  int w = W; // enabled-warning {{Widget::operator int() declared with 'warning' attribute: don't call me!}}
+}
+#endif

llvm/docs/LangRef.rst

@@ -1594,12 +1594,18 @@ example:
     ``disable_sanitizer_instrumentation`` disables all kinds of instrumentation,
     taking precedence over the ``sanitize_<name>`` attributes and other compiler
     flags.
-``"dontcall"``
-    This attribute denotes that a diagnostic should be emitted when a call of a
-    function with this attribute is not eliminated via optimization. Front ends
-    can provide optional ``srcloc`` metadata nodes on call sites of such
-    callees to attach information about where in the source language such a
-    call came from.
+``"dontcall-error"``
+    This attribute denotes that an error diagnostic should be emitted when a
+    call of a function with this attribute is not eliminated via optimization.
+    Front ends can provide optional ``srcloc`` metadata nodes on call sites of
+    such callees to attach information about where in the source language such a
+    call came from. A string value can be provided as a note.
+``"dontcall-warn"``
+    This attribute denotes that a warning diagnostic should be emitted when a
+    call of a function with this attribute is not eliminated via optimization.
+    Front ends can provide optional ``srcloc`` metadata nodes on call sites of
+    such callees to attach information about where in the source language such a
+    call came from. A string value can be provided as a note.
 ``"frame-pointer"``
     This attribute tells the code generator whether the function
     should keep the frame pointer. The code generator may emit the frame pointer

llvm/include/llvm/IR/DiagnosticInfo.h

@@ -33,6 +33,7 @@ namespace llvm {
 
 // Forward declarations.
 class DiagnosticPrinter;
+class CallInst;
 class Function;
 class Instruction;
 class InstructionCost;
@@ -1070,15 +1071,20 @@ public:
   }
 };
 
+void diagnoseDontCall(const CallInst &CI);
+
 class DiagnosticInfoDontCall : public DiagnosticInfo {
   StringRef CalleeName;
+  StringRef Note;
   unsigned LocCookie;
 
 public:
-  DiagnosticInfoDontCall(StringRef CalleeName, unsigned LocCookie)
-      : DiagnosticInfo(DK_DontCall, DS_Error), CalleeName(CalleeName),
+  DiagnosticInfoDontCall(StringRef CalleeName, StringRef Note,
+                         DiagnosticSeverity DS, unsigned LocCookie)
+      : DiagnosticInfo(DK_DontCall, DS), CalleeName(CalleeName), Note(Note),
         LocCookie(LocCookie) {}
   StringRef getFunctionName() const { return CalleeName; }
+  StringRef getNote() const { return Note; }
   unsigned getLocCookie() const { return LocCookie; }
   void print(DiagnosticPrinter &DP) const override;
   static bool classof(const DiagnosticInfo *DI) {

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

@@ -2344,14 +2344,7 @@ bool IRTranslator::translateCall(const User &U, MachineIRBuilder &MIRBuilder) {
   if (CI.isInlineAsm())
     return translateInlineAsm(CI, MIRBuilder);
 
-  if (F && F->hasFnAttribute("dontcall")) {
-    unsigned LocCookie = 0;
-    if (MDNode *MD = CI.getMetadata("srcloc"))
-      LocCookie =
-          mdconst::extract<ConstantInt>(MD->getOperand(0))->getZExtValue();
-    DiagnosticInfoDontCall D(F->getName(), LocCookie);
-    F->getContext().diagnose(D);
-  }
+  diagnoseDontCall(CI);
 
   Intrinsic::ID ID = Intrinsic::not_intrinsic;
   if (F && F->isIntrinsic()) {

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp

@@ -1152,15 +1152,7 @@ bool FastISel::lowerCall(const CallInst *CI) {
   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<ConstantInt>(MD->getOperand(0))->getZExtValue();
-      DiagnosticInfoDontCall D(F->getName(), LocCookie);
-      F->getContext().diagnose(D);
-    }
+  diagnoseDontCall(*CI);
 
   return lowerCallTo(CLI);
 }

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

@@ -8036,14 +8036,7 @@ void SelectionDAGBuilder::visitCall(const CallInst &I) {
   }
 
   if (Function *F = I.getCalledFunction()) {
-    if (F->hasFnAttribute("dontcall")) {
-      unsigned LocCookie = 0;
-      if (MDNode *MD = I.getMetadata("srcloc"))
-        LocCookie =
-            mdconst::extract<ConstantInt>(MD->getOperand(0))->getZExtValue();
-      DiagnosticInfoDontCall D(F->getName(), LocCookie);
-      DAG.getContext()->diagnose(D);
-    }
+    diagnoseDontCall(I);
 
     if (F->isDeclaration()) {
       // Is this an LLVM intrinsic or a target-specific intrinsic?

llvm/lib/IR/DiagnosticInfo.cpp

@@ -400,6 +400,34 @@ std::string DiagnosticInfoOptimizationBase::getMsg() const {
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
 
-void DiagnosticInfoDontCall::print(DiagnosticPrinter &DP) const {
-  DP << "call to " << getFunctionName() << " marked \"dontcall\"";
+void llvm::diagnoseDontCall(const CallInst &CI) {
+  auto *F = CI.getCalledFunction();
+  if (!F)
+    return;
+
+  for (int i = 0; i != 2; ++i) {
+    auto AttrName = i == 0 ? "dontcall-error" : "dontcall-warn";
+    auto Sev = i == 0 ? DS_Error : DS_Warning;
+
+    if (F->hasFnAttribute(AttrName)) {
+      unsigned LocCookie = 0;
+      auto A = F->getFnAttribute(AttrName);
+      if (MDNode *MD = CI.getMetadata("srcloc"))
+        LocCookie =
+            mdconst::extract<ConstantInt>(MD->getOperand(0))->getZExtValue();
+      DiagnosticInfoDontCall D(F->getName(), A.getValueAsString(), Sev,
+                               LocCookie);
+      F->getContext().diagnose(D);
+    }
+  }
+}
+
+void DiagnosticInfoDontCall::print(DiagnosticPrinter &DP) const {
+  DP << "call to " << getFunctionName() << " marked \"dontcall-";
+  if (getSeverity() == DiagnosticSeverity::DS_Error)
+    DP << "error\"";
+  else
+    DP << "warn\"";
+  if (!getNote().empty())
+    DP << ": " << getNote();
 }

llvm/test/CodeGen/X86/attr-dontcall.ll

@@ -2,10 +2,24 @@
 ; RUN: not llc -mtriple=x86_64 -global-isel=0 -fast-isel=1 -stop-after=finalize-isel < %s 2>&1 | FileCheck %s
 ; RUN: not llc -mtriple=x86_64 -global-isel=1 -fast-isel=0 -stop-after=irtranslator -global-isel-abort=0 < %s 2>&1 | FileCheck %s
 
-declare void @foo() "dontcall"
+declare void @foo() "dontcall-error"="e"
 define void @bar() {
   call void @foo()
   ret void
 }
 
-; CHECK: error: call to foo marked "dontcall"
+declare void @foo2() "dontcall-warn"="w"
+define void @bar2() {
+  call void @foo2()
+  ret void
+}
+
+declare void @foo3() "dontcall-warn"
+define void @bar3() {
+  call void @foo3()
+  ret void
+}
+
+; CHECK: error: call to foo marked "dontcall-error": e
+; CHECK: warning: call to foo2 marked "dontcall-warn": w
+; CHECK: warning: call to foo3 marked "dontcall-warn"{{$}}

llvm/test/ThinLTO/X86/dontcall.ll

@@ -7,16 +7,16 @@
 ; 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
+; attributes match!!! What should happen if we remove "dontcall-error" from the
 ; definition or declaration of @callee?
 
-; CHECK: call to callee marked "dontcall"
+; CHECK: call to callee marked "dontcall-error"
 
 ;--- 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 {
+define i32 @callee() "dontcall-error" noinline {
   ret i32 42
 }
 
@@ -24,7 +24,7 @@ define i32 @callee() "dontcall" noinline {
 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"
+declare i32 @callee() "dontcall-error"
 
 define i32 @caller() {
 entry:

llvm/utils/gn/secondary/clang/lib/CodeGen/BUILD.gn

@@ -13,6 +13,7 @@ static_library("CodeGen") {
     "//clang/lib/Lex",
     "//llvm/lib/Analysis",
     "//llvm/lib/Bitcode/Reader",
+    "//llvm/lib/Demangle",
     "//llvm/lib/IR",
     "//llvm/lib/IRReader",
     "//llvm/lib/LTO",