[DebugInfo] Improve handling of clobbered fragments
Summary:
Currently the DbgValueHistorymap only keeps track of clobbered registers
for the last debug value that it has encountered. This could lead to
preceding register-described debug values living on longer in the
location lists than they should. See PR40283 for an example. This
patch does not introduce tracking of multiple registers, but changes
the DbgValueHistoryMap structure to allow for that in a follow-up
patch. This patch is not NFC, as it at least fixes two bugs in
DwarfDebug (both are covered in the new clobbered-fragments.mir test):
* If a debug value was clobbered (its End pointer set), the value would
still be added to OpenRanges, meaning that the succeeding location list
entries could potentially contain stale values.
* If a debug value was clobbered, and there were non-overlapping
fragments that were still live after the clobbering, DwarfDebug would
not create a location list entry starting directly after the
clobbering instruction. This meant that the location list could have
a gap until the next debug value for the variable was encountered.
Before this patch, the history map was represented by <Begin, End>
pairs, where a new pair was created for each new debug value. When
dealing with partially overlapping register-described debug values, such
as in the following example:
DBG_VALUE $reg2, $noreg, !1, !DIExpression(DW_OP_LLVM_fragment, 32, 32)
[...]
DBG_VALUE $reg3, $noreg, !1, !DIExpression(DW_OP_LLVM_fragment, 64, 32)
[...]
$reg2 = insn1
[...]
$reg3 = insn2
the history map would then contain the entries `[<DV1, insn1>, [<DV2, insn2>]`.
This would leave it up to the users of the map to be aware of
the relative order of the instructions, which e.g. could make
DwarfDebug::buildLocationList() needlessly complex. Instead, this patch
makes the history map structure monotonically increasing by dropping the
End pointer, and replacing that with explicit clobbering entries in the
vector. Each debug value has an "end index", which if set, points to the
entry in the vector that ends the debug value. The ending entry can
either be an overlapping debug value, or an instruction which clobbers
the register that the debug value is described by. The ending entry's
instruction can thus either be excluded or included in the debug value's
range. If the end index is not set, the debug value that the entry
introduces is valid until the end of the function.
Changes to test cases:
* DebugInfo/X86/pieces-3.ll: The range of the first DBG_VALUE, which
describes that the fragment (0, 64) is located in RDI, was
incorrectly ended by the clobbering of RAX, which the second
(non-overlapping) DBG_VALUE was described by. With this patch we
get a second entry that only describes RDI after that clobbering.
* DebugInfo/ARM/partial-subreg.ll: This test seems to indiciate a bug
in LiveDebugValues that is caused by it not being aware of fragments.
I have added some comments in the test case about that. Also, before
this patch DwarfDebug would incorrectly include a register-described
debug value from a preceding block in a location list entry.
Reviewers: aprantl, probinson, dblaikie, rnk, bjope
Reviewed By: aprantl
Subscribers: javed.absar, kristof.beyls, jdoerfert, llvm-commits
Tags: #debug-info, #llvm
Differential Revision: https://reviews.llvm.org/D59941
llvm-svn: 358072
diff --git a/llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp b/llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
index 618fee9..5dec591 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
@@ -9,6 +9,7 @@
#include "llvm/CodeGen/DbgEntityHistoryCalculator.h"
#include "llvm/ADT/BitVector.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/CodeGen/MachineBasicBlock.h"
#include "llvm/CodeGen/MachineFunction.h"
@@ -30,6 +31,10 @@
#define DEBUG_TYPE "dwarfdebug"
+namespace {
+using EntryIndex = DbgValueHistoryMap::EntryIndex;
+}
+
// If @MI is a DBG_VALUE with debug value described by a
// defined register, returns the number of this register.
// In the other case, returns 0.
@@ -41,33 +46,39 @@
return MI.getOperand(0).isReg() ? MI.getOperand(0).getReg() : 0;
}
-void DbgValueHistoryMap::startEntry(InlinedEntity Var, const MachineInstr &MI) {
+bool DbgValueHistoryMap::startDbgValue(InlinedEntity Var,
+ const MachineInstr &MI,
+ EntryIndex &NewIndex) {
// Instruction range should start with a DBG_VALUE instruction for the
// variable.
assert(MI.isDebugValue() && "not a DBG_VALUE");
auto &Entries = VarEntries[Var];
- if (!Entries.empty() && !Entries.back().isClosed() &&
- Entries.back().getBegin()->isIdenticalTo(MI)) {
+ if (!Entries.empty() && Entries.back().isDbgValue() &&
+ !Entries.back().isClosed() &&
+ Entries.back().getInstr()->isIdenticalTo(MI)) {
LLVM_DEBUG(dbgs() << "Coalescing identical DBG_VALUE entries:\n"
- << "\t" << Entries.back().getBegin() << "\t" << MI
+ << "\t" << Entries.back().getInstr() << "\t" << MI
<< "\n");
- return;
+ return false;
}
- Entries.emplace_back(&MI);
+ Entries.emplace_back(&MI, Entry::DbgValue);
+ NewIndex = Entries.size() - 1;
+ return true;
}
-void DbgValueHistoryMap::endEntry(InlinedEntity Var, const MachineInstr &MI) {
+EntryIndex DbgValueHistoryMap::startClobber(InlinedEntity Var,
+ const MachineInstr &MI) {
auto &Entries = VarEntries[Var];
- assert(!Entries.empty() && "No range exists for variable!");
- Entries.back().endEntry(MI);
+ Entries.emplace_back(&MI, Entry::Clobber);
+ return Entries.size() - 1;
}
-void DbgValueHistoryMap::Entry::endEntry(const MachineInstr &MI) {
+void DbgValueHistoryMap::Entry::endEntry(EntryIndex Index) {
// For now, instruction ranges are not allowed to cross basic block
// boundaries.
- assert(Begin->getParent() == MI.getParent());
- assert(!isClosed() && "Range is already closed!");
- End = &MI;
+ assert(isDbgValue() && "Setting end index for non-debug value");
+ assert(!isClosed() && "End index has already been set");
+ EndIndex = Index;
}
unsigned DbgValueHistoryMap::getRegisterForVar(InlinedEntity Var) const {
@@ -77,7 +88,9 @@
const auto &Entries = I->second;
if (Entries.empty() || Entries.back().isClosed())
return 0;
- return isDescribedByReg(*Entries.back().getBegin());
+ if (Entries.back().isClobber())
+ return 0;
+ return isDescribedByReg(*Entries.back().getInstr());
}
void DbgLabelInstrMap::addInstr(InlinedEntity Label, const MachineInstr &MI) {
@@ -91,6 +104,12 @@
using InlinedEntity = DbgValueHistoryMap::InlinedEntity;
using RegDescribedVarsMap = std::map<unsigned, SmallVector<InlinedEntity, 1>>;
+// Keeps track of the debug value entries that are currently live for each
+// inlined entity. As the history map entries are stored in a SmallVector, they
+// may be moved at insertion of new entries, so store indices rather than
+// pointers.
+using DbgValueEntriesMap = std::map<InlinedEntity, SmallSet<EntryIndex, 1>>;
+
} // end anonymous namespace
// Claim that @Var is not described by @RegNo anymore.
@@ -116,16 +135,67 @@
VarSet.push_back(Var);
}
+static void clobberRegEntries(InlinedEntity Var, unsigned RegNo,
+ const MachineInstr &ClobberingInstr,
+ DbgValueEntriesMap &LiveEntries,
+ DbgValueHistoryMap &HistMap) {
+ EntryIndex ClobberIndex = HistMap.startClobber(Var, ClobberingInstr);
+
+ // TODO: Close all preceding live entries that are clobbered by this
+ // instruction.
+ EntryIndex ValueIndex = ClobberIndex - 1;
+ auto &ValueEntry = HistMap.getEntry(Var, ValueIndex);
+ ValueEntry.endEntry(ClobberIndex);
+ LiveEntries[Var].erase(ValueIndex);
+}
+
+/// Add a new debug value for \p Var. Closes all overlapping debug values.
+static void handleNewDebugValue(InlinedEntity Var, const MachineInstr &DV,
+ RegDescribedVarsMap &RegVars,
+ DbgValueEntriesMap &LiveEntries,
+ DbgValueHistoryMap &HistMap) {
+ // TODO: We should track all registers which this variable is currently
+ // described by.
+
+ if (unsigned PrevReg = HistMap.getRegisterForVar(Var))
+ dropRegDescribedVar(RegVars, PrevReg, Var);
+
+ EntryIndex NewIndex;
+ if (HistMap.startDbgValue(Var, DV, NewIndex)) {
+ // If we have created a new debug value entry, close all preceding
+ // live entries that overlap.
+ SmallVector<EntryIndex, 4> IndicesToErase;
+ const DIExpression *DIExpr = DV.getDebugExpression();
+ for (auto Index : LiveEntries[Var]) {
+ auto &Entry = HistMap.getEntry(Var, Index);
+ assert(Entry.isDbgValue() && "Not a DBG_VALUE in LiveEntries");
+ const MachineInstr &DV = *Entry.getInstr();
+ if (DIExpr->fragmentsOverlap(DV.getDebugExpression())) {
+ IndicesToErase.push_back(Index);
+ Entry.endEntry(NewIndex);
+ }
+ }
+ // Drop all entries that have ended, and mark the new entry as live.
+ for (auto Index : IndicesToErase)
+ LiveEntries[Var].erase(Index);
+ LiveEntries[Var].insert(NewIndex);
+ }
+
+ if (unsigned NewReg = isDescribedByReg(DV))
+ addRegDescribedVar(RegVars, NewReg, Var);
+}
+
// Terminate the location range for variables described by register at
// @I by inserting @ClobberingInstr to their history.
static void clobberRegisterUses(RegDescribedVarsMap &RegVars,
RegDescribedVarsMap::iterator I,
DbgValueHistoryMap &HistMap,
+ DbgValueEntriesMap &LiveEntries,
const MachineInstr &ClobberingInstr) {
// Iterate over all variables described by this register and add this
// instruction to their history, clobbering it.
for (const auto &Var : I->second)
- HistMap.endEntry(Var, ClobberingInstr);
+ clobberRegEntries(Var, I->first, ClobberingInstr, LiveEntries, HistMap);
RegVars.erase(I);
}
@@ -133,11 +203,12 @@
// @RegNo by inserting @ClobberingInstr to their history.
static void clobberRegisterUses(RegDescribedVarsMap &RegVars, unsigned RegNo,
DbgValueHistoryMap &HistMap,
+ DbgValueEntriesMap &LiveEntries,
const MachineInstr &ClobberingInstr) {
const auto &I = RegVars.find(RegNo);
if (I == RegVars.end())
return;
- clobberRegisterUses(RegVars, I, HistMap, ClobberingInstr);
+ clobberRegisterUses(RegVars, I, HistMap, LiveEntries, ClobberingInstr);
}
// Returns the first instruction in @MBB which corresponds to
@@ -204,6 +275,7 @@
const TargetLowering *TLI = MF->getSubtarget().getTargetLowering();
unsigned SP = TLI->getStackPointerRegisterToSaveRestore();
RegDescribedVarsMap RegVars;
+ DbgValueEntriesMap LiveEntries;
for (const auto &MBB : *MF) {
for (const auto &MI : MBB) {
if (!MI.isDebugInstr()) {
@@ -218,14 +290,15 @@
// If this is a virtual register, only clobber it since it doesn't
// have aliases.
if (TRI->isVirtualRegister(MO.getReg()))
- clobberRegisterUses(RegVars, MO.getReg(), DbgValues, MI);
+ clobberRegisterUses(RegVars, MO.getReg(), DbgValues, LiveEntries,
+ MI);
// If this is a register def operand, it may end a debug value
// range.
else {
for (MCRegAliasIterator AI(MO.getReg(), TRI, true); AI.isValid();
++AI)
if (ChangingRegs.test(*AI))
- clobberRegisterUses(RegVars, *AI, DbgValues, MI);
+ clobberRegisterUses(RegVars, *AI, DbgValues, LiveEntries, MI);
}
} else if (MO.isRegMask()) {
// If this is a register mask operand, clobber all debug values in
@@ -234,7 +307,7 @@
// Don't consider SP to be clobbered by register masks.
if (unsigned(I) != SP && TRI->isPhysicalRegister(I) &&
MO.clobbersPhysReg(I)) {
- clobberRegisterUses(RegVars, I, DbgValues, MI);
+ clobberRegisterUses(RegVars, I, DbgValues, LiveEntries, MI);
}
}
}
@@ -252,13 +325,7 @@
"Expected inlined-at fields to agree");
InlinedEntity Var(RawVar, MI.getDebugLoc()->getInlinedAt());
- if (unsigned PrevReg = DbgValues.getRegisterForVar(Var))
- dropRegDescribedVar(RegVars, PrevReg, Var);
-
- DbgValues.startEntry(Var, MI);
-
- if (unsigned NewReg = isDescribedByReg(MI))
- addRegDescribedVar(RegVars, NewReg, Var);
+ handleNewDebugValue(Var, MI, RegVars, LiveEntries, DbgValues);
} else if (MI.isDebugLabel()) {
assert(MI.getNumOperands() == 1 && "Invalid DBG_LABEL instruction!");
const DILabel *RawLabel = MI.getDebugLabel();
@@ -280,7 +347,8 @@
auto CurElem = I++; // CurElem can be erased below.
if (TRI->isVirtualRegister(CurElem->first) ||
ChangingRegs.test(CurElem->first))
- clobberRegisterUses(RegVars, CurElem, DbgValues, MBB.back());
+ clobberRegisterUses(RegVars, CurElem, DbgValues, LiveEntries,
+ MBB.back());
}
}
}
@@ -306,10 +374,20 @@
dbgs() << " --\n";
- for (const auto &Entry : Entries) {
- dbgs() << " Begin: " << *Entry.getBegin();
- if (Entry.getEnd())
- dbgs() << " End : " << *Entry.getEnd();
+ for (const auto &E : enumerate(Entries)) {
+ const auto &Entry = E.value();
+ dbgs() << " Entry[" << E.index() << "]: ";
+ if (Entry.isDbgValue())
+ dbgs() << "Debug value\n";
+ else
+ dbgs() << "Clobber\n";
+ dbgs() << " Instr: " << *Entry.getInstr();
+ if (Entry.isDbgValue()) {
+ if (Entry.getEndIndex() == NoEntry)
+ dbgs() << " - Valid until end of function\n";
+ else
+ dbgs() << " - Closed by Entry[" << Entry.getEndIndex() << "]\n";
+ }
dbgs() << "\n";
}
}