This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by emrekultursay on Apr 21 2021, 10:22 AM.

Details

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

Diff Detail

Event Timeline

emrekultursay created this revision.Apr 21 2021, 10:22 AM
emrekultursay requested review of this revision.Apr 21 2021, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2021, 10:22 AM
emrekultursay retitled this revision from Use forward type in pointer-to-member to [lldb] Use forward type in pointer-to-member.Apr 21 2021, 10:30 AM
emrekultursay edited the summary of this revision. (Show Details)
emrekultursay added a reviewer: 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?)

Added functionality/lazy-loading test.

Fix formatting.

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 accepted this revision.Apr 22 2021, 4:34 AM

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; }
This revision is now accepted and ready to land.Apr 22 2021, 4:34 AM

DependsOnParam2<ToLayout> x;

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

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

This revision was landed with ongoing or failed builds.Apr 26 2021, 6:24 AM
This revision was automatically updated to reflect the committed changes.

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.

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.