Skip to content

Commit 93ec698

Browse files
committed
Review comments, and attempted optimization
1 parent f61843f commit 93ec698

File tree

6 files changed

+153
-45
lines changed

6 files changed

+153
-45
lines changed

llvm/include/llvm/IR/DebugLoc.h

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,11 @@ namespace llvm {
2626
class Function;
2727

2828
#ifdef LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING
29-
struct DbgLocOriginBacktrace {
29+
struct DbgLocOriginStacktrace {
3030
static constexpr unsigned long MaxDepth = 16;
31-
SmallVector<std::pair<int, std::array<void *, MaxDepth>>, 0> Stacktraces;
32-
DbgLocOriginBacktrace(bool ShouldCollectTrace);
31+
size_t StacktraceIdx;
32+
DbgLocOriginStacktrace(bool ShouldCollectTrace);
33+
SmallVector<std::pair<int, std::array<void *, MaxDepth>>, 0> getStacktraces();
3334
void addTrace();
3435
};
3536

@@ -58,7 +59,7 @@ namespace llvm {
5859
DebugLocKind Kind;
5960
// Currently we only need to track the Origin of this DILoc when using a
6061
// normal empty DebugLoc, so only collect the stack trace in those cases.
61-
DbgLocOriginBacktrace Origin;
62+
DbgLocOriginStacktrace Origin;
6263
DILocAndCoverageTracking(bool NeedsStacktrace = true)
6364
: TrackingMDNodeRef(nullptr), Kind(DebugLocKind::Normal), Origin(NeedsStacktrace) {
6465
}
@@ -120,7 +121,7 @@ namespace llvm {
120121
#ifdef LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING
121122
DebugLoc(DebugLocKind Kind) : Loc(Kind) {}
122123
DebugLocKind getKind() const { return Loc.Kind; }
123-
DbgLocOriginBacktrace getOrigin() const { return Loc.Origin; }
124+
DbgLocOriginStacktrace getOrigin() const { return Loc.Origin; }
124125
DebugLoc getCopied() const {
125126
DebugLoc NewDL = *this;
126127
NewDL.Loc.Origin.addTrace();

llvm/include/llvm/IR/IRBuilder.h

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -90,17 +90,19 @@ class IRBuilderCallbackInserter : public IRBuilderDefaultInserter {
9090
/// Common base class shared among various IRBuilders.
9191
class IRBuilderBase {
9292
/// Pairs of (metadata kind, MDNode *) that should be added to all newly
93-
/// created instructions, like !dbg metadata.
93+
/// created instructions, excluding !dbg metadata, which is stored in the
94+
// StoredDL field.
9495
SmallVector<std::pair<unsigned, MDNode *>, 2> MetadataToCopy;
95-
96+
// The DebugLoc that will be applied to instructions inserted by this builder.
9697
DebugLoc StoredDL;
98+
// Tracks whether we have explicitly set a DebugLoc - valid or empty - in this
99+
// builder, to determine whether to copy StoredDL to inserted instructions.
100+
bool HasExplicitDL = false;
97101

98102
/// Add or update the an entry (Kind, MD) to MetadataToCopy, if \p MD is not
99103
/// null. If \p MD is null, remove the entry with \p Kind.
100104
void AddOrRemoveMetadataToCopy(unsigned Kind, MDNode *MD) {
101-
if (Kind == LLVMContext::MD_dbg) {
102-
StoredDL = DebugLoc(MD);
103-
}
105+
assert(Kind != LLVMContext::MD_dbg && "MD_dbg metadata must be stored in StoredDL");
104106

105107
if (!MD) {
106108
erase_if(MetadataToCopy, [Kind](const std::pair<unsigned, MDNode *> &KV) {
@@ -221,11 +223,10 @@ class IRBuilderBase {
221223

222224
/// Set location information used by debugging information.
223225
void SetCurrentDebugLocation(DebugLoc L) {
224-
AddOrRemoveMetadataToCopy(LLVMContext::MD_dbg, L.getAsMDNode());
225-
// Although we set StoredDL in the above call, we prefer to use the exact
226-
// DebugLoc we were given, so overwrite it here; the call is only needed to
227-
// update the entry in MetadataToCopy.
226+
// For !dbg metadata attachments, we use DebugLoc instead of the raw MDNode
227+
// to include optional introspection data for use in Debugify.
228228
StoredDL = std::move(L);
229+
HasExplicitDL = true;
229230
}
230231

231232
/// Set nosanitize metadata.
@@ -240,9 +241,10 @@ class IRBuilderBase {
240241
void CollectMetadataToCopy(Instruction *Src,
241242
ArrayRef<unsigned> MetadataKinds) {
242243
for (unsigned K : MetadataKinds) {
243-
AddOrRemoveMetadataToCopy(K, Src->getMetadata(K));
244244
if (K == LLVMContext::MD_dbg)
245245
SetCurrentDebugLocation(Src->getDebugLoc());
246+
else
247+
AddOrRemoveMetadataToCopy(K, Src->getMetadata(K));
246248
}
247249
}
248250

@@ -255,20 +257,12 @@ class IRBuilderBase {
255257

256258
/// Add all entries in MetadataToCopy to \p I.
257259
void AddMetadataToInst(Instruction *I) const {
258-
for (const auto &KV : MetadataToCopy) {
259-
if (KV.first == LLVMContext::MD_dbg) {
260-
// If `!I->getDebugLoc()` then we will call this below anyway, so skip
261-
// a duplicate call here.
262-
if (I->getDebugLoc())
263-
I->setDebugLoc(StoredDL.getCopied());
264-
continue;
265-
}
260+
for (const auto &KV : MetadataToCopy)
266261
I->setMetadata(KV.first, KV.second);
267-
}
268262
// If I does not have an existing DebugLoc and no DebugLoc has been set
269263
// here, we copy our DebugLoc to I anyway, because more likely than not I
270264
// is a new instruction whose DL should originate from this builder.
271-
if (!I->getDebugLoc())
265+
if (HasExplicitDL || !I->getDebugLoc())
272266
I->setDebugLoc(StoredDL.getCopied());
273267
}
274268

llvm/include/llvm/Support/Signals.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,20 @@ namespace sys {
7272
void PrintStackTrace(raw_ostream &OS, int Depth = 0);
7373

7474
#ifdef LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING
75+
#ifdef NDEBUG
76+
#error DebugLoc Coverage Tracking should not be enabled in Release builds.
77+
#endif
78+
/// Populates the given array with a stacktrace of the current program, up to MaxDepth frames.
79+
/// Returns the number of frames returned, which will be inserted into \p StackTrace from index 0.
80+
/// All entries after the returned depth will be unmodified.
81+
/// NB: This is only intended to be used for introspection of LLVM by Debugify, will not be
82+
/// enabled in release builds, and should not be relied on for other purposes.
7583
template <unsigned long MaxDepth> int getStackTrace(std::array<void *, MaxDepth> &StackTrace);
7684

7785
/// Takes a set of \p Addresses, symbolizes them and stores the result in the
7886
/// provided \p SymbolizedAddresses map.
87+
/// NB: This is only intended to be used for introspection of LLVM by Debugify, will not be
88+
/// enabled in release builds, and should not be relied on for other purposes.
7989
void symbolizeAddresses(AddressSet &Addresses,
8090
SymbolizedAddressMap &SymbolizedAddresses);
8191
#endif

llvm/lib/IR/DebugLoc.cpp

Lines changed: 115 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
//
77
//===----------------------------------------------------------------------===//
88

9+
#include "llvm/ADT/Hashing.h"
910
#include "llvm/IR/DebugLoc.h"
1011
#include "llvm/Config/llvm-config.h"
1112
#include "llvm/IR/DebugInfo.h"
@@ -16,21 +17,126 @@
1617

1718
using namespace llvm;
1819

20+
struct StacktraceOrList {
21+
union {
22+
std::array<void *, DbgLocOriginStacktrace::MaxDepth> Single;
23+
std::array<size_t, DbgLocOriginStacktrace::MaxDepth> STList;
24+
};
25+
uint8_t Size : 7;
26+
uint8_t IsList : 1;
27+
StacktraceOrList(int Size, std::array<size_t, DbgLocOriginStacktrace::MaxDepth> STList) : STList(STList), Size(Size), IsList(0) {}
28+
StacktraceOrList(int Size, std::array<void *, DbgLocOriginStacktrace::MaxDepth> Single) : Single(Single), Size(Size), IsList(1) {}
29+
StacktraceOrList(bool Tombstone = false) : Size(Tombstone ? 0x7F : 0), IsList(Tombstone ? 1 : 0) {
30+
static_assert(DbgLocOriginStacktrace::MaxDepth < 0x7F, "Size of backtrace array must fit in 7 bits - 1");
31+
}
32+
};
33+
34+
hash_code hash_value(const StacktraceOrList &S) {
35+
static_assert(DbgLocOriginStacktrace::MaxDepth < (1 << 7));
36+
hash_code Result;
37+
if (S.IsList) {
38+
Result = hash_combine_range(S.Single.begin(), S.Single.begin() + S.Size);
39+
} else {
40+
Result = hash_combine_range(S.STList.begin(), S.STList.begin() + S.Size);
41+
}
42+
return Result;
43+
}
44+
45+
template <> struct DenseMapInfo<StacktraceOrList> {
46+
static inline StacktraceOrList getEmptyKey() {
47+
return StacktraceOrList();
48+
}
49+
50+
static inline StacktraceOrList getTombstoneKey() {
51+
return StacktraceOrList(true);
52+
}
53+
54+
static unsigned getHashValue(const StacktraceOrList &Val) {
55+
return hash_value(Val);
56+
}
57+
58+
static bool isEqual(const StacktraceOrList &LHS, const StacktraceOrList &RHS) {
59+
if (LHS.IsList != RHS.IsList)
60+
return false;
61+
if (LHS.IsList)
62+
return ArrayRef(LHS.Single.begin(), LHS.Single.begin() + LHS.Size) == ArrayRef(RHS.Single.begin(), RHS.Single.begin() + RHS.Size);
63+
return ArrayRef(LHS.STList.begin(), LHS.STList.begin() + LHS.Size) == ArrayRef(RHS.STList.begin(), RHS.STList.begin() + RHS.Size);
64+
}
65+
};
66+
67+
// Storage for every unique stacktrace or list of stacktraces collected across this run of the program.
68+
static std::vector<StacktraceOrList> CollectedStacktraces(1, StacktraceOrList());
69+
// Mapping from a unique stacktrace or list of stacktraces to an index in the CollectedStacktraces vector.
70+
static DenseMap<StacktraceOrList, size_t> StacktraceStorageMap;
71+
72+
size_t getIndex(const StacktraceOrList &S) {
73+
// Special empty value that can't be inserted into the map.
74+
if (S.Size == 0)
75+
return 0;
76+
auto [It, R] = StacktraceStorageMap.insert({S, CollectedStacktraces.size()});
77+
if (R)
78+
CollectedStacktraces.push_back(S);
79+
return It->second;
80+
}
81+
inline StacktraceOrList getStacktraceFromIndex(size_t Idx) {
82+
return CollectedStacktraces[Idx];
83+
}
84+
// Given an index to an existing stored stacktrace, and a new
85+
size_t getIndexForAddedTrace(size_t Idx, StacktraceOrList S) {
86+
size_t AddedIdx = getIndex(S);
87+
if (Idx == 0)
88+
return AddedIdx;
89+
StacktraceOrList Current = getStacktraceFromIndex(Idx);
90+
StacktraceOrList New;
91+
if (Current.IsList) {
92+
std::array<size_t, DbgLocOriginStacktrace::MaxDepth> STList = {Idx, AddedIdx};
93+
New = StacktraceOrList(2, STList);
94+
} else {
95+
// There is no way to represent a STList-backtrace containing more than MaxDepth
96+
// backtraces, so just leave it unappended.
97+
// FIXME: We could rotate it, so that we get the MaxDepth-most recent
98+
// backtraces rather than the oldest?
99+
if (Current.Size == DbgLocOriginStacktrace::MaxDepth)
100+
return Idx;
101+
New = StacktraceOrList(Current.Size + 1, Current.STList);
102+
New.STList[Current.Size] = AddedIdx;
103+
}
104+
return getIndex(S);
105+
}
106+
107+
19108
DILocAndCoverageTracking::DILocAndCoverageTracking(const DILocation *L)
20109
: TrackingMDNodeRef(const_cast<DILocation *>(L)),
21110
Kind(DebugLocKind::Normal), Origin(!L) {}
22111

23-
DbgLocOriginBacktrace::DbgLocOriginBacktrace(bool ShouldCollectTrace) {
24-
if (ShouldCollectTrace) {
25-
auto &[Depth, Stacktrace] = Stacktraces.emplace_back();
26-
Depth = sys::getStackTrace(Stacktrace);
27-
}
112+
DbgLocOriginStacktrace::DbgLocOriginStacktrace(bool ShouldCollectTrace) : StacktraceIdx(0) {
113+
if (!ShouldCollectTrace)
114+
return;
115+
std::array<void *, DbgLocOriginStacktrace::MaxDepth> Stacktrace;
116+
int Depth = sys::getStackTrace(Stacktrace);
117+
StacktraceIdx = getIndexForAddedTrace(StacktraceIdx, StacktraceOrList(Depth, Stacktrace));
28118
}
29-
void DbgLocOriginBacktrace::addTrace() {
30-
if (Stacktraces.empty())
119+
void DbgLocOriginStacktrace::addTrace() {
120+
if (StacktraceIdx == 0)
31121
return;
32-
auto &[Depth, Stacktrace] = Stacktraces.emplace_back();
33-
Depth = sys::getStackTrace(Stacktrace);
122+
std::array<void *, DbgLocOriginStacktrace::MaxDepth> Stacktrace;
123+
int Depth = sys::getStackTrace(Stacktrace);
124+
StacktraceIdx = getIndex(StacktraceOrList(Depth, Stacktrace));
125+
}
126+
SmallVector<std::pair<int, std::array<void *, DbgLocOriginStacktrace::MaxDepth>>, 0> DbgLocOriginStacktrace::getStacktraces() {
127+
SmallVector<std::pair<int, std::array<void *, DbgLocOriginStacktrace::MaxDepth>>, 0> Stacktraces;
128+
if (StacktraceIdx == 0)
129+
return Stacktraces;
130+
StacktraceOrList S = getStacktraceFromIndex(StacktraceIdx);
131+
if (S.IsList) {
132+
Stacktraces.push_back({(int)S.Size, S.Single});
133+
} else {
134+
for (size_t SingleIdx : ArrayRef(S.STList.data(), S.Size)) {
135+
StacktraceOrList SingleS = getStacktraceFromIndex(SingleIdx);
136+
Stacktraces.push_back({(int)SingleS.Size, SingleS.Single});
137+
}
138+
}
139+
return Stacktraces;
34140
}
35141

36142
DebugLoc DebugLoc::getTemporary() {

llvm/lib/IR/IRBuilder.cpp

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -65,15 +65,10 @@ DebugLoc IRBuilderBase::getCurrentDebugLocation() const {
6565
return StoredDL;
6666
}
6767
void IRBuilderBase::SetInstDebugLocation(Instruction *I) const {
68-
for (const auto &KV : MetadataToCopy)
69-
if (KV.first == LLVMContext::MD_dbg) {
70-
I->setDebugLoc(StoredDL.getCopied());
71-
return;
72-
}
7368
// If I does not have an existing DebugLoc and no DebugLoc has been set
7469
// here, we copy our DebugLoc to I anyway, because more likely than not I
7570
// is a new instruction whose DL should originate from this builder.
76-
if (!I->getDebugLoc())
71+
if (HasExplicitDL || !I->getDebugLoc())
7772
I->setDebugLoc(StoredDL.getCopied());
7873
}
7974

llvm/lib/Transforms/Utils/Debugify.cpp

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -78,13 +78,14 @@ std::string symbolizeStacktrace(const Instruction *I) {
7878
sys::symbolizeAddresses(UnsymbolizedAddrs, SymbolizedAddrs);
7979
UnsymbolizedAddrs.clear();
8080
}
81-
DbgLocOriginBacktrace ST = I->getDebugLoc().getOrigin();
81+
DbgLocOriginStacktrace ST = I->getDebugLoc().getOrigin();
8282
std::string Result;
8383
raw_string_ostream OS(Result);
84-
for (size_t TraceIdx = 0; TraceIdx < ST.Stacktraces.size(); ++TraceIdx) {
84+
auto Stacktraces = ST.getStacktraces();
85+
for (size_t TraceIdx = 0; TraceIdx < Stacktraces.size(); ++TraceIdx) {
8586
if (TraceIdx != 0)
8687
OS << "========================================\n";
87-
auto &[Depth, Stacktrace] = ST.Stacktraces[TraceIdx];
88+
auto &[Depth, Stacktrace] = Stacktraces[TraceIdx];
8889
for (int Frame = 0; Frame < Depth; ++Frame) {
8990
assert(SymbolizedAddrs.contains(Stacktrace[Frame]) &&
9091
"Expected each address to have been symbolized.");
@@ -95,8 +96,9 @@ std::string symbolizeStacktrace(const Instruction *I) {
9596
return Result;
9697
}
9798
void collectStackAddresses(Instruction &I) {
98-
DbgLocOriginBacktrace ST = I.getDebugLoc().getOrigin();
99-
for (auto &[Depth, Stacktrace] : ST.Stacktraces) {
99+
DbgLocOriginStacktrace ST = I.getDebugLoc().getOrigin();
100+
auto Stacktraces = ST.getStacktraces();
101+
for (auto &[Depth, Stacktrace] : Stacktraces) {
100102
for (int Frame = 0; Frame < Depth; ++Frame) {
101103
void *Addr = Stacktrace[Frame];
102104
if (!SymbolizedAddrs.contains(Addr))

0 commit comments

Comments
 (0)