[Lldb-commits] [PATCH] D89812: [lldb][PDB] Add ObjectFile PDB plugin

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this is a good start. See inline comments for details. In addition to the test for the "target symbols add" flow it would be good to have a separate test for the ObjectFilePDB functionality. You can look at existing tests in `test/Shell/ObjectFile` for inspiration.



================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:10
+#include "ObjectFilePDB.h"
+
+#include "lldb/Core/Module.h"
----------------
You should be able to delete this line too clang-format should know to put this file first.


================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:45
+static UUID GetPDBUUID(InfoStream &IS) {
+  // This part is similar with what has done in ObjectFilePECOFF.
+  using llvm::support::endian::read16be;
----------------
There's also a third copy of this code in ProcessMinidump (MinidumpParser.cpp). I think it's time to factor this out somehow. We could either create a new file in the Utility module, or put this functionality in directly inside the UUID class -- I'm not currently sure what's better.


================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:172
+  lldb_private::UUID &uuid = module_spec.GetUUID();
+  if (!uuid.IsValid())
+    uuid = GetPDBUUID(*info_stream);
----------------
I'm pretty sure this check is bogus and can never be false.


================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:176-197
+  switch (dbi_stream->getMachineType()) {
+  case PDB_Machine::Amd64:
+    spec.SetTriple("x86_64-pc-windows");
+    specs.Append(module_spec);
+    break;
+  case PDB_Machine::x86:
+    spec.SetTriple("i386-pc-windows");
----------------
Could this use the same algorithm as `ObjectFilePDB::GetArchitecture` ?


================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:207-209
+Address ObjectFilePDB::GetBaseAddress() {
+  return Address(GetSectionList()->GetSectionAtIndex(0), 0);
+}
----------------
Leave this unimplemented. PDBs don't have sections and are not loaded into memory, so the function does not apply.


================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:215-216
+  m_sections_up = std::make_unique<SectionList>();
+  for (auto s : unified_section_list)
+    m_sections_up->AddSection(s);
+}
----------------
This shouldn't be necessary. What "normally" happens here is that an object file is adding own sections *into* the "unified" section list -- not the other way around. Since we're pretending that the pdb file has no sections (and that's kind of true) I think you should just return an empty section list here, and not touch the unified list at all.


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:145
     return nullptr;
-
   return objfile_up.release();
----------------
revert spurious changes.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:299
     std::unique_ptr<PDBFile> file_up =
         loadMatchingPDBFile(m_objfile_sp->GetFileSpec().GetPath(), m_allocator);
 
----------------
Instead of changing `loadMatchingPDBFile` I think we ought to change this to something like:
```
if (auto *pdb = llvm::dyn_cast<ObjectFilePDB>(m_objfile_sp)) {
  file_up = pdb->giveMeAPDBFile(); 
  // PS: giveMeAPDBFile is a new pdb-specific public method on ObjectFilePDB
  // PS: possibly this should not return a PDBFile, but a PDBIndex directly -- I don't know the details but the general idea is that it could/should reuse the pdb parsing state that the objectfile class has already created
} else 
  file_up = do_the_loadMatchingPDBFile_fallback();
```

The reason for that is that it avoids opening the pdb file twice (once in the object file and once here). Another reason is that I believe the entire `loadMatchingPDBFile` function should disappear. Finding the pdb file should not be the job of the symbol file class -- we have symbol vendors for that. However, we should keep the fallback for now to keep the patch small...


================
Comment at: lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp:14
+// Create lldbinit
+// RUN: echo -e "target symbols add %t/executable/bar.pdb\nquit" > %t/load-pdb.lldbinit
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t/executable/foo.exe -s \
----------------
I'd just pass commands to lldb directly via `-o`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89812