[clang][AST] Set correct DeclContext in ASTImporter lookup table for ParmVarDecl.

ParmVarDecl is created with translation unit as the parent DeclContext
and later moved to the correct DeclContext. ASTImporterLookupTable
should be updated at this move.

Reviewed By: martong

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

diff --git a/clang/include/clang/AST/ASTImporterLookupTable.h b/clang/include/clang/AST/ASTImporterLookupTable.h
index 407478a..47dca20 100644 (file)
--- a/clang/include/clang/AST/ASTImporterLookupTable.h
+++ b/clang/include/clang/AST/ASTImporterLookupTable.h
@@ -63,8 +63,24 @@ public:
   ASTImporterLookupTable(TranslationUnitDecl &TU);
   void add(NamedDecl *ND);
   void remove(NamedDecl *ND);
+  // Sometimes a declaration is created first with a temporarily value of decl
+  // context (often the translation unit) and later moved to the final context.
+  // This happens for declarations that are created before the final declaration
+  // context. In such cases the lookup table needs to be updated.
+  // (The declaration is in these cases not added to the temporary decl context,
+  // only its parent is set.)
+  // FIXME: It would be better to not add the declaration to the temporary
+  // context at all in the lookup table, but this requires big change in
+  // ASTImporter.
+  // The function should be called when the old context is definitely different
+  // from the new.
+  void update(NamedDecl *ND, DeclContext *OldDC);
   using LookupResult = DeclList;
   LookupResult lookup(DeclContext *DC, DeclarationName Name) const;
+  // Check if the `ND` is within the lookup table (with its current name) in
+  // context `DC`. This is intended for debug purposes when the DeclContext of a
+  // NamedDecl is changed.
+  bool contains(DeclContext *DC, NamedDecl *ND) const;
   void dump(DeclContext *DC) const;
   void dump() const;
 };

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c0fc376..66f384a 100644 (file)
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -3503,6 +3503,8 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   for (auto *Param : Parameters) {
     Param->setOwningFunction(ToFunction);
     ToFunction->addDeclInternal(Param);
+    if (ASTImporterLookupTable *LT = Importer.SharedState->getLookupTable())
+      LT->update(Param, Importer.getToContext().getTranslationUnitDecl());
   }
   ToFunction->setParams(Parameters);
 

diff --git a/clang/lib/AST/ASTImporterLookupTable.cpp b/clang/lib/AST/ASTImporterLookupTable.cpp
index e17d608..b78cc0c 100644 (file)
--- a/clang/lib/AST/ASTImporterLookupTable.cpp
+++ b/clang/lib/AST/ASTImporterLookupTable.cpp
@@ -117,6 +117,19 @@ void ASTImporterLookupTable::remove(NamedDecl *ND) {
     remove(ReDC, ND);
 }
 
+void ASTImporterLookupTable::update(NamedDecl *ND, DeclContext *OldDC) {
+  assert(OldDC != ND->getDeclContext() &&
+         "DeclContext should be changed before update");
+  if (contains(ND->getDeclContext(), ND)) {
+    assert(!contains(OldDC, ND) &&
+           "Decl should not be found in the old context if already in the new");
+    return;
+  }
+
+  remove(OldDC, ND);
+  add(ND);
+}
+
 ASTImporterLookupTable::LookupResult
 ASTImporterLookupTable::lookup(DeclContext *DC, DeclarationName Name) const {
   auto DCI = LookupTable.find(DC->getPrimaryContext());
@@ -131,6 +144,10 @@ ASTImporterLookupTable::lookup(DeclContext *DC, DeclarationName Name) const {
   return NamesI->second;
 }
 
+bool ASTImporterLookupTable::contains(DeclContext *DC, NamedDecl *ND) const {
+  return 0 < lookup(DC, ND->getDeclName()).count(ND);
+}
+
 void ASTImporterLookupTable::dump(DeclContext *DC) const {
   auto DCI = LookupTable.find(DC->getPrimaryContext());
   if (DCI == LookupTable.end())

diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index db87b2a..4ed4a09 100644 (file)
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -5375,6 +5375,33 @@ TEST_P(ErrorHandlingTest, ImportOfOverriddenMethods) {
   CheckError(FromFooC);
 }
 
+TEST_P(ErrorHandlingTest, ODRViolationWithinParmVarDecls) {
+  // Importing of 'f' and parameter 'P' should cause an ODR error.
+  // The error happens after the ParmVarDecl for 'P' was already created.
+  // This is a special case because the ParmVarDecl has a temporary DeclContext.
+  // Expected is no crash at error handling of ASTImporter.
+  constexpr auto ToTUCode = R"(
+      struct X {
+        char A;
+      };
+      )";
+  constexpr auto FromTUCode = R"(
+      struct X {
+        enum Y { Z };
+      };
+      void f(int P = X::Z);
+      )";
+  Decl *ToTU = getToTuDecl(ToTUCode, Lang_CXX11);
+  static_cast<void>(ToTU);
+  Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11);
+  auto *FromF = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(FromF);
+
+  auto *ImportedF = Import(FromF, Lang_CXX11);
+  EXPECT_FALSE(ImportedF);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
       R"(
@@ -6192,6 +6219,21 @@ TEST_P(ImportFunctions, CTADWithLocalTypedef) {
   ASSERT_TRUE(ToD);
 }
 
+TEST_P(ImportFunctions, ParmVarDeclDeclContext) {
+  constexpr auto FromTUCode = R"(
+      void f(int P);
+      )";
+  Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11);
+  auto *FromF = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(FromF);
+
+  auto *ImportedF = Import(FromF, Lang_CXX11);
+  EXPECT_TRUE(ImportedF);
+  EXPECT_TRUE(SharedStatePtr->getLookupTable()->contains(
+      ImportedF, ImportedF->getParamDecl(0)));
+}
+
 // FIXME Move these tests out of ASTImporterTest. For that we need to factor
 // out the ASTImporter specific pars from ASTImporterOptionSpecificTestBase
 // into a new test Fixture. Then we should lift up this Fixture to its own