[Lldb-commits] [PATCH] D116845: [LLDB][NativePDB] Add support for inlined functions
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Jan 10 02:23:48 PST 2022
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
More information about the lldb-commits
mailing list