[clang][CrossTU] Invalidate parent map after get cross TU definition.

[balazske]

Summary

Parent map of ASTContext is built once. If this happens and later
the TU is modified by getCrossTUDefinition the parent map does not
contain the newly imported objects and has to be re-created.

Invalidation of the parent map is added to the CrossTranslationUnitContext.
It could be added to ASTImporter as well but for now this task remains the
responsibility of the user of ASTImporter. Reason for this is mostly that
ASTImporter calls itself recursively.

[gamesh411]

I have found this in ASTContext.h:

// A traversal scope limits the parts of the AST visible to certain analyses.
   // RecursiveASTVisitor::TraverseAST will only visit reachable nodes, and
   // getParents() will only observe reachable parent edges.
   //
   // The scope is defined by a set of "top-level" declarations.
   // Initially, it is the entire TU: {getTranslationUnitDecl()}.
   // Changing the scope clears the parent cache, which is expensive to rebuild.
   std::vector<Decl *> getTraversalScope() const { return TraversalScope; }

The setTraversalScope resets the parentmap in ASTContext.cpp:

void ASTContext::setTraversalScope(const std::vector<Decl *> &TopLevelDecls) {
   TraversalScope = TopLevelDecls;
   getParentMapContext().clear();
 }

It seems to me that not resetting this cache could very well cause a bad AST being available.
Just curious: Is there an example that you are aware of, where this caused an actual problem?

[balazske]

I did not found problems related to traversal scope. At AST import Decl's are added only, but the existing ones should remain valid. So the top-level decls in TraversalScope should remain valid after import.

[martong]

clang/lib/CrossTU/CrossTranslationUnit.cpp

723

... the TU is modified by getCrossTUDefinition the parent map does not

contain the newly imported objects and has to be re-created.

I see we clear it here, but when/where will be the parent map re-created?

[balazske]

clang/lib/CrossTU/CrossTranslationUnit.cpp

723

It should be created at any access to it. (Actually I did not check how this works after the test showed that the fix works.)

[balazske]

clang/lib/CrossTU/CrossTranslationUnit.cpp

723

ASTContext::getParentMapContext does the job.

[martong]

Ok, I checked and it seems the parent map context is used only by the AST matchers, and this is okay because it looks like there is not any storing or caching of the parent map objects (the type is never copied).

[balazske]

clang/lib/CrossTU/CrossTranslationUnit.cpp

723

More precisely, ParentMapContext::getParents re-creates the parent map after it was cleared. The parent maps in the ParentMapContext are cleared, not the parent map pointer in ASTContext.

clang/lib/CrossTU/CrossTranslationUnit.cpp

 //===--- CrossTranslationUnit.cpp - -----------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements the CrossTranslationUnit interface.
 //
 //===----------------------------------------------------------------------===//
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/AST/ASTImporter.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CrossTU/CrossTUDiagnostic.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Index/USRGeneration.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/Statistic.h"
handleAllErrors(ToDeclOrError.takeError(),
                       }
                     });
     return llvm::make_error<IndexError>(index_error_code::failed_import);
   }
   auto *ToDecl = cast<T>(*ToDeclOrError);
   assert(hasBodyOrInit(ToDecl) && "Imported Decl should have body or init.");
   ++NumGetCTUSuccess;
 
+  // Parent map is invalidated after changing the AST.
+  ToDecl->getASTContext().getParentMapContext().clear();
+
   return ToDecl;
 }
 
 llvm::Expected<const FunctionDecl *>
 CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD,
                                               ASTUnit *Unit) {
   return importDefinitionImpl(FD, Unit);
 }

clang/unittests/CrossTU/CrossTranslationUnitTest.cpp

 //===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "gtest/gtest.h"
auto FindFInTU = [](const TranslationUnitDecl *TU) {
       return FD;
     };
 
     const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
     const FunctionDecl *FD = FindFInTU(TU);
     assert(FD && FD->getName() == "f");
     bool OrigFDHasBody = FD->hasBody();
 
+    const DynTypedNodeList ParentsBeforeImport =
+        Ctx.getParentMapContext().getParents<Decl>(*FD);
+    ASSERT_FALSE(ParentsBeforeImport.empty());
+
     // Prepare the index file and the AST file.
     int ASTFD;
     llvm::SmallString<256> ASTFileName;
     ASSERT_FALSE(
         llvm::sys::fs::createTemporaryFile("f_ast", "ast", ASTFD, ASTFileName));
     llvm::ToolOutputFile ASTFile(ASTFileName, ASTFD);
 
     int IndexFD;
if (NewFDorError) {
           const FunctionDecl *FDWithDefinition = FindFInTU(OrigTU);
           EXPECT_TRUE(FDWithDefinition);
           if (FDWithDefinition) {
             EXPECT_EQ(FDWithDefinition->getName(), "f");
             EXPECT_TRUE(FDWithDefinition->isThisDeclarationADefinition());
             EXPECT_EQ(OrigSLoc, FDWithDefinition->getLocation());
           }
         }
-      }
-    }
+
+        // Check parent map.
+        const DynTypedNodeList ParentsAfterImport =
+            Ctx.getParentMapContext().getParents<Decl>(*FD);
+        const DynTypedNodeList ParentsOfImported =
+            Ctx.getParentMapContext().getParents<Decl>(*NewFD);
+        EXPECT_TRUE(
+            checkParentListsEq(ParentsBeforeImport, ParentsAfterImport));
+        EXPECT_FALSE(ParentsOfImported.empty());
+      }
+
+  static bool checkParentListsEq(const DynTypedNodeList &L1,
+                                 const DynTypedNodeList &L2) {
+    if (L1.size() != L2.size())
+      return false;
+    for (unsigned int I = 0; I < L1.size(); ++I)
+      if (L1[I] != L2[I])
+        return false;
+    return true;
   }
 
 private:
   CrossTranslationUnitContext CTU;
   bool *Success;
 };
 
 class CTUAction : public clang::ASTFrontendAction {