Skip to content

Commit

Permalink
[ASTImporter] Fields are imported first and reordered for correct layout
Browse files Browse the repository at this point in the history 
Fields are imported first and reordered for correct layout.
For partially imported record, layout computation is incorrect.

Differential Revision: https://reviews.llvm.org/D154764
  • Loading branch information
@danix800
danix800 committed Jul 18, 2023
1 parent 3be16bd commit e953669
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 53 deletions.
129 changes: 76 additions & 53 deletions clang/lib/AST/ASTImporter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -1850,52 +1851,33 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
// different 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));
}
}

Expand All @@ -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.
//
Expand All @@ -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) {
Expand All @@ -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());
Expand Down
21 changes: 21 additions & 0 deletions clang/unittests/AST/ASTImporterTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit e953669

Please sign in to comment.