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