Skip to content

[clang] report inlining decisions with -Wattribute-{warning|error} #73552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticFrontendKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -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'">;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where should the diagnostic strings go if not here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK, that is why I asked. It SOUNDS wrong, so I'm hopeful someone else can come along and tell us.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
Expand Down
8 changes: 8 additions & 0 deletions clang/lib/CodeGen/CodeGenAction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#include "llvm/Transforms/Utils/Cloning.h"

#include <optional>

using namespace clang;
using namespace llvm;

Expand Down Expand Up @@ -794,6 +795,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<StringRef> InliningDecisions = D.getInliningDecisions();
InliningDecisions.push_back(D.getCaller());
for (const auto &[index, value] : llvm::enumerate(InliningDecisions))
Diags.Report(index ? diag::note_fe_backend_inlined
: diag::note_fe_backend_in)
<< llvm::demangle(value);
}

void BackendConsumer::MisExpectDiagHandler(
Expand Down
21 changes: 21 additions & 0 deletions clang/test/Frontend/backend-attribute-error-warning-optimize.c
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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();
}
6 changes: 6 additions & 0 deletions clang/test/Frontend/backend-attribute-error-warning.c
Original file line number Diff line number Diff line change
Expand Up @@ -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'}}
}
12 changes: 12 additions & 0 deletions clang/test/Frontend/backend-attribute-error-warning.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
8 changes: 6 additions & 2 deletions llvm/docs/LangRef.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
14 changes: 9 additions & 5 deletions llvm/include/llvm/IR/DiagnosticInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include <functional>
#include <iterator>
#include <optional>
#include <string>

namespace llvm {

Expand Down Expand Up @@ -1099,22 +1098,27 @@ 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; }
void print(DiagnosticPrinter &DP) const override;
static bool classof(const DiagnosticInfo *DI) {
return DI->getKind() == DK_DontCall;
}
SmallVector<StringRef> getInliningDecisions() const;
};

} // end namespace llvm
Expand Down
25 changes: 23 additions & 2 deletions llvm/lib/IR/DiagnosticInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -449,3 +451,22 @@ void DiagnosticInfoDontCall::print(DiagnosticPrinter &DP) const {
if (!getNote().empty())
DP << ": " << getNote();
}

SmallVector<StringRef> DiagnosticInfoDontCall::getInliningDecisions() const {
SmallVector<StringRef> InliningDecisions;

if (MDN) {
const MDOperand &MO = MDN->getOperand(0);
if (auto *MDT = dyn_cast<MDTuple>(MO)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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??

for (const MDOperand &MO : MDT->operands()) {
if (auto *S = dyn_cast<MDString>(MO)) {
InliningDecisions.push_back(S->getString());
}
}
} else if (auto *S = dyn_cast<MDString>(MO)) {
InliningDecisions.push_back(S->getString());
}
}

return InliningDecisions;
}
13 changes: 13 additions & 0 deletions llvm/lib/Transforms/Utils/InlineFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
}
Expand Down
80 changes: 80 additions & 0 deletions llvm/test/Transforms/Inline/dontcall-attributes.ll
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
; RUN: opt -S -passes=inline < %s | FileCheck %s --check-prefixes=CHECK-BOTH,CHECK
; RUN: opt -S -passes=always-inline < %s | 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: define {{[^@]+}}@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: define {{[^@]+}}@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 @_Z1av() {
call void @fof()
ret void
}
define void @_Z1bv() {
; CHECK-LABEL: define {{[^@]+}}@_Z1bv(
; CHECK-NEXT: call void @fof(), !inlined.from !3
call void @_Z1av()
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: define {{[^@]+}}@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: define {{[^@]+}}@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 = !{!"_Z1av"}
; CHECK-NEXT: !4 = !{!"always_callee"}
; CHECK-ALWAYS: !0 = !{!"always_callee"}
; CHECK-ALWAYS-NEXT: !1 = !{!2}
; CHECK-ALWAYS-NEXT: !2 = !{!"always_callee", !"always_caller"}