[lldb] Use forward type in pointer-to-member

[emrekultursay]

Summary

This change is similar in spirit to the change at:
https://reviews.llvm.org/rG34c697c85e9d0af11a72ac4df5578aac94a627b3

It fixes the problem where the layout of a type was being accessed
while its base classes were not populated yet; which caused an
incorrect layout to be produced and cached.

This fixes PR50054

[teemperor]

Were you able to reduce the reproducer you got to a test? If it's really the same bug as in D100180 then I guess a similar test case should trigger the issue (e.g., a class with a pointer-to-member member that references some type that is currently being parsed?)

[emrekultursay]

Added functionality/lazy-loading test.

[emrekultursay]

Fix formatting.

[emrekultursay]

I added a lazy-loading test. I also attempted to write a test similar to lang/cpp/static_member_type_depending_on_parent_size/TestStaticMemberTypeDependingOnParentSize.py, but I could not get it to produce an incorrect struct size (just like I couldn't reduce the reproducer).

[teemperor]

The test below reproduces the eager layout generation (and the resulting crashes) for me. Just apply it on top and then this is good to go. Thanks for tracking this down!

diff --git a/lldb/test/API/lang/cpp/pointer_to_member_type_depending_on_parent_size/Makefile b/lldb/test/API/lang/cpp/pointer_to_member_type_depending_on_parent_size/Makefile
new file mode 100644
index 000000000000..99998b20bcb0
--- /dev/null
+++ b/lldb/test/API/lang/cpp/pointer_to_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/pointer_to_member_type_depending_on_parent_size/TestPointerToMemberTypeDependingOnParentSize.py b/lldb/test/API/lang/cpp/pointer_to_member_type_depending_on_parent_size/TestPointerToMemberTypeDependingOnParentSize.py
new file mode 100644
index 000000000000..fd978158e716
--- /dev/null
+++ b/lldb/test/API/lang/cpp/pointer_to_member_type_depending_on_parent_size/TestPointerToMemberTypeDependingOnParentSize.py
@@ -0,0 +1,24 @@
+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 pointer-to-member member which class part is the
+        surrounding class. LLDB should *not* try to generate the record layout
+        of the class when parsing pointer-to-member types while parsing debug
+        info (as the references class might not be complete when the type is
+        parsed).
+        """
+        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/pointer_to_member_type_depending_on_parent_size/main.cpp b/lldb/test/API/lang/cpp/pointer_to_member_type_depending_on_parent_size/main.cpp
new file mode 100644
index 000000000000..4787691ac9f7
--- /dev/null
+++ b/lldb/test/API/lang/cpp/pointer_to_member_type_depending_on_parent_size/main.cpp
@@ -0,0 +1,35 @@
+// 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 won't
+// check if all the types DependsOnParam2 is depending on for its layout are
+// currently parsed.
+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 {
+  // The class part of this pointer-to-member type has a memory layout that
+  // depends on the surrounding class. If LLDB eagerly tries to layout the
+  // class part of a pointer-to-member type while parsing, then layouting this
+  // type should cause a test failure (as we aren't done parsing ToLayout
+  // at this point).
+  int DependsOnParam2<ToLayout>::* pointer_to_member_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;
+};
+
+// Emit the definition of DependsOnParam2<ToLayout>. It seems Clang won't
+// emit the definition of a class template if it's only used in the class part
+// of a pointer-to-member type.
+DependsOnParam2<ToLayout> x;
+
+ToLayout test_var;
+
+int main() { return test_var.some_member; }

[emrekultursay]

Add test

[emrekultursay]

DependsOnParam2<ToLayout> x;

This must have been the magic glue I was missing, Thanks!

[teemperor]

I assume you don't have a commit access, so I'm going to land this for you tomorrow. Thanks again for the patch!

[stella.stamenova]

Looks like this is failing on the Windows LLDB bot:

https://lab.llvm.org/buildbot/#/builders/83/builds/6033

[teemperor]

Sorry and thanks for commenting Stella!

So it seems that using an incomplete type in the test is rejected by GCC and MSVC but accepted by Clang and ICC. I believe that should be accepted and that's also what the internet(TM) and Shafik are saying, so I'm going to skip the test for those compilers.

[teemperor]

Actually it seems Clang also rejects it when running on Windows? I would have assumed that only the -fms-compatibility-version=19.0 or something like -fdelayed-template-parsing matters here but I'm not sure why this is different on Windows. I'm skipping for GCC and Windows then.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

 TypeSP DWARFASTParserClang::ParsePointerToMemberType(
     const DWARFDIE &die, const ParsedDWARFTypeAttributes &attrs) {
   SymbolFileDWARF *dwarf = die.GetDWARF();
   Type *pointee_type = dwarf->ResolveTypeUID(attrs.type.Reference(), true);
   Type *class_type =
       dwarf->ResolveTypeUID(attrs.containing_type.Reference(), true);
 
   CompilerType pointee_clang_type = pointee_type->GetForwardCompilerType();
-  CompilerType class_clang_type = class_type->GetLayoutCompilerType();
+  CompilerType class_clang_type = class_type->GetForwardCompilerType();
 
   CompilerType clang_type = TypeSystemClang::CreateMemberPointerType(
       class_clang_type, pointee_clang_type);
 
   if (llvm::Optional<uint64_t> clang_type_size =
           clang_type.GetByteSize(nullptr)) {
     return std::make_shared<Type>(die.GetID(), dwarf, attrs.name,
                                   *clang_type_size, nullptr, LLDB_INVALID_UID,

lldb/test/API/functionalities/lazy-loading/TestLazyLoading.py

class TestCase(TestBase):
     struct_behind_ref_decl = [struct_decl_kind, "StructBehindRef"]
     struct_member_decl = [struct_decl_kind, "StructMember"]
     some_struct_decl = [struct_decl_kind, "SomeStruct"]
     other_struct_decl = [struct_decl_kind, "OtherStruct"]
     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"]
+    class_pointer_to_member_decl = [struct_decl_kind, "PointerToMember"]
     unused_class_member_decl = [struct_decl_kind, "UnusedClassMember"]
     unused_class_member_ptr_decl = [struct_decl_kind, "UnusedClassMemberPtr"]
 
     def assert_no_decls_loaded(self):
         """
         Asserts that no known declarations in this test are loaded
         into the module's AST.
         """
         self.assert_decl_not_loaded(self.struct_first_member_decl)
         self.assert_decl_not_loaded(self.struct_behind_ptr_decl)
         self.assert_decl_not_loaded(self.struct_behind_ref_decl)
         self.assert_decl_not_loaded(self.struct_member_decl)
         self.assert_decl_not_loaded(self.some_struct_decl)
         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.class_pointer_to_member_decl)
         self.assert_decl_not_loaded(self.unused_class_member_decl)
 
     def get_ast_dump(self):
         """Returns the dumped Clang AST of the module as a string"""
         res = lldb.SBCommandReturnObject()
         ci = self.dbg.GetCommandInterpreter()
         ci.HandleCommand('target modules dump ast a.out', res)
         self.assertTrue(res.Succeeded())
def test_class_function_access_member(self):
         self.assert_decl_loaded(self.class_we_enter_decl)
         # We loaded the unused members of this class.
         self.assert_decl_loaded(self.unused_class_member_decl)
         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)
+        # We didn't load the type of the unused pointer-to-member member.
+        self.assert_decl_not_completed(self.class_pointer_to_member_decl)
 
         # This should not have loaded anything else.
         self.assert_decl_not_loaded(self.other_struct_decl)
         self.assert_decl_not_loaded(self.some_struct_decl)
         self.assert_decl_not_loaded(self.class_in_namespace_decl)
 

lldb/test/API/functionalities/lazy-loading/main.cpp

 
 //----------------------------------------------------------------------------//
 // Class loading declarations.
 
 struct ClassMember { int i; };
 struct StaticClassMember { int i; };
 struct UnusedClassMember { int i; };
 struct UnusedClassMemberPtr { int i; };
+struct PointerToMember { int i; };
 
 namespace NS {
 class ClassInNamespace {
   int i;
 };
 class ClassWeEnter {
 public:
   int dummy; // Prevent bug where LLDB always completes first member.
   ClassMember member;
   static StaticClassMember static_member;
+  int (PointerToMember::*ptr_to_member);
   UnusedClassMember unused_member;
   UnusedClassMemberPtr *unused_member_ptr;
   int enteredFunction() {
     return member.i; // Location: class function
   }
 };
 StaticClassMember ClassWeEnter::static_member;
 };

lldb/test/API/lang/cpp/pointer_to_member_type_depending_on_parent_size/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/lang/cpp/pointer_to_member_type_depending_on_parent_size/TestPointerToMemberTypeDependingOnParentSize.py

+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 pointer-to-member member which class part is the
+        surrounding class. LLDB should *not* try to generate the record layout
+        of the class when parsing pointer-to-member types while parsing debug
+        info (as the references class might not be complete when the type is
+        parsed).
+        """
+        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")

lldb/test/API/lang/cpp/pointer_to_member_type_depending_on_parent_size/main.cpp

+// 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 won't
+// check if all the types DependsOnParam2 is depending on for its layout are
+// currently parsed.
+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 {
+  // The class part of this pointer-to-member type has a memory layout that
+  // depends on the surrounding class. If LLDB eagerly tries to layout the
+  // class part of a pointer-to-member type while parsing, then layouting this
+  // type should cause a test failure (as we aren't done parsing ToLayout
+  // at this point).
+  int DependsOnParam2<ToLayout>::* pointer_to_member_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;
+};
+
+// Emit the definition of DependsOnParam2<ToLayout>. It seems Clang won't
+// emit the definition of a class template if it's only used in the class part
+// of a pointer-to-member type.
+DependsOnParam2<ToLayout> x;
+
+ToLayout test_var;
+
+int main() { return test_var.some_member; }