labath added a comment.
I don't know much about PDBs, but the code seems ok. I particularly like the
test case.
The one part that bothers me is the "two-phased" setting of line tables. IIUC,
the problem is that the inline line info is in the block descriptor, so you
need to parse block info in order to create a line table, but in order to
actually create an lldb_private::Block, you need the call site info from the
caller, which is in the pdb line table. This creates a sort of a dependency
loop, is that right?
Although, at the moment, I can't think of anything that would break in this
setup, this two-phase design (where you temporarily install an incomplete line
table) definitely seems like a code smell, and a possible future maintenance
burden. Is there any way around that? AFAICT, the "temporary" line table is
accessed only from `SymbolFileNativePDB::ParseInlineSite`. Could we maybe
change that function so that it reads the line table from some temporary
location, so that we only install a "real" line table once it is final? The
CompileUnit objects have a [GS]etUserData methods which can be used to store
opaque pointers. Maybe you could store it there? Or maybe inside the
`CompileUnitIndex` ?
================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp:116
+ llvm::BinaryStreamReader reader(ss.getRecordData());
+ if (auto ec = inlinee_lines.initialize(reader)) {
+ consumeError(std::move(ec));
----------------
We usually use `ec` for variables of type `std::error_code`. This is doubly
confusing since you have also used auto (whose usage does not fit the current
guidelines -- "use when the type is obvious from the context").
================
Comment at:
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:346-349
+ lldbassert(sym.kind() == S_BLOCK32 || sym.kind() == S_INLINESITE);
- // This is a block. Its parent is either a function or another block. In
- // either case, its parent can be viewed as a block (e.g. a function contains
- // 1 big block. So just get the parent block and add this block to it.
- BlockSym block(static_cast<SymbolRecordKind>(sym.kind()));
- cantFail(SymbolDeserializer::deserializeAs<BlockSym>(sym, block));
- lldbassert(block.Parent != 0);
- PdbCompilandSymId parent_id(block_id.modi, block.Parent);
- Block &parent_block = GetOrCreateBlock(parent_id);
- lldb::user_id_t opaque_block_uid = toOpaqueUid(block_id);
BlockSP child_block = std::make_shared<Block>(opaque_block_uid);
+ if (sym.kind() == S_BLOCK32) {
----------------
Use a switch and put the assert on the default branch?
================
Comment at:
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h:164-165
private:
+ // This is a copy of LineTable::Entry, used only in ParseLineTable to
+ // deduplicate line table entries.
+ struct LineTableEntry {
----------------
Would you be able to use `LineTable::Entry` out-of-the box if it was made
public? I don't see any strong reason for why it would have to remain hidden...
================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h:313
+ llvm::DenseMap<lldb::user_id_t, std::shared_ptr<InlineSite>> m_inline_sites;
+ // A map from file id in records to file index in supported files.
+ llvm::DenseMap<uint32_t, uint32_t> m_file_indexes;
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116845/new/
https://reviews.llvm.org/D116845
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
