Skip to content
Open
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/lib/CodeGen/CGCall.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6295,6 +6295,8 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
DI->EmitFuncDeclForCallSite(
CI, DI->getFunctionType(CalleeDecl, ResTy, Args), CalleeGlobalDecl);
}
// Collect call site target information.
DI->addCallTarget(CI->getCalledFunction(), CalleeDecl, CI);
}

return Ret;
Expand Down
109 changes: 107 additions & 2 deletions clang/lib/CodeGen/CGDebugInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2430,6 +2430,9 @@ llvm::DISubprogram *CGDebugInfo::CreateCXXMemberFunction(

SPCache[Method->getCanonicalDecl()].reset(SP);

// Add the method declaration as a call target.
addCallTarget(MethodLinkageName, SP, /*CI=*/nullptr);

return SP;
}

Expand Down Expand Up @@ -4955,6 +4958,95 @@ void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,
Fn->setSubprogram(SP);
}

bool CGDebugInfo::generateVirtualCallSite() const {
// Check general conditions for call site generation.
return (getCallSiteRelatedAttrs() != llvm::DINode::FlagZero);
}

// Set the 'call_target' metadata in the call instruction.
void CGDebugInfo::addCallTargetMetadata(llvm::MDNode *MD, llvm::CallBase *CI) {
if (!MD || !CI)
return;
CI->setMetadata(llvm::LLVMContext::MD_call_target, MD);
}

// Finalize call_target generation.
void CGDebugInfo::finalizeCallTarget() {
if (!generateVirtualCallSite())
return;

for (auto &E : CallTargetCache) {
for (const auto &WH : E.second.second) {
llvm::CallBase *CI = dyn_cast_or_null<llvm::CallBase>(WH);
addCallTargetMetadata(E.second.first, CI);
}
}
}

void CGDebugInfo::addCallTarget(StringRef Name, llvm::MDNode *MD,
llvm::CallBase *CI) {
if (!generateVirtualCallSite())
return;

// Record only indirect calls.
if (CI && !CI->isIndirectCall())
return;

// Nothing to do.
if (Name.empty())
return;

auto It = CallTargetCache.find(Name);
if (It == CallTargetCache.end()) {
// First time we see 'Name'. Insert record for later finalize.
InstrList List;
if (CI)
List.push_back(CI);
CallTargetCache.try_emplace(Name, MD, std::move(List));
} else {
if (MD)
It->second.first.reset(MD);
if (CI) {
InstrList &List = It->second.second;
List.push_back(CI);
}
}
Comment on lines +4999 to +5013
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is doing some duplicate lookup - by using find then emplace it does the map lookup twice - usually good to try to avoid that, and it often simplifies the code too... I /think/ this is equivalent and simpler:

Suggested change
auto It = CallTargetCache.find(Name);
if (It == CallTargetCache.end()) {
// First time we see 'Name'. Insert record for later finalize.
InstrList List;
if (CI)
List.push_back(CI);
CallTargetCache.try_emplace(Name, MD, std::move(List));
} else {
if (MD)
It->second.first.reset(MD);
if (CI) {
InstrList &List = It->second.second;
List.push_back(CI);
}
}
auto It = CallTargetCache.insert(Name);
if (MD)
It->second.first.reset(MD);
if (CI)
It->second.second.push_back(CI);

}

void CGDebugInfo::addCallTarget(llvm::Function *F, const FunctionDecl *FD,
llvm::CallBase *CI) {
if (!generateVirtualCallSite())
return;

if (!F && !FD)
return;

// Ignore method types that never can be indirect calls.
if (!F && (isa<CXXConstructorDecl>(FD) || isa<CXXDestructorDecl>(FD) ||
FD->hasAttr<CUDAGlobalAttr>()))
return;

StringRef Name = (F && F->hasName()) ? F->getName() : CGM.getMangledName(FD);
addCallTarget(Name, /*MD=*/nullptr, CI);
}

