[lldb][TypeSystem] Ensure that ParmVarDecls have the correct DeclContext (#124279)

While sifting through this part of the code I noticed that when we parse
C++ methods, `DWARFASTParserClang` creates two sets of `ParmVarDecls`,
one in `ParseChildParameters` and once in `AddMethodToCXXRecordType`.
The former is unused when we're dealing with methods. Moreover, the
`ParmVarDecls` we created in `ParseChildParameters` were created with an
incorrect `clang::DeclContext` (namely the DeclContext of the function,
and not the function itself). In Clang, there's
`ParmVarDecl::setOwningFunction` to adjust the DeclContext of a
parameter if the parameter was created before the FunctionDecl. But we
never used it.

This patch removes the `ParmVarDecl` creation from
`ParseChildParameters` and instead creates a
`TypeSystemClang::CreateParameterDeclarations` that ensures we set the
DeclContext correctly.

Note there is one differences in how `ParmVarDecl`s would be created
now: we won't set a ClangASTMetadata entry for any of the parameters. I
don't think this was ever actually useful for parameter DIEs anyway.

This wasn't causing any concrete issues (that I know of), but was quite
surprising. And this way of setting the parameters seems easier to
reason about (in my opinion).

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

@@ -1272,7 +1272,7 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
     return_clang_type = m_ast.GetBasicType(eBasicTypeVoid);
 
   std::vector<CompilerType> function_param_types;
-  std::vector<clang::ParmVarDecl *> function_param_decls;
+  llvm::SmallVector<llvm::StringRef> function_param_names;
 
   // Parse the function children for the parameters
 
@@ -1284,7 +1284,7 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
   if (die.HasChildren()) {
     ParseChildParameters(containing_decl_ctx, die, is_variadic,
                          has_template_params, function_param_types,
-                         function_param_decls);
+                         function_param_names);
   }
 
   bool is_cxx_method = DeclKindIsCXXClass(containing_decl_ctx->getDeclKind());
@@ -1414,12 +1414,14 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
 
           LinkDeclContextToDIE(function_decl, die);
 
-          if (!function_param_decls.empty()) {
+          const clang::FunctionProtoType *function_prototype(
-            m_ast.SetFunctionParameters(function_decl, function_param_decls);
+              llvm::cast<clang::FunctionProtoType>(
+                  ClangUtil::GetQualType(clang_type).getTypePtr()));
+          const auto params = m_ast.CreateParameterDeclarations(
+              function_decl, *function_prototype, function_param_names);
+          function_decl->setParams(params);
           if (template_function_decl)
-              m_ast.SetFunctionParameters(template_function_decl,
+            template_function_decl->setParams(params);
-                                          function_param_decls);
-          }
 
           ClangASTMetadata metadata;
           metadata.SetUserID(die.GetID());
