[lldb] Add omitted abstract formal parameters in DWARF symbol files

This patch fixes a problem introduced by clang change
https://reviews.llvm.org/D95617 and described by
https://bugs.llvm.org/show_bug.cgi?id=50076#c6, where inlined functions
omit unused parameters both in the stack trace and in `frame var`
command. With this patch, the parameters are listed correctly in the
stack trace and in `frame var` command.

Specifically, we parse formal parameters from the abstract version of
inlined functions and use those formal parameters if they are missing
from the concrete version.

Differential Revision: https://reviews.llvm.org/D110571

Author: jaro-sevcik

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

@@ -3092,7 +3092,7 @@ size_t SymbolFileDWARF::ParseVariablesForContext(const SymbolContext &sc) {
         sc.comp_unit->SetVariableList(variables);
 
         m_index->GetGlobalVariables(*dwarf_cu, [&](DWARFDIE die) {
-          VariableSP var_sp(ParseVariableDIE(sc, die, LLDB_INVALID_ADDRESS));
+          VariableSP var_sp(ParseVariableDIECached(sc, die));
           if (var_sp) {
             variables->AddVariableIfUnique(var_sp);
             ++vars_added;
@@ -3106,6 +3106,26 @@ size_t SymbolFileDWARF::ParseVariablesForContext(const SymbolContext &sc) {
   return 0;
 }
 
+VariableSP SymbolFileDWARF::ParseVariableDIECached(const SymbolContext &sc,
+                                                   const DWARFDIE &die) {
+  if (!die)
+    return nullptr;
+
+  DIEToVariableSP &die_to_variable = die.GetDWARF()->GetDIEToVariable();
+
+  VariableSP var_sp = die_to_variable[die.GetDIE()];
+  if (var_sp)
+    return var_sp;
+
+  var_sp = ParseVariableDIE(sc, die, LLDB_INVALID_ADDRESS);
+  if (var_sp) {
+    die_to_variable[die.GetDIE()] = var_sp;
+    if (DWARFDIE spec_die = die.GetReferencedDIE(DW_AT_specification))
+      die_to_variable[spec_die.GetDIE()] = var_sp;
+  }
+  return var_sp;
+}
+
 VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
                                              const DWARFDIE &die,
                                              const lldb::addr_t func_low_pc) {
@@ -3115,9 +3135,6 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
   if (!die)
     return nullptr;
 
-  if (VariableSP var_sp = GetDIEToVariable()[die.GetDIE()])
-    return var_sp; // Already been parsed!
-
   const dw_tag_t tag = die.Tag();
   ModuleSP module = GetObjectFile()->GetModule();
 
@@ -3127,8 +3144,6 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
 
   DWARFAttributes attributes;
   const size_t num_attributes = die.GetAttributes(attributes);
-  DWARFDIE spec_die;
-  VariableSP var_sp;
   const char *name = nullptr;
   const char *mangled = nullptr;
   Declaration decl;
@@ -3175,9 +3190,6 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
     case DW_AT_location:
       location_form = form_value;
       break;
-    case DW_AT_specification:
-      spec_die = form_value.Reference();
-      break;
     case DW_AT_start_scope:
       // TODO: Implement this.
       break;
@@ -3188,6 +3200,7 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
     case DW_AT_description:
     case DW_AT_endianity:
     case DW_AT_segment:
+    case DW_AT_specification:
     case DW_AT_visibility:
     default:
     case DW_AT_abstract_origin:
@@ -3380,7 +3393,7 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
             location.Update_DW_OP_addr(exe_file_addr);
           } else {
             // Variable didn't make it into the final executable
-            return var_sp;
+            return nullptr;
           }
         }
       }
@@ -3426,35 +3439,25 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
     }
   }
 
