[PATCH] D125783: [llvm-dva] 08 - ELF Reader
Pavel Samolysov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 3 09:08:51 PDT 2022
psamolysov added a comment.
Here are a series of comments to the new patch. The most comments are just nits (replacing `map#insert` with `map#emplace` but sometimes a piecewise construct could make sense however; and manually constructing by `[const] Map::value_type &` references to a map entry in foreach loops with the already defined in the `std::map` class template `reference` and `const_reference` aliases. Also a couple of possible memory leaks and bugs related to handling `Map.end()` iterator have been found, have a look, please.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:90
void addCompileUnitOffset(LVOffset Offset, LVScopeCompileUnit *CompileUnit) {
CompileUnits.insert(std::make_pair(Offset, CompileUnit));
}
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:149-150
+ LVSectionIndex getDotTextSectionIndex() { return DotTextSectionIndex; }
+ virtual LVSectionIndex getSectionIndex(LVScope *Scope) {
+ return getDotTextSectionIndex();
----------------
Should both member functions be `const`?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:154
+
virtual bool isSystemEntry(LVElement *Element, StringRef Name = {}) {
return false;
----------------
This function looks like `const` too.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:489
+ void addPublicName(LVScope *Scope, LVAddress LowPC, LVAddress HighPC) {
+ PublicNames.insert(
+ std::make_pair(Scope, LVNameInfo(LowPC, HighPC - LowPC)));
----------------
These pairs: `Scope/LVNameInfo` and `LVNameInfo (LVAddress/uint64_t)` are relatively small but it could be more effective do not copy at least the first pair: `PublicNames.emplace(Scope, LVNameInfo(LowPC, HightPC - LowPC));` or do not copy all the pairs:
```
PublicNames.emplace(std::piecewise_construct,
std::forward_as_tuple(Scope),
std::forward_as_tuple(LowPC, HighPC - LowPC));
```
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:164
List = new ListType();
Map->insert(std::make_pair(Key, List));
}
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:190
+ SecondMap = new LVSecondMapType();
+ FirstMap.insert(std::make_pair(FirstKey, SecondMap));
+ } else {
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:197
+ if (SecondMap && SecondMap->find(SecondKey) == SecondMap->end())
+ SecondMap->insert(std::make_pair(SecondKey, Value));
+ }
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:247
+ sys::path::filename(ConvertedPath));
+ return std::string(Path);
+}
----------------
Just a nit. `return Path;` should work too but I haven't checked.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:35
+// Logical scope, Section address, Section index, IsComdat.
+struct LVSymbolTableEntry {
+ LVScope *Scope = nullptr;
----------------
The struct should either be marked as `final` or have a virtual destructor.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:48
+// Function names extracted from the object symbol table.
+class LVSymbolTable {
+ using LVSymbolNames = std::map<std::string, LVSymbolTableEntry>;
----------------
The class should either be marked as `final` or have a virtual destructor.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:87
+ if (SectionAddresses.find(Section.getAddress()) == SectionAddresses.end())
+ SectionAddresses.insert(std::make_pair(Section.getAddress(), Section));
+ }
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:154
+ ~LVBinaryReader() {
+ for (LVSectionRanges::value_type &Entry : SectionRanges) {
+ assert(Entry.second && "Range value is null.");
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:156
+ assert(Entry.second && "Range value is null.");
+ delete Entry.second;
+ }
----------------
`delete` a null pointer is not an issue, so the assert could be superfluous.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:101
+ // know if the referenced element has been created.
+ GlobalOffsets.insert(std::make_pair(Offset, nullptr));
+ }
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:132-133
+ Obj(Obj) {}
+ LVELFReader(const LVELFReader &) = delete;
+ LVELFReader &operator=(LVELFReader const &) = delete;
+ ~LVELFReader() = default;
----------------
Just a nit. In the copy constructor a west-const is used while in the copy assignment operation an east-const. Also there are some east-const for return values (in `LVSymbols const &GetSymbolsWithLocations() const` member for example). Could a single style be chosen?
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1269-1271
+ if (Iter != Map->begin())
+ Iter = std::prev(Iter);
+ return (Iter != Map->end()) ? Iter->second : nullptr;
----------------
Here could be a bug: if `Map->upper_bound(Address)` returns `Map->end()`, `Map->end()` is definitely not equal to `Map->begin()`, `Iter` changes it's value to an element previous to the `end` and the check `Iter != Map->end()` is forever be `true`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1435
+ Iter != PublicNames.end(); ++Iter)
+ SortedNames.insert(std::make_pair(Iter->first->getOffset(), Iter));
+
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1438
+ LVPublicNames::const_iterator Iter;
+ for (OffsetSorted::value_type &Entry : SortedNames) {
+ Iter = Entry.second;
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:133
+ ObjForArch.getAsArchive()) {
+ return createStringError(errorToErrorCode(ArchiveOrErr.takeError()), "%s",
+ ObjName.c_str());
----------------
Should here be a check that `ArchiveOrErr` is an error? Without the check, all the code on lines 135-137 looks like a dead code.
================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:165
+ return Err;
+ TheReaders.insert(TheReaders.end(), Readers.begin(), Readers.end());
+ }
----------------
Why not just `TheReaders.push_back`? `Readers` will always have zero or one element as long as the `createReader` function creates exactly one reader.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:28-29
+ if (SymbolNames.find(SymbolName) == SymbolNames.end()) {
+ SymbolNames.insert(std::make_pair(
+ SymbolName, LVSymbolTableEntry(Function, 0, SectionIndex, false)));
+ } else {
----------------
Just a recommendation, I'm not sure the proposed code is more readable or gives a large optimization but from another point of view it eliminates copying of a struct of 18-24 bytes (`LVSymbolTableEntry`).
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:47
+ if (SymbolNames.find(SymbolName) == SymbolNames.end())
+ SymbolNames.insert(
+ std::make_pair(SymbolName, LVSymbolTableEntry(nullptr, Address,
----------------
The previous comment about a `std::picewise_construct` construction could be applied here as well.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:159
+ // Note: The section index returned by 'getIndex()' is one based.
+ Sections.insert(std::make_pair(Section.getIndex(), Section));
+ addSectionAddress(Section);
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:178
+ dbgs() << "\nSections Information:\n";
+ for (LVSections::value_type &Entry : Sections) {
+ LVSectionIndex SectionIndex = Entry.first;
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:191
+ dbgs() << "\nObject Section Information:\n";
+ for (const LVSectionAddresses::value_type &Entry : SectionAddresses)
+ dbgs() << "[" << hexValue(Entry.first) << ":"
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:289
+ Iter = SectionAddresses.lower_bound(Address);
+ if (Iter != SectionAddresses.begin())
+ --Iter;
----------------
Should the `Iter` be checked that it is not `SectionAddresses.end()`? If the `Iter` is the `end()` iterator, `--Iter` just returns it to a "normal" value and all the checks for `end` in the callers (if these checks are there) will positive (sometimes false positive).
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:313
+ Range = new LVRange();
+ SectionRanges.insert(std::make_pair(SectionIndex, Range));
+ } else {
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:421
+ Line->setName(StringRef(Stream.str()).trim());
+ Instructions.push_back(Line);
+ break;
----------------
An allocated via `new` line - `Line` - is added to the `Instructions` vector. Then, the `Instructions` vector is moved into the `ScopeInstructions` class member but elements of the `ScopeInstructions` has never been deleted using `delete` (or I just miss it). This looks like a memory leak.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:441
+ // The scope in the assembler names is linked to its own instructions.
+ ScopeInstructions.insert(std::make_pair(Scope, std::move(Instructions)));
+ AssemblerMappings.add(SectionIndex, FirstAddress, Scope);
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:465
+ // records that represent the executable instructions.
+ for (const LVPublicNames::value_type &Name : CompileUnit->getPublicNames()) {
+ LVScope *Scope = Name.first;
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:710
+ Address = (*DebugLines)[End]->getAddress();
+ Buckets.push_back(LVBucket(Begin, End, Address, false));
+ }
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:718
+ Address = (*DebugLines)[End]->getAddress();
+ Buckets.push_back(LVBucket(Begin, End, Address, false));
+ }
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:734
+ LVLines Group;
+ for (LVSections::value_type &Entry : Sections) {
+ LVSectionIndex SectionIndex = Entry.first;
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:250
+
+ auto getFlag = [&](DWARFFormValue &FormValue) -> bool {
+ return FormValue.isFormClass(DWARFFormValue::FC_Flag) ? true : false;
----------------
It looks like the lambda captures nothing, so `[&]` could be replaced with `[]`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:251
+ auto getFlag = [&](DWARFFormValue &FormValue) -> bool {
+ return FormValue.isFormClass(DWARFFormValue::FC_Flag) ? true : false;
+ };
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:254
+
+ auto getBoundValue = [&](DWARFFormValue &FormValue) -> int64_t {
+ switch (FormValue.getForm()) {
----------------
It looks like the lambda captures nothing.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:463
+ if (!CurrentElement->isCompileUnit())
+ CurrentRanges.push_back(LVAddressRange(Range.LowPC, Range.HighPC));
+ }
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:541
+ // No previous references to this offset.
+ ElementTable.insert(std::make_pair(
+ Offset, LVElementEntry(CurrentElement, LVElementSet())));
----------------
Just a nit, if you find your variant more readable, no problem.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:724
+ for (const DWARFDebugLine::Row &Row : Lines->Rows) {
+ LVLineDebug *Line = new LVLineDebug();
+ CULines.push_back(Line);
----------------
An allocated in the heap `LVLineDebug` is appended into the `CULines` vector. The vector then goes into the `processLines` function and is cleared in the `LVELFReader::createScopes()` member function. I cannot find any place where the allocated `Line` is deleted.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:856
+ SymbolsWithLocations.clear();
+ CULines.clear();
+ }
----------------
It this correct that `CULines` (a class member) is cleared after each iteration over `CompileUnits`? If there are numerous `CompileUnit`s, `CULines` will just be cleared after the first iteration.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:868
+
+ auto processLocationExpression = [&](DWARFExpression &Expression) {
+ // DW_OP_const_type is variable-length and has 3
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1020
+// Get an element given the DIE offset.
+LVElement *LVELFReader::getElementForOffset(LVOffset offset,
+ LVElement *Element) {
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1028
+ ElementTable.emplace(
+ std::make_pair(offset, LVElementEntry(nullptr, LVElementSet{Element})));
+ else {
----------------
================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:42-43
+ LVScopes::const_iterator Iter = CompileUnits->begin();
+ LVScopeCompileUnit *CompileUnit = static_cast<LVScopeCompileUnit *>(*Iter);
+ EXPECT_NE(CompileUnit, nullptr);
+ return CompileUnit;
----------------
It could be better to check that the iterator `Iter` is not null before to dereference it on the previous line.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:70
+ EXPECT_EQ(CompileUnit->getBaseAddress(), 0);
+ EXPECT_EQ(CompileUnit->getProducer().startswith("clang"), 1);
+ EXPECT_STREQ(CompileUnit->getName().data(), "test.cpp");
----------------
================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:80
+ const LVLocations *Ranges = CompileUnit->getRanges();
+ EXPECT_NE(Ranges, nullptr);
+ ASSERT_EQ(Ranges->size(), 1);
----------------
It could make sense to check pointers on the `nullptr` in `ASSERT_NE` instead of `EXPECT_NE`. If a pointer is null, the test will just crash on the first dereference of the pointer.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:142-143
+ Element = MapElements[0x00000000c0]; // 'CONSTANT'
+ EXPECT_NE(Element->getName().find("CONSTANT"), StringRef::npos);
+ EXPECT_NE(Element, nullptr);
+ EXPECT_EQ(Element->getIsSymbol(), 1);
----------------
It could be better to swap these lines.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:147-148
+ Element = MapElements[0x000000002d]; // 'INTPTR'
+ EXPECT_NE(Element->getName().find("INTPTR"), StringRef::npos);
+ EXPECT_NE(Element, nullptr);
+ EXPECT_EQ(Element->getIsType(), 1);
----------------
It could be better to swap these lines.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:243
+// Logical elements properties.
+void elementProperties(SmallString<128> &InputsDir) {
+ // Reader options.
----------------
I believe this should work as well as for the `SmallVector<N>/SmallVectorImpl` pair.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:267
+// Logical elements selection.
+void elementSelection(SmallString<128> &InputsDir) {
+ // Reader options.
----------------
================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:298
+// Compare logical elements.
+void compareElements(SmallString<128> &InputsDir) {
+ // Reader options.
----------------
================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:331
+
+ SmallString<128> InputsDir = unittest::getInputFileDirectory(TestMainArgv0);
+
----------------
`SmallStringImpl InputDir` also should works but I haven't checked.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125783/new/
https://reviews.llvm.org/D125783
More information about the llvm-commits
mailing list