r342384 - [ASTImporter] Fix import of VarDecl init

Gabor Marton via cfe-commits cfe-commits at lists.llvm.org
Mon Sep 17 05:04:52 PDT 2018


Author: martong
Date: Mon Sep 17 05:04:52 2018
New Revision: 342384

URL: http://llvm.org/viewvc/llvm-project?rev=342384&view=rev
Log:
[ASTImporter] Fix import of VarDecl init

Summary:
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.

Reviewers: a_sidorin, xazax.hun, r.stahl, a.sidorin

Subscribers: rnkovacs, dkrupp, cfe-commits

Differential Revision: https://reviews.llvm.org/D51597

Modified:
    cfe/trunk/lib/AST/ASTImporter.cpp
    cfe/trunk/unittests/AST/ASTImporterTest.cpp

Modified: cfe/trunk/lib/AST/ASTImporter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=342384&r1=342383&r2=342384&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ASTImporter.cpp (original)
+++ cfe/trunk/lib/AST/ASTImporter.cpp Mon Sep 17 05:04:52 2018
@@ -107,9 +107,11 @@ namespace clang {
   }
 
   SmallVector<Decl*, 2> getCanonicalForwardRedeclChain(Decl* D) {
-    // Currently only FunctionDecl is supported
-    auto FD = cast<FunctionDecl>(D);
-    return getCanonicalForwardRedeclChain<FunctionDecl>(FD);
+    if (auto *FD = dyn_cast<FunctionDecl>(D))
+      return getCanonicalForwardRedeclChain<FunctionDecl>(FD);
+    if (auto *VD = dyn_cast<VarDecl>(D))
+      return getCanonicalForwardRedeclChain<VarDecl>(VD);
+    llvm_unreachable("Bad declaration kind");
   }
 
   void updateFlags(const Decl *From, Decl *To) {
@@ -280,10 +282,9 @@ namespace clang {
              (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 @@ bool ASTNodeImporter::ImportDefinition(R
   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<Expr *>(From->getAnyInitializer())));
+  Expr *FromInit = From->getInit();
+  if (!FromInit)
+    return false;
 
-  // FIXME: Other bits to merge?
+  Expr *ToInit = Importer.Import(const_cast<Expr *>(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::VisitObjCIvarDecl
 }
 
 Decl *ASTNodeImporter::VisitVarDecl(VarDecl *D) {
+
+  SmallVector<Decl*, 2> 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 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD
 
   // 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<NamedDecl *, 4> ConflictingDecls;
     unsigned IDNS = Decl::IDNS_Ordinary;
     SmallVector<NamedDecl *, 2> FoundDecls;
@@ -3166,7 +3185,23 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD
             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<VarDecl*>(FoundDInit));
+
+            FoundByLookup = FoundVar;
             break;
           }
 
@@ -3183,11 +3218,11 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD
                 return nullptr;
 
               FoundVar->setType(T);
-              MergeWithVar = FoundVar;
+              FoundByLookup = FoundVar;
               break;
             } else if (isa<IncompleteArrayType>(TArray) &&
                        isa<ConstantArrayType>(FoundArray)) {
-              MergeWithVar = FoundVar;
+              FoundByLookup = FoundVar;
               break;
             }
           }
@@ -3202,32 +3237,6 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD
       ConflictingDecls.push_back(FoundDecl);
     }
 
