[LLDB][DWARF] Use the same qualified name computation for Rust #165840
Conversation
|
@llvm/pr-subscribers-lldb Author: Kiva (imkiva) ChangesCurrently LLDB's enum AA {
A(u8)
}While when the Rust enum type name is the same as its variant name, the generated enum A {
A(u8)
}The problem was caused by The failure in Full diff: https://github.com/llvm/llvm-project/pull/165840.diff 3 Files Affected:
diff --git a/lldb/include/lldb/Target/Language.h b/lldb/include/lldb/Target/Language.h
index 9958b6ea2f815..f3aac6a324c34 100644
--- a/lldb/include/lldb/Target/Language.h
+++ b/lldb/include/lldb/Target/Language.h
@@ -436,6 +436,8 @@ class Language : public PluginInterface {
static bool LanguageIsC(lldb::LanguageType language);
+ static bool LanguageIsRust(lldb::LanguageType language);
+
/// Equivalent to \c LanguageIsC||LanguageIsObjC||LanguageIsCPlusPlus.
static bool LanguageIsCFamily(lldb::LanguageType language);
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index c049829f37219..388ec20cdc5fe 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1700,7 +1700,11 @@ void DWARFASTParserClang::GetUniqueTypeNameAndDeclaration(
// For C++, we rely solely upon the one definition rule that says
// only one thing can exist at a given decl context. We ignore the
// file and line that things are declared on.
- if (!die.IsValid() || !Language::LanguageIsCPlusPlus(language) ||
+ // For Rust, we do the same since Rust also has a similar qualified name?
+ // Is there a better way to do this for Rust?
+ if (!die.IsValid() ||
+ (!Language::LanguageIsCPlusPlus(language) &&
+ !Language::LanguageIsRust(language)) ||
unique_typename.IsEmpty())
return;
decl_declaration.Clear();
diff --git a/lldb/source/Target/Language.cpp b/lldb/source/Target/Language.cpp
index 8268d4ae4bb27..ad11fd94bb6b6 100644
--- a/lldb/source/Target/Language.cpp
+++ b/lldb/source/Target/Language.cpp
@@ -316,6 +316,10 @@ bool Language::LanguageIsCPlusPlus(LanguageType language) {
}
}
+bool Language::LanguageIsRust(LanguageType language) {
+ return language == eLanguageTypeRust;
+}
+
bool Language::LanguageIsObjC(LanguageType language) {
switch (language) {
case eLanguageTypeObjC:
|
lldb/source/Target/Language.cpp
Outdated
| } | ||
|
|
||
| bool Language::LanguageIsRust(LanguageType language) { | ||
| return language == eLanguageTypeRust; |
There was a problem hiding this comment.
Lets remove this API and just check the language type directly at the callsite for now
There was a problem hiding this comment.
Maybe eventually we'd want something like a "PretendsToBeCxx" API, which is true for rust and couple other languages. But that's out of scope
| // For Rust, we do the same since Rust also has a similar qualified name? | ||
| // Is there a better way to do this for Rust? | ||
| if (!die.IsValid() || | ||
| (!Language::LanguageIsCPlusPlus(language) && |
There was a problem hiding this comment.
can we split the language check into a separate if-statement? Makes it easier to read
| // file and line that things are declared on. | ||
| if (!die.IsValid() || !Language::LanguageIsCPlusPlus(language) || | ||
| // For Rust, we do the same since Rust also has a similar qualified name? | ||
| // Is there a better way to do this for Rust? |
There was a problem hiding this comment.
I would reword this to something like:
"FIXME: Rust pretends to be C++ for now, so use C++ name qualification rules"
Michael137
left a comment
Michael137
left a comment
There was a problem hiding this comment.
Could we add a test? The original crash from the issue should do?
Yes I am doing so. I noticed Rust tests used obj2yaml and I am learning it rn😆 |
A unit-test might be simpler. Check |
|
✅ With the latest revision this PR passed the Python code formatter. |
|
I added a test according to the existing |
| auto isCPlusPlusOrSimilar = Language::LanguageIsCPlusPlus(language) || | ||
| language == lldb::eLanguageTypeRust; | ||
| if (!die.IsValid() || !isCPlusPlusOrSimilar || unique_typename.IsEmpty()) |
There was a problem hiding this comment.
I would just do:
| auto isCPlusPlusOrSimilar = Language::LanguageIsCPlusPlus(language) || | |
| language == lldb::eLanguageTypeRust; | |
| if (!die.IsValid() || !isCPlusPlusOrSimilar || unique_typename.IsEmpty()) | |
| if (!Language::LanguageIsCPlusPlus(language) && !language == lldb::eLanguageTypeRust) | |
| return; | |
| if (!die.IsValid() || unique_typename.IsEmpty()) | |
| return; |
3 participants
Currently LLDB's
ParseRustVariantPartgenerates the followingCXXRecordDeclfor a Rust enumWhile when the Rust enum type name is the same as its variant name, the generated
CXXRecordDeclbecomes the following – there's a circular reference betweenstruct A$Variantandstruct A, causing #163048.The problem was caused by
GetUniqueTypeNameAndDeclarationnot returning the correct qualified name for DWARF DIEtest_issue::A::A, instead, it returnedA. This causedParseStructureLikeDIEto find the wrong typetest_issue::Aand returned early.The failure in
GetUniqueTypeNameAndDeclarationappears to stem from a language check that returns early unless the language is C++. I changed it so Rust follows the C++ path rather than returning. I’m not entirely sure this is the right approach — Rust’s qualified name rules look similar, but not identical? Alternatively, we could add a Rust-specific implementation that forms qualified names according to Rust's rules.