[lldb][TypeSystem] Ensure that ParmVarDecls have the correct DeclContext #124279
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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).
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesWhile sifting through this part of the code I noticed that when we parse C++ methods, This patch removes the 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). Full diff: https://github.com/llvm/llvm-project/pull/124279.diff 8 Files Affected:
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);
+}
|
|
Hmm one important difference here is that we would now not attach a name to the parameters that we create for free functions (we already don't do it for methods). I don't think having a name on the parameter is important for anything other than diagnostics of the expression evaluator. Hence the one test failure in We would now get: instead of I'm tempted to simply adjust the test-case and not worry about attaching names at all. But if others disagree, I guess we could plumb the parameter names from DWARF into |
labath
reviewed
Jan 27, 2025
| ClangUtil::GetQualType(clang_type).getTypePtr())); | ||
| auto params = m_ast.CreateParameterDeclarations(function_decl, | ||
| *function_prototype); | ||
| if (!params.empty()) { |
There was a problem hiding this comment.
Could we drop this check (call the function with an empty vector)?
Yea shouldn't be tooo bad. We could add a |
labath
approved these changes
Jan 27, 2025
| @@ -3157,7 +3158,8 @@ 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<CompilerType> &function_param_types, | |||
| llvm::SmallVector<llvm::StringRef> &function_param_names) { | |||
There was a problem hiding this comment.
Suggested change
| llvm::SmallVector<llvm::StringRef> &function_param_names) { | |
| llvm::SmallVectorImpl<llvm::StringRef> &function_param_names) { |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/18062 Here is the relevant piece of the build log for the reference |
klensy
reviewed
Feb 6, 2025
| @@ -3205,6 +3199,8 @@ void DWARFASTParserClang::ParseChildParameters( | |||
| break; | |||
| } | |||
| } | |||
|
|
|||
| assert(function_param_names.size() == function_param_names.size()); | |||
There was a problem hiding this comment.
This looks like typo? Comparing with self.
There was a problem hiding this comment.
hah good catch! yea that's meant to say:
assert(function_param_types.size() == function_param_names.size());
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While sifting through this part of the code I noticed that when we parse C++ methods,
DWARFASTParserClangcreates two sets ofParmVarDecls, one inParseChildParametersand once inAddMethodToCXXRecordType. The former is unused when we're dealing with methods. Moreover, theParmVarDeclswe created inParseChildParameterswere created with an incorrectclang::DeclContext(namely the DeclContext of the function, and not the function itself). In Clang, there'sParmVarDecl::setOwningFunctionto adjust the DeclContext of a parameter if the parameter was created before the FunctionDecl. But we never used it.This patch removes the
ParmVarDeclcreation fromParseChildParametersand instead creates aTypeSystemClang::CreateParameterDeclarationsthat ensures we set the DeclContext correctly.Note there are two differences in how
ParmVarDecls would be created now: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).