[DebugInfo] Fix line 0 records incorrectly having is_stmt set #166627
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-debuginfo Author: None (RoshanYSingh23) ChangesFixes issue #33870 Line 0 debug records should not have the is_stmt flag set in DWARF. This change ensures is_stmt is only set for non-zero line numbers. Changes made in DwarfDebug.cpp:
Test results: All 576 X86 DebugInfo tests pass. Full diff: https://github.com/llvm/llvm-project/pull/166627.diff 1 Files Affected:
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 567acf75d1b8d..31035e8085c36 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -101,13 +101,12 @@ static cl::opt<AccelTableKind> AccelTables(
clEnumValN(AccelTableKind::Dwarf, "Dwarf", "DWARF")),
cl::init(AccelTableKind::Default));
-static cl::opt<DefaultOnOff>
-DwarfInlinedStrings("dwarf-inlined-strings", cl::Hidden,
- cl::desc("Use inlined strings rather than string section."),
- cl::values(clEnumVal(Default, "Default for platform"),
- clEnumVal(Enable, "Enabled"),
- clEnumVal(Disable, "Disabled")),
- cl::init(Default));
+static cl::opt<DefaultOnOff> DwarfInlinedStrings(
+ "dwarf-inlined-strings", cl::Hidden,
+ cl::desc("Use inlined strings rather than string section."),
+ cl::values(clEnumVal(Default, "Default for platform"),
+ clEnumVal(Enable, "Enabled"), clEnumVal(Disable, "Disabled")),
+ cl::init(Default));
static cl::opt<bool>
NoDwarfRangesSection("no-dwarf-ranges-section", cl::Hidden,
@@ -231,9 +230,7 @@ void DebugLocDwarfExpression::commitTemporaryBuffer() {
TmpBuf->Comments.clear();
}
-const DIType *DbgVariable::getType() const {
- return getVariable()->getType();
-}
+const DIType *DbgVariable::getType() const { return getVariable()->getType(); }
/// Get .debug_loc entry for the instruction range starting at MI.
static DbgValueLoc getDebugLocValue(const MachineInstr *MI) {
@@ -371,8 +368,9 @@ DwarfDebug::DwarfDebug(AsmPrinter *A)
UseAllLinkageNames = DwarfLinkageNames == AllLinkageNames;
unsigned DwarfVersionNumber = Asm->TM.Options.MCOptions.DwarfVersion;
- unsigned DwarfVersion = DwarfVersionNumber ? DwarfVersionNumber
- : MMI->getModule()->getDwarfVersion();
+ unsigned DwarfVersion = DwarfVersionNumber
+ ? DwarfVersionNumber
+ : MMI->getModule()->getDwarfVersion();
// Use dwarf 4 by default if nothing is requested. For NVPTX, use dwarf 2.
DwarfVersion =
TT.isNVPTX() ? 2 : (DwarfVersion ? DwarfVersion : dwarf::DWARF_VERSION);
@@ -433,7 +431,8 @@ DwarfDebug::DwarfDebug(AsmPrinter *A)
UseDebugMacroSection =
DwarfVersion >= 5 || (UseGNUDebugMacro && !useSplitDwarf());
if (DwarfOpConvert == Default)
- EnableOpConvert = !((tuneForGDB() && useSplitDwarf()) || (tuneForLLDB() && !TT.isOSBinFormatMachO()));
+ EnableOpConvert = !((tuneForGDB() && useSplitDwarf()) ||
+ (tuneForLLDB() && !TT.isOSBinFormatMachO()));
else
EnableOpConvert = (DwarfOpConvert == Enable);
@@ -857,14 +856,16 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
assert(std::next(Suc) == BundleEnd &&
"More than one instruction in call delay slot");
// Try to interpret value loaded by instruction.
- if (!interpretNextInstr(&*Suc, ForwardedRegWorklist, Params, ClobberedRegUnits))
+ if (!interpretNextInstr(&*Suc, ForwardedRegWorklist, Params,
+ ClobberedRegUnits))
return;
}
// Search for a loading value in forwarding registers.
for (; I != MBB->rend(); ++I) {
// Try to interpret values loaded by instruction.
- if (!interpretNextInstr(&*I, ForwardedRegWorklist, Params, ClobberedRegUnits))
+ if (!interpretNextInstr(&*I, ForwardedRegWorklist, Params,
+ ClobberedRegUnits))
return;
}
@@ -1106,8 +1107,7 @@ DwarfDebug::getOrCreateDwarfCompileUnit(const DICompileUnit *DIUnit) {
if (auto *CU = CUMap.lookup(DIUnit))
return *CU;
- if (useSplitDwarf() &&
- !shareAcrossDWOCUs() &&
+ if (useSplitDwarf() && !shareAcrossDWOCUs() &&
(!DIUnit->getSplitDebugInlining() ||
DIUnit->getEmissionKind() == DICompileUnit::FullDebug) &&
!CUMap.empty()) {
@@ -1201,7 +1201,6 @@ void DwarfDebug::beginModule(Module *M) {
(useSplitDwarf() ? SkeletonHolder : InfoHolder)
.setStringOffsetsStartSym(Asm->createTempSymbol("str_offsets_base"));
-
// Create the symbols that designates the start of the DWARF v5 range list
// and locations list tables. They are located past the table headers.
if (getDwarfVersion() >= 5) {
@@ -1415,7 +1414,7 @@ void DwarfDebug::finalizeModuleInfo() {
TLOF.getDwarfMacinfoSection()->getBeginSymbol());
}
}
- }
+ }
// Emit all frontend-produced Skeleton CUs, i.e., Clang modules.
for (auto *CUNode : MMI->getModule()->debug_compile_units())
@@ -1491,7 +1490,7 @@ void DwarfDebug::endModule() {
emitDebugRanges();
if (useSplitDwarf())
- // Emit info into a debug macinfo.dwo section.
+ // Emit info into a debug macinfo.dwo section.
emitDebugMacinfoDWO();
else
// Emit info into a debug macinfo/macro section.
@@ -1533,8 +1532,8 @@ void DwarfDebug::endModule() {
// FIXME: AbstractVariables.clear();
}
-void DwarfDebug::ensureAbstractEntityIsCreatedIfScoped(DwarfCompileUnit &CU,
- const DINode *Node, const MDNode *ScopeNode) {
+void DwarfDebug::ensureAbstractEntityIsCreatedIfScoped(
+ DwarfCompileUnit &CU, const DINode *Node, const MDNode *ScopeNode) {
if (CU.getExistingAbstractEntity(Node))
return;
@@ -1580,7 +1579,8 @@ void DwarfDebug::collectVariableInfoFromMFTable(
continue;
}
- ensureAbstractEntityIsCreatedIfScoped(TheCU, Var.first, Scope->getScopeNode());
+ ensureAbstractEntityIsCreatedIfScoped(TheCU, Var.first,
+ Scope->getScopeNode());
// If we have already seen information for this variable, add to what we
// already know.
@@ -1607,7 +1607,7 @@ void DwarfDebug::collectVariableInfoFromMFTable(
}
auto RegVar = std::make_unique<DbgVariable>(
- cast<DILocalVariable>(Var.first), Var.second);
+ cast<DILocalVariable>(Var.first), Var.second);
if (VI.inStackSlot())
RegVar->emplace<Loc::MMI>(VI.Expr, VI.getStackSlot());
else
@@ -1723,8 +1723,7 @@ static bool validThroughout(LexicalScopes &LScopes,
// [4-) [(@g, fragment 0, 96)]
bool DwarfDebug::buildLocationList(SmallVectorImpl<DebugLocEntry> &DebugLoc,
const DbgValueHistoryMap::Entries &Entries) {
- using OpenRange =
- std::pair<DbgValueHistoryMap::EntryIndex, DbgValueLoc>;
+ using OpenRange = std::pair<DbgValueHistoryMap::EntryIndex, DbgValueLoc>;
SmallVector<OpenRange, 4> OpenRanges;
bool isSafeForSingleLocation = true;
const MachineInstr *StartDebugMI = nullptr;
@@ -1750,8 +1749,7 @@ bool DwarfDebug::buildLocationList(SmallVectorImpl<DebugLocEntry> &DebugLoc,
EndLabel = Asm->MBBSectionRanges[EndMBB.getSectionID()].EndLabel;
if (EI->isClobber())
EndMI = EI->getInstr();
- }
- else if (std::next(EI)->isClobber())
+ } else if (std::next(EI)->isClobber())
EndLabel = getLabelAfterInsn(std::next(EI)->getInstr());
else
EndLabel = getLabelBeforeInsn(std::next(EI)->getInstr());
@@ -1889,17 +1887,15 @@ DbgEntity *DwarfDebug::createConcreteEntity(DwarfCompileUnit &TheCU,
const MCSymbol *Sym) {
ensureAbstractEntityIsCreatedIfScoped(TheCU, Node, Scope.getScopeNode());
if (isa<const DILocalVariable>(Node)) {
- ConcreteEntities.push_back(
- std::make_unique<DbgVariable>(cast<const DILocalVariable>(Node),
- Location));
- InfoHolder.addScopeVariable(&Scope,
- cast<DbgVariable>(ConcreteEntities.back().get()));
+ ConcreteEntities.push_back(std::make_unique<DbgVariable>(
+ cast<const DILocalVariable>(Node), Location));
+ InfoHolder.addScopeVariable(
+ &Scope, cast<DbgVariable>(ConcreteEntities.back().get()));
} else if (isa<const DILabel>(Node)) {
ConcreteEntities.push_back(
- std::make_unique<DbgLabel>(cast<const DILabel>(Node),
- Location, Sym));
+ std::make_unique<DbgLabel>(cast<const DILabel>(Node), Location, Sym));
InfoHolder.addScopeLabel(&Scope,
- cast<DbgLabel>(ConcreteEntities.back().get()));
+ cast<DbgLabel>(ConcreteEntities.back().get()));
}
return ConcreteEntities.back().get();
}
@@ -1935,8 +1931,8 @@ void DwarfDebug::collectEntityInfo(DwarfCompileUnit &TheCU,
continue;
Processed.insert(IV);
- DbgVariable *RegVar = cast<DbgVariable>(createConcreteEntity(TheCU,
- *Scope, LocalVar, IV.second));
+ DbgVariable *RegVar = cast<DbgVariable>(
+ createConcreteEntity(TheCU, *Scope, LocalVar, IV.second));
const MachineInstr *MInsn = HistoryMapEntries.front().getInstr();
assert(MInsn->isDebugValue() && "History must begin with debug value");
@@ -2150,7 +2146,8 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
(!PrevInstBB ||
PrevInstBB->getSectionID() == MI->getParent()->getSectionID());
bool ForceIsStmt = ForceIsStmtInstrs.contains(MI);
- if (PrevInstInSameSection && !ForceIsStmt && DL.isSameSourceLocation(PrevInstLoc)) {
+ if (PrevInstInSameSection && !ForceIsStmt &&
+ DL.isSameSourceLocation(PrevInstLoc)) {
// If we have an ongoing unspecified location, nothing to do here.
if (!DL)
return;
@@ -2199,12 +2196,16 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
if (DL.getLine() == 0 && LastAsmLine == 0)
return;
if (MI == PrologEndLoc) {
- Flags |= DWARF2_FLAG_PROLOGUE_END | DWARF2_FLAG_IS_STMT;
+ Flags |= DWARF2_FLAG_PROLOGUE_END;
+ // Don't set is_stmt for line 0
+ if (DL.getLine() != 0)
+ Flags |= DWARF2_FLAG_IS_STMT;
PrologEndLoc = nullptr;
}
if (ScopeUsesKeyInstructions) {
- if (IsKey)
+ // Don't set is_stmt for line 0
+ if (IsKey && DL.getLine() != 0)
Flags |= DWARF2_FLAG_IS_STMT;
} else {
// If the line changed, we call that a new statement; unless we went to
@@ -2370,7 +2371,8 @@ static void recordSourceLine(AsmPrinter &Asm, unsigned Line, unsigned Col,
const MachineInstr *
DwarfDebug::emitInitialLocDirective(const MachineFunction &MF, unsigned CUID) {
// Don't deal with functions that have no instructions.
- if (llvm::all_of(MF, [](const MachineBasicBlock &MBB) { return MBB.empty(); }))
+ if (llvm::all_of(MF,
+ [](const MachineBasicBlock &MBB) { return MBB.empty(); }))
return nullptr;
std::pair<const MachineInstr *, bool> PrologEnd = findPrologueEndLoc(&MF);
@@ -2400,8 +2402,11 @@ DwarfDebug::emitInitialLocDirective(const MachineFunction &MF, unsigned CUID) {
(void)getOrCreateDwarfCompileUnit(SP->getUnit());
// We'd like to list the prologue as "not statements" but GDB behaves
// poorly if we do that. Revisit this with caution/GDB (7.5+) testing.
- ::recordSourceLine(*Asm, SP->getScopeLine(), 0, SP, DWARF2_FLAG_IS_STMT,
- CUID, getDwarfVersion(), getUnits());
+ // However, we should not set is_stmt for line 0.
+ unsigned ScopeLine = SP->getScopeLine();
+ unsigned Flags = (ScopeLine != 0) ? DWARF2_FLAG_IS_STMT : 0;
+ ::recordSourceLine(*Asm, ScopeLine, 0, SP, Flags, CUID, getDwarfVersion(),
+ getUnits());
return PrologEndLoc;
}
@@ -2677,7 +2682,8 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
CurFn = MF;
auto *SP = MF->getFunction().getSubprogram();
- assert(LScopes.empty() || SP == LScopes.getCurrentFunctionScope()->getScopeNode());
+ assert(LScopes.empty() ||
+ SP == LScopes.getCurrentFunctionScope()->getScopeNode());
if (SP->getUnit()->getEmissionKind() == DICompileUnit::NoDebug)
return;
@@ -2738,7 +2744,8 @@ void DwarfDebug::endFunctionImpl(const MachineFunction *MF) {
const Function &F = MF->getFunction();
const DISubprogram *SP = F.getSubprogram();
- assert(CurFn == MF &&
+ assert(
+ CurFn == MF &&
"endFunction should be called with the same function as beginFunction");
// Set DwarfDwarfCompileUnitID in MCContext to default value.
@@ -3090,15 +3097,16 @@ void DwarfDebug::emitDebugLocEntry(ByteStreamer &Streamer,
Offset++;
for (unsigned I = 0; I < Op.getDescription().Op.size(); ++I) {
if (Op.getDescription().Op[I] == Encoding::BaseTypeRef) {
- unsigned Length =
- Streamer.emitDIERef(*CU->ExprRefedBaseTypes[Op.getRawOperand(I)].Die);
+ unsigned Length = Streamer.emitDIERef(
+ *CU->ExprRefedBaseTypes[Op.getRawOperand(I)].Die);
// Make sure comments stay aligned.
for (unsigned J = 0; J < Length; ++J)
if (Comment != End)
Comment++;
} else {
for (uint64_t J = Offset; J < Op.getOperandEndOffset(I); ++J)
- Streamer.emitInt8(Data.getData()[J], Comment != End ? *(Comment++) : "");
+ Streamer.emitInt8(Data.getData()[J],
+ Comment != End ? *(Comment++) : "");
}
Offset = Op.getOperandEndOffset(I);
}
@@ -3205,8 +3213,7 @@ void DwarfDebug::emitDebugLocValue(const AsmPrinter &AP, const DIBasicType *BT,
void DebugLocEntry::finalize(const AsmPrinter &AP,
DebugLocStream::ListBuilder &List,
- const DIBasicType *BT,
- DwarfCompileUnit &TheCU) {
+ const DIBasicType *BT, DwarfCompileUnit &TheCU) {
assert(!Values.empty() &&
"location list entries without values are redundant");
assert(Begin != End && "unexpected location list entry with empty range");
@@ -3216,9 +3223,8 @@ void DebugLocEntry::finalize(const AsmPrinter &AP,
const DbgValueLoc &Value = Values[0];
if (Value.isFragment()) {
// Emit all fragments that belong to the same variable and range.
- assert(llvm::all_of(Values, [](DbgValueLoc P) {
- return P.isFragment();
- }) && "all values are expected to be fragments");
+ assert(llvm::all_of(Values, [](DbgValueLoc P) { return P.isFragment(); }) &&
+ "all values are expected to be fragments");
assert(llvm::is_sorted(Values) && "fragments are expected to be sorted");
for (const auto &Fragment : Values)
@@ -3239,7 +3245,8 @@ void DwarfDebug::emitDebugLocEntryLocation(const DebugLocStream::Entry &Entry,
Asm->OutStreamer->AddComment("Loc expr size");
if (getDwarfVersion() >= 5)
Asm->emitULEB128(DebugLocs.getBytes(Entry).size());
- else if (DebugLocs.getBytes(Entry).size() <= std::numeric_limits<uint16_t>::max())
+ else if (DebugLocs.getBytes(Entry).size() <=
+ std::numeric_limits<uint16_t>::max())
Asm->emitInt16(DebugLocs.getBytes(Entry).size());
else {
// The entry is too big to fit into 16 bit, drop it as there is nothing we
@@ -3291,13 +3298,12 @@ static MCSymbol *emitLoclistsTableHeader(AsmPrinter *Asm,
}
template <typename Ranges, typename PayloadEmitter>
-static void emitRangeList(
- DwarfDebug &DD, AsmPrinter *Asm, MCSymbol *Sym, const Ranges &R,
- const DwarfCompileUnit &CU, unsigned BaseAddressx, unsigned OffsetPair,
- unsigned StartxLength, unsigned EndOfList,
- StringRef (*StringifyEnum)(unsigned),
- bool ShouldUseBaseAddress,
- PayloadEmitter EmitPayload) {
+static void
+emitRangeList(DwarfDebug &DD, AsmPrinter *Asm, MCSymbol *Sym, const Ranges &R,
+ const DwarfCompileUnit &CU, unsigned BaseAddressx,
+ unsigned OffsetPair, unsigned StartxLength, unsigned EndOfList,
+ StringRef (*StringifyEnum)(unsigned), bool ShouldUseBaseAddress,
+ PayloadEmitter EmitPayload) {
auto Size = Asm->MAI->getCodePointerSize();
bool UseDwarf5 = DD.getDwarfVersion() >= 5;
@@ -3398,7 +3404,8 @@ static void emitRangeList(
}
// Handles emission of both debug_loclist / debug_loclist.dwo
-static void emitLocList(DwarfDebug &DD, AsmPrinter *Asm, const DebugLocStream::List &List) {
+static void emitLocList(DwarfDebug &DD, AsmPrinter *Asm,
+ const DebugLocStream::List &List) {
emitRangeList(DD, Asm, List.Label, DD.getDebugLocs().getEntries(List),
*List.CU, dwarf::DW_LLE_base_addressx,
dwarf::DW_LLE_offset_pair, dwarf::DW_LLE_startx_length,
@@ -3428,17 +3435,15 @@ void DwarfDebug::emitDebugLocImpl(MCSection *Sec) {
// Emit locations into the .debug_loc/.debug_loclists section.
void DwarfDebug::emitDebugLoc() {
- emitDebugLocImpl(
- getDwarfVersion() >= 5
- ? Asm->getObjFileLowering().getDwarfLoclistsSection()
- : Asm->getObjFileLowering().getDwarfLocSection());
+ emitDebugLocImpl(getDwarfVersion() >= 5
+ ? Asm->getObjFileLowering().getDwarfLoclistsSection()
+ : Asm->getObjFileLowering().getDwarfLocSection());
}
// Emit locations into the .debug_loc.dwo/.debug_loclists.dwo section.
void DwarfDebug::emitDebugLocDWO() {
if (getDwarfVersion() >= 5) {
- emitDebugLocImpl(
- Asm->getObjFileLowering().getDwarfLoclistsDWOSection());
+ emitDebugLocImpl(Asm->getObjFileLowering().getDwarfLoclistsDWOSection());
return;
}
@@ -3626,16 +3631,16 @@ void DwarfDebug::emitDebugARanges() {
/// Emit a single range list. We handle both DWARF v5 and earlier.
static void emitRangeList(DwarfDebug &DD, AsmPrinter *Asm,
const RangeSpanList &List) {
- emitRangeList(DD, Asm, List.Label, List.Ranges, *List.CU,
- dwarf::DW_RLE_base_addressx, dwarf::DW_RLE_offset_pair,
- dwarf::DW_RLE_startx_length, dwarf::DW_RLE_end_of_list,
- llvm::dwarf::RangeListEncodingString,
- List.CU->getCUNode()->getRangesBaseAddress() ||
- DD.getDwarfVersion() >= 5,
- [](auto) {});
+ emitRangeList(
+ DD, Asm, List.Label, List.Ranges, *List.CU, dwarf::DW_RLE_base_addressx,
+ dwarf::DW_RLE_offset_pair, dwarf::DW_RLE_startx_length,
+ dwarf::DW_RLE_end_of_list, llvm::dwarf::RangeListEncodingString,
+ List.CU->getCUNode()->getRangesBaseAddress() || DD.getDwarfVersion() >= 5,
+ [](auto) {});
}
-void DwarfDebug::emitDebugRangesImpl(const DwarfFile &Holder, MCSection *Section) {
+void DwarfDebug::emitDebugRangesImpl(const DwarfFile &Holder,
+ MCSection *Section) {
if (Holder.getRangeLists().empty())
return;
@@ -4059,11 +4064,11 @@ void DwarfDebug::addAccelNameImpl(
assert(((&Current == &AccelTypeUnitsDebugNames) ||
((&Current == &AccelDebugNames) &&
(Unit.getUnitDie().getTag() != dwarf::DW_TAG_type_unit))) &&
- "Kind is CU but TU is being processed.");
+ "Kind is CU but TU is being processed.");
assert(((&Current == &AccelDebugNames) ||
((&Current == &AccelTypeUnitsDebugNames) &&
(Unit.getUnitDie().getTag() == dwarf::DW_TAG_type_unit))) &&
- "Kind is TU but CU is being processed.");
+ "Kind is TU but CU is being processed.");
// The type unit can be discarded, so need to add references to final
// acceleration table once we know it's complete and we emit it.
Current.addName(Ref, Die, Unit.getUniqueID(),
|
OCHyams
left a comment
OCHyams
left a comment
There was a problem hiding this comment.
Thanks for taking a look at the issue
Please could you remove unrelated formatting changes from the patch? It adds "noise" to reviews - typically we only format lines that are touched by the patch (git diff -U0 | py -3 clang\tools\clang-format\clang-format-diff.py -p1 -binary "\bin\clang-format" -i).
Test results: All 576 X86 DebugInfo tests pass.
As you're changing functionality, this patch will need to come with a/some new test/tests.
In the linked ticket it says
There is one test related to prologue-end that seems to be testing that it's okay for prologue-end to be line 0, and I ran out of time to investigate any further.
Do you know which test that is? I wonder if there's any git-history which could be relevant
Fixes issue llvm#33870 (Bugzilla llvm#34522) Line 0 debug records should not have the is_stmt flag set in DWARF. This change ensures is_stmt is only set for non-zero line numbers. Changes made in DwarfDebug.cpp: - beginInstruction: Only set is_stmt for prologue_end if line != 0 - beginInstruction: Only set is_stmt for key instructions if line != 0 - emitInitialLocDirective: Set is_stmt=0 if scopeLine == 0 Added two test cases to verify the behavior: - llvm/test/DebugInfo/X86/line-0-no-is-stmt.ll: Verifies line 0 does not get is_stmt - llvm/test/DebugInfo/X86/line-nonzero-has-is-stmt.ll: Verifies non-zero lines do get is_stmt
RoshanYSingh23
force-pushed
the
fix-issue-33870
branch
from
273271e to
2c45677
Compare
|
@OCHyams
I apologize for the initial sloppiness with the formatting - this is my first contribution to LLVM and I'm still learning the review process and best practices. If you have any other suggestions or concerns about the implementation or tests, I'd be very grateful to address them. Thank you for taking the time to review this! |
OCHyams
left a comment
OCHyams
left a comment
There was a problem hiding this comment.
Sorry it's taken a while to get back to you, thanks for making those changes. I've left a couple of nits and one question, but overall SGTM
| if (MI == PrologEndLoc) { | ||
| Flags |= DWARF2_FLAG_PROLOGUE_END | DWARF2_FLAG_IS_STMT; | ||
| Flags |= DWARF2_FLAG_PROLOGUE_END; | ||
| // Don't set is_stmt for line 0 |
There was a problem hiding this comment.
| // Don't set is_stmt for line 0 | |
| // Don't set is_stmt for line 0. |
|
|
||
| if (ScopeUsesKeyInstructions) { | ||
| if (IsKey) | ||
| // Don't set is_stmt for line 0 |
There was a problem hiding this comment.
| // Don't set is_stmt for line 0 | |
| // Don't set is_stmt for line 0. |
| // the entry block. | ||
| assert(MI->getParent() == &*MI->getMF()->begin()); | ||
| recordSourceLine(SP->getScopeLine(), 0, SP, | ||
| DWARF2_FLAG_PROLOGUE_END | DWARF2_FLAG_IS_STMT); |
There was a problem hiding this comment.
As mentioned in a comment earlier, I feel like it's probably a bug if scope line is zero. But if we keep the other check for scope line in this patch, for completeness sake it should probably be replicated here.
| // However, we should not set is_stmt for line 0. | ||
| unsigned ScopeLine = SP->getScopeLine(); | ||
| unsigned Flags = (ScopeLine != 0) ? DWARF2_FLAG_IS_STMT : 0; | ||
| ::recordSourceLine(*Asm, ScopeLine, 0, SP, Flags, CUID, getDwarfVersion(), |
There was a problem hiding this comment.
I wonder line zero scope lines can really occur "naturally", I would be slightly surprised. I suppose there's little harm in adding coverage here too but it feels to me like a scope line of 0 would probably mean there's a bug somewhere.
@jmorse do you have a gut instinct on this?
There was a problem hiding this comment.
Gut instinct, given that the scope line is directly extracted from the subprogram, is that it can't be a legitimate outcome of optimisation: the scope line is a static property of the source code, not of the program under transformation.
There was a problem hiding this comment.
In which case I feel it's better to keep this code as it was previously, as it's almost certainly a bug if this is a line 0 and not particularly harmful if that occurs. So overall better to minimise change and complexity.
I don't feel massively strongly about it, as long as it's consistent between this line and the other scopeLine usage in this file, so if others feel differently I wouldn't insist.
| ; CHECK: 0x{{[0-9a-f]+}} 0 0 | ||
| ; CHECK-NOT: is_stmt | ||
| ; CHECK: end_sequence |
There was a problem hiding this comment.
Can we CHECK: the entire line instead? I feel that will be clearer to understand visually
| ; With scopeLine=10, we should see line 10 with is_stmt and prologue_end | ||
| ; CHECK: 0x{{[0-9a-f]+}} 10 {{[0-9]+}} {{[0-9]+}} {{[0-9]+}} {{[0-9]+}} {{[0-9]+}} is_stmt prologue_end |
There was a problem hiding this comment.
Possibly worth adding table headers so it's clear what each column is?
alternatively, I think ; CHECK: 0x{{[0-9a-f]+}} 10 {{.*}} is_stmt prologue_end is probably fine.
4 participants
Fixes issue #33870
Line 0 debug records should not have the is_stmt flag set in DWARF. This change ensures is_stmt is only set for non-zero line numbers.
Changes made in DwarfDebug.cpp:
Test results: All 576 X86 DebugInfo tests pass.