https://github.com/Michael137 created
https://github.com/llvm/llvm-project/pull/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.
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).
>From 23c7f4a3e067739a050964b3f1c2140787b0c31f Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuc...@gmail.com>
Date: Fri, 24 Jan 2025 13:33:07 +0000
Subject: [PATCH] [lldb][TypeSystem] Ensure that ParmVarDecls have the correct
DeclContext
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.
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).
---
.../SymbolFile/DWARF/DWARFASTParserClang.cpp | 31 ++--
.../SymbolFile/DWARF/DWARFASTParserClang.h | 3 +-
.../SymbolFile/NativePDB/PdbAstBuilder.cpp | 2 +-
.../Plugins/SymbolFile/PDB/PDBASTParser.cpp | 4 +-
.../TypeSystem/Clang/TypeSystemClang.cpp | 43 ++---
.../TypeSystem/Clang/TypeSystemClang.h | 16 +-
lldb/unittests/Symbol/TestTypeSystemClang.cpp | 42 +++++
.../DWARF/DWARFASTParserClangTests.cpp | 154 ++++++++++++++++++
8 files changed, 247 insertions(+), 48 deletions(-)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index e77188bfbd2e4a..30dec4bbf91c10 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1272,7 +1272,6 @@ 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;
// Parse the function children for the parameters
@@ -1283,8 +1282,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);
+ has_template_params, function_param_types);
}
bool is_cxx_method = DeclKindIsCXXClass(containing_decl_ctx->getDeclKind());
@@ -1414,11 +1412,15 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE
&die,
LinkDeclContextToDIE(function_decl, die);
- if (!function_param_decls.empty()) {
- m_ast.SetFunctionParameters(function_decl, function_param_decls);
+ const clang::FunctionProtoType *function_prototype(
+ llvm::cast<clang::FunctionProtoType>(
+ ClangUtil::GetQualType(clang_type).getTypePtr()));
+ auto params = m_ast.CreateParameterDeclarations(function_decl,
+ *function_prototype);
+ if (!params.empty()) {
+ function_decl->setParams(params);
if (template_function_decl)
- m_ast.SetFunctionParameters(template_function_decl,
- function_param_decls);
+ template_function_decl->setParams(params);
}
ClangASTMetadata metadata;
@@ -2380,7 +2382,6 @@
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;
StreamString sstr;
DWARFDeclContext decl_ctx = die.GetDWARFDeclContext();
@@ -2394,7 +2395,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);
sstr << "(";
for (size_t i = 0; i < param_types.size(); i++) {
if (i > 0)
@@ -3156,8 +3157,7 @@ bool DWARFASTParserClang::ParseChildMembers(
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) {
+ std::vector<CompilerType> &function_param_types) {
if (!parent_die)
return;
@@ -3168,7 +3168,6 @@ 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);
@@ -3176,14 +3175,6 @@ void DWARFASTParserClang::ParseChildParameters(
break;
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:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index a5c3746ada4c36..b1f0186cfa34a2 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -190,8 +190,7 @@ class DWARFASTParserClang : public
lldb_private::plugin::dwarf::DWARFASTParser {
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);
+ std::vector<lldb_private::CompilerType> &function_args);
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..1bb4bdf14c49d4 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,26 @@ void TypeSystemClang::SetFloatingInitializerForVariable(
ast, init_value, true, qt.getUnqualifiedType(), SourceLocation()));
}
+llvm::SmallVector<clang::ParmVarDecl *>
+TypeSystemClang::CreateParameterDeclarations(
+ clang::FunctionDecl *func, const clang::FunctionProtoType &prototype) {
+ assert(func);
+
+ llvm::SmallVector<clang::ParmVarDecl *, 12> params;
+ for (unsigned param_index = 0; param_index < prototype.getNumParams();
+ ++param_index) {
+ auto *param =
+ CreateParameterDeclaration(func, /*owning_module=*/{},
/*name=*/nullptr,
+
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 +7862,9 @@ 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));
+ auto params =
+ CreateParameterDeclarations(cxx_method_decl, *method_function_prototype);
+ cxx_method_decl->setParams(params);
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..02b1a1db98e70f 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,19 @@ 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.
+ ///
+ /// \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);
+
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..21da219c1a1468 100644
--- a/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
+++ b/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
@@ -1082,3 +1082,157 @@ 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.
+
+ const char *yamldata = R"(
+--- !ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+ Machine: EM_AARCH64
+DWARF:
+ debug_str:
+ - func
+ - int
+ - short
+ 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_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: 0x1f
+
+# DW_TAG_formal_parameter
+# DW_AT_type [DW_FORM_ref4] (short)
+ - AbbrCode: 0x4
+ Values:
+ - Value: 0x26
+
+ - 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: 0x5
+ 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: 0x5
+ 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 func = llvm::cast<clang::FunctionDecl>(result.front());
+
+ EXPECT_EQ(func->getNumParams(), 2U);
+ EXPECT_EQ(func->getParamDecl(0)->getDeclContext(), func);
+ EXPECT_EQ(func->getParamDecl(1)->getDeclContext(), func);
+}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
