[clang] e953669 - [ASTImporter] Fields are imported first and reordered for correct layout
via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 18 02:08:25 PDT 2023
Author: dingfei
Date: 2023-07-18T17:07:19+08:00
New Revision: e9536698720ec524cc8b72599363622bc1a31558
URL: https://github.com/llvm/llvm-project/commit/e9536698720ec524cc8b72599363622bc1a31558
DIFF: https://github.com/llvm/llvm-project/commit/e9536698720ec524cc8b72599363622bc1a31558.diff
LOG: [ASTImporter] Fields are imported first and reordered for correct layout
Fields are imported first and reordered for correct layout.
For partially imported record, layout computation is incorrect.
Differential Revision: https://reviews.llvm.org/D154764
Added:
Modified:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 429fa5829eb4f2..39c7a8fa397048 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -433,6 +433,7 @@ namespace clang {
Decl *From, DeclContext *&ToDC, DeclContext *&ToLexicalDC);
Error ImportImplicitMethods(const CXXRecordDecl *From, CXXRecordDecl *To);
+ Error ImportFieldDeclDefinition(const FieldDecl *From, const FieldDecl *To);
Expected<CXXCastPath> ImportCastPath(CastExpr *E);
Expected<APValue> ImportAPValue(const APValue &FromValue);
@@ -1850,52 +1851,33 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
//
diff erent values in two distinct translation units.
ChildErrorHandlingStrategy HandleChildErrors(FromDC);
+ auto MightNeedReordering = [](const Decl *D) {
+ return isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D) || isa<FriendDecl>(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<FieldDecl>(From);
- Decl *ImportedDecl = *ImportedOrErr;
- FieldDecl *FieldTo = dyn_cast_or_null<FieldDecl>(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<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);
- HandleChildErrors.handleChildImportResult(ChildErrors,
- std::move(Err));
- }
- }
- }
- } else {
+ if (!ImportedOrErr) {
HandleChildErrors.handleChildImportResult(ChildErrors,
ImportedOrErr.takeError());
+ continue;
+ }
+ FieldDecl *FieldFrom = dyn_cast_or_null<FieldDecl>(From);
+ Decl *ImportedDecl = *ImportedOrErr;
+ FieldDecl *FieldTo = dyn_cast_or_null<FieldDecl>(ImportedDecl);
+ if (FieldFrom && FieldTo) {
+ Error Err = ImportFieldDeclDefinition(FieldFrom, FieldTo);
+ HandleChildErrors.handleChildImportResult(ChildErrors, std::move(Err));
}
}
@@ -1910,7 +1892,7 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
// 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 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
// 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<RecordDecl>(FromDC);
- if (!FromRD)
- return ChildErrors;
auto ToDCOrErr = Importer.ImportContext(FromDC);
if (!ToDCOrErr) {
@@ -1935,26 +1914,70 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
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<FieldDecl>(D) || isa<IndirectFieldDecl>(D) || isa<FriendDecl>(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<RecordType>();
+ const RecordType *RecordTo = ToType->getAs<RecordType>();
+
+ 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
index 91b1b868af9f85..3a1058f5e3fe90 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -8007,6 +8007,27 @@ TEST_P(ASTImporterOptionSpecificTestBase, ImportDeductionGuideDifferentOrder) {
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<CXXMethodDecl>().match(
+ FromTU, cxxMethodDecl(hasName("A::m")));
+ Import(FromF, Lang_CXX11);
+}
+
TEST_P(ASTImporterOptionSpecificTestBase,
ImportRecordWithLayoutRequestingExpr) {
TranslationUnitDecl *FromTU = getTuDecl(
More information about the cfe-commits
mailing list