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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