-  if (symbol_context_scope) {
-    auto type_sp = std::make_shared<SymbolFileType>(
-        *this, GetUID(type_die_form.Reference()));
-
-    if (use_type_size_for_value && type_sp->GetType())
-      location.UpdateValue(
-          const_value_form.Unsigned(),
-          type_sp->GetType()->GetByteSize(nullptr).getValueOr(0),
-          die.GetCU()->GetAddressByteSize());
-
-    var_sp = std::make_shared<Variable>(
-        die.GetID(), name, mangled, type_sp, scope, symbol_context_scope,
-        scope_ranges, &decl, location, is_external, is_artificial,
-        location_is_const_value_data, is_static_member);
-  } else {
+  if (!symbol_context_scope) {
     // Not ready to parse this variable yet. It might be a global or static
     // variable that is in a function scope and the function in the symbol
     // context wasn't filled in yet
-    return var_sp;
+    return nullptr;
   }
-  // Cache var_sp even if NULL (the variable was just a specification or was
-  // missing vital information to be able to be displayed in the debugger
-  // (missing location due to optimization, etc)) so we don't re-parse this
-  // DIE over and over later...
-  GetDIEToVariable()[die.GetDIE()] = var_sp;
-  if (spec_die)
-    GetDIEToVariable()[spec_die.GetDIE()] = var_sp;
 
-  return var_sp;
+  auto type_sp = std::make_shared<SymbolFileType>(
+      *this, GetUID(type_die_form.Reference()));
+
+  if (use_type_size_for_value && type_sp->GetType())
+    location.UpdateValue(const_value_form.Unsigned(),
+                         type_sp->GetType()->GetByteSize(nullptr).getValueOr(0),
+                         die.GetCU()->GetAddressByteSize());
+
+  return std::make_shared<Variable>(
+      die.GetID(), name, mangled, type_sp, scope, symbol_context_scope,
+      scope_ranges, &decl, location, is_external, is_artificial,
+      location_is_const_value_data, is_static_member);
 }
 
 DWARFDIE
@@ -3518,7 +3521,9 @@ void SymbolFileDWARF::ParseAndAppendGlobalVariable(
     return;
   }
 
-  // We haven't already parsed it, lets do that now.
+  // We haven't parsed the variable yet, lets do that now. Also, let us include
+  // the variable in the relevant compilation unit's variable list, if it
+  // exists.
   VariableListSP variable_list_sp;
   DWARFDIE sc_parent_die = GetParentSymbolContextDIE(die);
   dw_tag_t parent_tag = sc_parent_die.Tag();
@@ -3545,7 +3550,7 @@ void SymbolFileDWARF::ParseAndAppendGlobalVariable(
     return;
   }
 
-  var_sp = ParseVariableDIE(sc, die, LLDB_INVALID_ADDRESS);
+  var_sp = ParseVariableDIECached(sc, die);
   if (!var_sp)
     return;
 
@@ -3554,32 +3559,109 @@ void SymbolFileDWARF::ParseAndAppendGlobalVariable(
     variable_list_sp->AddVariableIfUnique(var_sp);
 }
 
