diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -433,6 +433,7 @@ Decl *From, DeclContext *&ToDC, DeclContext *&ToLexicalDC); Error ImportImplicitMethods(const CXXRecordDecl *From, CXXRecordDecl *To); + Error ImportFieldDeclDefinition(const FieldDecl *From, const FieldDecl *To); Expected ImportCastPath(CastExpr *E); Expected ImportAPValue(const APValue &FromValue); @@ -1850,52 +1851,33 @@ // different values in two distinct translation units. ChildErrorHandlingStrategy HandleChildErrors(FromDC); + auto MightNeedReordering = [](const Decl *D) { + return isa(D) || isa(D) || isa(D); + }; + + // Import everything that might need reordering first. Error ChildErrors = Error::success(); for (auto *From : FromDC->decls()) { + if (!MightNeedReordering(From)) + continue; + ExpectedDecl ImportedOrErr = import(From); // If we are in the process of ImportDefinition(...) for a RecordDecl we // want to make sure that we are also completing each FieldDecl. There // are currently cases where this does not happen and this is correctness // fix since operations such as code generation will expect this to be so. - if (ImportedOrErr) { - FieldDecl *FieldFrom = dyn_cast_or_null(From); - Decl *ImportedDecl = *ImportedOrErr; - FieldDecl *FieldTo = dyn_cast_or_null(ImportedDecl); - if (FieldFrom && FieldTo) { - 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 definition. - 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(); - const RecordType *RecordTo = FieldTo->getType()->getAs(); - - if (RecordFrom && RecordTo) { - FromRecordDecl = RecordFrom->getDecl(); - ToRecordDecl = RecordTo->getDecl(); - } - } - - if (FromRecordDecl && ToRecordDecl) { - if (FromRecordDecl->isCompleteDefinition() && - !ToRecordDecl->isCompleteDefinition()) { - Error Err = ImportDefinition(FromRecordDecl, ToRecordDecl); - HandleChildErrors.handleChildImportResult(ChildErrors, - std::move(Err)); - } - } - } - } else { + if (!ImportedOrErr) { HandleChildErrors.handleChildImportResult(ChildErrors, ImportedOrErr.takeError()); + continue; + } + FieldDecl *FieldFrom = dyn_cast_or_null(From); + Decl *ImportedDecl = *ImportedOrErr; + FieldDecl *FieldTo = dyn_cast_or_null(ImportedDecl); + if (FieldFrom && FieldTo) { + Error Err = ImportFieldDeclDefinition(FieldFrom, FieldTo); + HandleChildErrors.handleChildImportResult(ChildErrors, std::move(Err)); } } @@ -1910,7 +1892,7 @@ // During the import of `a` we import first the dependencies in sequence, // thus the order would be `c`, `b`, `a`. We will get the normal order by // first removing the already imported members and then adding them in the - // order as they apper in the "from" context. + // order as they appear in the "from" context. // // Keeping field order is vital because it determines structure layout. // @@ -1922,9 +1904,6 @@ // interface in LLDB is implemented by the means of the ASTImporter. However, // calling an import at this point would result in an uncontrolled import, we // must avoid that. - const auto *FromRD = dyn_cast(FromDC); - if (!FromRD) - return ChildErrors; auto ToDCOrErr = Importer.ImportContext(FromDC); if (!ToDCOrErr) { @@ -1935,26 +1914,70 @@ DeclContext *ToDC = *ToDCOrErr; // Remove all declarations, which may be in wrong order in the // lexical DeclContext and then add them in the proper order. - for (auto *D : FromRD->decls()) { - if (isa(D) || isa(D) || isa(D)) { - assert(D && "DC contains a null decl"); - Decl *ToD = Importer.GetAlreadyImportedOrNull(D); + for (auto *D : FromDC->decls()) { + if (!MightNeedReordering(D)) + continue; + + assert(D && "DC contains a null decl"); + if (Decl *ToD = Importer.GetAlreadyImportedOrNull(D)) { // Remove only the decls which we successfully imported. - if (ToD) { - assert(ToDC == ToD->getLexicalDeclContext() && ToDC->containsDecl(ToD)); - // Remove the decl from its wrong place in the linked list. - ToDC->removeDecl(ToD); - // Add the decl to the end of the linked list. - // This time it will be at the proper place because the enclosing for - // loop iterates in the original (good) order of the decls. - ToDC->addDeclInternal(ToD); - } + assert(ToDC == ToD->getLexicalDeclContext() && ToDC->containsDecl(ToD)); + // Remove the decl from its wrong place in the linked list. + ToDC->removeDecl(ToD); + // Add the decl to the end of the linked list. + // This time it will be at the proper place because the enclosing for + // loop iterates in the original (good) order of the decls. + ToDC->addDeclInternal(ToD); } } + // Import everything else. + for (auto *From : FromDC->decls()) { + if (MightNeedReordering(From)) + continue; + + ExpectedDecl ImportedOrErr = import(From); + if (!ImportedOrErr) + HandleChildErrors.handleChildImportResult(ChildErrors, + ImportedOrErr.takeError()); + } + return ChildErrors; } +Error ASTNodeImporter::ImportFieldDeclDefinition(const FieldDecl *From, + const FieldDecl *To) { + 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 definition. + QualType FromType = From->getType(); + QualType ToType = To->getType(); + if (FromType->isArrayType()) { + // getBaseElementTypeUnsafe(...) handles multi-dimensonal arrays for us. + FromRecordDecl = FromType->getBaseElementTypeUnsafe()->getAsRecordDecl(); + ToRecordDecl = ToType->getBaseElementTypeUnsafe()->getAsRecordDecl(); + } + + if (!FromRecordDecl || !ToRecordDecl) { + const RecordType *RecordFrom = FromType->getAs(); + const RecordType *RecordTo = ToType->getAs(); + + if (RecordFrom && RecordTo) { + FromRecordDecl = RecordFrom->getDecl(); + ToRecordDecl = RecordTo->getDecl(); + } + } + + if (FromRecordDecl && ToRecordDecl) { + if (FromRecordDecl->isCompleteDefinition() && + !ToRecordDecl->isCompleteDefinition()) + return ImportDefinition(FromRecordDecl, ToRecordDecl); + } + + return Error::success(); +} + Error ASTNodeImporter::ImportDeclContext( Decl *FromD, DeclContext *&ToDC, DeclContext *&ToLexicalDC) { auto ToDCOrErr = Importer.ImportContext(FromD->getDeclContext()); diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -8007,6 +8007,27 @@ ToDGOther); } +TEST_P(ASTImporterOptionSpecificTestBase, + ImportFieldsFirstForCorrectRecordLayoutTest) { + // UnaryOperator(&) triggers RecordLayout computation, which relies on + // correctly imported fields. + auto Code = + R"( + class A { + int m() { + return &((A *)0)->f1 - &((A *)0)->f2; + } + int f1; + int f2; + }; + )"; + Decl *FromTU = getTuDecl(Code, Lang_CXX11); + + auto *FromF = FirstDeclMatcher().match( + FromTU, cxxMethodDecl(hasName("A::m"))); + Import(FromF, Lang_CXX11); +} + TEST_P(ASTImporterOptionSpecificTestBase, ImportRecordWithLayoutRequestingExpr) { TranslationUnitDecl *FromTU = getTuDecl(