[PATCH] D125783: [llvm-dva] 08 - ELF Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:247
+                    sys::path::filename(ConvertedPath));
+  return std::string(Path);
+}
----------------
psamolysov wrote:
> Just a nit. `return Path;` should work too but I haven't checked.
Due to the changes described here https://reviews.llvm.org/D125778#inline-1235357 it is required the explicit conversion.


================
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.");
----------------
psamolysov wrote:
> 
Nice change.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:156
+      assert(Entry.second && "Range value is null.");
+      delete Entry.second;
+    }
----------------
psamolysov wrote:
> `delete` a null pointer is not an issue, so the assert could be superfluous.
Removed the assertion.


================
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);
----------------
psamolysov wrote:
> 
Replaced to use `emplace`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:178
+    dbgs() << "\nSections Information:\n";
+    for (LVSections::value_type &Entry : Sections) {
+      LVSectionIndex SectionIndex = Entry.first;
----------------
psamolysov wrote:
> 
Nice change.


================
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) << ":"
----------------
psamolysov wrote:
> 
Nice change.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:313
+    Range = new LVRange();
+    SectionRanges.insert(std::make_pair(SectionIndex, Range));
+  } else {
----------------
psamolysov wrote:
> 
Replaced to used ``emplace`.


================
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);
----------------
psamolysov wrote:
> 
Replaced to use `emplace`.


================
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;
----------------
psamolysov wrote:
> 
Nice change.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:710
+    Address = (*DebugLines)[End]->getAddress();
+    Buckets.push_back(LVBucket(Begin, End, Address, false));
+  }
----------------
psamolysov wrote:
> 
Replaced to use `emplace_back`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:718
+    Address = (*DebugLines)[End]->getAddress();
+    Buckets.push_back(LVBucket(Begin, End, Address, false));
+  }
----------------
psamolysov wrote:
> 
Replaced to use `emplace_back`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:734
+  LVLines Group;
+  for (LVSections::value_type &Entry : Sections) {
+    LVSectionIndex SectionIndex = Entry.first;
----------------
psamolysov wrote:
> 
Nice change.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:250
+
+  auto getFlag = [&](DWARFFormValue &FormValue) -> bool {
+    return FormValue.isFormClass(DWARFFormValue::FC_Flag) ? true : false;
----------------
psamolysov wrote:
> It looks like the lambda captures nothing, so `[&]` could be replaced with `[]`.
Replaced to `[]`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:251
+  auto getFlag = [&](DWARFFormValue &FormValue) -> bool {
+    return FormValue.isFormClass(DWARFFormValue::FC_Flag) ? true : false;
+  };
----------------
psamolysov wrote:
> 
Removed ` ? true : false`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:254
+
+  auto getBoundValue = [&](DWARFFormValue &FormValue) -> int64_t {
+    switch (FormValue.getForm()) {
----------------
psamolysov wrote:
> It looks like the lambda captures nothing.
Replaced to `[]`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:463
+        if (!CurrentElement->isCompileUnit())
+          CurrentRanges.push_back(LVAddressRange(Range.LowPC, Range.HighPC));
+      }
----------------
psamolysov wrote:
> 
Replaced to use `emplace_back`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:868
+
+  auto processLocationExpression = [&](DWARFExpression &Expression) {
+    // DW_OP_const_type is variable-length and has 3
----------------
psamolysov wrote:
> 
Added `const`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1020
+// Get an element given the DIE offset.
+LVElement *LVELFReader::getElementForOffset(LVOffset offset,
+                                            LVElement *Element) {
----------------
psamolysov wrote:
> 
Changed to `Offset`.


================
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;
----------------
psamolysov wrote:
> It could be better to check that the iterator `Iter` is not null before to dereference it on the previous line.
Good point. Added `EXPECT_NE(Iter, nullptr);`.


================
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");
----------------
psamolysov wrote:
> 
Replace with `EXPECT_TRUE`.


================
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);
----------------
psamolysov wrote:
> It could be better to swap these lines.
Good catch.


================
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);
----------------
psamolysov wrote:
> It could be better to swap these lines.
Good catch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125783/new/

https://reviews.llvm.org/D125783