[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:60
+  void Dump(Stream *s) override { *s << "PlaceholderObjectFile\n"; }
+  uint32_t GetAddressByteSize() const override { return 0; }
+  uint32_t GetDependentModules(FileSpecList &file_list) override { return 0; }
----------------
Should we return something valid for this function?


================
Comment at: source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h:92
+
+  bool HasIndexForModule(uint16_t modi) const;
 };
----------------
This method name is a little bit confusing.  Is it referring to a numeric index like 1, 2, 3, 4, 5.  Or a `CompilandIndexItem`?  I would call this something like `HasCompilandInfo(uint16_t modi)`


================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:132-135
+    if (so.segment == 0) {
+      lldbassert(so.offset == 0);
+      continue;
+    }
----------------
What kind of symbols have a segment and offset of 0?


================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:150-157
     // The odds of an error in some function such as GetSegmentAndOffset or
     // MakeVirtualAddress are much higher than the odds of encountering bad
     // debug info, so assert that this item was inserted in the map as opposed
     // to having already been there.
-    lldbassert(insert_result.second);
+    //
+    // TODO: revisit this check since it fires for apparently valid PDBs
+    //
----------------
If we're going to comment it out, then just delete the code IMO.

Do you have some llvm-pdbutil output that demonstrates this on a valid PDB?  I guess we'd be looking for 2 symbols with the same Virtual Address.  Seems odd to have that, but maybe an example of where it's happening would make it clear whether this is actually a valid case.


================
Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:106
 static std::unique_ptr<PDBFile>
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator &allocator) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+                    llvm::BumpPtrAllocator &allocator) {
----------------
Perhaps `obj_file` should be a reference just to clarify that it can't be null.


================
Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:114-115
 
-  auto *obj = llvm::dyn_cast<llvm::object::COFFObjectFile>(binary.getBinary());
-  if (!obj)
-    return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
+    OwningBinary<Binary> binary = std::move(*expected_binary);
----------------
Instead of trying to load it as a PE/COFF fail and then trying something else if it fails (hoping that it's a minidump), perhaps we could use `llvm::identify_magic()` to figure out what it is actually is first.  That function currently cannot detect a Windows minidump, but we coudl teach it to support that.  I think that would make this code more logical.  

```
if (type == exe) {
} else if (type == minidump) {
} else {
  // actual error
}
```


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55142