danix800 created this revision.
danix800 added a project: clang.
Herald added a subscriber: martong.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: All.
danix800 requested review of this revision.
Herald added a subscriber: cfe-commits.
Fixes two cases on fields circular refs:
1. Field in-class initializer is deferred until all fields are imported. No
reordering for fields is needed. For final lexical order, see
https://reviews.llvm.org/D156093
2. UnaryOperator(&)'s creation might need layout of some records whose fields
importation are still on fly, the layout is incorrectly computed and cached.
Clients relying on this will not work properly or crash direclty (e.g
StaticAnalyzer's MemRegion.cpp (calculateOffset)). Use
UnaryOperator::CreateEmpty() instead of UnaryOperator::Create() to avoid this
computation.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156201
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//
+#include "clang/AST/RecordLayout.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/Support/SmallVectorMemoryBuffer.h"
@@ -1539,6 +1540,83 @@
Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"}))));
}
+TEST_P(ASTImporterOptionSpecificTestBase, FieldCircularRef) {
+ Decl *From, *To;
+ std::tie(From, To) = getImportedDecl(
+ R"s(
+ struct declToImport {
+ int a{a};
+ int b = c;
+ int c = b + c + a;
+ };
+ )s",
+ Lang_CXX11, "", Lang_CXX11);
+
+ auto FieldBWithInit = has(fieldDecl(
+ hasName("b"),
+ hasInClassInitializer(ignoringImpCasts(memberExpr(
+ hasObjectExpression(cxxThisExpr()), member(hasName("c")))))));
+ auto FieldCWithInit = has(fieldDecl(
+ hasName("c"),
+ hasInClassInitializer(binaryOperator(hasLHS(binaryOperator())))));
+ auto Pattern = cxxRecordDecl(FieldBWithInit, FieldCWithInit,
+ hasFieldOrder({"a", "b", "c"}));
+ MatchVerifier<Decl> Verifier;
+ ASSERT_TRUE(Verifier.match(From, Pattern));
+ EXPECT_TRUE(Verifier.match(To, Pattern));
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase,
+ ImportFieldDirectlyWithInitializerContainingCircularRef) {
+ auto Code = R"s(
+ struct declToImport {
+ int a{a};
+ int b = c;
+ int c = b + c + a;
+ };
+ )s";
+
+ auto *FromField = FirstDeclMatcher<FieldDecl>().match(
+ getTuDecl(Code, Lang_CXX11), fieldDecl(hasName("b")));
+ auto *ToField = Import(FromField, Lang_CXX11);
+
+ auto Pattern = fieldDecl(
+ hasName("b"),
+ hasInClassInitializer(ignoringImpCasts(memberExpr(
+ hasObjectExpression(cxxThisExpr()), member(hasName("c"))))));
+
+ MatchVerifier<Decl> Verifier;
+ ASSERT_TRUE(Verifier.match(FromField, Pattern));
+ EXPECT_TRUE(Verifier.match(ToField, Pattern));
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, FieldCircularIndirectRef) {
+ Decl *From, *To;
+ std::tie(From, To) = getImportedDecl(
+ R"s(
+ static int ref_A();
+ static int ref_B();
+ struct A {
+ int a = ref_B();
+ };
+ struct B {
+ int b = ref_A();
+ };
+ int ref_B() { B b; return b.b; }
+ int ref_A() { A a; return a.a; }
+ )s",
+ Lang_CXX11, "", Lang_CXX11, "A");
+
+ auto InitByRefB =
+ hasInClassInitializer(callExpr(callee(functionDecl(hasName("ref_B")))));
+ auto FieldAWithInit = has(fieldDecl(hasName("a"), InitByRefB));
+ auto Pattern =
+ cxxRecordDecl(hasName("A"), FieldAWithInit, hasFieldOrder({"a"}));
+ MatchVerifier<Decl> Verifier;
+ ASSERT_TRUE(Verifier.match(From, Pattern));
+ EXPECT_TRUE(Verifier.match(To, Pattern));
+}
+
TEST_P(ASTImporterOptionSpecificTestBase,
CXXRecordDeclFieldAndIndirectFieldOrder) {
Decl *From, *To;
@@ -8028,6 +8106,45 @@
Import(FromF, Lang_CXX11);
}
+TEST_P(ASTImporterOptionSpecificTestBase,
+ ImportCirularRefFieldsWithoutCorruptedRecordLayoutCacheTest) {
+ // Import sequence: A => A.b => B => B.f() => ... => UnaryOperator(&) => ...
+ //
+ // UnaryOperator(&) force computing RecordLayout of 'A' while it's still not
+ // completely imported.
+ auto Code =
+ R"(
+ class B;
+ class A {
+ B* b;
+ int c;
+ };
+ class B {
+ A *f() { return &((B *)0)->a; }
+ A a;
+ };
+ )";
+
+ auto *FromR = FirstDeclMatcher<CXXRecordDecl>().match(
+ getTuDecl(Code, Lang_CXX11), cxxRecordDecl(hasName("A")));
+ FromR = FromR->getDefinition();
+ auto &FromAST = FromR->getASTContext();
+ auto *ToR = Import(FromR, Lang_CXX11);
+ auto &ToAST = ToR->getASTContext();
+
+ uint64_t SecondFieldOffset = FromAST.getTypeSize(FromAST.VoidPtrTy);
+
+ EXPECT_TRUE(FromR->isCompleteDefinition());
+ const auto &FromLayout = FromAST.getASTRecordLayout(FromR);
+ EXPECT_TRUE(FromLayout.getFieldOffset(0) == 0);
+ EXPECT_TRUE(FromLayout.getFieldOffset(1) == SecondFieldOffset);
+
+ EXPECT_TRUE(ToR->isCompleteDefinition());
+ const auto &ToLayout = ToAST.getASTRecordLayout(ToR);
+ EXPECT_TRUE(ToLayout.getFieldOffset(0) == 0);
+ EXPECT_TRUE(ToLayout.getFieldOffset(1) == SecondFieldOffset);
+}
+
TEST_P(ASTImporterOptionSpecificTestBase,
ImportRecordWithLayoutRequestingExpr) {
TranslationUnitDecl *FromTU = getTuDecl(
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1881,30 +1881,6 @@
}
}
- // We reorder declarations in RecordDecls because they may have another order
- // in the "to" context than they have in the "from" context. This may happen
- // e.g when we import a class like this:
- // struct declToImport {
- // int a = c + b;
- // int b = 1;
- // int c = 2;
- // };
- // 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 appear in the "from" context.
- //
- // Keeping field order is vital because it determines structure layout.
- //
- // Here and below, we cannot call field_begin() method and its callers on
- // ToDC if it has an external storage. Calling field_begin() will
- // automatically load all the fields by calling
- // LoadFieldsFromExternalStorage(). LoadFieldsFromExternalStorage() would
- // call ASTImporter::Import(). This is because the ExternalASTSource
- // 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.
-
auto ToDCOrErr = Importer.ImportContext(FromDC);
if (!ToDCOrErr) {
consumeError(std::move(ChildErrors));
@@ -1912,23 +1888,26 @@
}
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 : FromDC->decls()) {
- if (!MightNeedReordering(D))
- continue;
+ // Import in-class initializers for fields.
+ for (auto *D : FromDC->decls()) {
assert(D && "DC contains a null decl");
- if (Decl *ToD = Importer.GetAlreadyImportedOrNull(D)) {
- // Remove only the decls which we successfully imported.
- 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);
- }
+ auto *FromField = dyn_cast<FieldDecl>(D);
+ if (!FromField || !FromField->hasInClassInitializer())
+ continue;
+ Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+ if (!ToD || ToDC != ToD->getLexicalDeclContext() ||
+ !ToDC->containsDecl(ToD))
+ continue;
+ auto *ToField = cast<FieldDecl>(ToD);
+ Error Err = Error::success();
+ auto ToInitializer = importChecked(Err, FromField->getInClassInitializer());
+ // Might already been imported.
+ if (ToField->getInClassInitializer())
+ continue;
+ if (ToInitializer)
+ ToField->setInClassInitializer(ToInitializer);
+ HandleChildErrors.handleChildImportResult(ChildErrors, std::move(Err));
}
// Import everything else.
@@ -3925,7 +3904,7 @@
auto ToTInfo = importChecked(Err, D->getTypeSourceInfo());
auto ToBitWidth = importChecked(Err, D->getBitWidth());
auto ToInnerLocStart = importChecked(Err, D->getInnerLocStart());
- auto ToInitializer = importChecked(Err, D->getInClassInitializer());
+ // Deferring in-class initializer to avoid circular refs
if (Err)
return std::move(Err);
const Type *ToCapturedVLAType = nullptr;
@@ -3948,8 +3927,6 @@
return std::move(Err);
ToField->setAccess(D->getAccess());
ToField->setLexicalDeclContext(LexicalDC);
- if (ToInitializer)
- ToField->setInClassInitializer(ToInitializer);
ToField->setImplicit(D->isImplicit());
if (ToCapturedVLAType)
ToField->setCapturedVLAType(cast<VariableArrayType>(ToCapturedVLAType));
@@ -7394,10 +7371,15 @@
if (Err)
return std::move(Err);
- return UnaryOperator::Create(
- Importer.getToContext(), ToSubExpr, E->getOpcode(), ToType,
- E->getValueKind(), E->getObjectKind(), ToOperatorLoc, E->canOverflow(),
- E->getFPOptionsOverride());
+ auto *UO = UnaryOperator::CreateEmpty(Importer.getToContext(),
+ E->hasStoredFPFeatures());
+ UO->setType(ToType);
+ UO->setSubExpr(ToSubExpr);
+ UO->setOpcode(E->getOpcode());
+ UO->setOperatorLoc(ToOperatorLoc);
+ UO->setCanOverflow(E->canOverflow());
+
+ return UO;
}
ExpectedStmt
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