void CGDebugInfo::removeCallTarget(StringRef Name) {
if (!generateVirtualCallSite())
return;

auto It = CallTargetCache.find(Name);
if (It != CallTargetCache.end())
CallTargetCache.erase(It);
Comment on lines +5037 to +5039
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
auto It = CallTargetCache.find(Name);
if (It != CallTargetCache.end())
CallTargetCache.erase(It);
CallTargetCache.erase(Name);

}

void CGDebugInfo::removeCallTarget(llvm::Function *F) {
if (!generateVirtualCallSite())
return;

if (F && F->hasName())
removeCallTarget(F->getName());
}

void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
QualType CalleeType,
GlobalDecl CalleeGlobalDecl) {
Expand All @@ -4978,9 +5070,13 @@ void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
// If there is no DISubprogram attached to the function being called,
// create the one describing the function in order to have complete
// call site debug info.
if (!CalleeDecl->isStatic() && !CalleeDecl->isInlined())
if (!CalleeDecl->isStatic() && !CalleeDecl->isInlined()) {
EmitFunctionDecl(CalleeGlobalDecl, CalleeDecl->getLocation(), CalleeType,
Func);
// For each call instruction emitted, add the call site target metadata.
if (llvm::DISubprogram *SP = Func->getSubprogram())
addCallTarget(SP->getLinkageName(), SP, /*CI=*/nullptr);
}
}

void CGDebugInfo::EmitInlineFunctionStart(CGBuilderTy &Builder, GlobalDecl GD) {
Expand Down Expand Up @@ -5082,8 +5178,14 @@ void CGDebugInfo::EmitFunctionEnd(CGBuilderTy &Builder, llvm::Function *Fn) {
}
FnBeginRegionCount.pop_back();

if (Fn && Fn->getSubprogram())
if (Fn && Fn->getSubprogram()) {
// As the debug info for the given function has been emitted, add its
// name and node to the call site target information.
llvm::DISubprogram *SP = Fn->getSubprogram();
addCallTarget(SP->getLinkageName(), SP, /*CI=*/nullptr);

DBuilder.finalizeSubprogram(Fn->getSubprogram());
}
}

CGDebugInfo::BlockByRefType
Expand Down Expand Up @@ -6498,6 +6600,9 @@ void CGDebugInfo::finalize() {
if (auto MD = TypeCache[RT])
DBuilder.retainType(cast<llvm::DIType>(MD));

// Generate call_target information.
finalizeCallTarget();

DBuilder.finalize();
}

