Author: Michael Buch Date: 2025-01-27T14:56:47Z New Revision: 081723b9db84e78d7dd240b46af2aeb3b51b00be
URL: https://github.com/llvm/llvm-project/commit/081723b9db84e78d7dd240b46af2aeb3b51b00be DIFF: https://github.com/llvm/llvm-project/commit/081723b9db84e78d7dd240b46af2aeb3b51b00be.diff LOG: [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). Added: Modified: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h lldb/unittests/Symbol/TestTypeSystemClang.cpp lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp Removed: ################################################################################ diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp index e77188bfbd2e4a..6602dd763ba693 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp +++ b/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()) { - m_ast.SetFunctionParameters(function_decl, function_param_decls); - if (template_function_decl) - m_ast.SetFunctionParameters(template_function_decl, - function_param_decls); - } + const clang::FunctionProtoType *function_prototype( + 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) + template_function_decl->setParams(params); 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) { diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h index a5c3746ada4c36..d1eb2bcc2592ed 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h @@ -186,12 +186,12 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser { const lldb::AccessType default_accessibility, lldb_private::ClangASTImporter::LayoutInfo &layout_info); - void - ParseChildParameters(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<clang::ParmVarDecl *> &function_param_decls); + void ParseChildParameters( + 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_param_types, + llvm::SmallVectorImpl<llvm::StringRef> &function_param_names); size_t ParseChildEnumerators( const lldb_private::CompilerType &compiler_type, bool is_signed, diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp index 0c71df625ae348..5d4b22d08b1110 100644 --- a/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp +++ b/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, diff --git a/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp b/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp index fa3530a0c22ff3..990bacd89bf346 100644 --- a/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp +++ b/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp @@ -975,8 +975,8 @@ PDBASTParser::GetDeclForSymbol(const llvm::pdb::PDBSymbol &symbol) { } } } - if (params.size()) - m_ast.SetFunctionParameters(decl, params); + if (params.size() && decl) + decl->setParams(params); m_uid_to_decl[sym_id] = decl; diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp index 47051f2e68090f..fc3dbfa311c9b8 100644 --- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp +++ b/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> ¶meter_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 - - llvm::SmallVector<clang::ParmVarDecl *, 12> params; - - 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)); + // Parameters on member function declarations in DWARF generally don't + // have names, so we omit them when creating the ParmVarDecls. + cxx_method_decl->setParams(CreateParameterDeclarations( + cxx_method_decl, *method_function_prototype, /*parameter_names=*/{})); AddAccessSpecifierDecl(cxx_record_decl, getASTContext(), GetCXXRecordDeclAccess(cxx_record_decl), diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h index 678eaed381fd49..83f954270e3093 100644 --- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h +++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h @@ -489,9 +489,6 @@ class TypeSystemClang : public TypeSystem { const char *name, const CompilerType ¶m_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 @@ class TypeSystemClang : public TypeSystem { 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> ¶m_names); + clang::CXXMethodDecl *AddMethodToCXXRecordType( lldb::opaque_compiler_type_t type, llvm::StringRef name, const char *mangled_name, const CompilerType &method_type, diff --git a/lldb/unittests/Symbol/TestTypeSystemClang.cpp b/lldb/unittests/Symbol/TestTypeSystemClang.cpp index a2d1f6db802777..23374062127e0e 100644 --- a/lldb/unittests/Symbol/TestTypeSystemClang.cpp +++ b/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); +} diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp index 8adda6fba3a0b0..6c77736113da3f 100644 --- a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp +++ b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp @@ -1082,3 +1082,173 @@ TEST_F(DWARFASTParserClangTests, TestParseSubroutine_ExplicitObjectParameter) { 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"); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
