[clang] report inlining decisions with -Wattribute-{warning|error} #73552
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Due to inlining, descovering which specific call site to a function with the attribute "warning" or "error" is painful. In the IR record inlining decisions in metadata when inlining a callee that itself contains a call to a dontcall-error or dontcall-warn fn. Print this info so that it's clearer which call site is problematic. There's still some limitations with this approach; macro expansion is not recorded. Fixes: ClangBuiltLinux/linux#1571 Differential Revision: https://reviews.llvm.org/D141451
llvmbot
added
clang
|
(Initial import from https://reviews.llvm.org/D141451) |
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-clang Author: Nick Desaulniers (nickdesaulniers) ChangesDue to inlining, descovering which specific call site to a function with In the IR record inlining decisions in metadata when inlining a callee Print this info so that it's clearer which call site is problematic. There's still some limitations with this approach; macro expansion is Fixes: ClangBuiltLinux/linux#1571 Differential Revision: https://reviews.llvm.org/D141451 Full diff: https://github.com/llvm/llvm-project/pull/73552.diff 10 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index 715e0c0dc8fa84e..0909b1f59175be9 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -93,6 +93,8 @@ def err_fe_backend_error_attr :
def warn_fe_backend_warning_attr :
Warning<"call to '%0' declared with 'warning' attribute: %1">, BackendInfo,
InGroup<BackendWarningAttributes>;
+def note_fe_backend_in : Note<"called by function '%0'">;
+def note_fe_backend_inlined : Note<"inlined by function '%0'">;
def err_fe_invalid_code_complete_file : Error<
"cannot locate code-completion file %0">, DefaultFatal;
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index a31a271ed77d1ca..66e040741e2718d 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -52,6 +52,8 @@
#include "llvm/Transforms/Utils/Cloning.h"
#include <optional>
+#include <string>
+
using namespace clang;
using namespace llvm;
@@ -794,6 +796,14 @@ void BackendConsumer::DontCallDiagHandler(const DiagnosticInfoDontCall &D) {
? diag::err_fe_backend_error_attr
: diag::warn_fe_backend_warning_attr)
<< llvm::demangle(D.getFunctionName()) << D.getNote();
+
+ SmallVector<std::string, 4> InliningDecisions;
+ D.getInliningDecisions(InliningDecisions);
+ InliningDecisions.push_back(D.getCaller().str());
+ for (auto Dec : llvm::enumerate(InliningDecisions))
+ Diags.Report(Dec.index() ? diag::note_fe_backend_inlined
+ : diag::note_fe_backend_in)
+ << llvm::demangle(Dec.value());
}
void BackendConsumer::MisExpectDiagHandler(
diff --git a/clang/test/Frontend/backend-attribute-error-warning-optimize.c b/clang/test/Frontend/backend-attribute-error-warning-optimize.c
index d3951e3b6b1f57d..0bfc50ff8985c39 100644
--- a/clang/test/Frontend/backend-attribute-error-warning-optimize.c
+++ b/clang/test/Frontend/backend-attribute-error-warning-optimize.c
@@ -9,6 +9,7 @@ int x(void) {
}
void baz(void) {
foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'baz'}}
if (x())
bar();
}
@@ -20,3 +21,23 @@ void indirect(void) {
quux = foo;
quux();
}
+
+static inline void a(int x) {
+ if (x == 10)
+ foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'a'}}
+ // expected-note@* {{inlined by function 'b'}}
+ // expected-note@* {{inlined by function 'd'}}
+}
+
+static inline void b() {
+ a(10);
+}
+
+void c() {
+ a(9);
+}
+
+void d() {
+ b();
+}
diff --git a/clang/test/Frontend/backend-attribute-error-warning.c b/clang/test/Frontend/backend-attribute-error-warning.c
index c3c7803479aac96..c87a47053e5c0f8 100644
--- a/clang/test/Frontend/backend-attribute-error-warning.c
+++ b/clang/test/Frontend/backend-attribute-error-warning.c
@@ -23,11 +23,17 @@ duplicate_warnings(void);
void baz(void) {
foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'baz'}}
if (x())
bar(); // expected-error {{call to 'bar' declared with 'error' attribute: oh no bar}}
+ // expected-note@* {{called by function 'baz'}}
quux(); // enabled-warning {{call to 'quux' declared with 'warning' attribute: oh no quux}}
+ // enabled-note@* {{called by function 'baz'}}
__compiletime_assert_455(); // expected-error {{call to '__compiletime_assert_455' declared with 'error' attribute: demangle me}}
+ // expected-note@* {{called by function 'baz'}}
duplicate_errors(); // expected-error {{call to 'duplicate_errors' declared with 'error' attribute: two}}
+ // expected-note@* {{called by function 'baz'}}
duplicate_warnings(); // enabled-warning {{call to 'duplicate_warnings' declared with 'warning' attribute: two}}
+ // enabled-note@* {{called by function 'baz'}}
}
diff --git a/clang/test/Frontend/backend-attribute-error-warning.cpp b/clang/test/Frontend/backend-attribute-error-warning.cpp
index 1ed549c271241ab..537e798649f7508 100644
--- a/clang/test/Frontend/backend-attribute-error-warning.cpp
+++ b/clang/test/Frontend/backend-attribute-error-warning.cpp
@@ -24,13 +24,19 @@ duplicate_warnings(void);
void baz(void) {
foo(); // expected-error {{call to 'foo()' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'baz()'}}
if (x())
bar(); // expected-error {{call to 'bar()' declared with 'error' attribute: oh no bar}}
+ // expected-note@* {{called by function 'baz()'}}
quux(); // enabled-warning {{call to 'quux()' declared with 'warning' attribute: oh no quux}}
+ // enabled-note@* {{called by function 'baz()'}}
__compiletime_assert_455(); // expected-error {{call to '__compiletime_assert_455()' declared with 'error' attribute: demangle me}}
+ // expected-note@* {{called by function 'baz()'}}
duplicate_errors(); // expected-error {{call to 'duplicate_errors()' declared with 'error' attribute: two}}
+ // expected-note@* {{called by function 'baz()'}}
duplicate_warnings(); // enabled-warning {{call to 'duplicate_warnings()' declared with 'warning' attribute: two}}
+ // enabled-note@* {{called by function 'baz()'}}
}
#ifdef __cplusplus
@@ -46,15 +52,21 @@ struct Widget {
void baz_cpp(void) {
foo(); // expected-error {{call to 'foo()' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'baz_cpp()'}}
if (x())
bar(); // expected-error {{call to 'bar()' declared with 'error' attribute: oh no bar}}
+ // expected-note@* {{called by function 'baz_cpp()'}}
quux(); // enabled-warning {{call to 'quux()' declared with 'warning' attribute: oh no quux}}
+ // enabled-note@* {{called by function 'baz_cpp()'}}
// 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}}
+ // expected-note@* {{called by function 'baz_cpp()'}}
nocall<int>(42); // expected-error {{call to 'int nocall<int>(int)' declared with 'error' attribute: demangle me, too}}
+ // expected-note@* {{called by function 'baz_cpp()'}}
Widget W;
int w = W; // enabled-warning {{call to 'Widget::operator int()' declared with 'warning' attribute: don't call me!}}
+ // enabled-note@* {{called by function 'baz_cpp()'}}
}
#endif
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index e448c5ed5c5d947..c3127dcaf1ef80a 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -1803,13 +1803,17 @@ example:
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.
+ call came from. A string value can be provided as a note. The optimizer may
+ add the optional ``inlined.from`` metadata to call sites which front ends
+ might consume to display more precise diagnostics.
``"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.
+ call came from. A string value can be provided as a note. The optimizer may
+ add the optional ``inlined.from`` metadata to call sites which front ends
+ might consume to display more precise diagnostics.
``fn_ret_thunk_extern``
This attribute tells the code generator that returns from functions should
be replaced with jumps to externally-defined architecture-specific symbols.
diff --git a/llvm/include/llvm/IR/DiagnosticInfo.h b/llvm/include/llvm/IR/DiagnosticInfo.h
index 628445fe9fb2cca..3f64c0acd293ed5 100644
--- a/llvm/include/llvm/IR/DiagnosticInfo.h
+++ b/llvm/include/llvm/IR/DiagnosticInfo.h
@@ -1099,15 +1099,19 @@ class DiagnosticInfoSrcMgr : public DiagnosticInfo {
void diagnoseDontCall(const CallInst &CI);
class DiagnosticInfoDontCall : public DiagnosticInfo {
+ StringRef CallerName;
StringRef CalleeName;
StringRef Note;
unsigned LocCookie;
+ MDNode *MDN;
public:
- DiagnosticInfoDontCall(StringRef CalleeName, StringRef Note,
- DiagnosticSeverity DS, unsigned LocCookie)
- : DiagnosticInfo(DK_DontCall, DS), CalleeName(CalleeName), Note(Note),
- LocCookie(LocCookie) {}
+ DiagnosticInfoDontCall(StringRef CallerName, StringRef CalleeName,
+ StringRef Note, DiagnosticSeverity DS,
+ unsigned LocCookie, MDNode *MDN)
+ : DiagnosticInfo(DK_DontCall, DS), CallerName(CallerName),
+ CalleeName(CalleeName), Note(Note), LocCookie(LocCookie), MDN(MDN) {}
+ StringRef getCaller() const { return CallerName; }
StringRef getFunctionName() const { return CalleeName; }
StringRef getNote() const { return Note; }
unsigned getLocCookie() const { return LocCookie; }
@@ -1115,6 +1119,8 @@ class DiagnosticInfoDontCall : public DiagnosticInfo {
static bool classof(const DiagnosticInfo *DI) {
return DI->getKind() == DK_DontCall;
}
+ void
+ getInliningDecisions(SmallVectorImpl<std::string> &InliningDecisions) const;
};
} // end namespace llvm
diff --git a/llvm/lib/IR/DiagnosticInfo.cpp b/llvm/lib/IR/DiagnosticInfo.cpp
index 342c4cbbc39d65f..bacb4c01a1342ca 100644
--- a/llvm/lib/IR/DiagnosticInfo.cpp
+++ b/llvm/lib/IR/DiagnosticInfo.cpp
@@ -12,6 +12,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/IR/DiagnosticInfo.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/Twine.h"
#include "llvm/ADT/iterator_range.h"
@@ -433,8 +434,9 @@ void llvm::diagnoseDontCall(const CallInst &CI) {
if (MDNode *MD = CI.getMetadata("srcloc"))
LocCookie =
mdconst::extract<ConstantInt>(MD->getOperand(0))->getZExtValue();
- DiagnosticInfoDontCall D(F->getName(), A.getValueAsString(), Sev,
- LocCookie);
+ DiagnosticInfoDontCall D(CI.getParent()->getParent()->getName(),
+ F->getName(), A.getValueAsString(), Sev,
+ LocCookie, CI.getMetadata("inlined.from"));
F->getContext().diagnose(D);
}
}
@@ -449,3 +451,20 @@ void DiagnosticInfoDontCall::print(DiagnosticPrinter &DP) const {
if (!getNote().empty())
DP << ": " << getNote();
}
+
+void DiagnosticInfoDontCall::getInliningDecisions(
+ SmallVectorImpl<std::string> &InliningDecisions) const {
+ if (!MDN)
+ return;
+
+ const MDOperand &MO = MDN->getOperand(0);
+ if (auto *MDT = dyn_cast<MDTuple>(MO)) {
+ for (const MDOperand &MO : MDT->operands()) {
+ if (auto *S = dyn_cast<MDString>(MO)) {
+ InliningDecisions.push_back(S->getString().str());
+ }
+ }
+ } else if (auto *S = dyn_cast<MDString>(MO)) {
+ InliningDecisions.push_back(S->getString().str());
+ }
+}
diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index 39d5f6e53c1de48..81ffc416ffd5c0a 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -2520,6 +2520,19 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI,
// inlining, commonly when the callee is an intrinsic.
if (MarkNoUnwind && !CI->doesNotThrow())
CI->setDoesNotThrow();
+
+ const Function *Callee = CI->getCalledFunction();
+ if (Callee && (Callee->hasFnAttribute("dontcall-error") ||
+ Callee->hasFnAttribute("dontcall-warn"))) {
+ Metadata *MD = MDString::get(CI->getContext(), CalledFunc->getName());
+ if (MDNode *N = CI->getMetadata("inlined.from")) {
+ TempMDTuple Temp = cast<MDTuple>(N)->clone();
+ Temp->push_back(MD);
+ MD = MDNode::replaceWithUniqued(std::move(Temp));
+ }
+ MDTuple *MDT = MDNode::get(CI->getContext(), {MD});
+ CI->setMetadata("inlined.from", MDT);
+ }
}
}
}
diff --git a/llvm/test/Transforms/Inline/dontcall-attributes.ll b/llvm/test/Transforms/Inline/dontcall-attributes.ll
new file mode 100644
index 000000000000000..acf8c6a62aad619
--- /dev/null
+++ b/llvm/test/Transforms/Inline/dontcall-attributes.ll
@@ -0,0 +1,84 @@
+; RUN: opt -S -o - -passes=inline %s \
+; RUN: | FileCheck %s --check-prefixes=CHECK-BOTH,CHECK
+; RUN: opt -S -o - -passes=always-inline %s \
+; RUN: | FileCheck %s --check-prefixes=CHECK-BOTH,CHECK-ALWAYS
+
+declare void @foo() "dontcall-warn"="oh no"
+declare void @fof() "dontcall-error"="oh no"
+
+define void @bar(i32 %x) {
+ %cmp = icmp eq i32 %x, 10
+ br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+ call void @foo()
+ br label %if.end
+
+if.end:
+ ret void
+}
+
+define void @quux() {
+ call void @bar(i32 9)
+ ret void
+}
+
+; Test that @baz's call to @foo has metadata with inlining info.
+define void @baz() {
+; CHECK-LABEL: @baz(
+; CHECK-NEXT: call void @foo(), !inlined.from !0
+;
+ call void @bar(i32 10)
+ ret void
+}
+
+; Test that @zing's call to @foo has unique metadata from @baz's call to @foo.
+define void @zing() {
+; CHECK-LABEL: @zing(
+; CHECK-NEXT: call void @foo(), !inlined.from !1
+;
+ call void @baz()
+ ret void
+}
+
+; Same test but @fof has fn attr "dontcall-error"="..." rather than
+; "dontcall-warn"="...".
+define void @a() {
+ call void @fof()
+ ret void
+}
+define void @b() {
+; CHECK-LABEL: @b(
+; CHECK-NEXT: call void @fof(), !inlined.from !3
+ call void @a()
+ ret void
+}
+
+; Add some tests for alwaysinline.
+define void @always_callee() alwaysinline {
+ call void @fof()
+ ret void
+}
+define void @always_caller() alwaysinline {
+; CHECK-BOTH-LABEL: @always_caller(
+; CHECK-NEXT: call void @fof(), !inlined.from !4
+; CHECK-ALWAYS-NEXT: call void @fof(), !inlined.from !0
+ call void @always_callee()
+ ret void
+}
+define void @always_caller2() alwaysinline {
+; CHECK-BOTH-LABEL: @always_caller2(
+; CHECK-NEXT: call void @fof(), !inlined.from !5
+; CHECK-ALWAYS-NEXT: call void @fof(), !inlined.from !1
+ call void @always_caller()
+ ret void
+}
+
+; CHECK: !0 = !{!"bar"}
+; CHECK-NEXT: !1 = !{!2}
+; CHECK-NEXT: !2 = !{!"bar", !"baz"}
+; CHECK-NEXT: !3 = !{!"a"}
+; CHECK-NEXT: !4 = !{!"always_callee"}
+; CHECK-ALWAYS: !0 = !{!"always_callee"}
+; CHECK-ALWAYS-NEXT: !1 = !{!2}
+; CHECK-ALWAYS-NEXT: !2 = !{!"always_callee", !"always_caller"}
|
|
@llvm/pr-subscribers-llvm-transforms Author: Nick Desaulniers (nickdesaulniers) ChangesDue to inlining, descovering which specific call site to a function with In the IR record inlining decisions in metadata when inlining a callee Print this info so that it's clearer which call site is problematic. There's still some limitations with this approach; macro expansion is Fixes: ClangBuiltLinux/linux#1571 Differential Revision: https://reviews.llvm.org/D141451 Full diff: https://github.com/llvm/llvm-project/pull/73552.diff 10 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index 715e0c0dc8fa84e..0909b1f59175be9 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -93,6 +93,8 @@ def err_fe_backend_error_attr :
def warn_fe_backend_warning_attr :
Warning<"call to '%0' declared with 'warning' attribute: %1">, BackendInfo,
InGroup<BackendWarningAttributes>;
+def note_fe_backend_in : Note<"called by function '%0'">;
+def note_fe_backend_inlined : Note<"inlined by function '%0'">;
def err_fe_invalid_code_complete_file : Error<
"cannot locate code-completion file %0">, DefaultFatal;
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index a31a271ed77d1ca..66e040741e2718d 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -52,6 +52,8 @@
#include "llvm/Transforms/Utils/Cloning.h"
#include <optional>
+#include <string>
+
using namespace clang;
using namespace llvm;
@@ -794,6 +796,14 @@ void BackendConsumer::DontCallDiagHandler(const DiagnosticInfoDontCall &D) {
? diag::err_fe_backend_error_attr
: diag::warn_fe_backend_warning_attr)
<< llvm::demangle(D.getFunctionName()) << D.getNote();
+
+ SmallVector<std::string, 4> InliningDecisions;
+ D.getInliningDecisions(InliningDecisions);
+ InliningDecisions.push_back(D.getCaller().str());
+ for (auto Dec : llvm::enumerate(InliningDecisions))
+ Diags.Report(Dec.index() ? diag::note_fe_backend_inlined
+ : diag::note_fe_backend_in)
+ << llvm::demangle(Dec.value());
}
void BackendConsumer::MisExpectDiagHandler(
diff --git a/clang/test/Frontend/backend-attribute-error-warning-optimize.c b/clang/test/Frontend/backend-attribute-error-warning-optimize.c
index d3951e3b6b1f57d..0bfc50ff8985c39 100644
--- a/clang/test/Frontend/backend-attribute-error-warning-optimize.c
+++ b/clang/test/Frontend/backend-attribute-error-warning-optimize.c
@@ -9,6 +9,7 @@ int x(void) {
}
void baz(void) {
foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'baz'}}
if (x())
bar();
}
@@ -20,3 +21,23 @@ void indirect(void) {
quux = foo;
quux();
}
+
+static inline void a(int x) {
+ if (x == 10)
+ foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'a'}}
+ // expected-note@* {{inlined by function 'b'}}
+ // expected-note@* {{inlined by function 'd'}}
+}
+
+static inline void b() {
+ a(10);
+}
+
+void c() {
+ a(9);
+}
+
+void d() {
+ b();
+}
diff --git a/clang/test/Frontend/backend-attribute-error-warning.c b/clang/test/Frontend/backend-attribute-error-warning.c
index c3c7803479aac96..c87a47053e5c0f8 100644
--- a/clang/test/Frontend/backend-attribute-error-warning.c
+++ b/clang/test/Frontend/backend-attribute-error-warning.c
@@ -23,11 +23,17 @@ duplicate_warnings(void);
void baz(void) {
foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'baz'}}
if (x())
bar(); // expected-error {{call to 'bar' declared with 'error' attribute: oh no bar}}
+ // expected-note@* {{called by function 'baz'}}
quux(); // enabled-warning {{call to 'quux' declared with 'warning' attribute: oh no quux}}
+ // enabled-note@* {{called by function 'baz'}}
__compiletime_assert_455(); // expected-error {{call to '__compiletime_assert_455' declared with 'error' attribute: demangle me}}
+ // expected-note@* {{called by function 'baz'}}
duplicate_errors(); // expected-error {{call to 'duplicate_errors' declared with 'error' attribute: two}}
+ // expected-note@* {{called by function 'baz'}}
duplicate_warnings(); // enabled-warning {{call to 'duplicate_warnings' declared with 'warning' attribute: two}}
+ // enabled-note@* {{called by function 'baz'}}
}
diff --git a/clang/test/Frontend/backend-attribute-error-warning.cpp b/clang/test/Frontend/backend-attribute-error-warning.cpp
index 1ed549c271241ab..537e798649f7508 100644
--- a/clang/test/Frontend/backend-attribute-error-warning.cpp
+++ b/clang/test/Frontend/backend-attribute-error-warning.cpp
@@ -24,13 +24,19 @@ duplicate_warnings(void);
void baz(void) {
foo(); // expected-error {{call to 'foo()' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'baz()'}}
if (x())
bar(); // expected-error {{call to 'bar()' declared with 'error' attribute: oh no bar}}
+ // expected-note@* {{called by function 'baz()'}}
quux(); // enabled-warning {{call to 'quux()' declared with 'warning' attribute: oh no quux}}
+ // enabled-note@* {{called by function 'baz()'}}
__compiletime_assert_455(); // expected-error {{call to '__compiletime_assert_455()' declared with 'error' attribute: demangle me}}
+ // expected-note@* {{called by function 'baz()'}}
duplicate_errors(); // expected-error {{call to 'duplicate_errors()' declared with 'error' attribute: two}}
+ // expected-note@* {{called by function 'baz()'}}
duplicate_warnings(); // enabled-warning {{call to 'duplicate_warnings()' declared with 'warning' attribute: two}}
+ // enabled-note@* {{called by function 'baz()'}}
}
#ifdef __cplusplus
@@ -46,15 +52,21 @@ struct Widget {
void baz_cpp(void) {
foo(); // expected-error {{call to 'foo()' declared with 'error' attribute: oh no foo}}
+ // expected-note@* {{called by function 'baz_cpp()'}}
if (x())
bar(); // expected-error {{call to 'bar()' declared with 'error' attribute: oh no bar}}
+ // expected-note@* {{called by function 'baz_cpp()'}}
quux(); // enabled-warning {{call to 'quux()' declared with 'warning' attribute: oh no quux}}
+ // enabled-note@* {{called by function 'baz_cpp()'}}
// 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}}
+ // expected-note@* {{called by function 'baz_cpp()'}}
nocall<int>(42); // expected-error {{call to 'int nocall<int>(int)' declared with 'error' attribute: demangle me, too}}
+ // expected-note@* {{called by function 'baz_cpp()'}}
Widget W;
int w = W; // enabled-warning {{call to 'Widget::operator int()' declared with 'warning' attribute: don't call me!}}
+ // enabled-note@* {{called by function 'baz_cpp()'}}
}
#endif
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index e448c5ed5c5d947..c3127dcaf1ef80a 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -1803,13 +1803,17 @@ example:
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.
+ call came from. A string value can be provided as a note. The optimizer may
+ add the optional ``inlined.from`` metadata to call sites which front ends
+ might consume to display more precise diagnostics.
``"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.
+ call came from. A string value can be provided as a note. The optimizer may
+ add the optional ``inlined.from`` metadata to call sites which front ends
+ might consume to display more precise diagnostics.
``fn_ret_thunk_extern``
This attribute tells the code generator that returns from functions should
be replaced with jumps to externally-defined architecture-specific symbols.
diff --git a/llvm/include/llvm/IR/DiagnosticInfo.h b/llvm/include/llvm/IR/DiagnosticInfo.h
index 628445fe9fb2cca..3f64c0acd293ed5 100644
--- a/llvm/include/llvm/IR/DiagnosticInfo.h
+++ b/llvm/include/llvm/IR/DiagnosticInfo.h
@@ -1099,15 +1099,19 @@ class DiagnosticInfoSrcMgr : public DiagnosticInfo {
void diagnoseDontCall(const CallInst &CI);
class DiagnosticInfoDontCall : public DiagnosticInfo {
+ StringRef CallerName;
StringRef CalleeName;
StringRef Note;
unsigned LocCookie;
+ MDNode *MDN;
public:
- DiagnosticInfoDontCall(StringRef CalleeName, StringRef Note,
- DiagnosticSeverity DS, unsigned LocCookie)
- : DiagnosticInfo(DK_DontCall, DS), CalleeName(CalleeName), Note(Note),
- LocCookie(LocCookie) {}
+ DiagnosticInfoDontCall(StringRef CallerName, StringRef CalleeName,
+ StringRef Note, DiagnosticSeverity DS,
+ unsigned LocCookie, MDNode *MDN)
+ : DiagnosticInfo(DK_DontCall, DS), CallerName(CallerName),
+ CalleeName(CalleeName), Note(Note), LocCookie(LocCookie), MDN(MDN) {}
+ StringRef getCaller() const { return CallerName; }
StringRef getFunctionName() const { return CalleeName; }
StringRef getNote() const { return Note; }
unsigned getLocCookie() const { return LocCookie; }
@@ -1115,6 +1119,8 @@ class DiagnosticInfoDontCall : public DiagnosticInfo {
static bool classof(const DiagnosticInfo *DI) {
return DI->getKind() == DK_DontCall;
}
+ void
+ getInliningDecisions(SmallVectorImpl<std::string> &InliningDecisions) const;
};
} // end namespace llvm
diff --git a/llvm/lib/IR/DiagnosticInfo.cpp b/llvm/lib/IR/DiagnosticInfo.cpp
index 342c4cbbc39d65f..bacb4c01a1342ca 100644
--- a/llvm/lib/IR/DiagnosticInfo.cpp
+++ b/llvm/lib/IR/DiagnosticInfo.cpp
@@ -12,6 +12,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/IR/DiagnosticInfo.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/Twine.h"
#include "llvm/ADT/iterator_range.h"
@@ -433,8 +434,9 @@ void llvm::diagnoseDontCall(const CallInst &CI) {
if (MDNode *MD = CI.getMetadata("srcloc"))
LocCookie =
mdconst::extract<ConstantInt>(MD->getOperand(0))->getZExtValue();
- DiagnosticInfoDontCall D(F->getName(), A.getValueAsString(), Sev,
- LocCookie);
+ DiagnosticInfoDontCall D(CI.getParent()->getParent()->getName(),
+ F->getName(), A.getValueAsString(), Sev,
+ LocCookie, CI.getMetadata("inlined.from"));
F->getContext().diagnose(D);
}
}
@@ -449,3 +451,20 @@ void DiagnosticInfoDontCall::print(DiagnosticPrinter &DP) const {
if (!getNote().empty())
DP << ": " << getNote();
}
+
+void DiagnosticInfoDontCall::getInliningDecisions(
+ SmallVectorImpl<std::string> &InliningDecisions) const {
+ if (!MDN)
+ return;
+
+ const MDOperand &MO = MDN->getOperand(0);
+ if (auto *MDT = dyn_cast<MDTuple>(MO)) {
+ for (const MDOperand &MO : MDT->operands()) {
+ if (auto *S = dyn_cast<MDString>(MO)) {
+ InliningDecisions.push_back(S->getString().str());
+ }
+ }
+ } else if (auto *S = dyn_cast<MDString>(MO)) {
+ InliningDecisions.push_back(S->getString().str());
+ }
+}
diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index 39d5f6e53c1de48..81ffc416ffd5c0a 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -2520,6 +2520,19 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI,
// inlining, commonly when the callee is an intrinsic.
if (MarkNoUnwind && !CI->doesNotThrow())
CI->setDoesNotThrow();
+
+ const Function *Callee = CI->getCalledFunction();
+ if (Callee && (Callee->hasFnAttribute("dontcall-error") ||
+ Callee->hasFnAttribute("dontcall-warn"))) {
+ Metadata *MD = MDString::get(CI->getContext(), CalledFunc->getName());
+ if (MDNode *N = CI->getMetadata("inlined.from")) {
+ TempMDTuple Temp = cast<MDTuple>(N)->clone();
+ Temp->push_back(MD);
+ MD = MDNode::replaceWithUniqued(std::move(Temp));
+ }
+ MDTuple *MDT = MDNode::get(CI->getContext(), {MD});
+ CI->setMetadata("inlined.from", MDT);
+ }
}
}
}
diff --git a/llvm/test/Transforms/Inline/dontcall-attributes.ll b/llvm/test/Transforms/Inline/dontcall-attributes.ll
new file mode 100644
index 000000000000000..acf8c6a62aad619
--- /dev/null
+++ b/llvm/test/Transforms/Inline/dontcall-attributes.ll
@@ -0,0 +1,84 @@
+; RUN: opt -S -o - -passes=inline %s \
+; RUN: | FileCheck %s --check-prefixes=CHECK-BOTH,CHECK
+; RUN: opt -S -o - -passes=always-inline %s \
+; RUN: | FileCheck %s --check-prefixes=CHECK-BOTH,CHECK-ALWAYS
+
+declare void @foo() "dontcall-warn"="oh no"
+declare void @fof() "dontcall-error"="oh no"
+
+define void @bar(i32 %x) {
+ %cmp = icmp eq i32 %x, 10
+ br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+ call void @foo()
+ br label %if.end
+
+if.end:
+ ret void
+}
+
+define void @quux() {
+ call void @bar(i32 9)
+ ret void
+}
+
+; Test that @baz's call to @foo has metadata with inlining info.
+define void @baz() {
+; CHECK-LABEL: @baz(
+; CHECK-NEXT: call void @foo(), !inlined.from !0
+;
+ call void @bar(i32 10)
+ ret void
+}
+
+; Test that @zing's call to @foo has unique metadata from @baz's call to @foo.
+define void @zing() {
+; CHECK-LABEL: @zing(
+; CHECK-NEXT: call void @foo(), !inlined.from !1
+;
+ call void @baz()
+ ret void
+}
+
+; Same test but @fof has fn attr "dontcall-error"="..." rather than
+; "dontcall-warn"="...".
+define void @a() {
+ call void @fof()
+ ret void
+}
+define void @b() {
+; CHECK-LABEL: @b(
+; CHECK-NEXT: call void @fof(), !inlined.from !3
+ call void @a()
+ ret void
+}
+
+; Add some tests for alwaysinline.
+define void @always_callee() alwaysinline {
+ call void @fof()
+ ret void
+}
+define void @always_caller() alwaysinline {
+; CHECK-BOTH-LABEL: @always_caller(
+; CHECK-NEXT: call void @fof(), !inlined.from !4
+; CHECK-ALWAYS-NEXT: call void @fof(), !inlined.from !0
+ call void @always_callee()
+ ret void
+}
+define void @always_caller2() alwaysinline {
+; CHECK-BOTH-LABEL: @always_caller2(
+; CHECK-NEXT: call void @fof(), !inlined.from !5
+; CHECK-ALWAYS-NEXT: call void @fof(), !inlined.from !1
+ call void @always_caller()
+ ret void
+}
+
+; CHECK: !0 = !{!"bar"}
+; CHECK-NEXT: !1 = !{!2}
+; CHECK-NEXT: !2 = !{!"bar", !"baz"}
+; CHECK-NEXT: !3 = !{!"a"}
+; CHECK-NEXT: !4 = !{!"always_callee"}
+; CHECK-ALWAYS: !0 = !{!"always_callee"}
+; CHECK-ALWAYS-NEXT: !1 = !{!2}
+; CHECK-ALWAYS-NEXT: !2 = !{!"always_callee", !"always_caller"}
|
hopefully NRVO kicks in *shrug* addresses https://reviews.llvm.org/D141451#inline-1446132
nickdesaulniers
requested review from
MaskRay,
AaronBallman,
aeubanks,
dwblaikie,
erichkeane,
rnk and
kees
|
Note to reviewers, I imported this to a GH PR from https://reviews.llvm.org/D141451, and addressed comments from https://reviews.llvm.org/D141451#4311399 down. I did have a related RFC, but the proposal from @rnk is a yak shave that I don't intend to pursue. Upon rereviewing https://reviews.llvm.org/D141451, it seems there was just some minor nits to resolve. Bringing this back up because @kees reminded me how painful this is. |
erichkeane
reviewed
Nov 27, 2023
| @@ -93,6 +93,8 @@ def err_fe_backend_error_attr : | |||
| def warn_fe_backend_warning_attr : | |||
| Warning<"call to '%0' declared with 'warning' attribute: %1">, BackendInfo, | |||
| InGroup<BackendWarningAttributes>; | |||
| def note_fe_backend_in : Note<"called by function '%0'">; | |||
There was a problem hiding this comment.
Is this the right file for this? It seems weird to do codegen in a 'frontend kinds' diagnostics list, though I'm not convinced I know enough to ask this question well enough.
There was a problem hiding this comment.
Where should the diagnostic strings go if not here?
There was a problem hiding this comment.
IDK, that is why I asked. It SOUNDS wrong, so I'm hopeful someone else can come along and tell us.
There was a problem hiding this comment.
All of the diagnostics for backend warnings are defined in this file. (see existing lines 66-95).
As is, this note is emitted alongside the err/warn from existing lines 91-95, so it makes sense to put the new notes immediately after those existing definitions.
If my newly added backend warning notes belong in a new file, then all of the above belong in a new file. But that seems orthogonal to my patch.
clang/lib/CodeGen/CodeGenAction.cpp
Outdated
| @@ -794,6 +796,13 @@ void BackendConsumer::DontCallDiagHandler(const DiagnosticInfoDontCall &D) { | |||
| ? diag::err_fe_backend_error_attr | |||
| : diag::warn_fe_backend_warning_attr) | |||
| << llvm::demangle(D.getFunctionName()) << D.getNote(); | |||
|
|
|||
| SmallVector<std::string> InliningDecisions = D.getInliningDecisions(); | |||
There was a problem hiding this comment.
Could we use StringRef here instead? It seems to make more sense to me.
There was a problem hiding this comment.
done in e7ed56d PTAL
clang/lib/CodeGen/CodeGenAction.cpp
Outdated
|
|
||
| SmallVector<std::string> InliningDecisions = D.getInliningDecisions(); | ||
| InliningDecisions.push_back(D.getCaller().str()); | ||
| for (auto [index, value] : llvm::enumerate(InliningDecisions)) |
There was a problem hiding this comment.
Suggested change
| for (auto [index, value] : llvm::enumerate(InliningDecisions)) | |
| for (const auto &[index, value] : llvm::enumerate(InliningDecisions)) |
Seems we don't modify either, and value is a string, so requiring a copy of the pair is expensive.
There was a problem hiding this comment.
done in d3a221a PTAL
nickdesaulniers
commented
Nov 27, 2023
clang/lib/CodeGen/CodeGenAction.cpp
Outdated
| SmallVector<StringRef> InliningDecisions = D.getInliningDecisions(); | ||
| InliningDecisions.push_back(D.getCaller()); | ||
| for (const auto &[index, value] : llvm::enumerate(InliningDecisions)) | ||
| Diags.Report(LocCookie, index ? diag::note_fe_backend_inlined |
There was a problem hiding this comment.
Ah right, the use of LocCookie is problematic here.
Take this example (from the RFC):
#include <string.h>
__attribute__((error("bad memcpy"))) void bad(void);
static void *my_memcpy(void *restrict dest, size_t dest_size,
const void *restrict src, size_t src_size,
size_t n) {
if (n > dest_size || n > src_size)
bad();
return memcpy(dest, src, n);
}
void my_driver (void) {
unsigned char src [42], dst [42];
my_memcpy(dst, 42, src, 42, 0);
my_memcpy(dst, 42, src, 42, 42);
my_memcpy(dst, 42, src, 42, 4096);
my_memcpy(dst, 42, src, 42, 1);
}No LocCookie:
$ clang /tmp/x.c -O2
/tmp/x.c:8:9: error: call to 'bad' declared with 'error' attribute: bad memcpy
8 | bad();
| ^
note: called by function 'my_memcpy'
note: inlined by function 'my_driver'
1 error generated.w/ LocCookie:
$ clang /tmp/x.c -O2
/tmp/x.c:8:9: error: call to 'bad' declared with 'error' attribute: bad memcpy
8 | bad();
| ^
/tmp/x.c:8:9: note: called by function 'my_memcpy'
/tmp/x.c:8:9: note: inlined by function 'my_driver'
1 error generated.The LocCookie is pointing to the function call to bad();, not the call to my_memcpy because that information was not retained. (We don't retain such debug info unless debug info (or optimization remarks) were requested; I discuss this in the RFC. I think it's preferable to have no line+col info on the note rather than wrong line+col info).
cc @MaskRay since this change was in response to https://reviews.llvm.org/D141451#inline-1445409
nickdesaulniers
commented
Nov 27, 2023
llvm/lib/IR/DiagnosticInfo.cpp
Outdated
Comment on lines
458
to
469
| if (MDN) { | ||
| const MDOperand &MO = MDN->getOperand(0); | ||
| if (auto *MDT = dyn_cast<MDTuple>(MO)) { | ||
| for (const MDOperand &MO : MDT->operands()) { | ||
| if (auto *S = dyn_cast<MDString>(MO)) { | ||
| if (MDN) | ||
| if (auto *MDT = dyn_cast<MDTuple>(MDN->getOperand(0))) | ||
| for (const MDOperand &MO : MDT->operands()) | ||
| if (auto *S = dyn_cast<MDString>(MO)) | ||
| InliningDecisions.push_back(S->getString().str()); | ||
| } | ||
| } | ||
| } else if (auto *S = dyn_cast<MDString>(MO)) { | ||
| InliningDecisions.push_back(S->getString().str()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This change is wrong; it seems I don't have this captured in the existing tests, but doing a local integration test:
#include <string.h>
__attribute__((error("bad memcpy"))) void bad(void);
static void *my_memcpy(void *restrict dest, size_t dest_size,
const void *restrict src, size_t src_size,
size_t n) {
if (n > dest_size || n > src_size)
bad();
return memcpy(dest, src, n);
}
void my_driver (void) {
unsigned char src [42], dst [42];
my_memcpy(dst, 42, src, 42, 0);
my_memcpy(dst, 42, src, 42, 42);
my_memcpy(dst, 42, src, 42, 4096);
my_memcpy(dst, 42, src, 42, 1);
}Before this change:
/tmp/x.c:8:9: error: call to 'bad' declared with 'error' attribute: bad memcpy
8 | bad();
| ^
/tmp/x.c:8:9: note: called by function 'my_memcpy'
/tmp/x.c:8:9: note: inlined by function 'my_driver'After this change:
$ /tmp/x.c:8:9: error: call to 'bad' declared with 'error' attribute: bad memcpy
8 | bad();
| ^
/tmp/x.c:8:9: note: called by function 'my_memcpy'
1 error generated.
There was a problem hiding this comment.
We want it to read like "before", yes?
There was a problem hiding this comment.
yes; note:
- github is having some kind of outtage currently. https://www.githubstatus.com/ currently mentions an issue with PRs. I have pushed a revert but it's not yet being reflected in the GH UI.
- I need to fix the issue pointed out https://reviews.llvm.org/D141451#inline-1446137, which I haven't done yet. The revert was incorrect.
This reverts commit 239ab3e.
I'd push on that a bit more. As I mentioned on the phab review, I think it'd be unfortunate to build a second system for tracking inlining apart from the debug info. Guess it comes down to whether some sufficient quorum insist on that being necessary yak shaving here or not. |
nickdesaulniers
commented
Nov 27, 2023
|
|
||
| if (MDN) { | ||
| const MDOperand &MO = MDN->getOperand(0); | ||
| if (auto *MDT = dyn_cast<MDTuple>(MO)) { |
There was a problem hiding this comment.
Copying @aeubanks 's comment from the phab review:
seems simpler if this is always a MDTuple instead of special casing one entry to be MDString
For the life of me I cannot figure out why when an MDTuple is created via:
Metadata *MD = MDString::get(CI->getContext(), CalledFunc->getName());
MDTuple *MDT = MDNode::get(CI->getContext(), {MD});
CI->setMetadata("inlined.from", MDT);you get
!10 = !{!"my_memcpy"}but when you copy a MDTuple and then append to it, you get a layer of indirection:
Metadata *MD = MDString::get(CI->getContext(), CalledFunc->getName());
if (MDNode *N = CI->getMetadata("inlined.from")) {
TempMDTuple Temp = cast<MDTuple>(N)->clone();
Temp->push_back(MD);
MD = MDNode::replaceWithUniqued(std::move(Temp));
}
MDTuple *MDT = MDNode::get(CI->getContext(), {MD});
CI->setMetadata("inlined.from", MDT);!10 = !{!11}
!11 = !{!"my_memcpy", !"my_driver"}when I would have expected:
!10 = !{!"my_memcpy", !"my_driver"}am I holding the API wrong??
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Due to inlining, descovering which specific call site to a function with
the attribute "warning" or "error" is painful.
In the IR record inlining decisions in a new metadata tuple "inlined.from" when inlining a callee
that itself contains a call to a dontcall-error or dontcall-warn fn.
Print this info so that it's clearer which call site is problematic.
There's still some limitations with this approach; macro expansion is
not recorded.
Fixes: ClangBuiltLinux/linux#1571
Differential Revision: https://reviews.llvm.org/D141451