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

[Zequan Wu via Phabricator via lldb-commits]

zequanwu marked an inline comment as done.
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:
> 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`.


================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:207-209
+Address ObjectFilePDB::GetBaseAddress() {
+  return Address(GetSectionList()->GetSectionAtIndex(0), 0);
+}
----------------
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.


================
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);
+}
----------------
labath wrote:
> 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.
The reason why I copy section list from `unified_section_list` to `m_sections_up` is to let `GetBaseAddress` be able to return the right base address.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89812