[PATCH] D82568: [clang][CrossTU] Invalidate parent map after get cross TU definition.
balazske marked an inline comment as done. balazske added inline comments. Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:723 + // Parent map is invalidated after changing the AST. + ToDecl->getASTContext().getParentMapContext().clear(); + balazske wrote: > balazske wrote: > > martong wrote: > > > > ... 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? > > 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.) > `ASTContext::getParentMapContext` does the job. 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`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82568/new/ https://reviews.llvm.org/D82568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82568: [clang][CrossTU] Invalidate parent map after get cross TU definition.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf3b344661048: [clang][CrossTU] Invalidate parent map after
get cross TU definition. (authored by balazske).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82568/new/
https://reviews.llvm.org/D82568
Files:
clang/lib/CrossTU/CrossTranslationUnit.cpp
clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
Index: clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
+++ clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -8,6 +8,7 @@
#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"
@@ -44,6 +45,10 @@
assert(FD && FD->getName() == "f");
bool OrigFDHasBody = FD->hasBody();
+const DynTypedNodeList ParentsBeforeImport =
+Ctx.getParentMapContext().getParents(*FD);
+ASSERT_FALSE(ParentsBeforeImport.empty());
+
// Prepare the index file and the AST file.
int ASTFD;
llvm::SmallString<256> ASTFileName;
@@ -105,10 +110,29 @@
EXPECT_EQ(OrigSLoc, FDWithDefinition->getLocation());
}
}
+
+// Check parent map.
+const DynTypedNodeList ParentsAfterImport =
+Ctx.getParentMapContext().getParents(*FD);
+const DynTypedNodeList ParentsOfImported =
+Ctx.getParentMapContext().getParents(*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;
Index: clang/lib/CrossTU/CrossTranslationUnit.cpp
===
--- clang/lib/CrossTU/CrossTranslationUnit.cpp
+++ clang/lib/CrossTU/CrossTranslationUnit.cpp
@@ -12,6 +12,7 @@
#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"
@@ -718,6 +719,9 @@
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;
}
Index: clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
+++ clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -8,6 +8,7 @@
#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"
@@ -44,6 +45,10 @@
assert(FD && FD->getName() == "f");
bool OrigFDHasBody = FD->hasBody();
+const DynTypedNodeList ParentsBeforeImport =
+Ctx.getParentMapContext().getParents(*FD);
+ASSERT_FALSE(ParentsBeforeImport.empty());
+
// Prepare the index file and the AST file.
int ASTFD;
llvm::SmallString<256> ASTFileName;
@@ -105,10 +110,29 @@
EXPECT_EQ(OrigSLoc, FDWithDefinition->getLocation());
}
}
+
+// Check parent map.
+const DynTypedNodeList ParentsAfterImport =
+Ctx.getParentMapContext().getParents(*FD);
+const DynTypedNodeList ParentsOfImported =
+Ctx.getParentMapContext().getParents(*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;
Index: clang/lib/CrossTU/CrossTranslationUnit.cpp
===
--- clang/lib/CrossTU/CrossTranslationUnit.cpp
+++ clang/lib/CrossTU/CrossTranslationUnit.cpp
@@ -12,6 +12,
[PATCH] D82568: [clang][CrossTU] Invalidate parent map after get cross TU definition.
martong accepted this revision. martong added a comment. 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). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82568/new/ https://reviews.llvm.org/D82568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82568: [clang][CrossTU] Invalidate parent map after get cross TU definition.
balazske marked an inline comment as done. balazske added inline comments. Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:723 + // Parent map is invalidated after changing the AST. + ToDecl->getASTContext().getParentMapContext().clear(); + balazske wrote: > martong wrote: > > > ... 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? > 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.) `ASTContext::getParentMapContext` does the job. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82568/new/ https://reviews.llvm.org/D82568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82568: [clang][CrossTU] Invalidate parent map after get cross TU definition.
balazske marked an inline comment as done. balazske added inline comments. Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:723 + // Parent map is invalidated after changing the AST. + ToDecl->getASTContext().getParentMapContext().clear(); + martong wrote: > > ... 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? 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.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82568/new/ https://reviews.llvm.org/D82568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82568: [clang][CrossTU] Invalidate parent map after get cross TU definition.
martong added inline comments. Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:723 + // Parent map is invalidated after changing the AST. + ToDecl->getASTContext().getParentMapContext().clear(); + > ... 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82568/new/ https://reviews.llvm.org/D82568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82568: [clang][CrossTU] Invalidate parent map after get cross TU definition.
balazske added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82568/new/ https://reviews.llvm.org/D82568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82568: [clang][CrossTU] Invalidate parent map after get cross TU definition.
gamesh411 accepted this revision.
gamesh411 added a comment.
This revision is now accepted and ready to land.
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 getTraversalScope() const { return TraversalScope; }
The setTraversalScope resets the parentmap in ASTContext.cpp:
void ASTContext::setTraversalScope(const std::vector &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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82568/new/
https://reviews.llvm.org/D82568
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82568: [clang][CrossTU] Invalidate parent map after get cross TU definition.
balazske created this revision.
Herald added subscribers: cfe-commits, martong, gamesh411, Szelethus, dkrupp.
Herald added a project: clang.
balazske added reviewers: gamesh411, martong.
Herald added a subscriber: rnkovacs.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D82568
Files:
clang/lib/CrossTU/CrossTranslationUnit.cpp
clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
Index: clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
+++ clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -8,6 +8,7 @@
#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"
@@ -44,6 +45,10 @@
assert(FD && FD->getName() == "f");
bool OrigFDHasBody = FD->hasBody();
+const DynTypedNodeList ParentsBeforeImport =
+Ctx.getParentMapContext().getParents(*FD);
+ASSERT_FALSE(ParentsBeforeImport.empty());
+
// Prepare the index file and the AST file.
int ASTFD;
llvm::SmallString<256> ASTFileName;
@@ -105,10 +110,29 @@
EXPECT_EQ(OrigSLoc, FDWithDefinition->getLocation());
}
}
+
+// Check parent map.
+const DynTypedNodeList ParentsAfterImport =
+Ctx.getParentMapContext().getParents(*FD);
+const DynTypedNodeList ParentsOfImported =
+Ctx.getParentMapContext().getParents(*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;
Index: clang/lib/CrossTU/CrossTranslationUnit.cpp
===
--- clang/lib/CrossTU/CrossTranslationUnit.cpp
+++ clang/lib/CrossTU/CrossTranslationUnit.cpp
@@ -12,6 +12,7 @@
#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"
@@ -718,6 +719,9 @@
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;
}
Index: clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
+++ clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -8,6 +8,7 @@
#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"
@@ -44,6 +45,10 @@
assert(FD && FD->getName() == "f");
bool OrigFDHasBody = FD->hasBody();
+const DynTypedNodeList ParentsBeforeImport =
+Ctx.getParentMapContext().getParents(*FD);
+ASSERT_FALSE(ParentsBeforeImport.empty());
+
// Prepare the index file and the AST file.
int ASTFD;
llvm::SmallString<256> ASTFileName;
@@ -105,10 +110,29 @@
EXPECT_EQ(OrigSLoc, FDWithDefinition->getLocation());
}
}
+
+// Check parent map.
+const DynTypedNodeList ParentsAfterImport =
+Ctx.getParentMapContext().getParents(*FD);
+const DynTypedNodeList ParentsOfImported =
+Ctx.getParentMapContext().getParents(*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
