From 17c3eafb2e30dd507517bd072c27cbdf5ab36141 Mon Sep 17 00:00:00 2001 From: Gabor Marton Date: Mon, 1 Jul 2019 12:44:39 +0000 Subject: [PATCH] [ASTImporter] Propagate error from ImportDeclContext Summary: During analysis of one project we failed to import one CXXDestructorDecl. But since we did not propagate the error in importDeclContext we had a CXXRecordDecl without a destructor. Then the analyzer engine had a CallEvent where the nonexistent dtor was requested (crash). Solution is to propagate the errors we have during importing a DeclContext. Reviewers: a_sidorin, a.sidorin, shafik Subscribers: rnkovacs, dkrupp, Szelethus, gamesh411, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D63603 llvm-svn: 364752 --- clang/lib/AST/ASTImporter.cpp | 33 ++++++++++--- clang/unittests/AST/ASTImporterTest.cpp | 87 +++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 6 deletions(-) diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index edefa45..097046d 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -57,6 +57,7 @@ #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Casting.h" @@ -1631,16 +1632,32 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) { auto ToDCOrErr = Importer.ImportContext(FromDC); return ToDCOrErr.takeError(); } + + // We use strict error handling in case of records and enums, but not + // with e.g. namespaces. + // + // FIXME Clients of the ASTImporter should be able to choose an + // appropriate error handling strategy for their needs. For instance, + // they may not want to mark an entire namespace as erroneous merely + // because there is an ODR error with two typedefs. As another example, + // the client may allow EnumConstantDecls with same names but with + // different values in two distinct translation units. + bool AccumulateChildErrors = isa(FromDC); + + Error ChildErrors = Error::success(); llvm::SmallVector ImportedDecls; for (auto *From : FromDC->decls()) { ExpectedDecl ImportedOrErr = import(From); - if (!ImportedOrErr) - // Ignore the error, continue with next Decl. - // FIXME: Handle this case somehow better. - consumeError(ImportedOrErr.takeError()); + if (!ImportedOrErr) { + if (AccumulateChildErrors) + ChildErrors = + joinErrors(std::move(ChildErrors), ImportedOrErr.takeError()); + else + consumeError(ImportedOrErr.takeError()); + } } - return Error::success(); + return ChildErrors; } Error ASTNodeImporter::ImportDeclContext( @@ -1698,6 +1715,11 @@ Error ASTNodeImporter::ImportDefinition( } To->startDefinition(); + // Complete the definition even if error is returned. + // The RecordDecl may be already part of the AST so it is better to + // have it in complete state even if something is wrong with it. + auto DefinitionCompleter = + llvm::make_scope_exit([To]() { To->completeDefinition(); }); if (Error Err = setTypedefNameForAnonDecl(From, To, Importer)) return Err; @@ -1822,7 +1844,6 @@ Error ASTNodeImporter::ImportDefinition( if (Error Err = ImportDeclContext(From, /*ForceImport=*/true)) return Err; - To->completeDefinition(); return Error::success(); } diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp index 41a041f..215dc10 100644 --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -4738,6 +4738,93 @@ TEST_P(ErrorHandlingTest, ErrorHappensAfterNodeIsCreatedAndLinked) { EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct); } +// An error should be set for a class if we cannot import one member. +TEST_P(ErrorHandlingTest, ErrorIsPropagatedFromMemberToClass) { + TranslationUnitDecl *FromTU = getTuDecl( + std::string(R"( + class X { + void f() { )") + ErroneousStmt + R"( } // This member has the error + // during import. + void ok(); // The error should not prevent importing this. + }; // An error will be set for X too. + )", + Lang_CXX); + auto *FromX = FirstDeclMatcher().match( + FromTU, cxxRecordDecl(hasName("X"))); + CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX); + + // An error is set for X. + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct); + + // An error is set for f(). + auto *FromF = FirstDeclMatcher().match( + FromTU, cxxMethodDecl(hasName("f"))); + OptErr = Importer->getImportDeclErrorIfAny(FromF); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct); + // And any subsequent import should fail. + CXXMethodDecl *ImportedF = Import(FromF, Lang_CXX); + EXPECT_FALSE(ImportedF); + + // There is no error set for ok(). + auto *FromOK = FirstDeclMatcher().match( + FromTU, cxxMethodDecl(hasName("ok"))); + OptErr = Importer->getImportDeclErrorIfAny(FromOK); + EXPECT_FALSE(OptErr); + // And we should be able to import. + CXXMethodDecl *ImportedOK = Import(FromOK, Lang_CXX); + EXPECT_TRUE(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(ImportedOK->getDeclContext()); + EXPECT_TRUE(ToX->isCompleteDefinition()); +} + +TEST_P(ErrorHandlingTest, ErrorIsNotPropagatedFromMemberToNamespace) { + TranslationUnitDecl *FromTU = getTuDecl( + std::string(R"( + namespace X { + void f() { )") + ErroneousStmt + R"( } // This member has the error + // during import. + void ok(); // The error should not prevent importing this. + }; // An error will be set for X too. + )", + Lang_CXX); + auto *FromX = FirstDeclMatcher().match( + FromTU, namespaceDecl(hasName("X"))); + NamespaceDecl *ImportedX = Import(FromX, Lang_CXX); + + // There is no error set for X. + EXPECT_TRUE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_FALSE(OptErr); + + // An error is set for f(). + auto *FromF = FirstDeclMatcher().match( + FromTU, functionDecl(hasName("f"))); + OptErr = Importer->getImportDeclErrorIfAny(FromF); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct); + // And any subsequent import should fail. + FunctionDecl *ImportedF = Import(FromF, Lang_CXX); + EXPECT_FALSE(ImportedF); + + // There is no error set for ok(). + auto *FromOK = FirstDeclMatcher().match( + FromTU, functionDecl(hasName("ok"))); + OptErr = Importer->getImportDeclErrorIfAny(FromOK); + EXPECT_FALSE(OptErr); + // And we should be able to import. + FunctionDecl *ImportedOK = Import(FromOK, Lang_CXX); + EXPECT_TRUE(ImportedOK); +} + INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest, DefaultTestValuesForRunOptions, ); -- 2.7.4