[lldb] Refactor variable parsing in DWARF symbol file

[jarin]

Summary

This is in preparation for a fix of parsing omitted abstract
parameters of inlined functions (see the bug
https://bugs.llvm.org/show_bug.cgi?id=50076#c6 for more details). The
main idea is to move the recursive parsing of variables in a function
context into a separate method (ParseVariablesInFunctionContext).

We also move the setup of block variable list to the DIE node that
starts the block rather than setting up the list when processing
the first variable DIE in that block.

[jarin]

Hi, could you possibly take a look at this change?

The main motivation for this patch is to move the setup of the variable list for parsing children to the DIE node that corresponds to the block containing the children. This will be particularly important for DW_TAG_inlined_subroutine because after a recent clang change (https://reviews.llvm.org/D95617), we can have inlined subroutines which contain no parameters (and thus no children) in the concrete DIE block, but still have some parameters in the abstract function (referenced by the inlined subroutine's DW_AT_abstract_origin attribute). If we started building the variable list when parsing the first variable in a function, we would never create the variable lists for the abstract parameters if they were all omitted.

Note that this change is just a refactoring for the actual change that will introduce parsing of the omitted abstract parameters. The actual change will be uploaded shortly.

[shafik]

Just a nitpick.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3606

/*can_create=*/true

3615

/*can_create=*/true

[jarin]

Thank you for taking a look, shafik@. I hope you don't mind if I address your comments later, once a full review arrives from Pavel (or Johannes).

[labath]

The patch seems reasonable to me, and is an improvement over the status quo.

The thing I'm left wondering is the lambda in ParseVariablesInFunctionContext. It covers like 95% of the enclosing function. Could we just make a separate method for that?

[jarin]

Separated ParseVariablesInFunctionContext's lambda into a method.

[jarin]

Added the /*can_create=*/ comments.

[jarin]

Thank you for the review! Does the separate method look more reasonable?

[jarin]

Avoid nullptr deref/ref.

[jarin]

Pavel, could you take another look? Interestingly the null-deref problem was already fixed in the follow-up patch, maybe I just did not land quickly enough :-)).

[labath]

looks good

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

lldb_private::Type *ResolveTypeUID(const DWARFDIE &die,
                                      bool assert_not_being_parsed);
 
   lldb_private::Type *ResolveTypeUID(const DIERef &die_ref);
 
   lldb::VariableSP ParseVariableDIE(const lldb_private::SymbolContext &sc,
                                     const DWARFDIE &die,
                                     const lldb::addr_t func_low_pc);
 
-  size_t ParseVariables(const lldb_private::SymbolContext &sc,
-                        const DWARFDIE &orig_die,
-                        const lldb::addr_t func_low_pc, bool parse_siblings,
-                        bool parse_children,
-                        lldb_private::VariableList *cc_variable_list = nullptr);
+  void
+  ParseAndAppendGlobalVariable(const lldb_private::SymbolContext &sc,
+                               const DWARFDIE &die,
+                               lldb_private::VariableList &cc_variable_list);
+
+  size_t ParseVariablesInFunctionContext(const lldb_private::SymbolContext &sc,
+                                         const DWARFDIE &die,
+                                         const lldb::addr_t func_low_pc);
+
+  size_t ParseVariablesInFunctionContextRecursive(
+      const lldb_private::SymbolContext &sc, const DWARFDIE &die,
+      const lldb::addr_t func_low_pc,
+      lldb_private::VariableList &variable_list);
 
   bool ClassOrStructIsVirtual(const DWARFDIE &die);
 
   // Given a die_offset, figure out the symbol context representing that die.
   bool ResolveFunction(const DWARFDIE &die, bool include_inlines,
                        lldb_private::SymbolContextList &sc_list);
 
   /// Resolve functions and (possibly) blocks for the given file address and a

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

if (parent_decl_ctx) {
         CompilerDeclContext actual_parent_decl_ctx =
             dwarf_ast->GetDeclContextContainingUIDFromDWARF(die);
         if (!actual_parent_decl_ctx ||
             actual_parent_decl_ctx != parent_decl_ctx)
           return true;
       }
     }
 
-    ParseVariables(sc, die, LLDB_INVALID_ADDRESS, false, false, &variables);
+    ParseAndAppendGlobalVariable(sc, die, variables);
     while (pruned_idx < variables.GetSize()) {
       VariableSP var_sp = variables.GetVariableAtIndex(pruned_idx);
       if (name_is_mangled ||
           var_sp->GetName().GetStringRef().contains(name.GetStringRef()))
         ++pruned_idx;
       else
         variables.RemoveVariableAtIndex(pruned_idx);
     }
if (!sc.module_sp)
       sc.module_sp = m_objfile_sp->GetModule();
     assert(sc.module_sp);
 
     DWARFCompileUnit *dwarf_cu = llvm::dyn_cast<DWARFCompileUnit>(die.GetCU());
     if (!dwarf_cu)
       return true;
     sc.comp_unit = GetCompUnitForDWARFCompUnit(*dwarf_cu);
 
