[PATCH] D125784: [llvm-debuginfo-analyzer] 09 - CodeView Reader
Paul Robinson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 22 13:42:41 PDT 2022
probinson added a comment.
Partway through. "Saving my work."
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h:37
+};
+} // namespace llvm
+
----------------
Looks funny to close off a namespace and then immediately open it again.
I could see doing that after the forward declaration of SymbolGroup (below) as a way to delimit "things from outside of `logicalview`, and then you would introduce all the local things with the familiar pair of lines:
```
namespace llvm {
namespace logicalview {
```
But all this is just aesthetics, up to you really.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h:63
+
+// The ELF reader use the DWARF constants to create the logical elements.
+// The DW_TAG_* and DW_AT_* are used to select the logical object and to
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h:177
+ /// This function taken from (COFFDumper.cpp).
+ /// It can be moved to the COFF library.
+ Error initializeFileAndStringTables(BinaryStreamReader &Reader);
----------------
Use TODO: or FIXME: comments for things that should be done later.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:116
+private:
+ LVCodeViewReader *Reader = nullptr;
+ const llvm::object::coff_section *CoffSection;
----------------
`Reader` is initialized in the constructor using a constructor parameter.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:126
+
+// Current elements during the processing of a RecordType or RecordSymbol.
+extern LVElement *CurrentElement;
----------------
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.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:134
+class LVSymbolVisitor final : public SymbolVisitorCallbacks {
+ LVCodeViewReader *Reader = nullptr;
+ ScopedPrinter &W;
----------------
`Reader` is explicitly initialized in the constructor.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:140
+ LVSymbolVisitorDelegate *ObjDelegate;
+ LVShared *Shared = nullptr;
+
----------------
`Shared` is explicitly initialized in the constructor.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:149
+
+ bool HasIds = false;
+ bool InFunctionScope = false;
----------------
`HasIds` is explicitly initialized in the constructor.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:155
+ RegisterId LocalFrameRegister;
+ RegisterId ParamFrameRegister;
+
----------------
Initialize these to `RegisterId::NONE` ?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:240
+class LVLogicalVisitor final {
+ LVCodeViewReader *Reader = nullptr;
+ ScopedPrinter &W;
----------------
`Reader` is explicitly initialized in the constructor.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:249
+
+ LVShared *Shared = nullptr;
+
----------------
`Shared` is explicitly initialized in the constructor.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:245
void LVElement::generateName(std::string &Prefix) const {
+ // CodeView elements that are not referenced and not finalized.
LVScope *Scope = getParentScope();
----------------
I'm not sure what this means. As written, it would describe a collection of elements that aren't referenced or finalized, but there's no such collection here.
If you remove "that", I understand it to mean this method doesn't do anything with those CodeView elements.
If the phrase means something else, I'd need an explanation.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:183
+ // The current CodeView Reader implementation does not have support for
+ // multiple compile units. Until a proper offset calculation, check only
+ // in the current compile unit.
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:184
+ // multiple compile units. Until a proper offset calculation, check only
+ // in the current compile unit.
if (CompileUnits.size()) {
----------------
This sounds like a FIXME/TODO kind of comment.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1795
// DW_AT_specification DW_FORM_ref4 0x00000048
+ // The CodeView does not include any information at the class level to
+ // mark the member function as external.
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:2051
+ // Start traversing the scopes root and transform the element name.
+ TraverseScope(this);
+}
----------------
I don't understand the reason to put the entire method implementation into a lambda, and then call it once.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:112
+ if (InTemplate && !AngleCount)
+ InTemplate = false;
+ }
----------------
I'd eliminate the `InTemplate` variable and check `AngleCount` instead, but up to you.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:333
+ // that refer to internal runtime structures, that we do not process. Those
+ // typedes are marked as 'system'. They have an associated logical type,
+ // but the underlying type always is null.
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:53
+ if (Obj.isCOFF()) {
+ COFFObjectFile *COFF = dyn_cast<COFFObjectFile>(&Obj);
+ return new LVCodeViewReader(Filename, FileFormatName, *COFF, W,
----------------
`dyn_cast` is used when the result might not be of the specified type, in which case you get a null pointer. I think `cast` is the right thing to use here.
================
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 =
----------------
`Twine` has an implicit constructor from `SmallString`, I don't think you need to call it explicitly.
================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:152
+
+ // Search in the directory derivated from the given 'Filename' for a
+ // matching object file (.o, .obj, .lib) or a matching executable file
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:192
+ "Binary object format in '%s' does not have debug info.",
+ TheFilename.c_str());
+ }
----------------
No need for local variable `TheFilename` .
================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:272
+ return createStringError(errorToErrorCode(std::move(Err)), "%s",
+ TheFilename.c_str());
+ }
----------------
No need to construct `TheFilename` explicitly.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:839
+// 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,
----------------
I think `recorded` makes it sound like this function puts them into CUInlineeLines.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:843
+ SmallVector<LVInlineeLine::iterator> InlineeIters;
+ std::function<void(LVScope * Parent)> FindInlinedScopes =
+ [&](LVScope *Parent) {
----------------
Isn't a lambda usually defined with `auto`?
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:57
+uint32_t PointerToRawData = 0;
+#define ABSOLUTE_OFFSET(offset) (PointerToRawData + offset)
+
----------------
I don't find any uses of `ABSOLUTE_OFFSET` which means it and the (bad!) unscoped global (bad!) `PointerToRawData` can be eliminated.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:59
+
+std::string LVCodeViewReader::getSymbolKindName(SymbolKind Kind) {
+ switch (Kind) {
----------------
It looks like this is almost exactly getSymbolKindName from SymbolDumper.cpp. I don't see an easy way to make use of that function, though.
Return type should be a StringRef, no need to make a copy of the string literal.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:68
+ }
+}
+
----------------
Might need an `llvm_unreachable` here to avoid warnings about falling off the end of the function without a return value.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:132
+ if (resolveSymbolName(CoffSection, RelocOffset, Symbol))
+ *RelocSym = "";
+}
----------------
`RelocSym` might be null (checked above).
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:392
+ if (BuffOrErr.getError()) {
+ // The server name does not exist. Try in the directory as the input file.
+ ServerName = createAlternativePath(InputFilename, ServerName);
----------------
?
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:400
+ "File '%s' does not exist.",
+ TheFilename.c_str());
+ }
----------------
No need to construct `TheFilename` explicitly.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:412
+ return createStringError(errorToErrorCode(std::move(Err)), "%s",
+ TheFilename.c_str());
+ }
----------------
`SmallString` has a direct method for getting a C string, much more efficient than going through std::string (no copying).
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:422
+ Expected<InfoStream &> expectedInfo = Pdb.getPDBInfoStream();
+ if (expectedInfo && expectedInfo->getGuid() != TS.getGuid())
+ return createStringError(errc::invalid_argument, "signature_out_of_date");
----------------
If we have to check `expectedInfo` anyway, let's not assume `getPDBInfoStream` succeeds.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:427
+ // the server. We need to keep the original input source, as reading other
+ // sections wil require the input associated with the loaded object file.
+ TypeServer = std::make_shared<InputFile>(&Pdb);
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:467
+ "Binary object format in '%s' is not supported.",
+ TheFilename.c_str());
+ }
----------------
`SmallString` has a more efficient direct conversion to C string.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:474
+ return createStringError(errc::not_supported, "'%s' is not a COFF object.",
+ TheFilename.c_str());
+ }
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:514
+ }
+ break;
+ }
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:665
+
+ PointerToRawData = getObj().getCOFFSection(Section)->PointerToRawData;
+
----------------
`PointerToRawData` is not used anywhere.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:697
+ if (SubType & SubsectionIgnoreFlag)
+ SubType &= ~SubsectionIgnoreFlag;
+
----------------
Might as well turn this off unconditionally.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:777
+
+ // Find the associate symbol table information.
+ LVSymbolTableEntry SymbolTableEntry = getSymbolTableEntry(SymbolName);
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:929
+ if (BuffOrErr.getError()) {
+ std::string TheFilename(ExePath);
+ return createStringError(errc::bad_file_descriptor,
----------------
In this context I think `ExePath` is the class member? which is already a std::string.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:944
+ if (errorToErrorCode(BinOrErr.takeError())) {
+ std::string TheFilename(ExePath);
+ return createStringError(errc::not_supported,
----------------
I think `ExePath` is already a std::string.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1019
+ // The CodeView compile unit containing the global symbols does not
+ // have a name; use the and it is marked as such. Just append the
+ // '_global' string.
----------------
"use the and it is marked as such." Could you rephrase?
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1037
+ } else
+ consumeError(Sym.takeError());
+ }
----------------
Please put braces around this, for consistency with the matching `if`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1043
+ } else
+ consumeError(ExpectedSyms.takeError());
+ }
----------------
Please put braces around this, for consistency with the matching `if`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1170
+
+// In order to create the scopes, the CodeView Reader:
+// = Traverse the TPI/IPI stream (Type visitor):
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1175
+// = Traverse the symbols section (Symbol visitor):
+// It creates the scopes tree and creates the required logical elements, by
+// using the collected indexes from the type visitor.
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1226
+
+std::string LVCodeViewReader::getRegisterName(LVSmall Opcode,
+ uint64_t Operands[2]) {
----------------
Is this an overload called from generic code? I don't see it used anywhere in this patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125784/new/
https://reviews.llvm.org/D125784
More information about the llvm-commits
mailing list