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

[balazske]

Summary

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

[martong]

Thanks, looks good to me, with a nit in the tests.

clang/unittests/AST/ASTImporterTest.cpp

6233

Instead of .empty(), perhaps we should examine if the found entry is indeed the imported ParmVarDecl.

[balazske]

Added contains for correct check of ASTImporterLookupTable content.

[martong]

In D103231#2795794, @balazske wrote:

Added contains for correct check of ASTImporterLookupTable content.

Okay, that looks good, but I just realized we should not have "bare" assertions. Could you please add some explanatory textual description for the new assertions?

clang/lib/AST/ASTImporterLookupTable.cpp

121

Could you please add some explanatory textual description for the new assertions?

[balazske]

Added assert messages.

[martong]

Thanks!

clang/include/clang/AST/ASTImporterLookupTable.h

class ASTImporterLookupTable {
   void remove(DeclContext *DC, NamedDecl *ND);
 
   DCMap LookupTable;
 
 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;
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_ASTIMPORTERLOOKUPTABLE_H

clang/lib/AST/ASTImporter.cpp

ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   ToFunction->setExplicitlyDefaulted(D->isExplicitlyDefaulted());
   ToFunction->setDeletedAsWritten(D->isDeletedAsWritten());
   ToFunction->setRangeEnd(ToEndLoc);
 
   // Set the parameters.
   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);
 
   // We need to complete creation of FunctionProtoTypeLoc manually with setting
   // params it refers to.
   if (TInfo) {
     if (auto ProtoLoc =
         TInfo->getTypeLoc().IgnoreParens().getAs<FunctionProtoTypeLoc>()) {

clang/lib/AST/ASTImporterLookupTable.cpp

void ASTImporterLookupTable::remove(NamedDecl *ND) {
   assert(ND);
   DeclContext *DC = ND->getDeclContext()->getPrimaryContext();
   remove(DC, ND);
   DeclContext *ReDC = DC->getRedeclContext()->getPrimaryContext();
   if (DC != ReDC)
     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());
   if (DCI == LookupTable.end())
     return {};
 
   const auto &FoundNameMap = DCI->second;
   auto NamesI = FoundNameMap.find(Name);
   if (NamesI == FoundNameMap.end())
     return {};
 
   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())
     llvm::errs() << "empty\n";
   const auto &FoundNameMap = DCI->second;
   for (const auto &Entry : FoundNameMap) {
     DeclarationName Name = Entry.first;
     llvm::errs() << "==== Name: ";

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ErrorHandlingTest, ImportOfOverriddenMethods) {
   };
   CheckError(FromFooA);
   EXPECT_FALSE(Import(FromFooB, Lang_CXX11));
   CheckError(FromFooB);
   EXPECT_FALSE(Import(FromFooC, Lang_CXX11));
   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"(
       void f() {
         auto L = [](){};
       }
       )",
       Lang_CXX11, "input0.cc");
Decl *TU = getTuDecl(
       )",
       Lang_CXX17, "input.cc");
   auto *FromD = FirstDeclMatcher<CXXDeductionGuideDecl>().match(
       TU, cxxDeductionGuideDecl());
   auto *ToD = Import(FromD, Lang_CXX17);
   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
 // implementation file and only then could we reuse the Fixture in other AST
 // unitttests.
 struct CTAD : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(CTAD, DeductionGuideShouldReferToANonLocalTypedef) {