-    ParseVariables(sc, die, LLDB_INVALID_ADDRESS, false, false, &variables);
+    ParseAndAppendGlobalVariable(sc, die, variables);
 
     return variables.GetSize() - original_size < max_matches;
   });
 }
 
 bool SymbolFileDWARF::ResolveFunction(const DWARFDIE &orig_die,
                                       bool include_inlines,
                                       SymbolContextList &sc_list) {
if (sc.function) {
 
       dw_addr_t func_lo_pc = LLDB_INVALID_ADDRESS;
       DWARFRangeList ranges;
       if (function_die.GetDIE()->GetAttributeAddressRanges(
               function_die.GetCU(), ranges,
               /*check_hi_lo_pc=*/true))
         func_lo_pc = ranges.GetMinRangeBase(0);
       if (func_lo_pc != LLDB_INVALID_ADDRESS) {
-        const size_t num_variables = ParseVariables(
-            sc, function_die.GetFirstChild(), func_lo_pc, true, true);
+        const size_t num_variables =
+            ParseVariablesInFunctionContext(sc, function_die, func_lo_pc);
 
         // Let all blocks know they have parse all their variables
         sc.function->GetBlock(false).SetDidParseVariables(true, true);
         return num_variables;
       }
     } else if (sc.comp_unit) {
       DWARFUnit *dwarf_cu = DebugInfo().GetUnitAtIndex(sc.comp_unit->GetID());
 
for (DWARFDIE child_die : die.children()) {
       if (result_die)
         return result_die;
     }
   }
 
   return DWARFDIE();
 }
 
