[Lldb-commits] [lldb] 34c697c - [lldb] Don't recursively load types of static member variables in the DWARF AST parser
Raphael Isemann via lldb-commits
lldb-commits at lists.llvm.org
Mon Apr 12 05:37:40 PDT 2021
Author: Raphael Isemann
Date: 2021-04-12T14:37:07+02:00
New Revision: 34c697c85e9d0af11a72ac4df5578aac94a627b3
URL: https://github.com/llvm/llvm-project/commit/34c697c85e9d0af11a72ac4df5578aac94a627b3
DIFF: https://github.com/llvm/llvm-project/commit/34c697c85e9d0af11a72ac4df5578aac94a627b3.diff
LOG: [lldb] Don't recursively load types of static member variables in the DWARF AST parser
When LLDB's DWARF parser is parsing the member DIEs of a struct/class it
currently fully resolves the types of static member variables in a class before
adding the respective `VarDecl` to the record.
For record types fully resolving the type will also parse the member DIEs of the
respective class. The other way of resolving is just 'forward' resolving the type
which will try to load only the minimum amount of information about the type
(for records that would only be the name/kind of the type). Usually we always
resolve types on-demand so it's rarely useful to speculatively fully resolve
them on the first use.
This patch changes makes that we only 'forward' resolve the types of static
members. This solves the fact that LLDB unnecessarily loads debug information
to parse the type if it's maybe not needed later and it also avoids a crash where
the parsed type might in turn reference the surrounding class that is currently
being parsed.
The new test case demonstrates the crash that might happen. The crash happens
with the following steps:
1. We parse class `ToLayout` and it's members.
2. We parse the static class member and fully resolve its type
(`DependsOnParam2<ToLayout>`).
3. That type has a non-static class member `DependsOnParam1<ToLayout>` for which
LLDB will try to calculate the size.
4. The layout (and size)`DependsOnParam1<ToLayout>` turns depends on the
`ToLayout` size/layout.
5. Clang will calculate the record layout/size for `ToLayout` even though we are
currently parsing it and it's missing it's non-static member.
The created is missing the offset for the yet unparsed non-static member. If we
later try to get the offset we end up hitting different asserts. Most common is
the one in `TypeSystemClang::DumpValue` where it checks that the record layout
has offsets for the current FieldDecl.
```
assert(field_idx < record_layout.getFieldCount());
```
Fixed rdar://67910011
Reviewed By: shafik
Differential Revision: https://reviews.llvm.org/D100180
Added:
lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/Makefile
lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/TestStaticMemberTypeDependingOnParentSize.py
lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/main.cpp
Modified:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/test/API/functionalities/lazy-loading/TestLazyLoading.py
lldb/test/API/functionalities/lazy-loading/main.cpp
Removed:
################################################################################
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index af01a8f535184..b9e10f94bf6cc 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2534,7 +2534,7 @@ void DWARFASTParserClang::ParseSingleMember(
if (accessibility == eAccessNone)
accessibility = eAccessPublic;
TypeSystemClang::AddVariableToRecordType(
- class_clang_type, name, var_type->GetLayoutCompilerType(),
+ class_clang_type, name, var_type->GetForwardCompilerType(),
accessibility);
}
return;
diff --git a/lldb/test/API/functionalities/lazy-loading/TestLazyLoading.py b/lldb/test/API/functionalities/lazy-loading/TestLazyLoading.py
index dddd4dceb3718..326315c838e5c 100644
--- a/lldb/test/API/functionalities/lazy-loading/TestLazyLoading.py
+++ b/lldb/test/API/functionalities/lazy-loading/TestLazyLoading.py
@@ -40,6 +40,7 @@ def setUp(self):
class_in_namespace_decl = [class_decl_kind, "ClassInNamespace"]
class_we_enter_decl = [class_decl_kind, "ClassWeEnter"]
class_member_decl = [struct_decl_kind, "ClassMember"]
+ class_static_member_decl = [struct_decl_kind, "StaticClassMember"]
unused_class_member_decl = [struct_decl_kind, "UnusedClassMember"]
unused_class_member_ptr_decl = [struct_decl_kind, "UnusedClassMemberPtr"]
@@ -56,6 +57,7 @@ def assert_no_decls_loaded(self):
self.assert_decl_not_loaded(self.other_struct_decl)
self.assert_decl_not_loaded(self.class_in_namespace_decl)
self.assert_decl_not_loaded(self.class_member_decl)
+ self.assert_decl_not_loaded(self.class_static_member_decl)
self.assert_decl_not_loaded(self.unused_class_member_decl)
def get_ast_dump(self):
@@ -228,6 +230,8 @@ def test_class_function_access_member(self):
self.assert_decl_not_completed(self.unused_class_member_ptr_decl)
# We loaded the member we used.
self.assert_decl_loaded(self.class_member_decl)
+ # We didn't load the type of the unused static member.
+ self.assert_decl_not_completed(self.class_static_member_decl)
# This should not have loaded anything else.
self.assert_decl_not_loaded(self.other_struct_decl)
diff --git a/lldb/test/API/functionalities/lazy-loading/main.cpp b/lldb/test/API/functionalities/lazy-loading/main.cpp
index 34d62b483b64f..bb8f56e277ced 100644
--- a/lldb/test/API/functionalities/lazy-loading/main.cpp
+++ b/lldb/test/API/functionalities/lazy-loading/main.cpp
@@ -23,6 +23,7 @@ struct OtherStruct {
// Class loading declarations.
struct ClassMember { int i; };
+struct StaticClassMember { int i; };
struct UnusedClassMember { int i; };
struct UnusedClassMemberPtr { int i; };
@@ -34,12 +35,14 @@ class ClassWeEnter {
public:
int dummy; // Prevent bug where LLDB always completes first member.
ClassMember member;
+ static StaticClassMember static_member;
UnusedClassMember unused_member;
UnusedClassMemberPtr *unused_member_ptr;
int enteredFunction() {
return member.i; // Location: class function
}
};
+StaticClassMember ClassWeEnter::static_member;
};
//----------------------------------------------------------------------------//
diff --git a/lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/Makefile b/lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/TestStaticMemberTypeDependingOnParentSize.py b/lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/TestStaticMemberTypeDependingOnParentSize.py
new file mode 100644
index 0000000000000..ceeef9a496cc6
--- /dev/null
+++ b/lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/TestStaticMemberTypeDependingOnParentSize.py
@@ -0,0 +1,22 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+ mydir = TestBase.compute_mydir(__file__)
+
+ @no_debug_info_test
+ def test(self):
+ """
+ This tests a static member with a type which size depends on the
+ surrounding class. LLDB should *not* try to generate the record layout
+ for those types while parsing the members from debug info.
+ """
+ self.build()
+ self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+ # Force the record layout for 'ToLayout' to be generated by printing
+ # a value of it's type.
+ self.expect("target variable test_var")
diff --git a/lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/main.cpp b/lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/main.cpp
new file mode 100644
index 0000000000000..7944585fa1b0a
--- /dev/null
+++ b/lldb/test/API/lang/cpp/static_member_type_depending_on_parent_size/main.cpp
@@ -0,0 +1,28 @@
+// This class just serves as an indirection between LLDB and Clang. LLDB might
+// be tempted to check the member type of DependsOnParam2 for whether it's
+// in some 'currently-loading' state before trying to produce the record layout.
+// By inheriting from ToLayout this will make LLDB just check if
+// DependsOnParam1 is currently being loaded (which it's not) but it will
+template <typename ToLayoutParam> struct DependsOnParam1 : ToLayoutParam {};
+// This class forces the memory layout of it's type parameter to be created.
+template <typename ToLayoutParam> struct DependsOnParam2 {
+ DependsOnParam1<ToLayoutParam> m;
+};
+
+// This is the class that LLDB has to generate the record layout for.
+struct ToLayout {
+ // A static member variable which memory layout depends on the surrounding
+ // class. This comes first so that if we accidentially generate the layout
+ // for static member types we end up recursively going back to 'ToLayout'
+ // before 'some_member' has been loaded.
+ static DependsOnParam2<ToLayout> a_static_member;
+ // Some dummy member variable. This is only there so that Clang can detect
+ // that the record layout is inconsistent (i.e., the number of fields in the
+ // layout doesn't fit to the fields in the declaration).
+ int some_member;
+};
+DependsOnParam2<ToLayout> ToLayout::a_static_member;
+
+ToLayout test_var;
+
+int main() { return test_var.some_member; }
More information about the lldb-commits
mailing list