labath added a comment.
I think this is a good start. See inline comments for details. In addition to
the test for the "target symbols add" flow it would be good to have a separate
test for the ObjectFilePDB functionality. You can look at existing tests in
`test/Shell/ObjectFile` for inspiration.
================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:10
+#include "ObjectFilePDB.h"
+
+#include "lldb/Core/Module.h"
----------------
You should be able to delete this line too clang-format should know to put this
file first.
================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:45
+static UUID GetPDBUUID(InfoStream &IS) {
+ // This part is similar with what has done in ObjectFilePECOFF.
+ using llvm::support::endian::read16be;
----------------
There's also a third copy of this code in ProcessMinidump (MinidumpParser.cpp).
I think it's time to factor this out somehow. We could either create a new file
in the Utility module, or put this functionality in directly inside the UUID
class -- I'm not currently sure what's better.
================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:172
+ lldb_private::UUID &uuid = module_spec.GetUUID();
+ if (!uuid.IsValid())
+ uuid = GetPDBUUID(*info_stream);
----------------
I'm pretty sure this check is bogus and can never be false.
================
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");
----------------
Could this use the same algorithm as `ObjectFilePDB::GetArchitecture` ?
================
Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:207-209
+Address ObjectFilePDB::GetBaseAddress() {
+ return Address(GetSectionList()->GetSectionAtIndex(0), 0);
+}
----------------
Leave this unimplemented. PDBs don't have sections and are not loaded into
memory, so the function does not apply.
================
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);
+}
----------------
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.
================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:145
return nullptr;
-
return objfile_up.release();
----------------
revert spurious changes.
================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:299
std::unique_ptr<PDBFile> file_up =
loadMatchingPDBFile(m_objfile_sp->GetFileSpec().GetPath(),
m_allocator);
----------------
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...
================
Comment at: lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp:14
+// Create lldbinit
+// RUN: echo -e "target symbols add %t/executable/bar.pdb\nquit" >
%t/load-pdb.lldbinit
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t/executable/foo.exe -s \
----------------
I'd just pass commands to lldb directly via `-o`.
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
