[Lldb-commits] [lldb] [lldb] Fix expressions that involve nested structs/classes/unions. (PR #77029)
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Fri Jan 5 09:08:18 PST 2024
https://github.com/clayborg updated https://github.com/llvm/llvm-project/pull/77029
>From ceb740dcce0ac870b1ef145d41670385a1d24f1c Mon Sep 17 00:00:00 2001
From: Greg Clayton <clayborg at gmail.com>
Date: Thu, 4 Jan 2024 16:17:44 -0800
Subject: [PATCH 1/3] Fix expressions that involve nested
structs/classes/unions.
The LLDB expression parser relies on using the external AST source support in LLDB. This allows us to find a class at the root namespace level, but it wouldn't allow us to find nested classes all of the time. When LLDB finds a class via this mechanism, it would be able to complete this class when needed, but during completion, we wouldn't populate nested types within this class which would prevent us from finding contained types when needed as clang would expect them to be present if a class was completed. When we parse a type for a class, struct or union, we make a forward declaration to the class which can be completed. Now when the class is completed, we also add any contained types to the class' declaration context which now allows these types to be found. If we have a struct that contains a struct, we will add the forward declaration of the contained structure which can be c
ompleted later. Having this forward declaration makes it possible for LLDB to find everything it needs now.
This should fix an existing issue: https://github.com/llvm/llvm-project/issues/53904
Previously, contained types could be parsed by accident and allow expression to complete successfully. Other times we would have to run an expression multiple times because our old type lookup from our expressions would cau
se a type to be parsed, but not used in the current expression, but this would have parsed a type into the containing decl context and the expression might succeed if it is run again.
---
.../SymbolFile/DWARF/DWARFASTParserClang.cpp | 44 +++++++++++-
.../SymbolFile/DWARF/DWARFASTParserClang.h | 1 +
.../API/commands/expression/nested/Makefile | 3 +
.../nested/TestNestedExpressions.py | 70 +++++++++++++++++++
.../API/commands/expression/nested/main.cpp | 35 ++++++++++
5 files changed, 151 insertions(+), 2 deletions(-)
create mode 100644 lldb/test/API/commands/expression/nested/Makefile
create mode 100644 lldb/test/API/commands/expression/nested/TestNestedExpressions.py
create mode 100644 lldb/test/API/commands/expression/nested/main.cpp
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 3e08f2550081f2..233de2f1ac58cc 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2150,6 +2150,7 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
SymbolFileDWARF *dwarf = die.GetDWARF();
ClangASTImporter::LayoutInfo layout_info;
+ std::vector<DWARFDIE> contained_type_dies;
if (die.HasChildren()) {
const bool type_is_objc_object_or_interface =
@@ -2175,7 +2176,8 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
DelayedPropertyList delayed_properties;
ParseChildMembers(die, clang_type, bases, member_function_dies,
- delayed_properties, default_accessibility, layout_info);
+ contained_type_dies, delayed_properties,
+ default_accessibility, layout_info);
// Now parse any methods if there were any...
for (const DWARFDIE &die : member_function_dies)
@@ -2231,6 +2233,13 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
if (record_decl)
GetClangASTImporter().SetRecordLayout(record_decl, layout_info);
}
+ // Now parse all contained types inside of the class. We make forward
+ // declarations to all classes, but we need the CXXRecordDecl to have decls
+ // for all contained types because we don't get asked for them via the
+ // external AST support.
+ for (const DWARFDIE &die : contained_type_dies)
+ dwarf->ResolveType(die);
+
return (bool)clang_type;
}
@@ -2260,7 +2269,7 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die,
// Disable external storage for this type so we don't get anymore
// clang::ExternalASTSource queries for this type.
- m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), false);
+ //m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), false);
if (!die)
return false;
@@ -3106,10 +3115,39 @@ void DWARFASTParserClang::ParseSingleMember(
std::make_pair(field_decl, field_bit_offset));
}
+static bool IsTypeTag(dw_tag_t tag) {
+ switch (tag) {
+ case DW_TAG_typedef:
+ case DW_TAG_base_type:
+ case DW_TAG_pointer_type:
+ case DW_TAG_reference_type:
+ case DW_TAG_rvalue_reference_type:
+ case DW_TAG_const_type:
+ case DW_TAG_restrict_type:
+ case DW_TAG_volatile_type:
+ case DW_TAG_atomic_type:
+ case DW_TAG_unspecified_type:
+ case DW_TAG_structure_type:
+ case DW_TAG_union_type:
+ case DW_TAG_class_type:
+ case DW_TAG_enumeration_type:
+ case DW_TAG_inlined_subroutine:
+ case DW_TAG_subprogram:
+ case DW_TAG_subroutine_type:
+ case DW_TAG_array_type:
+ case DW_TAG_ptr_to_member_type:
+ return true;
+ default:
+ break;
+ }
+ return false;
+}
+
bool DWARFASTParserClang::ParseChildMembers(
const DWARFDIE &parent_die, CompilerType &class_clang_type,
std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
std::vector<DWARFDIE> &member_function_dies,
+ std::vector<DWARFDIE> &contained_type_dies,
DelayedPropertyList &delayed_properties,
const AccessType default_accessibility,
ClangASTImporter::LayoutInfo &layout_info) {
@@ -3159,6 +3197,8 @@ bool DWARFASTParserClang::ParseChildMembers(
break;
default:
+ if (IsTypeTag(tag))
+ contained_type_dies.push_back(die);
break;
}
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 3e28e54d6220db..8d4af203bb2871 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -175,6 +175,7 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
lldb_private::CompilerType &class_compiler_type,
std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
std::vector<lldb_private::plugin::dwarf::DWARFDIE> &member_function_dies,
+ std::vector<lldb_private::plugin::dwarf::DWARFDIE> &contained_type_dies,
DelayedPropertyList &delayed_properties,
const lldb::AccessType default_accessibility,
lldb_private::ClangASTImporter::LayoutInfo &layout_info);
diff --git a/lldb/test/API/commands/expression/nested/Makefile b/lldb/test/API/commands/expression/nested/Makefile
new file mode 100644
index 00000000000000..99998b20bcb050
--- /dev/null
+++ b/lldb/test/API/commands/expression/nested/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/expression/nested/TestNestedExpressions.py b/lldb/test/API/commands/expression/nested/TestNestedExpressions.py
new file mode 100644
index 00000000000000..7f194e921e5628
--- /dev/null
+++ b/lldb/test/API/commands/expression/nested/TestNestedExpressions.py
@@ -0,0 +1,70 @@
+"""
+Test calling an expression with errors that a FixIt can fix.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class NestedExpressions(TestBase):
+
+ def test_enum_in_nested_structs(self):
+ """
+ Test expressions that references an enumeration in nested structs.
+ """
+ self.build()
+ exe_path = self.getBuildArtifact("a.out")
+ target = self.dbg.CreateTarget(exe_path)
+ self.assertTrue(target, "Target: %s is not valid." % (exe_path))
+ self.expect_expr("A::B::C::EnumType::Eleven",
+ result_type="A::B::C::EnumType",
+ result_value="Eleven")
+
+ def test_struct_in_nested_structs(self):
+ """
+ Test expressions that references a struct in nested structs.
+ """
+ self.build()
+ exe_path = self.getBuildArtifact("a.out")
+ target = self.dbg.CreateTarget(exe_path)
+ self.assertTrue(target, "Target: %s is not valid." % (exe_path))
+ self.expect_expr("sizeof(A::B::C)", result_value="1")
+ self.expect_expr("sizeof(A::B)", result_value="2")
+
+ def test_static_in_nested_structs(self):
+ """
+ Test expressions that references a static variable in nested structs.
+ """
+ self.build()
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "Stop here to evaluate expressions", lldb.SBFileSpec("main.cpp")
+ )
+ self.expect_expr("A::B::C::enum_static",
+ result_type="A::B::C::EnumType",
+ result_value="Eleven")
+
+ def test_enum_in_nested_namespaces(self):
+ """
+ Test expressions that references an enumeration in nested namespaces.
+ """
+ self.build()
+ exe_path = self.getBuildArtifact("a.out")
+ target = self.dbg.CreateTarget(exe_path)
+ self.assertTrue(target, "Target: %s is not valid." % (exe_path))
+ self.expect_expr("a::b::c::Color::Blue",
+ result_type="a::b::c::Color",
+ result_value="Blue")
+
+ def test_static_in_nested_namespaces(self):
+ """
+ Test expressions that references an enumeration in nested namespaces.
+ """
+ self.build()
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "Stop here to evaluate expressions", lldb.SBFileSpec("main.cpp")
+ )
+ self.expect_expr("a::b::c::d",
+ result_type="int",
+ result_value="12")
diff --git a/lldb/test/API/commands/expression/nested/main.cpp b/lldb/test/API/commands/expression/nested/main.cpp
new file mode 100644
index 00000000000000..9f8baaf35510d2
--- /dev/null
+++ b/lldb/test/API/commands/expression/nested/main.cpp
@@ -0,0 +1,35 @@
+namespace a {
+ namespace b {
+ namespace c {
+ static int d = 12;
+ enum Color {
+ Red, Green, Blue
+ };
+ }
+ }
+}
+
+struct A {
+ int _a = 'a';
+ struct B {
+ short _b = 'b';
+ struct C {
+ char _c = 'c';
+ enum EnumType : int {
+ Eleven = 11
+ };
+ static EnumType enum_static;
+ };
+ };
+};
+
+A::B::C::EnumType A::B::C::enum_static = A::B::C::Eleven;
+
+int foo() {
+ a::b::c::Color color = a::b::c::Blue;
+ return A::B::C::enum_static == a::b::c::d && ((int)color == 0);
+}
+
+int main() {
+ return foo(); // Stop here to evaluate expressions
+}
>From fd231b308b7e2b361f0632dccb8f130c6c4fa24f Mon Sep 17 00:00:00 2001
From: Greg Clayton <clayborg at gmail.com>
Date: Fri, 5 Jan 2024 09:04:31 -0800
Subject: [PATCH 2/3] Fix review comments.
Fixes:
- revert the code that commetned out turning off external AST for a class, this was from initial testing
- remove IsTypeTag() and use llvm::dwarf::isType()
---
.../SymbolFile/DWARF/DWARFASTParserClang.cpp | 32 ++-----------------
1 file changed, 2 insertions(+), 30 deletions(-)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 233de2f1ac58cc..97cfdc1b3dcf1e 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2269,7 +2269,7 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die,
// Disable external storage for this type so we don't get anymore
// clang::ExternalASTSource queries for this type.
- //m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), false);
+ m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), false);
if (!die)
return false;
@@ -3115,34 +3115,6 @@ void DWARFASTParserClang::ParseSingleMember(
std::make_pair(field_decl, field_bit_offset));
}
-static bool IsTypeTag(dw_tag_t tag) {
- switch (tag) {
- case DW_TAG_typedef:
- case DW_TAG_base_type:
- case DW_TAG_pointer_type:
- case DW_TAG_reference_type:
- case DW_TAG_rvalue_reference_type:
- case DW_TAG_const_type:
- case DW_TAG_restrict_type:
- case DW_TAG_volatile_type:
- case DW_TAG_atomic_type:
- case DW_TAG_unspecified_type:
- case DW_TAG_structure_type:
- case DW_TAG_union_type:
- case DW_TAG_class_type:
- case DW_TAG_enumeration_type:
- case DW_TAG_inlined_subroutine:
- case DW_TAG_subprogram:
- case DW_TAG_subroutine_type:
- case DW_TAG_array_type:
- case DW_TAG_ptr_to_member_type:
- return true;
- default:
- break;
- }
- return false;
-}
-
bool DWARFASTParserClang::ParseChildMembers(
const DWARFDIE &parent_die, CompilerType &class_clang_type,
std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
@@ -3197,7 +3169,7 @@ bool DWARFASTParserClang::ParseChildMembers(
break;
default:
- if (IsTypeTag(tag))
+ if (llvm::dwarf::isType(tag))
contained_type_dies.push_back(die);
break;
}
>From e5aac3b38124117c6e5077f1ae077378984aaf25 Mon Sep 17 00:00:00 2001
From: Greg Clayton <clayborg at gmail.com>
Date: Fri, 5 Jan 2024 09:07:50 -0800
Subject: [PATCH 3/3] Run clang-format
---
.../SymbolFile/DWARF/DWARFASTParserClang.cpp | 1 -
.../API/commands/expression/nested/main.cpp | 20 ++++++++-----------
2 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 97cfdc1b3dcf1e..009722b85aa186 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2240,7 +2240,6 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
for (const DWARFDIE &die : contained_type_dies)
dwarf->ResolveType(die);
-
return (bool)clang_type;
}
diff --git a/lldb/test/API/commands/expression/nested/main.cpp b/lldb/test/API/commands/expression/nested/main.cpp
index 9f8baaf35510d2..620535fa09534b 100644
--- a/lldb/test/API/commands/expression/nested/main.cpp
+++ b/lldb/test/API/commands/expression/nested/main.cpp
@@ -1,13 +1,11 @@
namespace a {
- namespace b {
- namespace c {
- static int d = 12;
- enum Color {
- Red, Green, Blue
- };
- }
- }
-}
+namespace b {
+namespace c {
+static int d = 12;
+enum Color { Red, Green, Blue };
+} // namespace c
+} // namespace b
+} // namespace a
struct A {
int _a = 'a';
@@ -15,9 +13,7 @@ struct A {
short _b = 'b';
struct C {
char _c = 'c';
- enum EnumType : int {
- Eleven = 11
- };
+ enum EnumType : int { Eleven = 11 };
static EnumType enum_static;
};
};
More information about the lldb-commits
mailing list