[PATCH] D125784: [llvm-debuginfo-analyzer] 09 - CodeView Reader

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Comments through the end of llvm/lib.

My goodness this is a large patch.  I don't really know anything about CodeView so the reader/visitor modules didn't get as much attention as they probably deserve.
Will try to finish tomorrow.



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:226
+  using LookupSet = std::set<StringRef>;
+  LookupSet DeductedScopes;
+  LookupSet UnresolvedScopes;
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:259
+    LVStringRefs::size_type FirstNamespace = 0;
+    LVStringRefs::size_type FirstNonNamespace = Components.size() - 1;
+    for (LVStringRefs::size_type Index = 0; Index < Components.size();
----------------
`FirstNonNamespace` will be set to 0 on the first iteration of the loop, which cannot be a zero-trip loop because we know `Components.size()` is nonzero.  So this initialization is dead.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:267
+        break;
+    }
+    return std::make_tuple(FirstNamespace, FirstNonNamespace);
----------------
I think this loop could be replaced with something along the lines of
```
LVStringRefs::iterator Iter = std::find_if(Components.begin(), Components.end(), 
    [](StringRef Name) {
        return IdentifiedNamespaces.find(Name) == IdentifiedNamespaces.end();
    });
LVStringRefs::size_type FirstNonNamespace = std::distance(Components.begin(), Iter);
```
find_if returns an iterator into Components for the first non-namespace, and then std::distance gets the index.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:314
+  LVCodeViewReader *Reader = nullptr;
+  LVLogicalVisitor *Visitor = nullptr;
+  LVForwardReferences ForwardReferences;
----------------
Reader and Visitor are initialized by the constructor, so remove `= nullptr` here.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:403-406
+  // - deducted scopes (class, structure, union and enum) and
+  // - unresolved scopes, that can represent namespaces or any deducted.
+  // Before creating the namespaces, we have to traverse the unresolved
+  // and remove any references to already deducted scopes.
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:441
+      std::string TheComponent(Component);
+      dbgs() << formatv("'{0}'\n", TheComponent.c_str());
+    }
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:482
+    std::string TheScopedName(ScopedName);
+    dbgs() << formatv("ScopedName: '{0}'\n", TheScopedName.c_str());
+  });
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:687
+// Current elements during the processing of a RecordType or RecordSymbol.
+LVElement *CurrentElement = nullptr;
+LVScope *CurrentScope = nullptr;
----------------
`CurrentElement` appears to be used in only one method, so it can be made local to that method.
It would be best to find some other home for the other three globals here.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:766
+
+  CurrentElement = LogicalVisitor->createElement(Kind);
+  if (!CurrentElement) {
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:1068
+    DefRangeFramePointerRelFullScopeSym &DefRangeFramePointerRelFullScope) {
+  // Defranges don't have types, just registers and code offsets.
+  LLVM_DEBUG({
----------------
Otherwise it reads as "de-franges" to me :)


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:1095
+    CVSymbol &Record, DefRangeFramePointerRelSym &DefRangeFramePointerRel) {
+  // Defranges don't have types, just registers and code offsets.
+  LLVM_DEBUG({
----------------
etc. I'll stop marking each one.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:1400
+    LVScope *AbstractFunction = new LVScopeFunction();
+    if (AbstractFunction) {
+      AbstractFunction->setIsSubprogram();
----------------
Usually `new` is assumed to return non-null, why check it just in this one place?


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:2257
+
+  // The incomming element, does not have a defined kind. Use the given
+  // modifiers to complete its type. A type can have more than one modifier;
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:2323
+
+  // Type pointer is point to.
+  LVType *Pointer = static_cast<LVType *>(Element);
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:3375
+    std::string TheName(Name);
+    OS << format("%20s", TheName.c_str());
+    NewLine();
----------------
Unless format() understands StringRef directly?  I'm not sure.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:3404
+      break;
+    }
+  }
----------------
Isn't this `for` loop (which does a `break` on the first iteration) equivalent to
```
if (!Locations->empty())
  ParentLowPC = Locations->begin()->getLowerAddress();
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:3407
+
+  // For the given inlinesite, get the initial line number and it's
+  // source filename. Update the logical scope representing it.
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:3496
+        SeenHighAddress = true;
+        break;
+      }
----------------
`break` not needed here


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

https://reviews.llvm.org/D125784