@@ -2380,7 +2382,7 @@ DWARFASTParserClang::ConstructDemangledNameFromDWARF(const DWARFDIE &die) {
   bool is_variadic = false;
   bool has_template_params = false;
   std::vector<CompilerType> param_types;
-  std::vector<clang::ParmVarDecl *> param_decls;
+  llvm::SmallVector<llvm::StringRef> param_names;
   StreamString sstr;
 
   DWARFDeclContext decl_ctx = die.GetDWARFDeclContext();
@@ -2394,7 +2396,7 @@ DWARFASTParserClang::ConstructDemangledNameFromDWARF(const DWARFDIE &die) {
       die, GetCXXObjectParameter(die, *containing_decl_ctx));
 
   ParseChildParameters(containing_decl_ctx, die, is_variadic,
-                       has_template_params, param_types, param_decls);
+                       has_template_params, param_types, param_names);
   sstr << "(";
   for (size_t i = 0; i < param_types.size(); i++) {
     if (i > 0)
@@ -3157,7 +3159,7 @@ void DWARFASTParserClang::ParseChildParameters(
     clang::DeclContext *containing_decl_ctx, const DWARFDIE &parent_die,
     bool &is_variadic, bool &has_template_params,
     std::vector<CompilerType> &function_param_types,
-    std::vector<clang::ParmVarDecl *> &function_param_decls) {
+    llvm::SmallVectorImpl<llvm::StringRef> &function_param_names) {
   if (!parent_die)
     return;
 
@@ -3168,22 +3170,14 @@ void DWARFASTParserClang::ParseChildParameters(
       if (die.GetAttributeValueAsUnsigned(DW_AT_artificial, 0))
         continue;
 
-      const char *name = die.GetName();
       DWARFDIE param_type_die = die.GetAttributeValueAsReferenceDIE(DW_AT_type);
 
       Type *type = die.ResolveTypeUID(param_type_die);
       if (!type)
         break;
 
+      function_param_names.emplace_back(die.GetName());
       function_param_types.push_back(type->GetForwardCompilerType());
-
-      clang::ParmVarDecl *param_var_decl = m_ast.CreateParameterDeclaration(
-          containing_decl_ctx, GetOwningClangModule(die), name,
-          type->GetForwardCompilerType(), clang::StorageClass::SC_None);
-      assert(param_var_decl);
-      function_param_decls.push_back(param_var_decl);
-
-      m_ast.SetMetadataAsUserID(param_var_decl, die.GetID());
     } break;
 
     case DW_TAG_unspecified_parameters:
@@ -3205,6 +3199,8 @@ void DWARFASTParserClang::ParseChildParameters(
       break;
     }
   }
+
+  assert(function_param_names.size() == function_param_names.size());
 }
 
 clang::Decl *DWARFASTParserClang::GetClangDeclForDIE(const DWARFDIE &die) {

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

@@ -186,12 +186,12 @@ protected:
       const lldb::AccessType default_accessibility,
       lldb_private::ClangASTImporter::LayoutInfo &layout_info);
 
-  void
+  void ParseChildParameters(
-  ParseChildParameters(clang::DeclContext *containing_decl_ctx,
+      clang::DeclContext *containing_decl_ctx,
       const lldb_private::plugin::dwarf::DWARFDIE &parent_die,
       bool &is_variadic, bool &has_template_params,
-                       std::vector<lldb_private::CompilerType> &function_args,
+      std::vector<lldb_private::CompilerType> &function_param_types,
-                       std::vector<clang::ParmVarDecl *> &function_param_decls);
+      llvm::SmallVectorImpl<llvm::StringRef> &function_param_names);
 
   size_t ParseChildEnumerators(
       const lldb_private::CompilerType &compiler_type, bool is_signed,

lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

@@ -1137,7 +1137,7 @@ void PdbAstBuilder::CreateFunctionParameters(PdbCompilandSymId func_id,
   }
 
   if (!params.empty() && params.size() == param_count)
-    m_clang.SetFunctionParameters(&function_decl, params);
+    function_decl.setParams(params);
 }
 
 clang::QualType PdbAstBuilder::CreateEnumType(PdbTypeSymId id,

lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

@@ -975,8 +975,8 @@ PDBASTParser::GetDeclForSymbol(const llvm::pdb::PDBSymbol &symbol) {
         }
       }
     }
-    if (params.size())
+    if (params.size() && decl)
-      m_ast.SetFunctionParameters(decl, params);
+      decl->setParams(params);
 
     m_uid_to_decl[sym_id] = decl;
 

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

@@ -2217,12 +2217,6 @@ ParmVarDecl *TypeSystemClang::CreateParameterDeclaration(
   return decl;
 }
 
-void TypeSystemClang::SetFunctionParameters(
-    FunctionDecl *function_decl, llvm::ArrayRef<ParmVarDecl *> params) {
-  if (function_decl)
-    function_decl->setParams(params);
-}
-
 CompilerType
 TypeSystemClang::CreateBlockPointerType(const CompilerType &function_type) {
   QualType block_type = m_ast_up->getBlockPointerType(
@@ -7708,6 +7702,32 @@ void TypeSystemClang::SetFloatingInitializerForVariable(
       ast, init_value, true, qt.getUnqualifiedType(), SourceLocation()));
 }
 
