martong updated this revision to Diff 207288.
martong marked an inline comment as done.
martong added a comment.
- Move ImportPathTy's funcitons in-class, add class comment
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62375/new/
https://reviews.llvm.org/D62375
Files:
clang/include/clang/AST/ASTImporter.h
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -358,6 +358,106 @@
EXPECT_EQ(0U, count);
}
+struct ImportPath : ASTImporterOptionSpecificTestBase {
+ Decl *FromTU;
+ FunctionDecl *D0, *D1, *D2;
+ ImportPath() {
+ FromTU = getTuDecl("void f(); void f(); void f();", Lang_CXX);
+ auto Pattern = functionDecl(hasName("f"));
+ D0 = FirstDeclMatcher<FunctionDecl>().match(FromTU, Pattern);
+ D2 = LastDeclMatcher<FunctionDecl>().match(FromTU, Pattern);
+ D1 = D2->getPreviousDecl();
+ }
+};
+
+TEST_P(ImportPath, Push) {
+ ASTImporter::ImportPathTy path;
+ path.push(D0);
+ EXPECT_FALSE(path.hasCycleAtBack());
+}
+
+TEST_P(ImportPath, SmallCycle) {
+ ASTImporter::ImportPathTy path;
+ path.push(D0);
+ path.push(D0);
+ EXPECT_TRUE(path.hasCycleAtBack());
+ path.pop();
+ EXPECT_FALSE(path.hasCycleAtBack());
+ path.push(D0);
+ EXPECT_TRUE(path.hasCycleAtBack());
+}
+
+TEST_P(ImportPath, GetSmallCycle) {
+ ASTImporter::ImportPathTy path;
+ path.push(D0);
+ path.push(D0);
+ EXPECT_TRUE(path.hasCycleAtBack());
+ std::array<Decl* ,2> Res;
+ int i = 0;
+ for (Decl *Di : path.getCycleAtBack()) {
+ Res[i++] = Di;
+ }
+ ASSERT_EQ(i, 2);
+ EXPECT_EQ(Res[0], D0);
+ EXPECT_EQ(Res[1], D0);
+}
+
+TEST_P(ImportPath, GetCycle) {
+ ASTImporter::ImportPathTy path;
+ path.push(D0);
+ path.push(D1);
+ path.push(D2);
+ path.push(D0);
+ EXPECT_TRUE(path.hasCycleAtBack());
+ std::array<Decl* ,4> Res;
+ int i = 0;
+ for (Decl *Di : path.getCycleAtBack()) {
+ Res[i++] = Di;
+ }
+ ASSERT_EQ(i, 4);
+ EXPECT_EQ(Res[0], D0);
+ EXPECT_EQ(Res[1], D2);
+ EXPECT_EQ(Res[2], D1);
+ EXPECT_EQ(Res[3], D0);
+}
+
+TEST_P(ImportPath, CycleAfterCycle) {
+ ASTImporter::ImportPathTy path;
+ path.push(D0);
+ path.push(D1);
+ path.push(D0);
+ path.push(D1);
+ path.push(D2);
+ path.push(D0);
+ EXPECT_TRUE(path.hasCycleAtBack());
+ std::array<Decl* ,4> Res;
+ int i = 0;
+ for (Decl *Di : path.getCycleAtBack()) {
+ Res[i++] = Di;
+ }
+ ASSERT_EQ(i, 4);
+ EXPECT_EQ(Res[0], D0);
+ EXPECT_EQ(Res[1], D2);
+ EXPECT_EQ(Res[2], D1);
+ EXPECT_EQ(Res[3], D0);
+
+ path.pop();
+ path.pop();
+ path.pop();
+ EXPECT_TRUE(path.hasCycleAtBack());
+ i = 0;
+ for (Decl *Di : path.getCycleAtBack()) {
+ Res[i++] = Di;
+ }
+ ASSERT_EQ(i, 3);
+ EXPECT_EQ(Res[0], D0);
+ EXPECT_EQ(Res[1], D1);
+ EXPECT_EQ(Res[2], D0);
+
+ path.pop();
+ EXPECT_FALSE(path.hasCycleAtBack());
+}
+
TEST_P(ImportExpr, ImportStringLiteral) {
MatchVerifier<Decl> Verifier;
testImport(
@@ -4547,12 +4647,6 @@
EXPECT_EQ(*Res.begin(), A);
}
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
- ::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(
- ParameterizedTests, CanonicalRedeclChain,
- ::testing::Values(ArgVector()),);
// FIXME This test is disabled currently, upcoming patches will make it
// possible to enable.
@@ -4770,19 +4864,99 @@
CXXMethodDecl *ImportedF = Import(FromF, Lang_CXX);
EXPECT_FALSE(ImportedF);
- // There is no error set for ok().
+ // There is an error set for the other member too.
auto *FromOK = FirstDeclMatcher<CXXMethodDecl>().match(
FromTU, cxxMethodDecl(hasName("ok")));
OptErr = Importer->getImportDeclErrorIfAny(FromOK);
- EXPECT_FALSE(OptErr);
- // And we should be able to import.
+ EXPECT_TRUE(OptErr);
+ // Cannot import the other member.
CXXMethodDecl *ImportedOK = Import(FromOK, Lang_CXX);
- EXPECT_TRUE(ImportedOK);
+ EXPECT_FALSE(ImportedOK);
+}
+
+// Check that an error propagates to the dependent AST nodes.
+// In the below code it means that an error in X should propagate to A.
+// And even to F since the containing A is erroneous.
+// And to all AST nodes which we visit during the import process which finally
+// ends up in a failure (in the error() function).
+TEST_P(ErrorHandlingTest, ErrorPropagatesThroughImportCycles) {
+ Decl *FromTU = getTuDecl(
+ std::string(R"(
+ namespace NS {
+ class A {
+ template <int I> class F {};
+ class X {
+ template <int I> friend class F;
+ void error() { )") + ErroneousStmt + R"( }
+ };
+ };
+
+ class B {};
+ } // NS
+ )",
+ Lang_CXX, "input0.cc");
+
+ auto *FromFRD = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("F"), isDefinition()));
+ auto *FromA = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("A"), isDefinition()));
+ auto *FromB = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("B"), isDefinition()));
+ auto *FromNS = FirstDeclMatcher<NamespaceDecl>().match(
+ FromTU, namespaceDecl(hasName("NS")));
+
+ // Start by importing the templated CXXRecordDecl of F.
+ // Import fails for that.
+ EXPECT_FALSE(Import(FromFRD, Lang_CXX));
+ // Import fails for A.
+ EXPECT_FALSE(Import(FromA, Lang_CXX));
+ // But we should be able to import the independent B.
+ EXPECT_TRUE(Import(FromB, Lang_CXX));
+ // And the namespace.
+ EXPECT_TRUE(Import(FromNS, Lang_CXX));
+
+ // An error is set to the templated CXXRecordDecl of F.
+ ASTImporter *Importer = findFromTU(FromFRD)->Importer.get();
+ Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromFRD);
+ EXPECT_TRUE(OptErr);
+
+ // An error is set to A.
+ OptErr = Importer->getImportDeclErrorIfAny(FromA);
+ EXPECT_TRUE(OptErr);
+
+ // There is no error set to B.
+ OptErr = Importer->getImportDeclErrorIfAny(FromB);
+ EXPECT_FALSE(OptErr);
- // Unwary clients may access X even if the error is set, so, at least make
- // sure the class is set to be complete.
- CXXRecordDecl *ToX = cast<CXXRecordDecl>(ImportedOK->getDeclContext());
- EXPECT_TRUE(ToX->isCompleteDefinition());
+ // There is no error set to NS.
+ OptErr = Importer->getImportDeclErrorIfAny(FromNS);
+ EXPECT_FALSE(OptErr);
+
+ // Check some of those decls whose ancestor is X, they all should have an
+ // error set if we visited them during an import process which finally failed.
+ // These decls are part of a cycle in an ImportPath.
+ // There would not be any error set for these decls if we hadn't follow the
+ // ImportPaths and the cycles.
+ OptErr = Importer->getImportDeclErrorIfAny(
+ FirstDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("F"))));
+ // An error is set to the 'F' ClassTemplateDecl.
+ EXPECT_TRUE(OptErr);
+ // An error is set to the FriendDecl.
+ OptErr = Importer->getImportDeclErrorIfAny(
+ FirstDeclMatcher<FriendDecl>().match(
+ FromTU, friendDecl()));
+ EXPECT_TRUE(OptErr);
+ // An error is set to the implicit class of A.
+ OptErr =
+ Importer->getImportDeclErrorIfAny(FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("A"), isImplicit())));
+ EXPECT_TRUE(OptErr);
+ // An error is set to the implicit class of X.
+ OptErr =
+ Importer->getImportDeclErrorIfAny(FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("X"), isImplicit())));
+ EXPECT_TRUE(OptErr);
}
TEST_P(ErrorHandlingTest, ErrorIsNotPropagatedFromMemberToNamespace) {
@@ -4828,9 +5002,18 @@
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
DefaultTestValuesForRunOptions, );
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
+ ::testing::Values(ArgVector()), );
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
+ ::testing::Values(ArgVector()), );
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
DefaultTestValuesForRunOptions, );
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
+ ::testing::Values(ArgVector()), );
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
DefaultTestValuesForRunOptions, );
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -7842,6 +7842,10 @@
if (!FromD)
return nullptr;
+ // Push FromD to the stack, and remove that when we return.
+ ImportPath.push(FromD);
+ auto ImportPathBuilder =
+ llvm::make_scope_exit([this]() { ImportPath.pop(); });
// Check whether there was a previous failed import.
// If yes return the existing error.
@@ -7853,6 +7857,10 @@
if (ToD) {
// If FromD has some updated flags after last import, apply it
updateFlags(FromD, ToD);
+ // If we encounter a cycle during an import then we save the relevant part
+ // of the import path associated to the Decl.
+ if (ImportPath.hasCycleAtBack())
+ SavedImportPaths[FromD].push_back(ImportPath.copyCycleAtBack());
return ToD;
}
@@ -7889,16 +7897,20 @@
// FIXME: AST may contain remaining references to the failed object.
}
- // Error encountered for the first time.
- assert(!getImportDeclErrorIfAny(FromD) &&
- "Import error already set for Decl.");
-
- // After takeError the error is not usable any more in ToDOrErr.
+ // After takeError the error is not usable anymore in ToDOrErr.
// Get a copy of the error object (any more simple solution for this?).
ImportError ErrOut;
handleAllErrors(ToDOrErr.takeError(),
[&ErrOut](const ImportError &E) { ErrOut = E; });
setImportDeclError(FromD, ErrOut);
+
+ // Set the error for all nodes which have been created before we
+ // recognized the error.
+ for (const auto &Path : SavedImportPaths[FromD])
+ for (Decl *Di : Path)
+ setImportDeclError(Di, ErrOut);
+ SavedImportPaths[FromD].clear();
+
// Do not return ToDOrErr, error was taken out of it.
return make_error<ImportError>(ErrOut);
}
@@ -7921,6 +7933,7 @@
Imported(FromD, ToD);
updateFlags(FromD, ToD);
+ SavedImportPaths[FromD].clear();
return ToDOrErr;
}
@@ -8641,9 +8654,10 @@
}
void ASTImporter::setImportDeclError(Decl *From, ImportError Error) {
- assert(ImportDeclErrors.find(From) == ImportDeclErrors.end() &&
- "Setting import error allowed only once for a Decl.");
- ImportDeclErrors[From] = Error;
+ auto InsertRes = ImportDeclErrors.insert({From, Error});
+ // Either we set the error for the first time, or we already had set one and
+ // now we want to set the same error.
+ assert(InsertRes.second || InsertRes.first->second.Error == Error.Error);
}
bool ASTImporter::IsStructurallyEquivalent(QualType From, QualType To,
Index: clang/include/clang/AST/ASTImporter.h
===================================================================
--- clang/include/clang/AST/ASTImporter.h
+++ clang/include/clang/AST/ASTImporter.h
@@ -26,6 +26,7 @@
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/Support/Error.h"
#include <utility>
@@ -87,6 +88,127 @@
using ImportedCXXBaseSpecifierMap =
llvm::DenseMap<const CXXBaseSpecifier *, CXXBaseSpecifier *>;
+ // An ImportPath is the list of the AST nodes which we visit during an
+ // Import call.
+ // If node `A` depends on node `B` then the path contains an `A`->`B` edge.
+ // From the call stack of the import functions we can read the very same
+ // path.
+ //
+ // Now imagine the following AST, where the `->` represents dependency in
+ // therms of the import.
+ // ```
+ // A->B->C->D
+ // `->E
+ // ```
+ // We would like to import A.
+ // The import behaves like a DFS, so we will visit the nodes in this order:
+ // ABCDE.
+ // During the visitation we will have the following ImportPaths:
+ // ```
+ // A
+ // AB
+ // ABC
+ // ABCD
+ // ABC
+ // AB
+ // ABE
+ // AB
+ // A
+ // ```
+ // If during the visit of E there is an error then we set an error for E,
+ // then as the call stack shrinks for B, then for A:
+ // ```
+ // A
+ // AB
+ // ABC
+ // ABCD
+ // ABC
+ // AB
+ // ABE // Error! Set an error to E
+ // AB // Set an error to B
+ // A // Set an error to A
+ // ```
+ // However, during the import we could import C and D without any error and
+ // they are independent from A,B and E.
+ // We must not set up an error for C and D.
+ // So, at the end of the import we have an entry in `ImportDeclErrors` for
+ // A,B,E but not for C,D.
+ //
+ // Now what happens if there is a cycle in the import path?
+ // Let's consider this AST:
+ // ```
+ // A->B->C->A
+ // `->E
+ // ```
+ // During the visitation we will have the below ImportPaths and if during
+ // the visit of E there is an error then we will set up an error for E,B,A.
+ // But what's up with C?
+ // ```
+ // A
+ // AB
+ // ABC
+ // ABCA
+ // ABC
+ // AB
+ // ABE // Error! Set an error to E
+ // AB // Set an error to B
+ // A // Set an error to A
+ // ```
+ // This time we know that both B and C are dependent on A.
+ // This means we must set up an error for C too.
+ // As the call stack reverses back we get to A and we must set up an error
+ // to all nodes which depend on A (this includes C).
+ // But C is no longer on the import path, it just had been previously.
+ // Such situation can happen only if during the visitation we had a cycle.
+ // If we didn't have any cycle, then the normal way of passing an Error
+ // object through the call stack could handle the situation.
+ // This is why we must track cycles during the import process for each
+ // visited declaration.
+ class ImportPathTy {
+ public:
+ using VecTy = llvm::SmallVector<Decl *, 32>;
+
+ void push(Decl *D) {
+ Nodes.push_back(D);
+ ++Aux[D];
+ }
+
+ void pop() {
+ if (Nodes.empty())
+ return;
+ --Aux[Nodes.back()];
+ Nodes.pop_back();
+ }
+
+ /// Returns true if the last element can be found earlier in the path.
+ bool hasCycleAtBack() const {
+ auto Pos = Aux.find(Nodes.back());
+ return Pos != Aux.end() && Pos->second > 1;
+ }
+
+ using Cycle = llvm::iterator_range<VecTy::const_reverse_iterator>;
+ Cycle getCycleAtBack() const {
+ assert(Nodes.size() >= 2);
+ return Cycle(Nodes.rbegin(),
+ std::find(Nodes.rbegin() + 1, Nodes.rend(), Nodes.back()) +
+ 1);
+ }
+
+ /// Returns the copy of the cycle.
+ VecTy copyCycleAtBack() const {
+ auto R = getCycleAtBack();
+ return VecTy(R.begin(), R.end());
+ }
+
+ private:
+ // All the nodes of the path.
+ VecTy Nodes;
+ // Auxiliary container to be able to answer "Do we have a cycle ending
+ // at last element?" as fast as possible.
+ // We count each Decl's occurrence over the path.
+ llvm::SmallDenseMap<Decl *, int, 32> Aux;
+ };
+
private:
/// Pointer to the import specific lookup table, which may be shared
@@ -96,6 +218,17 @@
/// If not set then the original C/C++ lookup is used.
ASTImporterLookupTable *LookupTable = nullptr;
+ /// The path which we go through during the import of a given AST node.
+ ImportPathTy ImportPath;
+ /// Sometimes we have to save some part of an import path, so later we can
+ /// set up properties to the saved nodes.
+ /// We may have several of these import paths associated to one Decl.
+ using SavedImportPathsForOneDecl =
+ llvm::SmallVector<ImportPathTy::VecTy, 32>;
+ using SavedImportPathsTy =
+ llvm::SmallDenseMap<Decl *, SavedImportPathsForOneDecl, 32>;
+ SavedImportPathsTy SavedImportPaths;
+
/// The contexts we're importing to and from.
ASTContext &ToContext, &FromContext;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