Expand Down
28 changes: 28 additions & 0 deletions clang/lib/CodeGen/CGDebugInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,15 @@ class CGDebugInfo {
/// that it is supported and enabled.
llvm::DINode::DIFlags getCallSiteRelatedAttrs() const;

/// Add call target information.
void addCallTarget(StringRef Name, llvm::MDNode *MD, llvm::CallBase *CI);
void addCallTarget(llvm::Function *F, const FunctionDecl *FD,
llvm::CallBase *CI);

/// Remove a call target entry for the given name or function.
void removeCallTarget(StringRef Name);
void removeCallTarget(llvm::Function *F);

private:
/// Amend \p I's DebugLoc with \p Group (its source atom group) and \p
/// Rank (lower nonzero rank is higher precedence). Does nothing if \p I
Expand Down Expand Up @@ -903,6 +912,25 @@ class CGDebugInfo {
/// If one exists, returns the linkage name of the specified \
/// (non-null) \c Method. Returns empty string otherwise.
llvm::StringRef GetMethodLinkageName(const CXXMethodDecl *Method) const;

/// For each 'DISuprogram' we store a list of call instructions 'CallBase'
/// that indirectly call such 'DISuprogram'. We use its linkage name to
/// update such list.
/// The 'CallTargetCache' is updated in the following scenarios:
/// - Both 'CallBase' and 'MDNode' are available.
/// - If only the 'CallBase' or 'MDNode' are available, the partial
/// information is added and later is completed when the missing item
/// ('CallBase' or 'MDNode') is available.
using InstrList = llvm::SmallVector<llvm::WeakVH, 2>;
using CallTargetEntry = std::pair<llvm::TrackingMDNodeRef, InstrList>;
llvm::SmallDenseMap<StringRef, CallTargetEntry> CallTargetCache;
Comment on lines +916 to +926
Copy link
Collaborator

Choose a reason for hiding this comment

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

This infrastructure seems a bit complicated/unforttunate - can we avoid the "cache" and conditional supporting by supporting this in the same way we inject function declarations into classes as-needed when emitting debug info for out of line definitions. Consider this example: https://godbolt.org/z/qdM5d35hc - the function declaration is emitted (& will be deduplicated (or won't be duplicated in the first place, I forget how we do it) if later in the file a definition was emitted (eg: if the ctor definition came after the ::f2 definition)) - we could always produce a function declaration and a class declaration for the call site - the metadata in the call_target would keep both alive, unless the call site were optimized away, in which case the type and declaration would be optimized away.

Doing this might also allow us a better representation even for the non-virtual cases, where I believe we have similar problems where the call site debug info is conditionally emitted only if the function declaration is emitted for some other reason - ie: the call site doesn't keep the debug info alive as it should. Which is awkward for users since the reasons the call site might appear or not are pretty opaque/unrelated to what the user is doing.


/// Generate call target information.
bool generateVirtualCallSite() const;

/// Add 'call_target' metadata to the 'call' instruction.
void addCallTargetMetadata(llvm::MDNode *MD, llvm::CallBase *CI);
void finalizeCallTarget();
};

/// A scoped helper to set the current debug location to the specified
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/CodeGen/CodeGenFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1510,6 +1510,8 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
// Clear non-distinct debug info that was possibly attached to the function
// due to an earlier declaration without the nodebug attribute
Fn->setSubprogram(nullptr);
if (CGDebugInfo *DI = getDebugInfo())
DI->removeCallTarget(Fn);
// Disable debug info indefinitely for this function
DebugInfo = nullptr;
}
Expand Down
36 changes: 36 additions & 0 deletions clang/test/DebugInfo/CXX/callsite-base.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// RUN: %clang_cc1 -triple=x86_64-linux -disable-llvm-passes -emit-llvm \
// RUN: -debug-info-kind=constructor -dwarf-version=5 -O1 %s \
// RUN: -o - | FileCheck %s -check-prefix CHECK-BASE

// Simple class with only virtual methods: inlined and not-inlined
// We check for a generated 'call_target' for:
// - 'one', 'two' and 'three'.

class CBase {
public:
virtual void one();
virtual void two();
virtual void three() {}
};
void CBase::one() {}

void bar(CBase *Base) {
Base->one();
Base->two();
Base->three();

CBase B;
B.one();
}

// CHECK-BASE: define {{.*}} @_Z3barP5CBase{{.*}} {
// CHECK-BASE: call void %1{{.*}} !dbg {{![0-9]+}}, !call_target [[BASE_ONE:![0-9]+]]
// CHECK-BASE: call void %3{{.*}} !dbg {{![0-9]+}}, !call_target [[BASE_TWO:![0-9]+]]
// CHECK-BASE: call void %5{{.*}} !dbg {{![0-9]+}}, !call_target [[BASE_THREE:![0-9]+]]
// CHECK-BASE: call void @_ZN5CBaseC1Ev{{.*}} !dbg {{![0-9]+}}
// CHECK-BASE: call void @_ZN5CBase3oneEv{{.*}} !dbg {{![0-9]+}}
// CHECK-BASE: }

