[lldb][ClangASTImporter] Import record layouts from origin if available

[Michael137]

Layout information for a record gets stored in the ClangASTImporter associated with the DWARFASTParserClang that originally parsed the record. LLDB sometimes moves clang types from one AST to another (in the reproducer the origin AST was a precompiled-header and the destination was the AST backing the executable). When clang then asks LLDB to layoutRecordType, it will do so with the help of the ClangASTImporter the type is associated with. If the type's origin is actually in a different LLDB module (and thus a different DWARFASTParserClang was used to set its layout info), we won't find the layout info in our local ClangASTImporter.

In the reproducer this meant we would drop the alignment info of the origin type and misread a variable's contents with frame var and expr.

There is logic in ClangASTSource::layoutRecordType to import an origin's layout info. This patch re-uses that infrastructure to import an origin's layout from one ClangASTImporter instance to another.

rdar://123274144

[Michael137]

relies on #83291

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

Layout information for a record gets stored in the ClangASTImporter associated with the DWARFASTParserClang that originally parsed the record. LLDB sometimes moves clang types from one AST to another (in the reproducer the origin AST was a precompiled-header and the destination was the AST backing the executable). When clang then asks LLDB to layoutRecordType, it will do so with the help of the ClangASTImporter the type is associated with. If the type's origin is actually in a different LLDB module (and thus a different DWARFASTParserClang was used to set its layout info), we won't find the layout info in our local ClangASTImporter.

In the reproducer this meant we would drop the alignment info of the origin type and misread a variable's contents with frame var and expr.

There is logic in ClangASTSource::layoutRecordType to import an origin's layout info. This patch re-uses that infrastructure to import an origin's layout from one ClangASTImporter instance to another.

rdar://123274144

---

Full diff: https://github.com/llvm/llvm-project/pull/83295.diff

5 Files Affected:

• (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp (+16-7)
• (added) lldb/test/API/lang/cpp/gmodules/alignment/Makefile (+4)
• (added) lldb/test/API/lang/cpp/gmodules/alignment/TestPchAlignment.py (+60)
• (added) lldb/test/API/lang/cpp/gmodules/alignment/main.cpp (+10)
• (added) lldb/test/API/lang/cpp/gmodules/alignment/pch.h (+21)

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
index 62a30c14912bc9..754191ad83fe8a 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -527,7 +527,6 @@ bool ClangASTImporter::LayoutRecordType(
         &vbase_offsets) {
   RecordDeclToLayoutMap::iterator pos =
       m_record_decl_to_layout_map.find(record_decl);
-  bool success = false;
   base_offsets.clear();
   vbase_offsets.clear();
   if (pos != m_record_decl_to_layout_map.end()) {
@@ -537,13 +536,23 @@ bool ClangASTImporter::LayoutRecordType(
     base_offsets.swap(pos->second.base_offsets);
     vbase_offsets.swap(pos->second.vbase_offsets);
     m_record_decl_to_layout_map.erase(pos);
-    success = true;
-  } else {
-    bit_size = 0;
-    alignment = 0;
-    field_offsets.clear();
+    return true;
   }
-  return success;
+
+  // It's possible that we calculated the layout in a different
+  // ClangASTImporter instance. Try to import such layout if
+  // our decl has an origin.
+  if (auto origin = GetDeclOrigin(record_decl); origin.Valid())
+    if (importRecordLayoutFromOrigin(record_decl, bit_size, alignment,
+                                     field_offsets, base_offsets,
+                                     vbase_offsets))
+      return true;
+
+  bit_size = 0;
+  alignment = 0;
+  field_offsets.clear();
+
+  return false;
 }
 
 void ClangASTImporter::SetRecordLayout(clang::RecordDecl *decl,
diff --git a/lldb/test/API/lang/cpp/gmodules/alignment/Makefile b/lldb/test/API/lang/cpp/gmodules/alignment/Makefile
new file mode 100644
index 00000000000000..a6c3e8ca84a3e4
--- /dev/null
+++ b/lldb/test/API/lang/cpp/gmodules/alignment/Makefile
@@ -0,0 +1,4 @@
+PCH_CXX_SOURCE = pch.h
+CXX_SOURCES = main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/gmodules/alignment/TestPchAlignment.py b/lldb/test/API/lang/cpp/gmodules/alignment/TestPchAlignment.py
new file mode 100644
index 00000000000000..535dd13d0ada48
--- /dev/null
+++ b/lldb/test/API/lang/cpp/gmodules/alignment/TestPchAlignment.py
@@ -0,0 +1,60 @@
+"""
+Tests that we correctly track AST layout info
+(specifically alignment) when moving AST nodes
+between ClangASTImporter instances (in this case,
+from pch to executable to expression AST).
+"""
+
+import lldb
+import os
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestPchAlignment(TestBase):
+    @add_test_categories(["gmodules"])
+    def test_expr(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(
+            self, "return data", lldb.SBFileSpec("main.cpp")
+        )
+
+        self.expect(
+            "frame variable data",
+            substrs=["row = 1", "col = 2", "row = 3", "col = 4", "stride = 5"],
+        )
+
+    @add_test_categories(["gmodules"])
+    def test_frame_var(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(
+            self, "return data", lldb.SBFileSpec("main.cpp")
+        )
+
+        self.expect_expr(
+            "data",
+            result_type="MatrixData",
+            result_children=[
+                ValueCheck(
+                    name="section",
+                    children=[
+                        ValueCheck(
+                            name="origin",
+                            children=[
+                                ValueCheck(name="row", value="1"),
+                                ValueCheck(name="col", value="2"),
+                            ],
+                        ),
+                        ValueCheck(
+                            name="size",
+                            children=[
+                                ValueCheck(name="row", value="3"),
+                                ValueCheck(name="col", value="4"),
+                            ],
+                        ),
+                    ],
+                ),
+                ValueCheck(name="stride", value="5"),
+            ],
+        )
diff --git a/lldb/test/API/lang/cpp/gmodules/alignment/main.cpp b/lldb/test/API/lang/cpp/gmodules/alignment/main.cpp
new file mode 100644
index 00000000000000..5481f3fad1ff76
--- /dev/null
+++ b/lldb/test/API/lang/cpp/gmodules/alignment/main.cpp
@@ -0,0 +1,10 @@
+int main(int argc, const char *argv[]) {
+  struct MatrixData data = {0};
+  data.section.origin.row = 1;
+  data.section.origin.col = 2;
+  data.section.size.row = 3;
+  data.section.size.col = 4;
+  data.stride = 5;
+
+  return data.section.size.row;
+}
diff --git a/lldb/test/API/lang/cpp/gmodules/alignment/pch.h b/lldb/test/API/lang/cpp/gmodules/alignment/pch.h
new file mode 100644
index 00000000000000..f0be272aa601aa
--- /dev/null
+++ b/lldb/test/API/lang/cpp/gmodules/alignment/pch.h
@@ -0,0 +1,21 @@
+#ifndef PCH_H_IN
+#define PCH_H_IN
+
+static const int kAlignment = 64;
+
+struct [[gnu::aligned(kAlignment)]] RowCol {
+  unsigned row;
+  unsigned col;
+};
+
+struct [[gnu::aligned(kAlignment)]] Submatrix {
+  struct RowCol origin;
+  struct RowCol size;
+};
+
+struct [[gnu::aligned(kAlignment)]] MatrixData {
+  struct Submatrix section;
+  unsigned stride;
+};
+
+#endif // _H_IN