+DIEArray
+SymbolFileDWARF::MergeBlockAbstractParameters(const DWARFDIE &block_die,
+                                              DIEArray &&variable_dies) {
+  // DW_TAG_inline_subroutine objects may omit DW_TAG_formal_parameter in
+  // instances of the function when they are unused (i.e., the parameter's
+  // location list would be empty). The current DW_TAG_inline_subroutine may
+  // refer to another DW_TAG_subprogram that might actually have the definitions
+  // of the parameters and we need to include these so they show up in the
+  // variables for this function (for example, in a stack trace). Let us try to
+  // find the abstract subprogram that might contain the parameter definitions
+  // and merge with the concrete parameters.
+
+  // Nothing to merge if the block is not an inlined function.
+  if (block_die.Tag() != DW_TAG_inlined_subroutine) {
+    return std::move(variable_dies);
+  }
+
+  // Nothing to merge if the block does not have abstract parameters.
+  DWARFDIE abs_die = block_die.GetReferencedDIE(DW_AT_abstract_origin);
+  if (!abs_die || abs_die.Tag() != DW_TAG_subprogram ||
+      !abs_die.HasChildren()) {
+    return std::move(variable_dies);
+  }
+
+  // For each abstract parameter, if we have its concrete counterpart, insert
+  // it. Otherwise, insert the abstract parameter.
+  DIEArray::iterator concrete_it = variable_dies.begin();
+  DWARFDIE abstract_child = abs_die.GetFirstChild();
+  DIEArray merged;
+  bool did_merge_abstract = false;
+  for (; abstract_child; abstract_child = abstract_child.GetSibling()) {
+    if (abstract_child.Tag() == DW_TAG_formal_parameter) {
+      if (concrete_it == variable_dies.end() ||
+          GetDIE(*concrete_it).Tag() != DW_TAG_formal_parameter) {
+        // We arrived at the end of the concrete parameter list, so all
+        // the remaining abstract parameters must have been omitted.
+        // Let us insert them to the merged list here.
+        merged.push_back(*abstract_child.GetDIERef());
+        did_merge_abstract = true;
+        continue;
+      }
+
+      DWARFDIE origin_of_concrete =
+          GetDIE(*concrete_it).GetReferencedDIE(DW_AT_abstract_origin);
+      if (origin_of_concrete == abstract_child) {
+        // The current abstract paramater is the origin of the current
+        // concrete parameter, just push the concrete parameter.
+        merged.push_back(*concrete_it);
+        ++concrete_it;
+      } else {
+        // Otherwise, the parameter must have been omitted from the concrete
+        // function, so insert the abstract one.
+        merged.push_back(*abstract_child.GetDIERef());
+        did_merge_abstract = true;
+      }
+    }
+  }
+
+  // Shortcut if no merging happened.
+  if (!did_merge_abstract)
+    return std::move(variable_dies);
+
+  // We inserted all the abstract parameters (or their concrete counterparts).
+  // Let us insert all the remaining concrete variables to the merged list.
+  // During the insertion, let us check there are no remaining concrete
+  // formal parameters. If that's the case, then just bailout from the merge -
+  // the variable list is malformed.
+  for (; concrete_it != variable_dies.end(); ++concrete_it) {
+    if (GetDIE(*concrete_it).Tag() == DW_TAG_formal_parameter) {
+      return std::move(variable_dies);
+    }
+    merged.push_back(*concrete_it);
+  }
+  return std::move(merged);
+}
+
 size_t SymbolFileDWARF::ParseVariablesInFunctionContext(
     const SymbolContext &sc, const DWARFDIE &die,
     const lldb::addr_t func_low_pc) {
   if (!die || !sc.function)
     return 0;
 
-  VariableList empty_variable_list;
-  // Since |die| corresponds to a Block instance, the recursive call will get
-  // a variable list from the block. |empty_variable_list| should remain empty.
+  DIEArray dummy_block_variables; // The recursive call should not add anything
+                                  // to this vector because |die| should be a
+                                  // subprogram, so all variables will be added
+                                  // to the subprogram's list.
   return ParseVariablesInFunctionContextRecursive(sc, die, func_low_pc,
-                                                  empty_variable_list);
+                                                  dummy_block_variables);
 }
 
+// This method parses all the variables in the blocks in the subtree of |die|,
+// and inserts them to the variable list for all the nested blocks.
+// The uninserted variables for the current block are accumulated in
+// |accumulator|.
 size_t SymbolFileDWARF::ParseVariablesInFunctionContextRecursive(
     const lldb_private::SymbolContext &sc, const DWARFDIE &die,
-    const lldb::addr_t func_low_pc, VariableList &variable_list) {
+    lldb::addr_t func_low_pc, DIEArray &accumulator) {
   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)) {
-    VariableSP var_sp(ParseVariableDIE(sc, die, func_low_pc));
-    if (var_sp) {
-      variable_list.AddVariableIfUnique(var_sp);
-      ++vars_added;
-    }
+    accumulator.push_back(*die.GetDIERef());
   }
 
   switch (tag) {
@@ -3611,29 +3693,45 @@ size_t SymbolFileDWARF::ParseVariablesInFunctionContextRecursive(
       block_variable_list_sp = std::make_shared<VariableList>();
       block->SetVariableList(block_variable_list_sp);
     }
+
+    DIEArray block_variables;
     for (DWARFDIE child = die.GetFirstChild(); child;
          child = child.GetSibling()) {
       vars_added += ParseVariablesInFunctionContextRecursive(
-          sc, child, func_low_pc, *block_variable_list_sp);
+          sc, child, func_low_pc, block_variables);
     }
