[Lldb-commits] [PATCH] D116845: [LLDB][NativePDB] Add support for inlined functions

[Pavel Labath via Phabricator via lldb-commits]

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