-size_t SymbolFileDWARF::ParseVariables(const SymbolContext &sc,
-                                       const DWARFDIE &orig_die,
-                                       const lldb::addr_t func_low_pc,
-                                       bool parse_siblings, bool parse_children,
-                                       VariableList *cc_variable_list) {
-  if (!orig_die)
-    return 0;
-
-  VariableListSP variable_list_sp;
+void SymbolFileDWARF::ParseAndAppendGlobalVariable(
+    const SymbolContext &sc, const DWARFDIE &die,
+    VariableList &cc_variable_list) {
+  if (!die)
+    return;
 
-  size_t vars_added = 0;
-  DWARFDIE die = orig_die;
-  while (die) {
-    dw_tag_t tag = die.Tag();
+  if (tag != DW_TAG_variable && tag != DW_TAG_constant)
+    return;
 
-    // Check to see if we have already parsed this variable or constant?
-    VariableSP var_sp = GetDIEToVariable()[die.GetDIE()];
-    if (var_sp) {
-      if (cc_variable_list)
-        cc_variable_list->AddVariableIfUnique(var_sp);
-    } else {
-      // We haven't already parsed it, lets do that now.
-      if ((tag == DW_TAG_variable) || (tag == DW_TAG_constant) ||
-          (tag == DW_TAG_formal_parameter && sc.function)) {
-        if (variable_list_sp.get() == nullptr) {
-          DWARFDIE sc_parent_die = GetParentSymbolContextDIE(orig_die);
-          dw_tag_t parent_tag = sc_parent_die.Tag();
-          switch (parent_tag) {
-          case DW_TAG_compile_unit:
-          case DW_TAG_partial_unit:
-            if (sc.comp_unit != nullptr) {
-              variable_list_sp = sc.comp_unit->GetVariableList(false);
-              if (variable_list_sp.get() == nullptr) {
-                variable_list_sp = std::make_shared<VariableList>();
-              }
-            } else {
-              GetObjectFile()->GetModule()->ReportError(
-                  "parent 0x%8.8" PRIx64 " %s with no valid compile unit in "
-                  "symbol context for 0x%8.8" PRIx64 " %s.\n",
-                  sc_parent_die.GetID(), sc_parent_die.GetTagAsCString(),
-                  orig_die.GetID(), orig_die.GetTagAsCString());
-            }
-            break;
-
-          case DW_TAG_subprogram:
-          case DW_TAG_inlined_subroutine:
-          case DW_TAG_lexical_block:
-            if (sc.function != nullptr) {
-              // Check to see if we already have parsed the variables for the
-              // given scope
-
-              Block *block = sc.function->GetBlock(true).FindBlockByID(
-                  sc_parent_die.GetID());
-              if (block == nullptr) {
-                // This must be a specification or abstract origin with a
-                // concrete block counterpart in the current function. We need
-                // to find the concrete block so we can correctly add the
-                // variable to it
-                const DWARFDIE concrete_block_die =
-                    FindBlockContainingSpecification(
-                        GetDIE(sc.function->GetID()),
-                        sc_parent_die.GetOffset());
-                if (concrete_block_die)
-                  block = sc.function->GetBlock(true).FindBlockByID(
-                      concrete_block_die.GetID());
-              }
-
-              if (block != nullptr) {
-                const bool can_create = false;
-                variable_list_sp = block->GetBlockVariableList(can_create);
-                if (variable_list_sp.get() == nullptr) {
-                  variable_list_sp = std::make_shared<VariableList>();
-                  block->SetVariableList(variable_list_sp);
-                }
-              }
-            }
-            break;
+    cc_variable_list.AddVariableIfUnique(var_sp);
+    return;
+  }
+
+  VariableListSP variable_list_sp;
+  DWARFDIE sc_parent_die = GetParentSymbolContextDIE(die);
+          sc_parent_die.GetID(), sc_parent_die.GetTagAsCString(), die.GetID(),
+          die.GetTagAsCString());
+      return;
 
-          default:
-            GetObjectFile()->GetModule()->ReportError(
-                "didn't find appropriate parent DIE for variable list for "
-                "0x%8.8" PRIx64 " %s.\n",
-                orig_die.GetID(), orig_die.GetTagAsCString());
-            break;
-          }
-        }
+        die.GetID(), die.GetTagAsCString());
+    return;
+
+  var_sp = ParseVariableDIE(sc, die, LLDB_INVALID_ADDRESS);
+  if (!var_sp)
+    return;
+
+  cc_variable_list.AddVariableIfUnique(var_sp);
+  if (variable_list_sp)
+    variable_list_sp->AddVariableIfUnique(var_sp);
+}
+
+size_t SymbolFileDWARF::ParseVariablesInFunctionContext(
+    const SymbolContext &sc, const DWARFDIE &die,
+    const lldb::addr_t func_low_pc) {
+  if (!die || !sc.function)
+    return 0;
+
+  Block *block =
+      sc.function->GetBlock(/*can_create=*/true).FindBlockByID(die.GetID());
+  const bool can_create = false;
+  VariableListSP variable_list_sp = block->GetBlockVariableList(can_create);
+  return ParseVariablesInFunctionContextRecursive(sc, die, func_low_pc,
+                                                  *variable_list_sp);
 
-        if (variable_list_sp) {
-          VariableSP var_sp(ParseVariableDIE(sc, die, func_low_pc));
-          if (var_sp) {
-            variable_list_sp->AddVariableIfUnique(var_sp);
-            if (cc_variable_list)
-              cc_variable_list->AddVariableIfUnique(var_sp);
-            ++vars_added;
-          }
-        }
-      }
+size_t SymbolFileDWARF::ParseVariablesInFunctionContextRecursive(
+    const lldb_private::SymbolContext &sc, const DWARFDIE &die,
+    const lldb::addr_t func_low_pc, VariableList &variable_list) {
+  size_t vars_added = 0;
+  dw_tag_t tag = die.Tag();
+
+  if ((tag == DW_TAG_variable) || (tag == DW_TAG_constant) ||
+      (tag == DW_TAG_formal_parameter)) {
+      variable_list.AddVariableIfUnique(var_sp);
+
+  switch (tag) {
+  case DW_TAG_subprogram:
+  case DW_TAG_inlined_subroutine:
+  case DW_TAG_lexical_block: {
+    // If we start a new block, compute a new block variable list and recurse.
+    Block *block =
+        sc.function->GetBlock(/*can_create=*/true).FindBlockByID(die.GetID());
+    if (block == nullptr) {
+      // This must be a specification or abstract origin with a
+      // concrete block counterpart in the current function. We need
+      // to find the concrete block so we can correctly add the
+      // variable to it.
+      const DWARFDIE concrete_block_die = FindBlockContainingSpecification(
+          GetDIE(sc.function->GetID()), die.GetOffset());
+      if (concrete_block_die)
+        block = sc.function->GetBlock(/*can_create=*/true)
+                    .FindBlockByID(concrete_block_die.GetID());
+
+    if (block == nullptr)
+      return 0;
+
+    const bool can_create = false;
+    VariableListSP block_variable_list_sp =
+        block->GetBlockVariableList(can_create);
+    if (block_variable_list_sp.get() == nullptr) {
+      block_variable_list_sp = std::make_shared<VariableList>();
+      block->SetVariableList(block_variable_list_sp);
+    }
+    for (DWARFDIE child = die.GetFirstChild(); child;
+         child = child.GetSibling()) {
+      vars_added += ParseVariablesInFunctionContextRecursive(
+          sc, child, func_low_pc, *block_variable_list_sp);
     }
 
-    bool skip_children = (sc.function == nullptr && tag == DW_TAG_subprogram);
+    break;
+  }
 
-    if (!skip_children && parse_children && die.HasChildren()) {
-      vars_added += ParseVariables(sc, die.GetFirstChild(), func_low_pc, true,
-                                   true, cc_variable_list);
+  default:
+    // Recurse to children with the same variable list.
+    for (DWARFDIE child = die.GetFirstChild(); child;
+         child = child.GetSibling()) {
+      vars_added += ParseVariablesInFunctionContextRecursive(
+          sc, child, func_low_pc, variable_list);
     }
 
-    if (parse_siblings)
-      die = die.GetSibling();
-    else
-      die.Clear();
+    break;
   }
+
   return vars_added;
 }
 
 /// Collect call site parameters in a DW_TAG_call_site DIE.
 static CallSiteParameterArray
 CollectCallSiteParameters(ModuleSP module, DWARFDIE call_site_die) {
   CallSiteParameterArray parameters;
   for (DWARFDIE child : call_site_die.children()) {
     if (child.Tag() != DW_TAG_call_site_parameter &&