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