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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
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");
----------------
zequanwu wrote:
> labath wrote:
> > Could this use the same algorithm as `ObjectFilePDB::GetArchitecture` ?
> No. This part is the same algorithm as the part in `ObjectFilePECOFF::GetModuleSpecifications`, https://github.com/llvm/llvm-project/blob/master/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp#L194-L215.
> 
> The `ObjectFilePDB::GetArchitecture` is same as `ObjectFilePECOFF::GetArchitecture`.
I'm not sure this kind of consistency actually helps. For example, in `Module::MatchesModuleSpec`, module's own architecture (as provided by `ObjectFile(PECOFF)::GetArchitecture`) is compared to the one in the ModuleSpec (generally coming from `ObjectFile(PDB)::GetModuleSpecifications`.

If there's any mismatch between the two ways of computing a ArchSpec, then there will be trouble, even though they are "symmetric".

That said, I think we can keep this like it is in this patch, but I'd like to clean up both implementations in a another patch.


================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:207-209
+Address ObjectFilePDB::GetBaseAddress() {
+  return Address(GetSectionList()->GetSectionAtIndex(0), 0);
+}
----------------
zequanwu wrote:
> labath wrote:
> > Leave this unimplemented. PDBs don't have sections and are not loaded into memory, so the function does not apply.
> This is actually necessary. It's called at `SymbolFileNativePDB::InitializeObject`. Otherwise, setting breakpoint won't work.
So, that's a bug in `SymbolFileNativePDB::InitializeObject`. It should be calling `m_objfile_sp->GetModule()->GetObjectFile()->GetBaseAddress()` (as done in e.g. `SymbolFileBreakpad::GetBaseFileAddress`) instead of `m_objfile_sp->GetBaseAddress()`. That will get you the base address of the main (exe) object file instead of the pdb. Up until now, that distinction was not important because `m_objfile_sp` was always the main file...


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:276-285
     if (!file_up) {
       auto module_sp = m_objfile_sp->GetModule();
       if (!module_sp)
         return 0;
       // See if any symbol file is specified through `--symfile` option.
       FileSpec symfile = module_sp->GetSymbolFileFileSpec();
       if (!symfile)
----------------
You should be able to delete this block now -- the code you're writing now supersedes (and enhances) it. If the module has a SymbolFileSpec, then this file will be loaded (as an ObjectFilePDB) in SymbolVendor::FindPlugin, and that instance will be passed to the SymbolFilePDB constructor.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:299
     std::unique_ptr<PDBFile> file_up =
         loadMatchingPDBFile(m_objfile_sp->GetFileSpec().GetPath(), m_allocator);
 
----------------
zequanwu wrote:
> labath wrote:
> > 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...
> From what I see, it seems like PDBFile is designed to be created when opening the pdb file. It doesn't allow copying it around.
> `NativeSession` holds a unique_ptr of PDBFile and `NativeSession::getPDBFile` returns the reference of object, so creating unique_ptr from it is not allowed.
> 
> I moved `loadPDBFile` to `ObjectFilePDB` as static function, so we can move `PDBFile` easily.
You don't need to copy it -- you can just have the two objects reference the same instance (stealing the PDBFile object from under ObjectFilePDB seems suboptimal). Probably the simplest way to achieve that is to change the PdbIndex class to use a raw pointer (and ObjectFilePDB to provide one). Then SymbolFilePDB can have an additional unique_ptr<PDBFile> member, which may or may not be empty (depending on whether it fetches object from ObjectFilePDB or loads it itself). Eventually, as we transition to always fetching the PDBFile from ObjectFilePDB, that member will disappear completely.

Also, I wasn't expecting you to take the `giveMeAPDBFile` name literally. :) I was deliberatly using a funky name as a placeholder because I wasn't sure of how exactly this would work -- I was trying to illustrate the general concept, and not a precise implementation. If what, I said in the previous paragraph works, then a "normal" name like `GetPDBFile` should work just fine...

Sorry about the confusion.


================
Comment at: lldb/test/Shell/ObjectFile/PDB/object.test:3
+# RUN: yaml2obj %p/Inputs/pdb.yaml -o %t.obj
+# RUN: lld-link %t.obj /debug /pdb:%t.pdb /entry:main /nodefaultlib
+# RUN: lldb-test object-file %t.pdb | FileCheck %s
----------------
Could we generate the pdb directly from yaml (perhaps via `llvm-pdbutil yaml2pdb`)? That way we could have a hardcoded UUID and explicit test for its parsing.


================
Comment at: lldb/test/Shell/ObjectFile/PDB/symbol.test:4
+# RUN: lld-link %t.obj /debug /pdb:%t.pdb /entry:main /nodefaultlib
+# RUN: lldb-test symbols %t.pdb | FileCheck %s
+
----------------
This is SymbolFile functionality, so it'd be better placed in `Shell/SymbolFile/PDB`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89812