// CHECK-BASE: [[BASE_TWO]] = {{.*}}!DISubprogram(name: "two", linkageName: "_ZN5CBase3twoEv"
// CHECK-BASE: [[BASE_ONE]] = {{.*}}!DISubprogram(name: "one", linkageName: "_ZN5CBase3oneEv"
// CHECK-BASE: [[BASE_THREE]] = {{.*}}!DISubprogram(name: "three", linkageName: "_ZN5CBase5threeEv"
77 changes: 77 additions & 0 deletions clang/test/DebugInfo/CXX/callsite-derived.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// RUN: %clang_cc1 -triple=x86_64-linux -disable-llvm-passes -emit-llvm \
// RUN: -debug-info-kind=constructor -dwarf-version=5 -O1 %s \
// RUN: -o - | FileCheck %s -check-prefix CHECK-DERIVED

// Simple base and derived class with virtual and static methods:
// We check for a generated 'call_target' for:
// - 'one', 'two' and 'three'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

one and two don't seem sufficiently interestingly different to me - is there something in there that leads to different codepaths in this patch? And three is presumably already tested since it's not virtual?


class CBase {
public:
virtual void one(bool Flag) {}
virtual void two(int P1, char P2) {}
static void three();
};

void CBase::three() {
}
void bar(CBase *Base);

void foo(CBase *Base) {
CBase::three();
}

class CDerived : public CBase {
public:
Comment on lines +24 to +25
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably use a struct (here and elsewhere) since it's public inheritance and members by default

void one(bool Flag) {}
void two(int P1, char P2) {}
};
void foo(CDerived *Derived);

int main() {
CBase B;
bar(&B);

CDerived D;
foo(&D);

return 0;
}

void bar(CBase *Base) {
Base->two(77, 'a');
}

void foo(CDerived *Derived) {
Derived->one(true);
}

// CHECK-DERIVED: define {{.*}} @_Z3fooP5CBase{{.*}} {
// CHECK-DERIVED: call void @_ZN5CBase5threeEv{{.*}} !dbg {{![0-9]+}}
// CHECK-DERIVED: }

// CHECK-DERIVED: define {{.*}} @main{{.*}} {
// CHECK-DERIVED: call void @_ZN5CBaseC1Ev{{.*}} !dbg {{![0-9]+}}
// CHECK-DERIVED: call void @_Z3barP5CBase{{.*}} !dbg {{![0-9]+}}
// CHECK-DERIVED: call void @_ZN8CDerivedC1Ev{{.*}} !dbg {{![0-9]+}}
// CHECK-DERIVED: call void @_Z3fooP8CDerived{{.*}} !dbg {{![0-9]+}}
// CHECK-DERIVED: }

// CHECK-DERIVED: define {{.*}} @_ZN5CBaseC1Ev{{.*}} {
// CHECK-DERIVED: call void @_ZN5CBaseC2Ev{{.*}} !dbg {{![0-9]+}}
// CHECK-DERIVED: }

// CHECK-DERIVED: define {{.*}} @_Z3barP5CBase{{.*}} {
// CHECK-DERIVED: call void %1{{.*}} !dbg {{![0-9]+}}, !call_target [[BASE_TWO:![0-9]+]]
// CHECK-DERIVED: }

// CHECK-DERIVED: define {{.*}} @_ZN8CDerivedC1Ev{{.*}} {
// CHECK-DERIVED: call void @_ZN8CDerivedC2Ev{{.*}} !dbg {{![0-9]+}}
// CHECK-DERIVED: }

// CHECK-DERIVED: define {{.*}} @_Z3fooP8CDerived{{.*}} {
// CHECK-DERIVED: call void %1{{.*}} !dbg {{![0-9]+}}, !call_target [[DERIVED_ONE:![0-9]+]]
// CHECK-DERIVED: }

// CHECK-DERIVED: [[BASE_TWO]] = {{.*}}!DISubprogram(name: "two", linkageName: "_ZN5CBase3twoEic"
// CHECK-DERIVED: [[DERIVED_ONE]] = {{.*}}!DISubprogram(name: "one", linkageName: "_ZN8CDerived3oneEb"
Loading