-
+    block_variables =
+        MergeBlockAbstractParameters(die, std::move(block_variables));
+    vars_added += PopulateBlockVariableList(*block_variable_list_sp, sc,
+                                            block_variables, func_low_pc);
     break;
   }
 
   default:
-    // Recurse to children with the same variable list.
+    // Recurse to children with the same variable accumulator.
     for (DWARFDIE child = die.GetFirstChild(); child;
          child = child.GetSibling()) {
       vars_added += ParseVariablesInFunctionContextRecursive(
-          sc, child, func_low_pc, variable_list);
+          sc, child, func_low_pc, accumulator);
     }
-
     break;
   }
 
   return vars_added;
 }
 
+size_t SymbolFileDWARF::PopulateBlockVariableList(
+    VariableList &variable_list, const lldb_private::SymbolContext &sc,
+    llvm::ArrayRef<DIERef> variable_dies, lldb::addr_t func_low_pc) {
+  // Parse the variable DIEs and insert them to the list.
+  for (auto &die : variable_dies) {
+    if (VariableSP var_sp = ParseVariableDIE(sc, GetDIE(die), func_low_pc)) {
+      variable_list.AddVariableIfUnique(var_sp);
+    }
+  }
+  return variable_dies.size();
+}
+
 /// Collect call site parameters in a DW_TAG_call_site DIE.
 static CallSiteParameterArray
 CollectCallSiteParameters(ModuleSP module, DWARFDIE call_site_die) {

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

@@ -379,6 +379,8 @@ class SymbolFileDWARF : public lldb_private::SymbolFile,
   lldb::VariableSP ParseVariableDIE(const lldb_private::SymbolContext &sc,
                                     const DWARFDIE &die,
                                     const lldb::addr_t func_low_pc);
+  lldb::VariableSP ParseVariableDIECached(const lldb_private::SymbolContext &sc,
+                                          const DWARFDIE &die);
 
   void
   ParseAndAppendGlobalVariable(const lldb_private::SymbolContext &sc,
@@ -391,8 +393,15 @@ class SymbolFileDWARF : public lldb_private::SymbolFile,
 
   size_t ParseVariablesInFunctionContextRecursive(
       const lldb_private::SymbolContext &sc, const DWARFDIE &die,
-      const lldb::addr_t func_low_pc,
-      lldb_private::VariableList &variable_list);
+      lldb::addr_t func_low_pc, DIEArray &accumulator);
+
+  size_t PopulateBlockVariableList(lldb_private::VariableList &variable_list,
+                                   const lldb_private::SymbolContext &sc,
+                                   llvm::ArrayRef<DIERef> variable_dies,
+                                   lldb::addr_t func_low_pc);
+
+  DIEArray MergeBlockAbstractParameters(const DWARFDIE &block_die,
+                                        DIEArray &&variable_dies);
 
   bool ClassOrStructIsVirtual(const DWARFDIE &die);
 

lldb/test/API/functionalities/unused-inlined-parameters/Makefile

@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -O1
+
+include Makefile.rules

lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py

@@ -0,0 +1,22 @@
+"""
+Test that unused inlined parameters are displayed.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestUnusedInlinedParameters(TestBase):
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test_unused_inlined_parameters(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.c"))
+
+        # For the unused parameters, only check the types.
+        self.assertIn("(void *) unused1 = <no location, value may have been optimized out>",
+                      lldbutil.get_description(self.frame().FindVariable("unused1")))
+        self.assertEqual(42, self.frame().FindVariable("used").GetValueAsUnsigned())
+        self.assertIn("(int) unused2 = <no location, value may have been optimized out>",
+                      lldbutil.get_description(self.frame().FindVariable("unused2")))

lldb/test/API/functionalities/unused-inlined-parameters/main.c

@@ -0,0 +1,12 @@
+#include <stdio.h>
+
+__attribute__((optnone)) __attribute__((nodebug)) void use(int used) {}
+
+__attribute__((always_inline)) void f(void *unused1, int used, int unused2) {
+  use(used); // break here
+}
+
+int main(int argc, char **argv) {
+  f(argv, 42, 1);
+  return 0;
+}