[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/Readers/LVBinaryReader.h:50
+  using LVSymbolNames = std::map<std::string, LVSymbolTableEntry>;
+  LVSymbolNames SymbolNames;
+
----------------
MaskRay wrote:
> Does an associative container like `StringMap` suffice?
> 
> If a sorted associative container is needed, consider std::map<StringRef, ...> if the keys are backed somewhere.
That is a good suggestion. But unfortunately not all the keys (strings) are backed somewhere.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:109
+
+  std::unique_ptr<const MCRegisterInfo> MRI = nullptr;
+  std::unique_ptr<const MCAsmInfo> MAI = nullptr;
----------------
MaskRay wrote:
> Drop `= nullptr` 
Dropped all `= nullptr`


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:48
+  // Current elements during the processing of a DIE.
+  LVElement *CurrentElement = nullptr;
+  LVScope *CurrentScope = nullptr;
----------------
MaskRay wrote:
> I haven't checked but whether a smart pointer is suitable here? You assign these fields with `new`ed objects. it is prone to a memory leak.
That is a good point.

Already discussed in a previous patch https://reviews.llvm.org/D125778#inline-1210290


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:133
+            ObjForArch.getAsArchive()) {
+      return createStringError(errorToErrorCode(ArchiveOrErr.takeError()), "%s",
+                               ObjName.c_str());
----------------
psamolysov wrote:
> 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.
Good catch.
```
      return createStringError(errorToErrorCode(ArchiveOrErr.takeError()), "%s",
                               ObjName.c_str());
```
Should be removed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:165
+      return Err;
+    TheReaders.insert(TheReaders.end(), Readers.begin(), Readers.end());
+  }
----------------
psamolysov wrote:
> Why not just `TheReaders.push_back`? `Readers` will always have zero or one element as long as the `createReader` function creates exactly one reader.
In the case of `MachOUniversalBinary` it can be:
- `MachOObjectFile`: it creates only one LVReader.
- `Archive`: it creates one `LVReader` for each embedded object.



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:421
+      Line->setName(StringRef(Stream.str()).trim());
+      Instructions.push_back(Line);
+      break;
----------------
psamolysov wrote:
> 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.
The best way to explain it is by looking at the following code:

```
// During the traversal of the debug information sections, we created the
// logical lines representing the disassembled instructions from the text
// section and the logical lines representing the line records from the
// debug line section. Using the ranges associated with the logical scopes,
// we will allocate those logical lines to their logical scopes.
void LVBinaryReader::processLines(LVLines *DebugLines,
                                  LVSectionIndex SectionIndex,
                                  LVScope *Function) {
{
  ..
  // Process collected lines.
  LVScope *Scope;
  for (LVLine *Line : *DebugLines) {
    ..
    // Add line object to scope.
    Scope->addElement(Line);     <---- LineDebug/LineAssembler added to Scope
   ..
  }
}
```
`LVLines` from the `ScopeInstructions` are inserted in `DebugLines` based on their associated addresses.

For each `LVLine`, the function finds the `LVScope` whose address range will contain its address. Then it add that `LVLine` to that scope. The `LVLine` can be a `LVLineDebug` or `LVLineAssembler`.
When the scope is destroyed, all its lines (`LVLineDebug` and/or `LVLineAssembler`) are deleted.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:80
+  const LVLocations *Ranges = CompileUnit->getRanges();
+  EXPECT_NE(Ranges, nullptr);
+  ASSERT_EQ(Ranges->size(), 1);
----------------
psamolysov wrote:
> 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.
That is a good point.
I believe a function that uses `ASSERT_NE` must be declared with `TEST` or `TEST_F` and I can't see how to return a value, which it is our case.
In order to use `ASSERT_NE` then we need to change how those functions are implemented and invoked.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125783