[Lldb-commits] [lldb] 6807f24 - [ASTImporter] Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[via lldb-commits]

Author: shafik
Date: 2020-09-21T14:57:00-07:00
New Revision: 6807f244fa67bb75ef09fb3db54743b5b358a7fa

URL: https://github.com/llvm/llvm-project/commit/6807f244fa67bb75ef09fb3db54743b5b358a7fa
DIFF: https://github.com/llvm/llvm-project/commit/6807f244fa67bb75ef09fb3db54743b5b358a7fa.diff

LOG: [ASTImporter] Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

When we fixed ImportDeclContext(...) in D71378 to make sure we complete each
FieldDecl of a RecordDecl when we are importing the definition we missed the
case where a FeildDecl was an ArrayType whose ElementType is a record.

This fix was motivated by a codegen crash during LLDB expression parsing. Since
we were not importing the definition we were crashing during layout which
required all the records be defined.

Differential Revision: https://reviews.llvm.org/D86660

Added: 
    lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/Makefile
    lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/TestImportDefinitionArrayType.py
    lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp

Modified: 
    clang/lib/AST/ASTImporter.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 11bf629ecdde2..60b6a9706d6ac 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -1742,12 +1742,28 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
       Decl *ImportedDecl = *ImportedOrErr;
       FieldDecl *FieldTo = dyn_cast_or_null<FieldDecl>(ImportedDecl);
       if (FieldFrom && FieldTo) {
-        const RecordType *RecordFrom = FieldFrom->getType()->getAs<RecordType>();
-        const RecordType *RecordTo = FieldTo->getType()->getAs<RecordType>();
-        if (RecordFrom && RecordTo) {
-          RecordDecl *FromRecordDecl = RecordFrom->getDecl();
-          RecordDecl *ToRecordDecl = RecordTo->getDecl();
+        RecordDecl *FromRecordDecl = nullptr;
+        RecordDecl *ToRecordDecl = nullptr;
+        // If we have a field that is an ArrayType we need to check if the array
+        // element is a RecordDecl and if so we need to import the defintion.
+        if (FieldFrom->getType()->isArrayType()) {
+          // getBaseElementTypeUnsafe(...) handles multi-dimensonal arrays for us.
+          FromRecordDecl = FieldFrom->getType()->getBaseElementTypeUnsafe()->getAsRecordDecl();
+          ToRecordDecl = FieldTo->getType()->getBaseElementTypeUnsafe()->getAsRecordDecl();
+        }
+
+        if (!FromRecordDecl || !ToRecordDecl) {
+          const RecordType *RecordFrom =
+              FieldFrom->getType()->getAs<RecordType>();
+          const RecordType *RecordTo = FieldTo->getType()->getAs<RecordType>();
+
+          if (RecordFrom && RecordTo) {
+            FromRecordDecl = RecordFrom->getDecl();
+            ToRecordDecl = RecordTo->getDecl();
+          }
+        }
 
+        if (FromRecordDecl && ToRecordDecl) {
           if (FromRecordDecl->isCompleteDefinition() &&
               !ToRecordDecl->isCompleteDefinition()) {
             Error Err = ImportDefinition(FromRecordDecl, ToRecordDecl);

diff  --git a/lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/Makefile b/lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

diff  --git a/lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/TestImportDefinitionArrayType.py b/lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/TestImportDefinitionArrayType.py
new file mode 100644
index 0000000000000..a485b94a680dc
--- /dev/null
+++ b/lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/TestImportDefinitionArrayType.py
@@ -0,0 +1,14 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestImportDefinitionArrayType(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.cpp"))
+
+        self.expect_expr("__private->o", result_type="char", result_value="'A'")

diff  --git a/lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp b/lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp
new file mode 100644
index 0000000000000..99fc7b727cc80
--- /dev/null
+++ b/lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp
@@ -0,0 +1,52 @@
+// This is a reproducer for a crash during codegen. The base issue is when we
+// Import the DeclContext we force FieldDecl that are RecordType to be defined
+// since we need these to be defined in order to layout the class.
+// This case involves an array member whose ElementType are records. In this
+// case we need to check the ElementType of an ArrayType and if it is a record
+// we need to import the definition.
+struct A {
+  int x;
+};
+
+struct B {
+  // When we import the all the FieldDecl we need to check if we have an
+  // ArrayType and then check if the ElementType is a RecordDecl and if so
+  // import the defintion. Otherwise during codegen we will attempt to layout A
+  // but won't be able to.
+  A s1[2];
+  A s2[2][2][3];
+  char o;
+};
+
+class FB {
+public:
+  union {
+    struct {
+      unsigned char *_s;
+    } t;
+    char *tt[1];
+  } U;
+
+  FB(B *p) : __private(p) {}
+
+  // We import A but we don't import the definition.
+  void f(A **bounds) {}
+
+  void init();
+
+private:
+  B *__private;
+};
+
+void FB::init() {
+  return; // break here
+}
+
+int main() {
+  B b;
+  FB fb(&b);
+
+  b.o = 'A';
+
+  fb.init();
+}