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

Thu Oct 22 18:28:33 PDT 2020

zequanwu 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");
----------------
labath wrote:
> 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.
So, we want to refactor out the common part of `ObjectFilePECOFF::GetArchitecture` and `ObjectFilePDB::GetModuleSpecifications` to keep them consistent? I will do it in another patch.

================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:207-209
+Address ObjectFilePDB::GetBaseAddress() {
+  return Address(GetSectionList()->GetSectionAtIndex(0), 0);
+}
----------------
labath wrote:
> 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...
Thanks for pointing it out. I'll update it and leave `GetBaseAddress` and `CreateSections` as unimplemented.

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:299
     std::unique_ptr<PDBFile> file_up =
         loadMatchingPDBFile(m_objfile_sp->GetFileSpec().GetPath(), m_allocator);

----------------
labath wrote:
> 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.
> Then SymbolFilePDB can have an additional unique_ptr<PDBFile> member
I think you meant `SymbolFileNativePDB`, but why do we need this? It already has a member `unique_ptr<PDBIndex>`. I might misunderstood. 

I changed it to have `loadPDBFile` return raw pointer and `ObjectFilePDB` has a raw pointer member and PdbIndex uses raw pointer.

================
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
----------------
labath wrote:
> 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.
I noticed when I use `llvm-pdbutil pdb2yaml` to generate yaml file from pdb file and then use `llvm-pdbutil yaml2pdb` to get pdb file back, the two pdb files are not the same. Also using `lldb-test symbols` with the pdb created from yaml file, got the error: `error: Module has no symbol vendor.` This doesn't happen if I use the original pdb file.

================
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
+
----------------
labath wrote:
> This is SymbolFile functionality, so it'd be better placed in `Shell/SymbolFile/PDB`
I put it here because I saw `symbol.yaml` which uses `lldb-test symbols` exists in `Shell/ObjectFile/PECOFF`. And the folder `Shell/SymbolFile` seems to contain tests for SymbolFile plugins.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89812