-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[lldb][DWARFASTParserClang] Added a check for the specialization existence #154123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[lldb][DWARFASTParserClang] Added a check for the specialization existence #154123
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-clang Author: None (tgs-sc) Changes
Full diff: https://github.com/llvm/llvm-project/pull/154123.diff 2 Files Affected:
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 6d819031cbef4..93571543f1c7d 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -187,6 +187,8 @@ void EmptySubobjectMap::ComputeEmptySubobjectSizes() {
// Check the bases.
for (const CXXBaseSpecifier &Base : Class->bases()) {
const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
+ // Assert to prevent infinite recursion.
+ assert(BaseDecl != Class && "Class cannot inherit from itself.");
CharUnits EmptySize;
const ASTRecordLayout &Layout = Context.getASTRecordLayout(BaseDecl);
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index c76d67b47b336..6643751cd237a 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1873,6 +1873,22 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
clang_type =
m_ast.CreateClassTemplateSpecializationType(class_specialization_decl);
+ // Try to find an existing specialization with these template arguments and
+ // template parameter list.
+ void *InsertPos = nullptr;
+ if (!class_template_decl->findSpecialization(template_param_infos.GetArgs(),
+ InsertPos))
+ // Add this specialization to the class template.
+ class_template_decl->AddSpecialization(class_specialization_decl,
+ InsertPos);
+ else {
+ dwarf->GetObjectFile()->GetModule()->ReportError(
+ "SymbolFileDWARF({0:p}) - Specialization for "
+ "clang::ClassTemplateDecl({1:p}) already exists.",
+ static_cast<void *>(this), static_cast<void *>(class_template_decl));
+ return TypeSP();
+ }
+
m_ast.SetMetadata(class_template_decl, metadata);
m_ast.SetMetadata(class_specialization_decl, metadata);
}
|
25d756b
to
835700e
Compare
Hmm yea guarding against incorrect DWARF has been mostly best effort. Can you provide a test-case for this? |
Also, please split out the clang change into a separate PR. The clang maintainers can comment there on whether that's a useful thing to have in place |
Sure. Originally this bug was discovered by debugging llc, but I minimized it to such example: --- b1.cc ---
--- b2.cc ---
Compilation: |
835700e
to
f19ba13
Compare
Addressed. I made a PR (#154134) to clang with adding assert as seeing backtrace with 16k functions is not very nice. |
@Michael137, is it OK? |
@JDevlieghere, can you also please look at this? |
This is a reproduction in godbolt: https://godbolt.org/z/aKWETszEY. |
Thanks, we will somehow need to turn this into a test. I'll have a more detailed look later in the week |
f19ba13
to
5272b4f
Compare
@Michael137, can you please take a look as a week has passed? I added unittest and updated godbolt reproduction. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
5a639e6
to
f86f3ac
Compare
Sorry for the delay. Thanks for adding the test. That was really helpful This is the DWARF of the test yaml:
So basically we have a |
@@ -1725,6 +1725,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc, | |||
const dw_tag_t tag = die.Tag(); | |||
SymbolFileDWARF *dwarf = die.GetDWARF(); | |||
LanguageType cu_language = SymbolFileDWARF::GetLanguage(*die.GetCU()); | |||
ModuleSP module_sp = dwarf->GetObjectFile()->GetModule(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert these drive-by changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
class_template_decl->AddSpecialization(class_specialization_decl, | ||
InsertPos); | ||
else { | ||
module_sp->ReportError("SymbolFileDWARF({0:p}) - Specialization for " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be reporting this to the user console. Particularly, a user wouldn't really be aware of what a Specialization for clang::ClassTemplateDecl
is and what the issue is with it already existing.
Lets just LogMessage
here for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
|
||
llvm::SmallVector<lldb::TypeSP, 3> types; | ||
for (DWARFDIE die : cu_die.children()) { | ||
if (die.Tag() == DW_TAG_structure_type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about also checking that:
llvm::StringRef(die.GetName()).starts_with("_Optional_payload")
So you don't parse the struct Type
. Then you just assert that one of the types is nullptr
and the other isn't. Makes the test a bit clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
@@ -599,6 +599,39 @@ TEST_F(DWARFASTParserClangTests, TestDefaultTemplateParamParsing) { | |||
} | |||
} | |||
|
|||
TEST_F(DWARFASTParserClangTests, TestSpecDeclExistsError) { | |||
// Tests checking error if ClassTemplateSpecializationDecl already exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Tests checking error if ClassTemplateSpecializationDecl already exists. | |
// Tests that parsing a ClassTemplateSpecializationDecl that already exists | |
// is handled gracefully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
# } | ||
# | ||
# YAML generated on Linux using obj2yaml on the above program | ||
# compiled with G++. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add a comment saying this is malformed DWARF that is missing DW_TAG_template_value_parameter
entries, which is important for the test because that makes the two specializations look like identical structure definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
if (!args.empty() && | ||
!class_template_decl->findSpecialization(args, InsertPos)) | ||
// Add this specialization to the class template. | ||
class_template_decl->AddSpecialization(class_specialization_decl, | ||
InsertPos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this into CreateClassTemplateSpecializationDecl
? That's where we also setInstantiationOf
.
That way we could even test this functionality in TestTypeSystemClang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid no as if we have already found specialization in this function, we should return nullptr. And as I see this may cause errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. What's preventing us from calling findSpecialization
on the newly created decl inside CreateClassTemplateSpecializationDecl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what should we do if we found this specialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't returning a nullptr
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
f076aea
to
407331b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (module remaining comments)
"SymbolFileDWARF({0:p}) - Specialization for " | ||
"clang::ClassTemplateDecl({1:p}) already exists.", | ||
static_cast<void *>(this), | ||
static_cast<void *>(class_template_decl)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the casts are not needed:
"SymbolFileDWARF({0:p}) - Specialization for " | |
"clang::ClassTemplateDecl({1:p}) already exists.", | |
static_cast<void *>(this), | |
static_cast<void *>(class_template_decl)); | |
"SymbolFileDWARF({0:p}) - Specialization for " | |
"clang::ClassTemplateDecl({1}, {2:p}) already exists.", | |
this, llvm::StringRef(attrs.name), class_template_decl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
if (!class_template_decl->findSpecialization(args, InsertPos)) { | ||
// Add this specialization to the class template. | ||
class_template_decl->AddSpecialization(class_template_specialization_decl, | ||
InsertPos); | ||
} else | ||
// Specialization exists, so return nullptr. | ||
return nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!class_template_decl->findSpecialization(args, InsertPos)) { | |
// Add this specialization to the class template. | |
class_template_decl->AddSpecialization(class_template_specialization_decl, | |
InsertPos); | |
} else | |
// Specialization exists, so return nullptr. | |
return nullptr; | |
if (class_template_decl->findSpecialization(args, InsertPos)) | |
return nullptr; | |
class_template_decl->AddSpecialization(class_template_specialization_decl, | |
InsertPos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
407331b
to
4c41e84
Compare
@Michael137, do you think that this PR(#154134) is still needed? |
"SymbolFileDWARF({0:p}) - Specialization for " | ||
"clang::ClassTemplateDecl({1}, {2:p}) already exists.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"SymbolFileDWARF({0:p}) - Specialization for " | |
"clang::ClassTemplateDecl({1}, {2:p}) already exists.", | |
"SymbolFileDWARF({0:p}) - Failed to create specialization for " | |
"clang::ClassTemplateDecl({1}, {2:p}).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
@@ -1676,6 +1676,13 @@ TypeSystemClang::CreateClassTemplateSpecializationDecl( | |||
class_template_specialization_decl->setInstantiationOf(class_template_decl); | |||
class_template_specialization_decl->setTemplateArgs( | |||
TemplateArgumentList::CreateCopy(ast, args)); | |||
// Specialization exists, so return nullptr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Specialization exists, so return nullptr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
void *InsertPos = nullptr; | ||
if (class_template_decl->findSpecialization(args, InsertPos)) | ||
return nullptr; | ||
// Add this specialization to the class template. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Add this specialization to the class template. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
@@ -1873,6 +1873,17 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc, | |||
clang_type = | |||
m_ast.CreateClassTemplateSpecializationType(class_specialization_decl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets move the new check before we call CreateClassTemplateSpecializationType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
Can you rebase your PR on |
I'll comment on that PR |
4c41e84
to
795c01c
Compare
Based on the test failure in |
But check to nullptr in DWARFASTParserClang.cpp fails in this case. |
@Michael137, probably we should return finding and adding specialization in DWARFASTParserClang.cpp? |
Head branch was pushed to by a user without write access
795c01c
to
cc0df38
Compare
That would be equivalent to the way your current version handles this right? I don't see how that would change anything. I now realize this is trickier than expected. E.g., the infinite recursion actually reproduces with valid DWARF. Consider:
This will crash:
There are some defensive checks against creating duplicate decls which relies on typenames: llvm-project/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp Lines 1934 to 1944 in 39572f5
However, in my reproducer it doesn't work because, in DWARF, the typenames differ:
It's just that in the LLDB AST, the template parameter types don't match what we encoded in the structure's name in DWARF. My first instinct is that we might need to adjust the heuristics of the |
Seems like problem is in Because if we look into the dwarf, it differs only with type_name:
I think the best solution would be to cover this type in this function. |
cc0df38
to
c6a6006
Compare
@Michael137, can you please rerun CI in this PR? I guess my fix is gonna work. |
@@ -959,6 +959,9 @@ CompilerType TypeSystemClang::GetBuiltinTypeForDWARFEncodingAndBitSize( | |||
if (type_name == "long double" && | |||
QualTypeMatchesBitSize(bit_size, ast, ast.LongDoubleTy)) | |||
return GetType(ast.LongDoubleTy); | |||
if (type_name == "__bf16" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets also add a case for Float16Ty
(i.e., _Float16
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a separate PR
OK, I will create one. With a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you think there is an error in the test?
Test:
Foo<_Float16, _Float16(1.0)> temp7;
Foo<__bf16, __bf16(1.0)> temp8;
Check:
value = self.expect_expr("temp8", result_type="Foo<__fp16, __fp16>")
self.assertFalse(value.GetType().GetTemplateArgumentValue(target, 1))
Current output:
AssertionError: 'Foo<__fp16, __fp16>' != 'Foo<__bf16, __bf16>'
- Foo<__fp16, __fp16>
? - -
+ Foo<__bf16, __bf16>
? + +
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new created PR: #157674
c6a6006
to
eebd3ce
Compare
eebd3ce
to
f158ed3
Compare
@Michael137, can you please rerun CI to this PR? As #157674 was merged, this one can be merged too I think. |
…tence While debugging an application with incorrect dwarf information, where DW_TAG_template_value_parameter was lost, I found that lldb does not check that the corresponding specialization exists. As a result, at the stage when ASTImporter works, the type is completed in such a way that it inherits from itself. And during the calculation of layout, an infinite recursion occurs. To catch this error, I added a corresponding check at the stage of restoring the type from dwarf information.
dec97f1
to
42a5c3d
Compare
I close it as it is actually merged (42a5c3d) |
Uh oh!
There was an error while loading. Please reload this page.