martong updated this revision to Diff 205811.
martong marked 3 inline comments as done.
martong added a comment.
- Test error values are set for AST nodes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62373/new/
https://reviews.llvm.org/D62373
Files:
clang/include/clang/AST/ASTImporter.h
clang/lib/AST/ASTImporter.cpp
clang/test/ASTMerge/class-template-partial-spec/test.cpp
clang/unittests/AST/ASTImporterFixtures.cpp
clang/unittests/AST/ASTImporterFixtures.h
clang/unittests/AST/ASTImporterTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -11,6 +11,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/FormatVariadic.h"
#include "clang/AST/DeclContextInternals.h"
@@ -23,6 +24,7 @@
using internal::Matcher;
using internal::BindableMatcher;
using llvm::StringMap;
+using llvm::formatv;
// Base class for those tests which use the family of `testImport` functions.
class TestImportBase : public CompilerOptionSpecificTest,
@@ -874,7 +876,7 @@
auto *From = FirstDeclMatcher<FunctionDecl>().match(
FromTU, functionDecl(hasName("declToImport")));
ASSERT_TRUE(From);
- auto *To = Import(From, Lang_C);
+ auto *To = Import(From, Lang_CXX);
EXPECT_EQ(To, nullptr);
}
@@ -4574,6 +4576,128 @@
EXPECT_EQ(Imported->getPreviousDecl(), Friend);
}
+struct ASTImporterWithFakeErrors : ASTImporter {
+ using ASTImporter::ASTImporter;
+ bool returnWithErrorInTest() override { return true; }
+};
+
+struct ErrorHandlingTest : ASTImporterOptionSpecificTestBase {
+ ErrorHandlingTest() {
+ Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ return new ASTImporterWithFakeErrors(ToContext, ToFileManager,
+ FromContext, FromFileManager,
+ MinimalImport, LookupTable);
+ };
+ }
+ // In this test we purposely report an error (UnsupportedConstruct) when
+ // importing the below stmt.
+ const StringRef ErroneousStmt = R"( asm(""); )";
+};
+
+// Check a case when no new AST node is created in the AST before encountering
+// the error.
+TEST_P(ErrorHandlingTest, ErrorHappensBeforeCreatingANewNode) {
+ TranslationUnitDecl *ToTU = getToTuDecl(
+ R"(
+ template <typename T>
+ class X {};
+ template <>
+ class X<int> { int a; };
+ )",
+ Lang_CXX);
+ TranslationUnitDecl *FromTU = getTuDecl(
+ R"(
+ template <typename T>
+ class X {};
+ template <>
+ class X<int> { double b; };
+ )",
+ Lang_CXX);
+ auto *FromSpec = FirstDeclMatcher<ClassTemplateSpecializationDecl>().match(
+ FromTU, classTemplateSpecializationDecl(hasName("X")));
+ ClassTemplateSpecializationDecl *ImportedSpec = Import(FromSpec, Lang_CXX);
+ EXPECT_FALSE(ImportedSpec);
+
+ // The original Decl is kept, no new decl is created.
+ EXPECT_EQ(DeclCounter<ClassTemplateSpecializationDecl>().match(
+ ToTU, classTemplateSpecializationDecl(hasName("X"))),
+ 1u);
+
+ // But an error is set to the counterpart in the "from" context.
+ ASTImporter *Importer = findFromTU(FromSpec)->Importer.get();
+ Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromSpec);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::NameConflict);
+}
+
+// Check a case when a new AST node is created but not linked to the AST before
+// encountering the error.
+TEST_P(ErrorHandlingTest,
+ ErrorHappensAfterCreatingTheNodeButBeforeLinkingThatToTheAST) {
+ std::string Code = formatv("void foo() { {0}; }", ErroneousStmt);
+ TranslationUnitDecl *FromTU =
+ getTuDecl(Code, Lang_CXX);
+ auto *FromFoo = FirstDeclMatcher<FunctionDecl>().match(
+ FromTU, functionDecl(hasName("foo")));
+
+ FunctionDecl *ImportedFoo = Import(FromFoo, Lang_CXX);
+ EXPECT_FALSE(ImportedFoo);
+
+ TranslationUnitDecl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+ // Created, but not linked.
+ EXPECT_EQ(
+ DeclCounter<FunctionDecl>().match(ToTU, functionDecl(hasName("foo"))),
+ 0u);
+
+ ASTImporter *Importer = findFromTU(FromFoo)->Importer.get();
+ Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromFoo);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+}
+
+// Check a case when a new AST node is created and linked to the AST before
+// encountering the error. The error is set for the counterpart of the nodes in
+// the "from" context.
+TEST_P(ErrorHandlingTest, ErrorHappensAfterNodeIsCreatedAndLinked) {
+ std::string Code = formatv(
+ R"(
+ void f();
+ void f() { {0}; }
+ )"
+ , ErroneousStmt);
+ TranslationUnitDecl *FromTU = getTuDecl(Code, Lang_CXX);
+ auto *FromProto = FirstDeclMatcher<FunctionDecl>().match(
+ FromTU, functionDecl(hasName("f")));
+ auto *FromDef =
+ LastDeclMatcher<FunctionDecl>().match(FromTU, functionDecl(hasName("f")));
+ FunctionDecl *ImportedProto = Import(FromProto, Lang_CXX);
+ EXPECT_FALSE(ImportedProto); // Could not import.
+ // However, we created two nodes in the AST. 1) the fwd decl 2) the
+ // definition. The definition is not added to its DC, but the fwd decl is
+ // there.
+ TranslationUnitDecl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+ EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, functionDecl(hasName("f"))),
+ 1u);
+ // Match the fwd decl.
+ auto *ToProto =
+ FirstDeclMatcher<FunctionDecl>().match(ToTU, functionDecl(hasName("f")));
+ EXPECT_TRUE(ToProto);
+ // An error is set to the counterpart in the "from" context both for the fwd
+ // decl and the definition.
+ ASTImporter *Importer = findFromTU(FromProto)->Importer.get();
+ Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromProto);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+ OptErr = Importer->getImportDeclErrorIfAny(FromDef);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+}
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
+ DefaultTestValuesForRunOptions, );
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
DefaultTestValuesForRunOptions, );
Index: clang/unittests/AST/ASTImporterFixtures.h
===================================================================
--- clang/unittests/AST/ASTImporterFixtures.h
+++ clang/unittests/AST/ASTImporterFixtures.h
@@ -124,7 +124,6 @@
void lazyInitLookupTable(TranslationUnitDecl *ToTU);
void lazyInitToAST(Language ToLang, StringRef ToSrcCode, StringRef FileName);
- TU *findFromTU(Decl *From);
protected:
std::unique_ptr<ASTImporterLookupTable> LookupTablePtr;
@@ -133,6 +132,9 @@
// We may have several From context but only one To context.
std::unique_ptr<ASTUnit> ToAST;
+ // Returns with the TU associated with the given Decl.
+ TU *findFromTU(Decl *From);
+
// Creates an AST both for the From and To source code and imports the Decl
// of the identifier into the To context.
// Must not be called more than once within the same test.
Index: clang/unittests/AST/ASTImporterFixtures.cpp
===================================================================
--- clang/unittests/AST/ASTImporterFixtures.cpp
+++ clang/unittests/AST/ASTImporterFixtures.cpp
@@ -161,7 +161,7 @@
}) == FromTUs.end());
ArgVector Args = getArgVectorForLanguage(Lang);
- FromTUs.emplace_back(SrcCode, FileName, Args);
+ FromTUs.emplace_back(SrcCode, FileName, Args, Creator);
TU &Tu = FromTUs.back();
return Tu.TUDecl;
Index: clang/test/ASTMerge/class-template-partial-spec/test.cpp
===================================================================
--- clang/test/ASTMerge/class-template-partial-spec/test.cpp
+++ clang/test/ASTMerge/class-template-partial-spec/test.cpp
@@ -1,5 +1,3 @@
-// FIXME: Crashes after r357394
-// XFAIL: *
// RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.1.ast %S/Inputs/class-template-partial-spec1.cpp
// RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.2.ast %S/Inputs/class-template-partial-spec2.cpp
// RUN: %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -253,11 +253,10 @@
LLVM_NODISCARD bool
GetImportedOrCreateSpecialDecl(ToDeclT *&ToD, CreateFunT CreateFun,
FromDeclT *FromD, Args &&... args) {
- // FIXME: This code is needed later.
- //if (Importer.getImportDeclErrorIfAny(FromD)) {
- // ToD = nullptr;
- // return true; // Already imported but with error.
- //}
+ if (Importer.getImportDeclErrorIfAny(FromD)) {
+ ToD = nullptr;
+ return true; // Already imported but with error.
+ }
ToD = cast_or_null<ToDeclT>(Importer.GetAlreadyImportedOrNull(FromD));
if (ToD)
return true; // Already imported.
@@ -5108,7 +5107,7 @@
}
} else { // ODR violation.
// FIXME HandleNameConflict
- return nullptr;
+ return make_error<ImportError>(ImportError::NameConflict);
}
}
@@ -5550,6 +5549,8 @@
ExpectedStmt ASTNodeImporter::VisitGCCAsmStmt(GCCAsmStmt *S) {
+ if (Importer.returnWithErrorInTest())
+ return make_error<ImportError>(ImportError::UnsupportedConstruct);
SmallVector<IdentifierInfo *, 4> Names;
for (unsigned I = 0, E = S->getNumOutputs(); I != E; I++) {
IdentifierInfo *ToII = Importer.Import(S->getOutputIdentifier(I));
@@ -7730,7 +7731,6 @@
void ASTImporter::RegisterImportedDecl(Decl *FromD, Decl *ToD) {
MapImported(FromD, ToD);
- AddToLookupTable(ToD);
}
Expected<QualType> ASTImporter::Import(QualType FromT) {
@@ -7803,6 +7803,11 @@
return nullptr;
+ // Check whether there was a previous failed import.
+ // If yes return the existing error.
+ if (auto Error = getImportDeclErrorIfAny(FromD))
+ return make_error<ImportError>(*Error);
+
// Check whether we've already imported this declaration.
Decl *ToD = GetAlreadyImportedOrNull(FromD);
if (ToD) {
@@ -7813,24 +7818,65 @@
// Import the declaration.
ExpectedDecl ToDOrErr = ImportImpl(FromD);
- if (!ToDOrErr)
+ if (!ToDOrErr) {
+ // Failed to import.
+
+ auto Pos = ImportedDecls.find(FromD);
+ if (Pos != ImportedDecls.end()) {
+ // Import failed after the object was created.
+ // Remove all references to it.
+ auto *ToD = Pos->second;
+ ImportedDecls.erase(Pos);
+
+ // ImportedDecls and ImportedFromDecls are not symmetric. It may happen
+ // (e.g. with namespaces) that several decls from the 'from' context are
+ // mapped to the same decl in the 'to' context. If we removed entries
+ // from the LookupTable here then we may end up removing them multiple
+ // times.
+
+ // The Lookuptable contains decls only which are in the 'to' context.
+ // Remove from the Lookuptable only if it is *imported* into the 'to'
+ // context (and do not remove it if it was added during the initial
+ // traverse of the 'to' context).
+ auto PosF = ImportedFromDecls.find(ToD);
+ if (PosF != ImportedFromDecls.end()) {
+ if (LookupTable)
+ if (auto *ToND = dyn_cast<NamedDecl>(ToD))
+ LookupTable->remove(ToND);
+ ImportedFromDecls.erase(PosF);
+ }
+
+ // FIXME: AST may contain remaining references to the failed object.
+ }
+
+ 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);
+ // Do not return ToDOrErr, error was taken out of it.
+ return make_error<ImportError>(ErrOut);
+ }
return ToDOrErr;
+ }
+
ToD = *ToDOrErr;
- // FIXME Use getImportDeclErrorIfAny() here (and return with the error) once
- // the error handling is finished in GetImportedOrCreateSpecialDecl().
+ // FIXME: Handle the "already imported with error" case. We can get here
+ // nullptr only if GetImportedOrCreateDecl returned nullptr (after a
+ // previously failed create was requested).
+ // Later GetImportedOrCreateDecl can be updated to return the error.
if (!ToD) {
- return nullptr;
+ auto Err = getImportDeclErrorIfAny(FromD);
+ assert(Err);
+ return make_error<ImportError>(*Err);
}
// Make sure that ImportImpl registered the imported decl.
assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
-
- // Once the decl is connected to the existing declarations, i.e. when the
- // redecl chain is properly set then we populate the lookup again.
- // This way the primary context will be able to find all decls.
- AddToLookupTable(ToD);
-
// Notify subclasses.
Imported(FromD, ToD);
@@ -8541,9 +8587,25 @@
// This mapping should be maintained only in this function. Therefore do not
// check for additional consistency.
ImportedFromDecls[To] = From;
+ AddToLookupTable(To);
return To;
}
+llvm::Optional<ImportError>
+ASTImporter::getImportDeclErrorIfAny(Decl *FromD) const {
+ auto Pos = ImportDeclErrors.find(FromD);
+ if (Pos != ImportDeclErrors.end())
+ return Pos->second;
+ else
+ return Optional<ImportError>();
+}
+
+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;
+}
+
bool ASTImporter::IsStructurallyEquivalent(QualType From, QualType To,
bool Complain) {
llvm::DenseMap<const Type *, const Type *>::iterator Pos =
Index: clang/include/clang/AST/ASTImporter.h
===================================================================
--- clang/include/clang/AST/ASTImporter.h
+++ clang/include/clang/AST/ASTImporter.h
@@ -116,6 +116,14 @@
/// context to the corresponding declarations in the "to" context.
llvm::DenseMap<Decl *, Decl *> ImportedDecls;
+ /// Mapping from the already-imported declarations in the "from"
+ /// context to the error status of the import of that declaration.
+ /// This map contains only the declarations that were not correctly
+ /// imported. The same declaration may or may not be included in
+ /// ImportedDecls. This map is updated continuously during imports and never
+ /// cleared (like ImportedDecls).
+ llvm::DenseMap<Decl *, ImportError> ImportDeclErrors;
+
/// Mapping from the already-imported declarations in the "to"
/// context to the corresponding declarations in the "from" context.
llvm::DenseMap<Decl *, Decl *> ImportedFromDecls;
@@ -148,6 +156,9 @@
/// decl on its own.
virtual Expected<Decl *> ImportImpl(Decl *From);
+ /// Used only in unittests to verify the behaviour of the error handling.
+ virtual bool returnWithErrorInTest() { return false; };
+
public:
/// \param ToContext The context we'll be importing into.
@@ -394,6 +405,8 @@
void RegisterImportedDecl(Decl *FromD, Decl *ToD);
/// Store and assign the imported declaration to its counterpart.
+ /// It may happen that several decls from the 'from' context are mapped to
+ /// the same decl in the 'to' context.
Decl *MapImported(Decl *From, Decl *To);
/// Called by StructuralEquivalenceContext. If a RecordDecl is
@@ -404,6 +417,14 @@
/// importation, eliminating this loop.
virtual Decl *GetOriginalDecl(Decl *To) { return nullptr; }
+ /// Return if import of the given declaration has failed and if yes
+ /// the kind of the problem. This gives the first error encountered with
+ /// the node.
+ llvm::Optional<ImportError> getImportDeclErrorIfAny(Decl *FromD) const;
+
+ /// Mark (newly) imported declaration with error.
+ void setImportDeclError(Decl *From, ImportError Error);
+
/// Determine whether the given types are structurally
/// equivalent.
bool IsStructurallyEquivalent(QualType From, QualType To,
@@ -414,7 +435,6 @@
/// \returns The index of the field in its parent context (starting from 0).
/// On error `None` is returned (parent context is non-record).
static llvm::Optional<unsigned> getFieldIndex(Decl *F);
-
};
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
