[lldb] Make sure Blocks always have a parent

[labath]

It's basically true already (except for a brief time during construction). This patch makes sure the objects are constructed with a valid parent and enforces this in the type system, which allows us to get rid of some nullptr checks.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

It's basically true already (except for a brief time during construction). This patch makes sure the objects are constructed with a valid parent and enforces this in the type system, which allows us to get rid of some nullptr checks.

---

Full diff: https://github.com/llvm/llvm-project/pull/117683.diff

7 Files Affected:

• (modified) lldb/include/lldb/Symbol/Block.h (+11-23)
• (modified) lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp (+2-2)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+1-3)
• (modified) lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp (+2-4)
• (modified) lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp (+1-3)
• (modified) lldb/source/Symbol/Block.cpp (+18-29)
• (modified) lldb/source/Symbol/Function.cpp (+1-2)

diff --git a/lldb/include/lldb/Symbol/Block.h b/lldb/include/lldb/Symbol/Block.h
index c9c4d5ad767d7e..7c7a66de831998 100644
--- a/lldb/include/lldb/Symbol/Block.h
+++ b/lldb/include/lldb/Symbol/Block.h
@@ -43,11 +43,13 @@ class Block : public UserID, public SymbolContextScope {
   typedef RangeVector<uint32_t, uint32_t, 1> RangeList;
   typedef RangeList::Entry Range;
 
-  /// Construct with a User ID \a uid, \a depth.
-  ///
-  /// Initialize this block with the specified UID \a uid. The \a depth in the
-  /// \a block_list is used to represent the parent, sibling, and child block
-  /// information and also allows for partial parsing at the block level.
+  // Creates a block representing the whole function. Only meant to be used from
+  // the Function class.
+  Block(Function &function, lldb::user_id_t function_uid);
+
+  ~Block() override;
+
+  /// Creates a block with the specified UID \a uid.
   ///
   /// \param[in] uid
   ///     The UID for a given block. This value is given by the
@@ -56,19 +58,7 @@ class Block : public UserID, public SymbolContextScope {
   ///     information data that it parses for further or more in
   ///     depth parsing. Common values would be the index into a
   ///     table, or an offset into the debug information.
-  ///
-  /// \see BlockList
-  Block(lldb::user_id_t uid);
-
-  /// Destructor.
-  ~Block() override;
-
-  /// Add a child to this object.
-  ///
-  /// \param[in] child_block_sp
-  ///     A shared pointer to a child block that will get added to
-  ///     this block.
-  void AddChild(const lldb::BlockSP &child_block_sp);
+  lldb::BlockSP CreateChild(lldb::user_id_t uid);
 
   /// Add a new offset range to this block.
   void AddRange(const Range &range);
@@ -317,10 +307,6 @@ class Block : public UserID, public SymbolContextScope {
                               const Declaration *decl_ptr,
                               const Declaration *call_decl_ptr);
 
-  void SetParentScope(SymbolContextScope *parent_scope) {
-    m_parent_scope = parent_scope;
-  }
-
   /// Set accessor for the variable list.
   ///
   /// Called by the SymbolFile plug-ins after they have parsed the variable
@@ -364,7 +350,7 @@ class Block : public UserID, public SymbolContextScope {
 protected:
   typedef std::vector<lldb::BlockSP> collection;
   // Member variables.
-  SymbolContextScope *m_parent_scope;
+  SymbolContextScope &m_parent_scope;
   collection m_children;
   RangeList m_ranges;
   lldb::InlineFunctionInfoSP m_inlineInfoSP; ///< Inlined function information.
@@ -382,6 +368,8 @@ class Block : public UserID, public SymbolContextScope {
 private:
   Block(const Block &) = delete;
   const Block &operator=(const Block &) = delete;
+
+  Block(lldb::user_id_t uid, SymbolContextScope &parent_scope);
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
index fadc19676609bf..df3bf157278daf 100644
--- a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
+++ b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
@@ -315,7 +315,8 @@ size_t SymbolFileBreakpad::ParseBlocksRecursive(Function &func) {
       if (record->InlineNestLevel == 0 ||
           record->InlineNestLevel <= last_added_nest_level + 1) {
         last_added_nest_level = record->InlineNestLevel;
-        BlockSP block_sp = std::make_shared<Block>(It.GetBookmark().offset);
+        BlockSP block_sp = blocks[record->InlineNestLevel]->CreateChild(
+            It.GetBookmark().offset);
         FileSpec callsite_file;
         if (record->CallSiteFileNum < m_files->size())
           callsite_file = (*m_files)[record->CallSiteFileNum];
@@ -333,7 +334,6 @@ size_t SymbolFileBreakpad::ParseBlocksRecursive(Function &func) {
         }
         block_sp->FinalizeRanges();
 
-        blocks[record->InlineNestLevel]->AddChild(block_sp);
         if (record->InlineNestLevel + 1 >= blocks.size()) {
           blocks.resize(blocks.size() + 1);
         }
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index c900f330b481bb..fe711c56958c44 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1328,9 +1328,7 @@ size_t SymbolFileDWARF::ParseBlocksRecursive(
 
         block = parent_block;
       } else {
-        BlockSP block_sp(new Block(die.GetID()));
-        parent_block->AddChild(block_sp);
-        block = block_sp.get();
+        block = parent_block->CreateChild(die.GetID()).get();
       }
       DWARFRangeList ranges;
       const char *name = nullptr;
diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
index 0f77b2e28004eb..d17fedf26b4c48 100644
--- a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -424,7 +424,7 @@ Block *SymbolFileNativePDB::CreateBlock(PdbCompilandSymId block_id) {
         m_index->MakeVirtualAddress(block.Segment, block.CodeOffset);
     lldb::addr_t func_base =
         func->GetAddressRange().GetBaseAddress().GetFileAddress();
-    BlockSP child_block = std::make_shared<Block>(opaque_block_uid);
+    BlockSP child_block = parent_block->CreateChild(opaque_block_uid);
     if (block_base >= func_base)
       child_block->AddRange(Block::Range(block_base - func_base, block.CodeSize));
     else {
@@ -437,7 +437,6 @@ Block *SymbolFileNativePDB::CreateBlock(PdbCompilandSymId block_id) {
           block_id.modi, block_id.offset, block_base,
           block_base + block.CodeSize, func_base);
     }
-    parent_block->AddChild(child_block);
     ast_builder->GetOrCreateBlockDecl(block_id);
     m_blocks.insert({opaque_block_uid, child_block});
     break;
@@ -450,8 +449,7 @@ Block *SymbolFileNativePDB::CreateBlock(PdbCompilandSymId block_id) {
     Block *parent_block = GetOrCreateBlock(inline_site->parent_id);
     if (!parent_block)
       return nullptr;
-    BlockSP child_block = std::make_shared<Block>(opaque_block_uid);
-    parent_block->AddChild(child_block);
+    BlockSP child_block = parent_block->CreateChild(opaque_block_uid);
     ast_builder->GetOrCreateInlinedFunctionDecl(block_id);
     // Copy ranges from InlineSite to Block.
     for (size_t i = 0; i < inline_site->ranges.GetSize(); ++i) {
diff --git a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
index c7b23aedfdccac..4935b0fbdfd877 100644
--- a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -421,9 +421,7 @@ static size_t ParseFunctionBlocksForPDBSymbol(
       if (raw_sym.getVirtualAddress() < func_file_vm_addr)
         break;
 
-      auto block_sp = std::make_shared<Block>(pdb_symbol->getSymIndexId());
-      parent_block->AddChild(block_sp);
-      block = block_sp.get();
+      block = parent_block->CreateChild(pdb_symbol->getSymIndexId()).get();
     } else
       llvm_unreachable("Unexpected PDB symbol!");
 
diff --git a/lldb/source/Symbol/Block.cpp b/lldb/source/Symbol/Block.cpp
index 8746a25e3fad5a..5cc87240f552ad 100644
--- a/lldb/source/Symbol/Block.cpp
+++ b/lldb/source/Symbol/Block.cpp
@@ -21,9 +21,11 @@
 using namespace lldb;
 using namespace lldb_private;
 
-Block::Block(lldb::user_id_t uid)
-    : UserID(uid), m_parent_scope(nullptr), m_children(), m_ranges(),
-      m_inlineInfoSP(), m_variable_list_sp(), m_parsed_block_info(false),
+Block::Block(Function &function, user_id_t function_uid)
+    : Block(function_uid, function) {}
+
+Block::Block(lldb::user_id_t uid, SymbolContextScope &parent_scope)
+    : UserID(uid), m_parent_scope(parent_scope), m_parsed_block_info(false),
       m_parsed_block_variables(false), m_parsed_child_blocks(false) {}
 
 Block::~Block() = default;
@@ -134,27 +136,20 @@ Block *Block::FindInnermostBlockByOffset(const lldb::addr_t offset) {
 }
 
 void Block::CalculateSymbolContext(SymbolContext *sc) {
-  if (m_parent_scope)
-    m_parent_scope->CalculateSymbolContext(sc);
+  m_parent_scope.CalculateSymbolContext(sc);
   sc->block = this;
 }
 
 lldb::ModuleSP Block::CalculateSymbolContextModule() {
-  if (m_parent_scope)
-    return m_parent_scope->CalculateSymbolContextModule();
-  return lldb::ModuleSP();
+  return m_parent_scope.CalculateSymbolContextModule();
 }
 
 CompileUnit *Block::CalculateSymbolContextCompileUnit() {
-  if (m_parent_scope)
-    return m_parent_scope->CalculateSymbolContextCompileUnit();
-  return nullptr;
+  return m_parent_scope.CalculateSymbolContextCompileUnit();
 }
 
 Function *Block::CalculateSymbolContextFunction() {
-  if (m_parent_scope)
-    return m_parent_scope->CalculateSymbolContextFunction();
-  return nullptr;
+  return m_parent_scope.CalculateSymbolContextFunction();
 }
 
 Block *Block::CalculateSymbolContextBlock() { return this; }
@@ -200,9 +195,7 @@ bool Block::Contains(const Range &range) const {
 }
 
 Block *Block::GetParent() const {
-  if (m_parent_scope)
-    return m_parent_scope->CalculateSymbolContextBlock();
-  return nullptr;
+  return m_parent_scope.CalculateSymbolContextBlock();
 }
 
 Block *Block::GetContainingInlinedBlock() {
@@ -354,8 +347,8 @@ void Block::AddRange(const Range &range) {
   if (parent_block && !parent_block->Contains(range)) {
     Log *log = GetLog(LLDBLog::Symbols);
     if (log) {
-      ModuleSP module_sp(m_parent_scope->CalculateSymbolContextModule());
-      Function *function = m_parent_scope->CalculateSymbolContextFunction();
+      ModuleSP module_sp(m_parent_scope.CalculateSymbolContextModule());
+      Function *function = m_parent_scope.CalculateSymbolContextFunction();
       const addr_t function_file_addr =
           function->GetAddressRange().GetBaseAddress().GetFileAddress();
       const addr_t block_start_addr = function_file_addr + range.GetRangeBase();
@@ -399,11 +392,9 @@ size_t Block::MemorySize() const {
   return mem_size;
 }
 
-void Block::AddChild(const BlockSP &child_block_sp) {
-  if (child_block_sp) {
-    child_block_sp->SetParentScope(this);
-    m_children.push_back(child_block_sp);
-  }
+BlockSP Block::CreateChild(user_id_t uid) {
+  m_children.push_back(std::shared_ptr<Block>(new Block(uid, *this)));
+  return m_children.back();
 }
 
 void Block::SetInlinedFunctionInfo(const char *name, const char *mangled,
@@ -520,13 +511,11 @@ void Block::SetDidParseVariables(bool b, bool set_children) {
 }
 
 Block *Block::GetSibling() const {
-  if (m_parent_scope) {
-    Block *parent_block = GetParent();
-    if (parent_block)
-      return parent_block->GetSiblingForChild(this);
-  }
+  if (Block *parent_block = GetParent())
+    return parent_block->GetSiblingForChild(this);
   return nullptr;
 }
+
 // A parent of child blocks can be asked to find a sibling block given
 // one of its child blocks
 Block *Block::GetSiblingForChild(const Block *child_block) const {
diff --git a/lldb/source/Symbol/Function.cpp b/lldb/source/Symbol/Function.cpp
index 0186d63e72879c..b346749ca06ec3 100644
--- a/lldb/source/Symbol/Function.cpp
+++ b/lldb/source/Symbol/Function.cpp
@@ -278,10 +278,9 @@ Function::Function(CompileUnit *comp_unit, lldb::user_id_t func_uid,
                    lldb::user_id_t type_uid, const Mangled &mangled, Type *type,
                    AddressRanges ranges)
     : UserID(func_uid), m_comp_unit(comp_unit), m_type_uid(type_uid),
-      m_type(type), m_mangled(mangled), m_block(func_uid),
+      m_type(type), m_mangled(mangled), m_block(*this, func_uid),
       m_ranges(std::move(ranges)), m_range(CollapseRanges(m_ranges)),
       m_frame_base(), m_flags(), m_prologue_byte_size(0) {
-  m_block.SetParentScope(this);
   assert(comp_unit != nullptr);
 }
 

[DavidSpickett]

Some small things you can address if you want but otherwise LGTM.