[PATCH] D125784: [llvm-dva] 09 - CodeView Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:171
+  void addInlineeLines(LVScope *Scope, LVLines &Lines) {
+    LVLines *InlineeLines = new LVLines();
+    *InlineeLines = std::move(Lines);
----------------
psamolysov wrote:
> An `InlineeLines` is a pointer to an in-heap allocated object. I've found that some lines are deleted in the `LVBinaryReader::includeInlineeLines` function but I'm not sure that all the allocated ones. Although, what if the function is never called after a call to the `addInlineeLines` one? Could you check that all the in-heap allocated and not deleted during the class' instance lifetime objects are deleted in the destructor?
For the CodeView Reader, the lines for inlined functions are processed in a separated step. Those lines are collected in `CUInlineeLines` which are processed by the `includeInlineeLines` funtion.

```
// Traverse the scopes for the given 'Function' looking for any inlined
// scopes with inlined lines, which are recorded in 'CUInlineeLines'.
void LVBinaryReader::includeInlineeLines(LVSectionIndex SectionIndex,
                                         LVScope *Function) {
  ..
        CULines.insert(Iter, InlineeLines->begin() + 1, InlineeLines->end());
  ..
```
Then those `CULines` are processed by the `void LVBinaryReader::processLines` function which move them to its enclosing logical `LVScope` and deleted at the end of the scope lifetime.

The same case as discussed in https://reviews.llvm.org/D125783 (ELF Reader).


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:350
+  std::string getCompileUnitName() { return CompileUnitName; }
+  void setCompileUnitName(std::string Name) { CompileUnitName = Name; }
+
----------------
psamolysov wrote:
> 
Changed to `CompileUnitName = std::move(Name);`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:157
+    std::vector<StringRef> ExecutableExtensions = {"exe", "dll"};
+    for (const StringRef &Extension : ExecutableExtensions) {
+      std::string ExecutableImage = searchForExe(Filename, Extension);
----------------
psamolysov wrote:
> It makes no sense to pass `StringRef` by reference.
Good point.


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:170
+    std::vector<StringRef> ObjectExtensions = {"o", "obj", "lib"};
+    for (const StringRef &Extension : ObjectExtensions) {
+      std::string ObjectImage = searchForObj(Filename, Extension);
----------------
psamolysov wrote:
> 
Good point.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:777
+  // know all the required information.
+  for (StringRef &SymbolName : SymbolNames) {
+    LLVM_DEBUG({
----------------
psamolysov wrote:
> `StringRef` is already a "reference".
Good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125784