[clang] 5967528 - [clang][ASTImporter] Fix an import error handling related bug.
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 13 01:12:28 PDT 2022
Author: Balázs Kéri
Date: 2022-04-13T10:11:33+02:00
New Revision: 596752863e27e6b4b89e34ecfcf5317a5bf46b52
URL: https://github.com/llvm/llvm-project/commit/596752863e27e6b4b89e34ecfcf5317a5bf46b52
DIFF: https://github.com/llvm/llvm-project/commit/596752863e27e6b4b89e34ecfcf5317a5bf46b52.diff
LOG: [clang][ASTImporter] Fix an import error handling related bug.
This bug can cause that more import errors are generated than necessary
and many objects fail to import. Chance of an invalid AST after these
imports increases.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D122525
Added:
Modified:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index b368dbf1d5ce9..d0041b83f7afb 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -138,6 +138,46 @@ namespace clang {
To->setIsUsed();
}
+ /// How to handle import errors that occur when import of a child declaration
+ /// of a DeclContext fails.
+ class ChildErrorHandlingStrategy {
+ /// This context is imported (in the 'from' domain).
+ /// It is nullptr if a non-DeclContext is imported.
+ const DeclContext *const FromDC;
+ /// Ignore import errors of the children.
+ /// If true, the context can be imported successfully if a child
+ /// of it failed to import. Otherwise the import errors of the child nodes
+ /// are accumulated (joined) into the import error object of the parent.
+ /// (Import of a parent can fail in other ways.)
+ bool const IgnoreChildErrors;
+
+ public:
+ ChildErrorHandlingStrategy(const DeclContext *FromDC)
+ : FromDC(FromDC), IgnoreChildErrors(!isa<TagDecl>(FromDC)) {}
+ ChildErrorHandlingStrategy(const Decl *FromD)
+ : FromDC(dyn_cast<DeclContext>(FromD)),
+ IgnoreChildErrors(!isa<TagDecl>(FromD)) {}
+
+ /// Process the import result of a child (of the current declaration).
+ /// \param ResultErr The import error that can be used as result of
+ /// importing the parent. This may be changed by the function.
+ /// \param ChildErr Result of importing a child. Can be success or error.
+ void handleChildImportResult(Error &ResultErr, Error &&ChildErr) {
+ if (ChildErr && !IgnoreChildErrors)
+ ResultErr = joinErrors(std::move(ResultErr), std::move(ChildErr));
+ else
+ consumeError(std::move(ChildErr));
+ }
+
+ /// Determine if import failure of a child does not cause import failure of
+ /// its parent.
+ bool ignoreChildErrorOnParent(Decl *FromChildD) const {
+ if (!IgnoreChildErrors || !FromDC)
+ return false;
+ return FromDC->containsDecl(FromChildD);
+ }
+ };
+
class ASTNodeImporter : public TypeVisitor<ASTNodeImporter, ExpectedType>,
public DeclVisitor<ASTNodeImporter, ExpectedDecl>,
public StmtVisitor<ASTNodeImporter, ExpectedStmt> {
@@ -1809,7 +1849,7 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
// because there is an ODR error with two typedefs. As another example,
// the client may allow EnumConstantDecls with same names but with
//
diff erent values in two distinct translation units.
- bool AccumulateChildErrors = isa<TagDecl>(FromDC);
+ ChildErrorHandlingStrategy HandleChildErrors(FromDC);
Error ChildErrors = Error::success();
for (auto *From : FromDC->decls()) {
@@ -1849,20 +1889,14 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
if (FromRecordDecl->isCompleteDefinition() &&
!ToRecordDecl->isCompleteDefinition()) {
Error Err = ImportDefinition(FromRecordDecl, ToRecordDecl);
-
- if (Err && AccumulateChildErrors)
- ChildErrors = joinErrors(std::move(ChildErrors), std::move(Err));
- else
- consumeError(std::move(Err));
+ HandleChildErrors.handleChildImportResult(ChildErrors,
+ std::move(Err));
}
}
}
} else {
- if (AccumulateChildErrors)
- ChildErrors =
- joinErrors(std::move(ChildErrors), ImportedOrErr.takeError());
- else
- consumeError(ImportedOrErr.takeError());
+ HandleChildErrors.handleChildImportResult(ChildErrors,
+ ImportedOrErr.takeError());
}
}
@@ -8799,8 +8833,20 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) {
// Set the error for all nodes which have been created before we
// recognized the error.
- for (const auto &Path : SavedImportPaths[FromD])
+ for (const auto &Path : SavedImportPaths[FromD]) {
+ // The import path contains import-dependency nodes first.
+ // Save the node that was imported as dependency of the current node.
+ Decl *PrevFromDi = FromD;
for (Decl *FromDi : Path) {
+ // Begin and end of the path equals 'FromD', skip it.
+ if (FromDi == FromD)
+ continue;
+ // We should not set import error on a node and all following nodes in
+ // the path if child import errors are ignored.
+ if (ChildErrorHandlingStrategy(FromDi).ignoreChildErrorOnParent(
+ PrevFromDi))
+ break;
+ PrevFromDi = FromDi;
setImportDeclError(FromDi, ErrOut);
//FIXME Should we remove these Decls from ImportedDecls?
// Set the error for the mapped to Decl, which is in the "to" context.
@@ -8810,6 +8856,7 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) {
// FIXME Should we remove these Decls from the LookupTable,
// and from ImportedFromDecls?
}
+ }
SavedImportPaths.erase(FromD);
// Do not return ToDOrErr, error was taken out of it.
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 7988603c4d734..d9b6f4969abf8 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -5821,6 +5821,53 @@ TEST_P(ErrorHandlingTest, ODRViolationWithinParmVarDecls) {
EXPECT_FALSE(ImportedF);
}
+TEST_P(ErrorHandlingTest, DoNotInheritErrorFromNonDependentChild) {
+ // Declarations should not inherit an import error from a child object
+ // if the declaration has no direct dependence to such a child.
+ // For example a namespace should not get import error if one of the
+ // declarations inside it fails to import.
+ // There was a special case in error handling (when "import path circles" are
+ // encountered) when this property was not held. This case is provoked by the
+ // following code.
+ constexpr auto ToTUCode = R"(
+ namespace ns {
+ struct Err {
+ char A;
+ };
+ }
+ )";
+ constexpr auto FromTUCode = R"(
+ namespace ns {
+ struct A {
+ using U = struct Err;
+ };
+ }
+ namespace ns {
+ struct Err {}; // ODR violation
+ void f(A) {}
+ }
+ )";
+
+ Decl *ToTU = getToTuDecl(ToTUCode, Lang_CXX11);
+ static_cast<void>(ToTU);
+ Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11);
+ auto *FromA = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("A"), hasDefinition()));
+ ASSERT_TRUE(FromA);
+ auto *ImportedA = Import(FromA, Lang_CXX11);
+ // 'A' can not be imported: ODR error at 'Err'
+ EXPECT_FALSE(ImportedA);
+ // When import of 'A' failed there was a "saved import path circle" that
+ // contained namespace 'ns' (A - U - Err - ns - f - A). This should not mean
+ // that every object in this path fails to import.
+
+ Decl *FromNS = FirstDeclMatcher<NamespaceDecl>().match(
+ FromTU, namespaceDecl(hasName("ns")));
+ EXPECT_TRUE(FromNS);
+ auto *ImportedNS = Import(FromNS, Lang_CXX11);
+ EXPECT_TRUE(ImportedNS);
+}
+
TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
Decl *FromTU = getTuDecl(
R"(
More information about the cfe-commits
mailing list