[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
This revision was automatically updated to reflect the committed changes.
Closed by commit rL342384: [ASTImporter] Fix import of VarDecl init (authored
by martong, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D51597
Files:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -107,9 +107,11 @@
}
SmallVector getCanonicalForwardRedeclChain(Decl* D) {
-// Currently only FunctionDecl is supported
-auto FD = cast(D);
-return getCanonicalForwardRedeclChain(FD);
+if (auto *FD = dyn_cast(D))
+ return getCanonicalForwardRedeclChain(FD);
+if (auto *VD = dyn_cast(D))
+ return getCanonicalForwardRedeclChain(VD);
+llvm_unreachable("Bad declaration kind");
}
void updateFlags(const Decl *From, Decl *To) {
@@ -280,10 +282,9 @@
(IDK == IDK_Default && !Importer.isMinimalImport());
}
+bool ImportInitializer(VarDecl *From, VarDecl *To);
bool ImportDefinition(RecordDecl *From, RecordDecl *To,
ImportDefinitionKind Kind = IDK_Default);
-bool ImportDefinition(VarDecl *From, VarDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
bool ImportDefinition(EnumDecl *From, EnumDecl *To,
ImportDefinitionKind Kind = IDK_Default);
bool ImportDefinition(ObjCInterfaceDecl *From, ObjCInterfaceDecl *To,
@@ -1424,18 +1425,26 @@
return false;
}
-bool ASTNodeImporter::ImportDefinition(VarDecl *From, VarDecl *To,
- ImportDefinitionKind Kind) {
+bool ASTNodeImporter::ImportInitializer(VarDecl *From, VarDecl *To) {
if (To->getAnyInitializer())
return false;
- // FIXME: Can we really import any initializer? Alternatively, we could force
- // ourselves to import every declaration of a variable and then only use
- // getInit() here.
- To->setInit(Importer.Import(const_cast(From->getAnyInitializer(;
+ Expr *FromInit = From->getInit();
+ if (!FromInit)
+return false;
- // FIXME: Other bits to merge?
+ Expr *ToInit = Importer.Import(const_cast(FromInit));
+ if (!ToInit)
+return true;
+ To->setInit(ToInit);
+ if (From->isInitKnownICE()) {
+EvaluatedStmt *Eval = To->ensureEvaluatedStmt();
+Eval->CheckedICE = true;
+Eval->IsICE = From->isInitICE();
+ }
+
+ // FIXME: Other bits to merge?
return false;
}
@@ -3138,6 +3147,16 @@
}
Decl *ASTNodeImporter::VisitVarDecl(VarDecl *D) {
+
+ SmallVector Redecls = getCanonicalForwardRedeclChain(D);
+ auto RedeclIt = Redecls.begin();
+ // Import the first part of the decl chain. I.e. import all previous
+ // declarations starting from the canonical decl.
+ for (; RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt)
+if (!Importer.Import(*RedeclIt))
+ return nullptr;
+ assert(*RedeclIt == D);
+
// Import the major distinguishing characteristics of a variable.
DeclContext *DC, *LexicalDC;
DeclarationName Name;
@@ -3150,8 +3169,8 @@
// Try to find a variable in our own ("to") context with the same name and
// in the same context as the variable we're importing.
+ VarDecl *FoundByLookup = nullptr;
if (D->isFileVarDecl()) {
-VarDecl *MergeWithVar = nullptr;
SmallVector ConflictingDecls;
unsigned IDNS = Decl::IDNS_Ordinary;
SmallVector FoundDecls;
@@ -3166,7 +3185,23 @@
D->hasExternalFormalLinkage()) {
if (Importer.IsStructurallyEquivalent(D->getType(),
FoundVar->getType())) {
-MergeWithVar = FoundVar;
+
+// The VarDecl in the "From" context has a definition, but in the
+// "To" context we already have a definition.
+VarDecl *FoundDef = FoundVar->getDefinition();
+if (D->isThisDeclarationADefinition() && FoundDef)
+ // FIXME Check for ODR error if the two definitions have
+ // different initializers?
+ return Importer.MapImported(D, FoundDef);
+
+// The VarDecl in the "From" context has an initializer, but in the
+// "To" context we already have an initializer.
+const VarDecl *FoundDInit = nullptr;
+if (D->getInit() && FoundVar->getAnyInitializer(FoundDInit))
+ // FIXME Diagnose ODR error if the two initializers are different?
+ return Importer.MapImported(D, const_cast(FoundDInit));
+
+FoundByLookup = FoundVar;
break;
}
@@ -3183,11 +3218,11 @@
return nullptr;
FoundVar->setType(T);
- MergeWithVar = FoundVar;
+ FoundByLookup = FoundVar;
break;
} else if (isa
[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
a_sidorin accepted this revision.
a_sidorin added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/AST/ASTImporter.cpp:1441
+ To->setInit(ToInit);
+ if (From->isInitKnownICE()) {
+EvaluatedStmt *Eval = To->ensureEvaluatedStmt();
martong wrote:
> a_sidorin wrote:
> > I see that this is only a code move but I realized that ASTReader and
> > ASTImporter handle this case differently. ASTReader code says:
> >
> > ```
> > if (Val > 1) { // IsInitKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
> > EvaluatedStmt *Eval = VD->ensureEvaluatedStmt();
> > Eval->CheckedICE = true;
> > Eval->IsICE = Val == 3;
> > }
> > ```
> > but ASTimporter sets these bits only if `isInitKnownICE()` is `true`. This
> > looks strange.
> The comment in ASTReader seems to be wrong and misleading.
> I checked the correspondent code in ASTWriter:
> ```
> Record.push_back(!D->isInitKnownICE() ? 1 : (D->isInitICE() ? 3 : 2));
> ```
> Thus, the comment in ASTReader should be:
> ```
> if (Val > 1) { // IsInitNOTKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
> ```
> So, `Val > 1` means that the original init expression written by the
> ASTWriter had the ICE-ness already determined.
> Thus the ASTImporter code seems correct to me.
Thank you for checking this!
The reason I was worrying about this code is that ASTReader/Writer are used in
XTU as well so a mismatch between them and ASTImporter can cost us some
annoying debug in the future.
Repository:
rC Clang
https://reviews.llvm.org/D51597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
martong updated this revision to Diff 165039.
martong marked an inline comment as done.
martong added a comment.
- Fix formatting and typo
Repository:
rC Clang
https://reviews.llvm.org/D51597
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1828,13 +1828,62 @@
{
Decl *FromTU =
getTuDecl("extern int x; int f() { return x; }", Lang_CXX, "input2.cc");
-auto *FromD =
-FirstDeclMatcher().match(FromTU, functionDecl());
+auto *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
Import(FromD, Lang_CXX);
}
EXPECT_TRUE(Imported2->isUsed(false));
}
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag2) {
+ auto Pattern = varDecl(hasName("x"));
+ VarDecl *ExistingD;
+ {
+Decl *ToTU = getToTuDecl("int x = 1;", Lang_CXX);
+ExistingD = FirstDeclMatcher().match(ToTU, Pattern);
+ }
+ EXPECT_FALSE(ExistingD->isUsed(false));
+ {
+Decl *FromTU = getTuDecl(
+"int x = 1; int f() { return x; }", Lang_CXX, "input1.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+ }
+ EXPECT_TRUE(ExistingD->isUsed(false));
+}
+
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag3) {
+ auto Pattern = varDecl(hasName("a"));
+ VarDecl *ExistingD;
+ {
+Decl *ToTU = getToTuDecl(
+R"(
+struct A {
+ static const int a = 1;
+};
+)", Lang_CXX);
+ExistingD = FirstDeclMatcher().match(ToTU, Pattern);
+ }
+ EXPECT_FALSE(ExistingD->isUsed(false));
+ {
+Decl *FromTU = getTuDecl(
+R"(
+struct A {
+ static const int a = 1;
+};
+const int *f() { return &A::a; } // requires storage,
+ // thus used flag will be set
+)", Lang_CXX, "input1.cc");
+auto *FromFunD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+auto *FromD = FirstDeclMatcher().match(FromTU, Pattern);
+ASSERT_TRUE(FromD->isUsed(false));
+Import(FromFunD, Lang_CXX);
+ }
+ EXPECT_TRUE(ExistingD->isUsed(false));
+}
+
TEST_P(ASTImporterTestBase, ReimportWithUsedFlag) {
auto Pattern = varDecl(hasName("x"));
@@ -3244,6 +3293,94 @@
EXPECT_TRUE(ToInitExpr->isGLValue());
}
+struct ImportVariables : ASTImporterTestBase {};
+
+TEST_P(ImportVariables, ImportOfOneDeclBringsInTheWholeChain) {
+ Decl *FromTU = getTuDecl(
+ R"(
+ struct A {
+static const int a = 1 + 2;
+ };
+ const int A::a;
+ )", Lang_CXX, "input1.cc");
+
+ auto *FromDWithInit = FirstDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with init
+ auto *FromDWithDef = LastDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with definition
+ ASSERT_NE(FromDWithInit, FromDWithDef);
+ ASSERT_EQ(FromDWithDef->getPreviousDecl(), FromDWithInit);
+
+ auto *ToD0 = cast(Import(FromDWithInit, Lang_CXX11));
+ auto *ToD1 = cast(Import(FromDWithDef, Lang_CXX11));
+ ASSERT_TRUE(ToD0);
+ ASSERT_TRUE(ToD1);
+ EXPECT_NE(ToD0, ToD1);
+ EXPECT_EQ(ToD1->getPreviousDecl(), ToD0);
+}
+
+TEST_P(ImportVariables, InitAndDefinitionAreInDifferentTUs) {
+ auto StructA =
+ R"(
+ struct A {
+static const int a = 1 + 2;
+ };
+ )";
+ Decl *ToTU = getToTuDecl(StructA, Lang_CXX);
+ Decl *FromTU = getTuDecl(std::string(StructA) + "const int A::a;", Lang_CXX,
+ "input1.cc");
+
+ auto *FromDWithInit = FirstDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with init
+ auto *FromDWithDef = LastDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with definition
+ ASSERT_EQ(FromDWithInit, FromDWithDef->getPreviousDecl());
+ ASSERT_TRUE(FromDWithInit->getInit());
+ ASSERT_FALSE(FromDWithInit->isThisDeclarationADefinition());
+ ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition());
+ ASSERT_FALSE(FromDWithDef->getInit());
+
+ auto *ToD = FirstDeclMatcher().match(
+ ToTU, varDecl(hasName("a"))); // Decl with init
+ ASSERT_TRUE(ToD->getInit());
+ ASSERT_FALSE(ToD->getDefinition());
+
+ auto *ImportedD = cast(Import(FromDWithDef, Lang_CXX11));
+ EXPECT_TRUE(ImportedD->getAnyInitializer());
+ EXPECT_TRUE(ImportedD->getDefinition());
+}
+
+TEST_P(ImportVariables, InitAndDefinitionAreInTheFromContext) {
+ auto StructA =
+ R"(
+ struct A {
+static const int a;
+ };
+ )";
+ Decl *ToTU = getToTuDecl(StructA, Lang_CXX);
+ Decl *FromTU = getTuDecl(std::string(StructA) + "const int A::a = 1 + 2;",
+ Lang_CXX, "input1.cc");
+
+ auto *FromDDeclarationOnly = FirstDeclMatcher().match(
+ FromTU, varDecl(hasName("a")));
+ auto *FromDWithDef = LastDeclMatcher().match(
+
[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
martong marked 3 inline comments as done.
martong added inline comments.
Comment at: lib/AST/ASTImporter.cpp:1441
+ To->setInit(ToInit);
+ if (From->isInitKnownICE()) {
+EvaluatedStmt *Eval = To->ensureEvaluatedStmt();
a_sidorin wrote:
> I see that this is only a code move but I realized that ASTReader and
> ASTImporter handle this case differently. ASTReader code says:
>
> ```
> if (Val > 1) { // IsInitKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
> EvaluatedStmt *Eval = VD->ensureEvaluatedStmt();
> Eval->CheckedICE = true;
> Eval->IsICE = Val == 3;
> }
> ```
> but ASTimporter sets these bits only if `isInitKnownICE()` is `true`. This
> looks strange.
The comment in ASTReader seems to be wrong and misleading.
I checked the correspondent code in ASTWriter:
```
Record.push_back(!D->isInitKnownICE() ? 1 : (D->isInitICE() ? 3 : 2));
```
Thus, the comment in ASTReader should be:
```
if (Val > 1) { // IsInitNOTKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
```
So, `Val > 1` means that the original init expression written by the ASTWriter
had the ICE-ness already determined.
Thus the ASTImporter code seems correct to me.
Repository:
rC Clang
https://reviews.llvm.org/D51597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
a_sidorin added a comment.
Hi Gabor,
The change looks mostly fine but the difference with ASTReader approach
disturbs me a bit.
Comment at: lib/AST/ASTImporter.cpp:1441
+ To->setInit(ToInit);
+ if (From->isInitKnownICE()) {
+EvaluatedStmt *Eval = To->ensureEvaluatedStmt();
I see that this is only a code move but I realized that ASTReader and
ASTImporter handle this case differently. ASTReader code says:
```
if (Val > 1) { // IsInitKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
EvaluatedStmt *Eval = VD->ensureEvaluatedStmt();
Eval->CheckedICE = true;
Eval->IsICE = Val == 3;
}
```
but ASTimporter sets these bits only if `isInitKnownICE()` is `true`. This
looks strange.
Comment at: lib/AST/ASTImporter.cpp:3190
+// The VarDecl in the "From" context has a definition, but in the
+// "To" context we already has a definition.
+VarDecl *FoundDef = FoundVar->getDefinition();
have (same below)
Comment at: unittests/AST/ASTImporterTest.cpp:3312
+ ASSERT_NE(FromDWithInit, FromDWithDef);
+ ASSERT_EQ(FromDWithDef->getPreviousDecl() ,FromDWithInit);
+
Formatting of comma is broken. Same below.
Repository:
rC Clang
https://reviews.llvm.org/D51597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
martong added inline comments. Comment at: unittests/AST/ASTImporterTest.cpp:3763 +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportImplicitMethods, +DefaultTestValuesForRunOptions, ); This hunk has nothing to do with this change, but previously we forgot to instantiate these test cases :( Repository: rC Clang https://reviews.llvm.org/D51597 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
martong created this revision.
martong added reviewers: a_sidorin, xazax.hun, r.stahl.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: a.sidorin.
The init expression of a VarDecl is overwritten in the "To" context if we
import a VarDecl without an init expression (and with a definition). Please
refer to the added tests, especially InitAndDefinitionAreInDifferentTUs. This
patch fixes the malfunction by importing the whole Decl chain similarly as we
did that in case of FunctionDecls. We handle the init expression similarly to
a definition, alas only one init expression will be in the merged ast.
Repository:
rC Clang
https://reviews.llvm.org/D51597
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1828,13 +1828,62 @@
{
Decl *FromTU =
getTuDecl("extern int x; int f() { return x; }", Lang_CXX, "input2.cc");
-auto *FromD =
-FirstDeclMatcher().match(FromTU, functionDecl());
+auto *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
Import(FromD, Lang_CXX);
}
EXPECT_TRUE(Imported2->isUsed(false));
}
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag2) {
+ auto Pattern = varDecl(hasName("x"));
+ VarDecl *ExistingD;
+ {
+Decl *ToTU = getToTuDecl("int x = 1;", Lang_CXX);
+ExistingD = FirstDeclMatcher().match(ToTU, Pattern);
+ }
+ EXPECT_FALSE(ExistingD->isUsed(false));
+ {
+Decl *FromTU = getTuDecl(
+"int x = 1; int f() { return x; }", Lang_CXX, "input1.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+ }
+ EXPECT_TRUE(ExistingD->isUsed(false));
+}
+
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag3) {
+ auto Pattern = varDecl(hasName("a"));
+ VarDecl *ExistingD;
+ {
+Decl *ToTU = getToTuDecl(
+R"(
+struct A {
+ static const int a = 1;
+};
+)", Lang_CXX);
+ExistingD = FirstDeclMatcher().match(ToTU, Pattern);
+ }
+ EXPECT_FALSE(ExistingD->isUsed(false));
+ {
+Decl *FromTU = getTuDecl(
+R"(
+struct A {
+ static const int a = 1;
+};
+const int *f() { return &A::a; } // requires storage,
+ // thus used flag will be set
+)", Lang_CXX, "input1.cc");
+auto *FromFunD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+auto *FromD = FirstDeclMatcher().match(FromTU, Pattern);
+ASSERT_TRUE(FromD->isUsed(false));
+Import(FromFunD, Lang_CXX);
+ }
+ EXPECT_TRUE(ExistingD->isUsed(false));
+}
+
TEST_P(ASTImporterTestBase, ReimportWithUsedFlag) {
auto Pattern = varDecl(hasName("x"));
@@ -3244,6 +3293,94 @@
EXPECT_TRUE(ToInitExpr->isGLValue());
}
+struct ImportVariables : ASTImporterTestBase {};
+
+TEST_P(ImportVariables, ImportOfOneDeclBringsInTheWholeChain) {
+ Decl *FromTU = getTuDecl(
+ R"(
+ struct A {
+static const int a = 1 + 2;
+ };
+ const int A::a;
+ )", Lang_CXX, "input1.cc");
+
+ auto *FromDWithInit = FirstDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with init
+ auto *FromDWithDef = LastDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with definition
+ ASSERT_NE(FromDWithInit, FromDWithDef);
+ ASSERT_EQ(FromDWithDef->getPreviousDecl() ,FromDWithInit);
+
+ auto *ToD0 = cast(Import(FromDWithInit, Lang_CXX11));
+ auto *ToD1 = cast(Import(FromDWithDef, Lang_CXX11));
+ ASSERT_TRUE(ToD0);
+ ASSERT_TRUE(ToD1);
+ EXPECT_NE(ToD0, ToD1);
+ EXPECT_EQ(ToD1->getPreviousDecl() ,ToD0);
+}
+
+TEST_P(ImportVariables, InitAndDefinitionAreInDifferentTUs) {
+ auto StructA =
+ R"(
+ struct A {
+static const int a = 1 + 2;
+ };
+ )";
+ Decl *ToTU = getToTuDecl(StructA, Lang_CXX);
+ Decl *FromTU = getTuDecl(std::string(StructA) + "const int A::a;", Lang_CXX,
+ "input1.cc");
+
+ auto *FromDWithInit = FirstDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with init
+ auto *FromDWithDef = LastDeclMatcher().match(
+ FromTU, varDecl(hasName("a"))); // Decl with definition
+ ASSERT_EQ(FromDWithInit, FromDWithDef->getPreviousDecl());
+ ASSERT_TRUE(FromDWithInit->getInit());
+ ASSERT_FALSE(FromDWithInit->isThisDeclarationADefinition());
+ ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition());
+ ASSERT_FALSE(FromDWithDef->getInit());
+
+ auto *ToD = FirstDeclMatcher().match(
+ ToTU, varDecl(hasName("a"))); // Decl with init
+ ASSERT_TRUE(ToD->getInit());
+ ASSERT_FALSE(ToD->getDefinition());
+
+ auto *ImportedD = cast(Import(FromDWithDef, Lang_CXX11));
+ EXPECT_TRUE(ImportedD->getAnyInitializer());
+ EX