-    if (MergeWithVar) {
-      // An equivalent variable with external linkage has been found. Link
-      // the two declarations, then merge them.
-      Importer.MapImported(D, MergeWithVar);
-      updateFlags(D, MergeWithVar);
-
-      if (VarDecl *DDef = D->getDefinition()) {
-        if (VarDecl *ExistingDef = MergeWithVar->getDefinition()) {
-          Importer.ToDiag(ExistingDef->getLocation(),
-                          diag::err_odr_variable_multiple_def)
-            << Name;
-          Importer.FromDiag(DDef->getLocation(), diag::note_odr_defined_here);
-        } else {
-          Expr *Init = Importer.Import(DDef->getInit());
-          MergeWithVar->setInit(Init);
-          if (DDef->isInitKnownICE()) {
-            EvaluatedStmt *Eval = MergeWithVar->ensureEvaluatedStmt();
-            Eval->CheckedICE = true;
-            Eval->IsICE = DDef->isInitICE();
-          }
-        }
-      }
-
-      return MergeWithVar;
-    }
-
     if (!ConflictingDecls.empty()) {
       Name = Importer.HandleNameConflict(Name, DC, IDNS,
                                          ConflictingDecls.data(),
@@ -3255,17 +3264,27 @@ Decl *ASTNodeImporter::VisitVarDecl(VarD
   ToVar->setAccess(D->getAccess());
   ToVar->setLexicalDeclContext(LexicalDC);
 
-  // Templated declarations should never appear in the enclosing DeclContext.
-  if (!D->getDescribedVarTemplate())
-    LexicalDC->addDeclInternal(ToVar);
+  if (FoundByLookup) {
+    auto *Recent = const_cast<VarDecl *>(FoundByLookup->getMostRecentDecl());
+    ToVar->setPreviousDecl(Recent);
+  }
 
-  // Merge the initializer.
-  if (ImportDefinition(D, ToVar))
+  if (ImportInitializer(D, ToVar))
     return nullptr;
 
   if (D->isConstexpr())
     ToVar->setConstexpr(true);
 
+  if (D->getDeclContext()->containsDeclAndLoad(D))
+    DC->addDeclInternal(ToVar);
+  if (DC != LexicalDC && D->getLexicalDeclContext()->containsDeclAndLoad(D))
+    LexicalDC->addDeclInternal(ToVar);
+
+  // Import the rest of the chain. I.e. import all subsequent declarations.
+  for (++RedeclIt; RedeclIt != Redecls.end(); ++RedeclIt)
+    if (!Importer.Import(*RedeclIt))
+      return nullptr;
+
   return ToVar;
 }
 
@@ -4914,12 +4933,7 @@ Decl *ASTNodeImporter::VisitVarTemplateS
     D2->setAccess(D->getAccess());
   }
 
-  // NOTE: isThisDeclarationADefinition() can return DeclarationOnly even if
-  // declaration has initializer. Should this be fixed in the AST?.. Anyway,
-  // we have to check the declaration for initializer - otherwise, it won't be
-  // imported.
-  if ((D->isThisDeclarationADefinition() || D->hasInit()) &&
-      ImportDefinition(D, D2))
+  if (ImportInitializer(D, D2))
     return nullptr;
 
   return D2;
@@ -7084,6 +7098,7 @@ Decl *ASTImporter::Import(Decl *FromD) {
   // Notify subclasses.
   Imported(FromD, ToD);
 
+  updateFlags(FromD, ToD);
   return ToD;
 }
 

Modified: cfe/trunk/unittests/AST/ASTImporterTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTImporterTest.cpp?rev=342384&r1=342383&r2=342384&view=diff
==============================================================================
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp (original)
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp Mon Sep 17 05:04:52 2018
@@ -1828,13 +1828,62 @@ TEST_P(ASTImporterTestBase, ImportDoesUp
   {
     Decl *FromTU =
         getTuDecl("extern int x; int f() { return x; }", Lang_CXX, "input2.cc");
-    auto *FromD =
-        FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+    auto *FromD = FirstDeclMatcher<FunctionDecl>().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<VarDecl>().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<FunctionDecl>().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<VarDecl>().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<FunctionDecl>().match(
+        FromTU, functionDecl(hasName("f")));
+    auto *FromD = FirstDeclMatcher<VarDecl>().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 @@ TEST_P(ASTImporterTestBase, InitListExpr
   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<VarDecl>().match(
+      FromTU, varDecl(hasName("a"))); // Decl with init
+  auto *FromDWithDef = LastDeclMatcher<VarDecl>().match(
+      FromTU, varDecl(hasName("a"))); // Decl with definition
+  ASSERT_NE(FromDWithInit, FromDWithDef);
+  ASSERT_EQ(FromDWithDef->getPreviousDecl(), FromDWithInit);
+
+  auto *ToD0 = cast<VarDecl>(Import(FromDWithInit, Lang_CXX11));
+  auto *ToD1 = cast<VarDecl>(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<VarDecl>().match(
+      FromTU, varDecl(hasName("a"))); // Decl with init
+  auto *FromDWithDef = LastDeclMatcher<VarDecl>().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<VarDecl>().match(
+      ToTU, varDecl(hasName("a"))); // Decl with init
+  ASSERT_TRUE(ToD->getInit());
+  ASSERT_FALSE(ToD->getDefinition());
+
+  auto *ImportedD = cast<VarDecl>(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<VarDecl>().match(
+      FromTU, varDecl(hasName("a")));
+  auto *FromDWithDef = LastDeclMatcher<VarDecl>().match(
+      FromTU, varDecl(hasName("a"))); // Decl with definition and with init.
+  ASSERT_EQ(FromDDeclarationOnly, FromDWithDef->getPreviousDecl());
+  ASSERT_FALSE(FromDDeclarationOnly->getInit());
+  ASSERT_FALSE(FromDDeclarationOnly->isThisDeclarationADefinition());
+  ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition());
+  ASSERT_TRUE(FromDWithDef->getInit());
+
+  auto *ToD = FirstDeclMatcher<VarDecl>().match(
+      ToTU, varDecl(hasName("a")));
+  ASSERT_FALSE(ToD->getInit());
+  ASSERT_FALSE(ToD->getDefinition());
+
+  auto *ImportedD = cast<VarDecl>(Import(FromDWithDef, Lang_CXX11));
+  EXPECT_TRUE(ImportedD->getAnyInitializer());
+  EXPECT_TRUE(ImportedD->getDefinition());
+}
+
 struct DeclContextTest : ASTImporterTestBase {};
 
 TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
@@ -3623,5 +3760,11 @@ INSTANTIATE_TEST_CASE_P(ParameterizedTes
                         ImportFunctionTemplateSpecializations,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportImplicitMethods,
+                        DefaultTestValuesForRunOptions, );
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
+                        DefaultTestValuesForRunOptions, );
+
 } // end namespace ast_matchers
 } // end namespace clang




More information about the cfe-commits mailing list