[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
martong added a comment. Jenkins is green: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29802/ Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63603/new/ https://reviews.llvm.org/D63603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
martong added a comment. @shafik I've been looking for any lldb regression in our Mac machine, could not find any. Now I am looking at http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ . I don't expect regression because here we changed logic about the error handling only, so I'd expect to have regression only if we had testcases in lldb for erroneous cases, but apparently there are no such tests. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63603/new/ https://reviews.llvm.org/D63603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
This revision was automatically updated to reflect the committed changes.
Closed by commit rL364752: [ASTImporter] Propagate error from ImportDeclContext
(authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D63603?vs=206250&id=207274#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63603/new/
https://reviews.llvm.org/D63603
Files:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/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 @@
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 @@
}
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 @@
if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
return Err;
- To->completeDefinition();
return Error::success();
}
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -4738,6 +4738,93 @@
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
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, The patch looks good. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63603/new/ https://reviews.llvm.org/D63603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
martong added a comment. @a_sidorin Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63603/new/ https://reviews.llvm.org/D63603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
martong updated this revision to Diff 206250.
martong marked 2 inline comments as done.
martong added a comment.
- Use make_scope_exit
- Add test ErrorIsNotPropagatedFromMemberToNamespace
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63603/new/
https://reviews.llvm.org/D63603
Files:
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
@@ -4738,6 +4738,93 @@
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, );
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ 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 @@
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
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
martong marked 4 inline comments as done.
martong added inline comments.
Comment at: clang/lib/AST/ASTImporter.cpp:1724
+ };
+ DefinitionCompleter CompleterRAII(To);
jkorous wrote:
> You might consider using just a lambda with `llvm::make_scope_exit`.
>
> https://llvm.org/doxygen/namespacellvm.html#a4896534f3c6278be56967444daf38e3f
>
> For example:
> ```
> To->startDefinition();
> auto DefinitionCompleter = llvm::make_scope_exit(
> [To](){To->completeDefinition();} );
> ```
Thanks! I have changed to make_scope_exit.
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4743
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
balazske wrote:
> Maybe add a similar test for namespace and check that the error is not
> propagated?
Ok, I have added that test.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63603/new/
https://reviews.llvm.org/D63603
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
martong updated this revision to Diff 206223.
martong added a comment.
- Remove the macro and use std::string.op+
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63603/new/
https://reviews.llvm.org/D63603
Files:
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
@@ -4738,6 +4738,53 @@
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());
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
DefaultTestValuesForRunOptions, );
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1631,16 +1631,32 @@
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(
@@ -1697,7 +1713,15 @@
return Error::success();
}
- 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.
+ struct DefinitionCompleter {
+RecordDecl *To;
+DefinitionCompleter(RecordDecl *To) : To(To) { To->startDefinition(); }
+~DefinitionCompleter() { To->completeDefinition(); }
+ };
+ DefinitionCompleter CompleterRAII(To);
if (Error Err = setTypedefNameForAnonDecl(From, To, Importer))
return Err;
@@ -1822,7 +1846,6 @@
if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
return Err;
- To->completeDefinition();
return Error::success();
}
__
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
balazske added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:4743 +} + INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest, Maybe add a similar test for namespace and check that the error is not propagated? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63603/new/ https://reviews.llvm.org/D63603 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
martong updated this revision to Diff 206018.
martong added a comment.
- Remove formatv b/c it can't handle braces in code
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63603/new/
https://reviews.llvm.org/D63603
Files:
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
@@ -4694,6 +4694,53 @@
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(
+ 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());
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
DefaultTestValuesForRunOptions, );
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1631,16 +1631,32 @@
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(
@@ -1697,7 +1713,15 @@
return Error::success();
}
- 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.
+ struct DefinitionCompleter {
+RecordDecl *To;
+DefinitionCompleter(RecordDecl *To) : To(To) { To->startDefinition(); }
+~DefinitionCompleter() { To->completeDefinition(); }
+ };
+ DefinitionCompleter CompleterRAII(To);
if (Error Err = setTypedefNameForAnonDecl(From, To, Importer))
return Err;
@@ -1822,7 +1846,6 @@
if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
return Err;
- To->completeDefinition();
return Error::success();
}
___
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
jkorous added a comment.
I don't have the insight to LGTM the whole change - just a nit about
implementation detail.
Comment at: clang/lib/AST/ASTImporter.cpp:1724
+ };
+ DefinitionCompleter CompleterRAII(To);
You might consider using just a lambda with `llvm::make_scope_exit`.
https://llvm.org/doxygen/namespacellvm.html#a4896534f3c6278be56967444daf38e3f
For example:
```
To->startDefinition();
auto DefinitionCompleter = llvm::make_scope_exit(
[To](){To->completeDefinition();} );
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63603/new/
https://reviews.llvm.org/D63603
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext
martong created this revision.
martong added a reviewer: a_sidorin.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: clang.
martong added a parent revision: D62373: [ASTImporter] Store import errors for
Decls.
martong added a child revision: D62375: [ASTImporter] Mark erroneous nodes in
from ctx.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D63603
Files:
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
@@ -4695,6 +4695,54 @@
EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
}
+// An error should be set for a class if we cannot import one member.
+TEST_P(ErrorHandlingTest, ErrorIsPropagatedFromMemberToClass) {
+ std::string Code = formatv(
+ R"(
+ class X {
+void f() { {0}; } // This member has the error during import.
+void ok();// The error should not prevent importing this.
+ }; // An error will be set for X too.
+ )",
+ ErroneousStmt);
+ TranslationUnitDecl *FromTU = getTuDecl(Code, 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());
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
DefaultTestValuesForRunOptions, );
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1631,16 +1631,32 @@
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(
@@ -1697,7 +1713,15 @@
return Error::success();
}
- 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 compl
