[PATCH] D58194: [DebugInfo] add SectionedAddress to DebugInfo interfaces.
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 13 12:31:58 PST 2019
dblaikie added a comment.
This has some relatively extensive changes to some of the llvm tools (like sancov, for instance) that aren't tested - perhaps it'd be better to leave those tools usage unmodified (adding the undef section value as a fix to the API change, but nothing more) with fixits and/or followup patches that implement the improved functionality possible with the new API, along with tests?
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:142-144
+ return (LHS.SectionIndex < RHS.SectionIndex) ||
+ (LHS.SectionIndex == RHS.SectionIndex &&
+ LHS.Address < RHS.Address);
----------------
Might be simpler to implement as:
return std::tie(LHS.SectionIndex, LHS.Address) < std::tie(RHS.SectionIndex, RHS.Address);
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:150
uint64_t Address;
+ /// If relocation information presented then this is the Index of section
+ /// which contains above address. Otherwise this is
----------------
Maybe "presented" could be replaced with "is present" here and below?
(and "index of /the/ section")
================
Comment at: llvm/include/llvm/Object/ObjectFile.h:139
+struct SectionedAddress {
+ SectionedAddress() : SectionIndex(UndefSection), Address(0) {}
+ SectionedAddress(uint64_t SI, uint64_t VMA)
----------------
Maybe use non-static data member initializers rather than constructors?
(& then drop the other two ctors too - let this be a plain struct)
================
Comment at: llvm/include/llvm/Object/ObjectFile.h:145-149
+ bool operator<(const SectionedAddress &Other) const {
+ return (this->SectionIndex < Other.SectionIndex) ||
+ (this->SectionIndex == Other.SectionIndex &&
+ this->Address < Other.Address);
+ }
----------------
If this can be changed to a plain struct, might be nice to move this operator out of the class definition (& generally/at least it should be a friend rather than a member, so that LHS and RHS get equal treatment for conversions, etc)
& implement this with std::tie similar to the previous suggestion, maybe?
================
Comment at: llvm/include/llvm/Object/ObjectFile.h:324
return errorCodeToError(object_error::parse_failed);
- };
+ }
----------------
Cleanups like this would ideally go in separate NFC (No Functional Change) commits - shouldn't require any review, feel free to just commit them if you have commit rights.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:273
+ raw_ostream &OS, DWARFDataExtractor &rnglistData,
+ llvm::function_ref<Optional<llvm::SectionedAddress>(uint32_t)>
+ LookupPooledAddress,
----------------
Is this llvm:: qualifier to disambiguate different things called SectionedAddress? Perhaps we could avoid having different things with that name and use only one of them - maybe move the one that's already in libDebugInfo into libObject? (perhaps as an incremental patch before the rest of this change)
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:973
- DWARFCompileUnit *CU = getCompileUnitForAddress(Address);
+ DWARFCompileUnit *CU = getCompileUnitForAddress(Address.Address);
if (!CU)
----------------
Presumably this is going to have ambiguity problems if it doesn't have the section too? Which CU does it return if there are two CUs both with address zero in them (because they each describe functions in different sections), for instance?
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:977-978
+
+ getFunctionNameAndStartLineForAddress(CU, Address.Address, Spec.FNKind,
+ Result.FunctionName, Result.StartLine);
if (Spec.FLIKind != FileLineInfoKind::None) {
----------------
Similarly here
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1058-1060
+ llvm::SectionedAddress Addr;
+ Addr.Address = Address.Address;
+ Addr.SectionIndex = Address.SectionIndex;
----------------
Yeah, seems like it'd be best to have one SectionedAddress type rather than mapping between these equivalent but different types.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:867-868
DWARFDebugLine::Row Row;
- Row.Address = Address;
+ Row.Address = Address.Address;
+ Row.SectionIndex = Address.SectionIndex;
RowIter FirstRow = Rows.begin() + Seq.FirstRowIndex;
----------------
Perhaps Row should contain a SectionedAddress rather than a separate Address and SectionIndex field (again, could be a preliminary or after-this-patch refactoring to clean it up if it'd otherwise introduce a lot of churn to this already fairly large patch)
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:890-893
+ // Search for relocatable addresses
+ uint32_t Result = lookupAddressMain(Address);
+
+ if (Result == UnknownRowIndex) {
----------------
Under what conditions is this code needed? (it looks like this would only fail if the section index was incorrect? Which makes me think/feel like this is maybe working around a bug of some kind?)
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:906
+uint32_t
+DWARFDebugLine::LineTable::lookupAddressMain(SectionedAddress Address) const {
if (Sequences.empty())
----------------
Maybe this should be "lookupAddressImpl" if it's the underlying implementation that is used by the more general lookupAddress entry point?
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:989-990
// Figure out the last row in the range.
- uint32_t LastRowIndex = findRowInSeq(CurSeq, EndAddr - 1);
+ SectionedAddress LastAddress = Address;
+ LastAddress.Address = EndAddr - 1;
+ uint32_t LastRowIndex = findRowInSeq(CurSeq, LastAddress);
----------------
Maybe this should be {Address.SectionIndex, EndAddr - 1} (though admittedly it's a bit subtle having to know if section index or address comes first) & passed in directly to findRowInSeq - no need to split out into a named variable, etc.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1072-1074
+ if (RowIndex == UnknownRowIndex) {
return false;
+ }
----------------
Looks like unrelated refactoring? (better in a separate commit)
================
Comment at: llvm/test/tools/llvm-objdump/Inputs/function-sections-line-numbers.cpp:1-25
+
+__attribute__((__noinline__))
+ int Pow(int number, int count);
+
+
+
+
----------------
Seems like a rather long/complicated test case. Is it sufficient to compile this with -ffunction-sections:
void f1() { }
void f2() { }
and then symbolize address 0?
================
Comment at: llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp:533-538
+ object::SectionedAddress Address;
+
+ Address.Address = VMAddress;
+ Address.SectionIndex = SectionIndex;
+
+ IndirectInstructions.insert(Address);
----------------
Could use the ctor:
object::SectionedAddress Address(SectionIndex, VMAddress);
or braced init:
IndirectInstructions.insert({SectionIndex, VMAddress});
(I think the latter's probably fine & worth doing across this patch - but I do appreciate the concern that without names one could easily mix up the section/address order - so perhaps you/other reviewers feel differentlny to me)
================
Comment at: llvm/tools/llvm-cfi-verify/lib/GraphBuilder.h:105
static GraphResult buildFlowGraph(const FileAnalysis &Analysis,
- uint64_t Address);
+ uint64_t SectionIndex, uint64_t Address);
----------------
Might as well pass a SectionedAddress here?
================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:420-421
+ ModuleAddress.Address = Lookup;
+ ModuleAddress.SectionIndex =
+ getModuleSectionIndexForAddress(Obj, ModuleAddress.Address);
+ if (DILineInfo LineInfo = DICtx.getLineInfoForAddress(ModuleAddress))
----------------
Perhaps this should be changed to produce more than one result? Maybe just leaving a FIXME (given that it's no worse with this change than it was before)
================
Comment at: llvm/tools/llvm-rtdyld/llvm-rtdyld.cpp:392-393
+ } else {
+ auto SecOrErr = Sym.getSection();
+ if (SecOrErr) {
+ Address.SectionIndex = SecOrErr.get()->getIndex();
----------------
You can roll the variable into the condition here:
if (auto SecOrErr = Sym.getSection())
& LLVM style tends not to put {} on single-line statements like this
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58194/new/
https://reviews.llvm.org/D58194
More information about the llvm-commits
mailing list