+llvm::SmallVector<clang::ParmVarDecl *>
+TypeSystemClang::CreateParameterDeclarations(
+    clang::FunctionDecl *func, const clang::FunctionProtoType &prototype,
+    const llvm::SmallVector<llvm::StringRef> &parameter_names) {
+  assert(func);
+  assert(parameter_names.empty() ||
+         parameter_names.size() == prototype.getNumParams());
+
+  llvm::SmallVector<clang::ParmVarDecl *, 12> params;
+  for (unsigned param_index = 0; param_index < prototype.getNumParams();
+       ++param_index) {
+    llvm::StringRef name =
+        !parameter_names.empty() ? parameter_names[param_index] : "";
+
+    auto *param =
+        CreateParameterDeclaration(func, /*owning_module=*/{}, name.data(),
+                                   GetType(prototype.getParamType(param_index)),
+                                   clang::SC_None, /*add_decl=*/false);
+    assert(param);
+
+    params.push_back(param);
+  }
+
+  return params;
+}
+
 clang::CXXMethodDecl *TypeSystemClang::AddMethodToCXXRecordType(
     lldb::opaque_compiler_type_t type, llvm::StringRef name,
     const char *mangled_name, const CompilerType &method_clang_type,
@@ -7848,20 +7868,10 @@ clang::CXXMethodDecl *TypeSystemClang::AddMethodToCXXRecordType(
         getASTContext(), mangled_name, /*literal=*/false));
   }
 
-  // Populate the method decl with parameter decls
+  // Parameters on member function declarations in DWARF generally don't
-
+  // have names, so we omit them when creating the ParmVarDecls.
-  llvm::SmallVector<clang::ParmVarDecl *, 12> params;
+  cxx_method_decl->setParams(CreateParameterDeclarations(
-
+      cxx_method_decl, *method_function_prototype, /*parameter_names=*/{}));
-  for (unsigned param_index = 0; param_index < num_params; ++param_index) {
-    params.push_back(clang::ParmVarDecl::Create(
-        getASTContext(), cxx_method_decl, clang::SourceLocation(),
-        clang::SourceLocation(),
-        nullptr, // anonymous
-        method_function_prototype->getParamType(param_index), nullptr,
-        clang::SC_None, nullptr));
-  }
-
-  cxx_method_decl->setParams(llvm::ArrayRef<clang::ParmVarDecl *>(params));
 
   AddAccessSpecifierDecl(cxx_record_decl, getASTContext(),
                          GetCXXRecordDeclAccess(cxx_record_decl),

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

@@ -489,9 +489,6 @@ public:
                              const char *name, const CompilerType &param_type,
                              int storage, bool add_decl = false);
 
-  void SetFunctionParameters(clang::FunctionDecl *function_decl,
-                             llvm::ArrayRef<clang::ParmVarDecl *> params);
-
   CompilerType CreateBlockPointerType(const CompilerType &function_type);
 
   // Array Types
@@ -976,6 +973,24 @@ public:
   SetFloatingInitializerForVariable(clang::VarDecl *var,
                                     const llvm::APFloat &init_value);
 
