martong updated this revision to Diff 206228.
martong added a comment.
- Remove the macro and use std::string.op+
- We may set up an error twice in case of a cycle, thus remove the assert
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,27 +4864,116 @@
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);
+}
- // 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());
+// 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);
+
+ // 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);
}
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
@@ -7840,10 +7840,23 @@
return FromDPos->second->getTranslationUnitDecl();
}
+// RAII class to maintain the import path.
+class ImportPathBuilder {
+ ASTImporter::ImportPathTy &Path;
+
+public:
+ ImportPathBuilder(ASTImporter::ImportPathTy &Path, Decl *FromD) : Path(Path) {
+ Path.push(FromD);
+ };
+ ~ImportPathBuilder() { Path.pop(); }
+};
+
Expected<Decl *> ASTImporter::Import(Decl *FromD) {
if (!FromD)
return nullptr;
+ // Push FromD to the stack, and remove that when we return.
+ ImportPathBuilder PathRAII(ImportPath, FromD);
// Check whether there was a previous failed import.
// If yes return the existing error.
@@ -7855,6 +7868,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;
}
@@ -7891,18 +7908,26 @@
// 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.
- // 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);
- // Do not return ToDOrErr, error was taken out of it.
- return make_error<ImportError>(ErrOut);
+ if (!getImportDeclErrorIfAny(FromD)) {
+ // Error encountered for the first time.
+ // After takeError the error is not usable any more 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);
+ }
+ return ToDOrErr;
}
ToD = *ToDOrErr;
@@ -7923,6 +7948,7 @@
Imported(FromD, ToD);
updateFlags(FromD, ToD);
+ SavedImportPaths[FromD].clear();
return ToDOrErr;
}
@@ -8643,8 +8669,6 @@
}
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;
}
@@ -8666,3 +8690,38 @@
Complain);
return Ctx.IsEquivalent(From, To);
}
+
+void ASTImporter::ImportPathTy::push(Decl *D) {
+ Nodes.push_back(D);
+ ++Aux[D];
+}
+
+void ASTImporter::ImportPathTy::pop() {
+ if (Nodes.empty())
+ return;
+ --Aux[Nodes.back()];
+ Nodes.pop_back();
+}
+
+Decl *ASTImporter::ImportPathTy::top() const {
+ if (!Nodes.empty())
+ return Nodes.back();
+ return nullptr;
+}
+
+bool ASTImporter::ImportPathTy::hasCycleAtBack() {
+ return Aux[Nodes.back()] > 1;
+}
+
+ASTImporter::ImportPathTy::Cycle
+ASTImporter::ImportPathTy::getCycleAtBack() const {
+ assert(Nodes.size() >= 2);
+ return Cycle(Nodes.rbegin(),
+ std::find(Nodes.rbegin() + 1, Nodes.rend(), Nodes.back()) + 1);
+}
+
+ASTImporter::ImportPathTy::VecTy
+ASTImporter::ImportPathTy::copyCycleAtBack() const {
+ auto R = getCycleAtBack();
+ return VecTy(R.begin(), R.end());
+}
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,30 @@
using ImportedCXXBaseSpecifierMap =
llvm::DenseMap<const CXXBaseSpecifier *, CXXBaseSpecifier *>;
+ class ImportPathTy {
+ public:
+ using VecTy = llvm::SmallVector<Decl *, 32>;
+
+ void push(Decl *D);
+ void pop();
+ Decl *top() const;
+
+ /// Returns true if the last element can be found earlier in the path.
+ bool hasCycleAtBack();
+ using Cycle = llvm::iterator_range<VecTy::const_reverse_iterator>;
+ Cycle getCycleAtBack() const;
+ /// Returns the copy of the cycle.
+ VecTy copyCycleAtBack() const;
+
+ 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 +121,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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
