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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:126
+
+// Current elements during the processing of a RecordType or RecordSymbol.
+extern LVElement *CurrentElement;
----------------
probinson wrote:
> These are globals?  That's undesirable in library code, is there some class you can put them in?  Define a "context" class maybe?  If that's not too awkward for the rest of the API.
The globals
```
  LVElement *CurrentElement;
  LVScope *CurrentScope;
  LVSymbol *CurrentSymbol;
  LVType *CurrentType;
```
are used by the `SymbolVisitor` and `LogicalVisitor`. We can have several `SymbolVisitor` instances but only one `LogicalVisitor` instance which is created by the CodeViewReader.

Moved those globals to the `LogicalVisitor` as public members.



================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:132
+  llvm::sys::path::replace_extension(ObjPath, Extension);
+  if (llvm::sys::fs::exists(Twine(ObjPath))) {
+    ErrorOr<std::unique_ptr<MemoryBuffer>> BuffOrErr =
----------------
probinson wrote:
> `Twine` has an implicit constructor from `SmallString`, I don't think you need to call it explicitly.
You are correct. Removed the explicit call.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:57
+uint32_t PointerToRawData = 0;
+#define ABSOLUTE_OFFSET(offset) (PointerToRawData + offset)
+
----------------
probinson wrote:
> I don't find any uses of `ABSOLUTE_OFFSET` which means it and the (bad!) unscoped global (bad!) `PointerToRawData` can be eliminated.
Removed `ABSOLUTE_OFFSET`, `PointerToRawData` and the comments. The idea behind them was to calculate an unique offset for the logical symbols and types (similar to the DWARF DIE offset). This limitation is already recorded for future work.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:665
+
+  PointerToRawData = getObj().getCOFFSection(Section)->PointerToRawData;
+
----------------
probinson wrote:
> `PointerToRawData` is not used anywhere.
Removed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1037
+          } else
+            consumeError(Sym.takeError());
+        }
----------------
probinson wrote:
> Please put braces around this, for consistency with the matching `if`.
Added braces.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1043
+    } else
+      consumeError(ExpectedSyms.takeError());
+  }
----------------
probinson wrote:
> Please put braces around this, for consistency with the matching `if`.
Added braces.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1226
+
+std::string LVCodeViewReader::getRegisterName(LVSmall Opcode,
+                                              uint64_t Operands[2]) {
----------------
probinson wrote:
> Is this an overload called from generic code? I don't see it used anywhere in this patch.
It is an overload method in the LVReader
```
class LVReader {
  ...
  virtual std::string getRegisterName(LVSmall Opcode, uint64_t Operands[2]) {
    ...
  }
  ...
};
```
It is called in 
```
LVOperation::getOperandsDWARFInfo() {
  ...
  getReader().getRegisterName(...);
  ...
}
std::string LVOperation::getOperandsCodeViewInfo() {
  ...
  getReader().getRegisterName(...);
  ...
}
```
They are called when print the debug locations.
```
void LVLocationSymbol::printExtra(raw_ostream &OS, bool Full) const {
  ...
             << (CodeViewLocation ? Operation->getOperandsCodeViewInfo()
                                  : Operation->getOperandsDWARFInfo());
  ...
}
```



================
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;
----------------
probinson wrote:
> `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.
The globals
```
  LVElement *CurrentElement;
  LVScope *CurrentScope;
  LVSymbol *CurrentSymbol;
  LVType *CurrentType;
```
are used by the `SymbolVisitor` and `LogicalVisitor`. We can have several `SymbolVisitor` instances but only one `LogicalVisitor` instance which is created by the CodeViewReader.

Moved those globals to the `LogicalVisitor` as public members.



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

https://reviews.llvm.org/D125784