shafik created this revision.
shafik added reviewers: martong, teemperor.
Herald added a subscriber: rnkovacs.
shafik requested review of this revision.
When we fixed `ImportDeclContext(...)` in D71378
<https://reviews.llvm.org/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.
https://reviews.llvm.org/D86660
Files:
clang/lib/AST/ASTImporter.cpp
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
Index: lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp
@@ -0,0 +1,51 @@
+// 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 s[2];
+ 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();
+}
Index: lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/TestImportDefinitionArrayType.py
===================================================================
--- /dev/null
+++ 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'")
Index: lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1743,12 +1743,34 @@
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.
+ const ArrayType *ArrayFrom =
+ FieldFrom->getType()->getAsArrayTypeUnsafe();
+ const ArrayType *ArrayTo = FieldTo->getType()->getAsArrayTypeUnsafe();
+
+ if (ArrayFrom && ArrayTo) {
+ QualType FromTy = ArrayFrom->getElementType();
+ QualType ToTy = ArrayTo->getElementType();
+
+ FromRecordDecl = FromTy->getAsRecordDecl();
+ ToRecordDecl = ToTy->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);
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