+  /// For each parameter type of \c prototype, creates a \c clang::ParmVarDecl
+  /// whose \c clang::DeclContext is \c context.
+  ///
+  /// \param[in] context Non-null \c clang::FunctionDecl which will be the \c
+  /// clang::DeclContext of each parameter created/returned by this function.
+  /// \param[in] prototype The \c clang::FunctionProtoType of \c context.
+  /// \param[in] param_names The ith element of this vector contains the name
+  /// of the ith parameter. This parameter may be unnamed, in which case the
+  /// ith entry in \c param_names is an empty string. This vector is either
+  /// empty, or will have an entry for *each* parameter of the prototype
+  /// regardless of whether a parameter is unnamed or not.
+  ///
+  /// \returns A list of newly created of non-null \c clang::ParmVarDecl (one
+  /// for each parameter of \c prototype).
+  llvm::SmallVector<clang::ParmVarDecl *> CreateParameterDeclarations(
+      clang::FunctionDecl *context, const clang::FunctionProtoType &prototype,
+      const llvm::SmallVector<llvm::StringRef> &param_names);
+
   clang::CXXMethodDecl *AddMethodToCXXRecordType(
       lldb::opaque_compiler_type_t type, llvm::StringRef name,
       const char *mangled_name, const CompilerType &method_type,

lldb/unittests/Symbol/TestTypeSystemClang.cpp

@@ -1040,3 +1040,45 @@ TEST_F(TestTypeSystemClang, GetDeclContextByNameWhenMissingSymbolFile) {
 
   EXPECT_TRUE(decls.empty());
 }
+
+TEST_F(TestTypeSystemClang, AddMethodToCXXRecordType_ParmVarDecls) {
+  // Tests that AddMethodToCXXRecordType creates ParmVarDecl's with
+  // a correct clang::DeclContext.
+
+  llvm::StringRef class_name = "S";
+  CompilerType t = clang_utils::createRecord(*m_ast, class_name);
+  m_ast->StartTagDeclarationDefinition(t);
+
+  CompilerType return_type = m_ast->GetBasicType(lldb::eBasicTypeVoid);
+  const bool is_virtual = false;
+  const bool is_static = false;
+  const bool is_inline = false;
+  const bool is_explicit = true;
+  const bool is_attr_used = false;
+  const bool is_artificial = false;
+
+  llvm::SmallVector<CompilerType> param_types{
+      m_ast->GetBasicType(lldb::eBasicTypeInt),
+      m_ast->GetBasicType(lldb::eBasicTypeShort)};
+  CompilerType function_type = m_ast->CreateFunctionType(
+      return_type, param_types.data(), /*num_params*/ param_types.size(),
+      /*variadic=*/false, /*quals*/ 0U);
+  m_ast->AddMethodToCXXRecordType(
+      t.GetOpaqueQualType(), "myFunc", nullptr, function_type,
+      lldb::AccessType::eAccessPublic, is_virtual, is_static, is_inline,
+      is_explicit, is_attr_used, is_artificial);
+
+  // Complete the definition and check the created record.
+  m_ast->CompleteTagDeclarationDefinition(t);
+
+  auto *record = llvm::cast<CXXRecordDecl>(ClangUtil::GetAsTagDecl(t));
+
+  auto method_it = record->method_begin();
+  ASSERT_NE(method_it, record->method_end());
+
+  EXPECT_EQ(method_it->getNumParams(), param_types.size());
+
+  // DeclContext of each parameter should be the CXXMethodDecl itself.
+  EXPECT_EQ(method_it->getParamDecl(0)->getDeclContext(), *method_it);
+  EXPECT_EQ(method_it->getParamDecl(1)->getDeclContext(), *method_it);
+}

lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp

@@ -1082,3 +1082,173 @@ DWARF:
             clang::Qualifiers::fromCVRMask(clang::Qualifiers::Const |
                                            clang::Qualifiers::Volatile));
 }
+
+TEST_F(DWARFASTParserClangTests, TestParseSubroutine_ParameterCreation) {
+  // Tests parsing of a C++ free function will create clang::ParmVarDecls with
+  // the correct clang::DeclContext.
+  //
+  // Also ensures we attach names to the ParmVarDecls (even when DWARF contains
+  // a mix of named/unnamed parameters).
+
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_AARCH64
+DWARF:
+  debug_str:
+    - func
+    - int
+    - short
+    - namedParam
+  debug_abbrev:
+    - ID:              0
+      Table:
+        - Code:            0x1
+          Tag:             DW_TAG_compile_unit
+          Children:        DW_CHILDREN_yes
+          Attributes:
+            - Attribute:       DW_AT_language
+              Form:            DW_FORM_data2
+        - Code:            0x2
+          Tag:             DW_TAG_structure_type
+          Children:        DW_CHILDREN_yes
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+        - Code:            0x3
+          Tag:             DW_TAG_subprogram
+          Children:        DW_CHILDREN_yes
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+            - Attribute:       DW_AT_declaration
+              Form:            DW_FORM_flag_present
+            - Attribute:       DW_AT_external
+              Form:            DW_FORM_flag_present
+        - Code:            0x4
+          Tag:             DW_TAG_formal_parameter
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_type
+              Form:            DW_FORM_ref4
+        - Code:            0x5
+          Tag:             DW_TAG_formal_parameter
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_type
+              Form:            DW_FORM_ref4
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+        - Code:            0x6
+          Tag:             DW_TAG_base_type
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_name
+              Form:            DW_FORM_strp
+            - Attribute:       DW_AT_encoding
+              Form:            DW_FORM_data1
+            - Attribute:       DW_AT_byte_size
+              Form:            DW_FORM_data1
+  debug_info:
+     - Version:         5
+       UnitType:        DW_UT_compile
+       AddrSize:        8
+       Entries:
+
+# DW_TAG_compile_unit
+#   DW_AT_language [DW_FORM_data2]    (DW_LANG_C_plus_plus)
+
+        - AbbrCode:        0x1
+          Values:
+            - Value:           0x04
+
+#     DW_TAG_subprogram
+#       DW_AT_name [DW_FORM_strp] ("func")
+        - AbbrCode:        0x3
+          Values:
+            - Value:           0x0
+            - Value:           0x1
+            - Value:           0x1
+
+#       DW_TAG_formal_parameter
+#         DW_AT_type [DW_FORM_ref4] (int)
+        - AbbrCode:        0x4
+          Values:
+            - Value: 0x23
+
+#       DW_TAG_formal_parameter
+#         DW_AT_type [DW_FORM_ref4] (short)
+#         DW_AT_name [DW_FORM_strp] ("namedParam")
+        - AbbrCode:        0x5
+          Values:
+            - Value: 0x2a
+            - Value: 0xf
+
+        - AbbrCode: 0x0
+
+#   DW_TAG_base_type
+#     DW_AT_name      [DW_FORM_strp] ("int")
+#     DW_AT_encoding  [DW_FORM_data1]
+#     DW_AT_byte_size [DW_FORM_data1]
+
+        - AbbrCode:        0x6
+          Values:
+            - Value:           0x0000000000000005
+            - Value:           0x0000000000000005 # DW_ATE_signed
+            - Value:           0x0000000000000004
+
+#   DW_TAG_base_type
+#     DW_AT_name      [DW_FORM_strp] ("short")
+#     DW_AT_encoding  [DW_FORM_data1]
+#     DW_AT_byte_size [DW_FORM_data1]
+
+        - AbbrCode:        0x6
+          Values:
+            - Value:           0x0000000000000009
+            - Value:           0x0000000000000005 # DW_ATE_signed
+            - Value:           0x0000000000000004
+
+        - AbbrCode: 0x0
+...
+)";
+  YAMLModuleTester t(yamldata);
+
+  DWARFUnit *unit = t.GetDwarfUnit();
+  ASSERT_NE(unit, nullptr);
+  const DWARFDebugInfoEntry *cu_entry = unit->DIE().GetDIE();
+  ASSERT_EQ(cu_entry->Tag(), DW_TAG_compile_unit);
+  ASSERT_EQ(unit->GetDWARFLanguageType(), DW_LANG_C_plus_plus);
+  DWARFDIE cu_die(unit, cu_entry);
+
+  auto ts_or_err =
+      cu_die.GetDWARF()->GetTypeSystemForLanguage(eLanguageTypeC_plus_plus);
+  ASSERT_TRUE(static_cast<bool>(ts_or_err));
+  llvm::consumeError(ts_or_err.takeError());
+
+  auto *ts = static_cast<TypeSystemClang *>(ts_or_err->get());
+  auto *parser = static_cast<DWARFASTParserClang *>(ts->GetDWARFParser());
+
+  auto subprogram = cu_die.GetFirstChild();
+  ASSERT_TRUE(subprogram.IsValid());
+  ASSERT_EQ(subprogram.Tag(), DW_TAG_subprogram);
+
+  SymbolContext sc;
+  bool new_type;
+  auto type_sp = parser->ParseTypeFromDWARF(sc, subprogram, &new_type);
+  ASSERT_NE(type_sp, nullptr);
+
+  auto result = ts->GetTranslationUnitDecl()->lookup(
+      clang_utils::getDeclarationName(*ts, "func"));
+  ASSERT_TRUE(result.isSingleResult());
+
+  auto const *func = llvm::cast<clang::FunctionDecl>(result.front());
+
+  EXPECT_EQ(func->getNumParams(), 2U);
+  EXPECT_EQ(func->getParamDecl(0)->getDeclContext(), func);
+  EXPECT_TRUE(func->getParamDecl(0)->getName().empty());
+  EXPECT_EQ(func->getParamDecl(1)->getDeclContext(), func);
+  EXPECT_EQ(func->getParamDecl(1)->getName(), "namedParam");
+}