[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
martong added a comment. Jenkins had one unrelated failure at http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29812/ Test Result (1 failure / +1) LLDB.Reproducer.TestGDBRemoteRepro.test After that the next build is green: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29813/ Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62376/new/ https://reviews.llvm.org/D62376 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
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/D62376/new/ https://reviews.llvm.org/D62376 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
This revision was automatically updated to reflect the committed changes.
Closed by commit rL364785: [ASTImporter] Mark erroneous nodes in shared st
(authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D62376?vs=206441=207332#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62376/new/
https://reviews.llvm.org/D62376
Files:
cfe/trunk/include/clang/AST/ASTImporter.h
cfe/trunk/include/clang/AST/ASTImporterSharedState.h
cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
cfe/trunk/lib/Frontend/ASTMerge.cpp
cfe/trunk/unittests/AST/ASTImporterFixtures.cpp
cfe/trunk/unittests/AST/ASTImporterFixtures.h
cfe/trunk/unittests/AST/ASTImporterTest.cpp
Index: cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
===
--- cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
+++ cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
@@ -437,10 +437,10 @@
return importDefinitionImpl(VD);
}
-void CrossTranslationUnitContext::lazyInitLookupTable(
+void CrossTranslationUnitContext::lazyInitImporterSharedSt(
TranslationUnitDecl *ToTU) {
- if (!LookupTable)
-LookupTable = llvm::make_unique(*ToTU);
+ if (!ImporterSharedSt)
+ImporterSharedSt = std::make_shared(*ToTU);
}
ASTImporter &
@@ -448,10 +448,10 @@
auto I = ASTUnitImporterMap.find(From.getTranslationUnitDecl());
if (I != ASTUnitImporterMap.end())
return *I->second;
- lazyInitLookupTable(Context.getTranslationUnitDecl());
+ lazyInitImporterSharedSt(Context.getTranslationUnitDecl());
ASTImporter *NewImporter = new ASTImporter(
Context, Context.getSourceManager().getFileManager(), From,
- From.getSourceManager().getFileManager(), false, LookupTable.get());
+ From.getSourceManager().getFileManager(), false, ImporterSharedSt);
ASTUnitImporterMap[From.getTranslationUnitDecl()].reset(NewImporter);
return *NewImporter;
}
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -12,7 +12,7 @@
//===--===//
#include "clang/AST/ASTImporter.h"
-#include "clang/AST/ASTImporterLookupTable.h"
+#include "clang/AST/ASTImporterSharedState.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/ASTDiagnostic.h"
#include "clang/AST/ASTStructuralEquivalence.h"
@@ -7701,11 +7701,16 @@
ASTImporter::ASTImporter(ASTContext , FileManager ,
ASTContext , FileManager ,
bool MinimalImport,
- ASTImporterLookupTable *LookupTable)
-: LookupTable(LookupTable), ToContext(ToContext), FromContext(FromContext),
+ std::shared_ptr SharedState)
+: SharedState(SharedState), ToContext(ToContext), FromContext(FromContext),
ToFileManager(ToFileManager), FromFileManager(FromFileManager),
Minimal(MinimalImport) {
+ // Create a default state without the lookup table: LLDB case.
+ if (!SharedState) {
+this->SharedState = std::make_shared();
+ }
+
ImportedDecls[FromContext.getTranslationUnitDecl()] =
ToContext.getTranslationUnitDecl();
}
@@ -7744,9 +7749,9 @@
// then the enum constant 'A' and the variable 'A' violates ODR.
// We can diagnose this only if we search in the redecl context.
DeclContext *ReDC = DC->getRedeclContext();
- if (LookupTable) {
+ if (SharedState->getLookupTable()) {
ASTImporterLookupTable::LookupResult LookupResult =
-LookupTable->lookup(ReDC, Name);
+SharedState->getLookupTable()->lookup(ReDC, Name);
return FoundDeclsTy(LookupResult.begin(), LookupResult.end());
} else {
// FIXME Can we remove this kind of lookup?
@@ -7758,9 +7763,7 @@
}
void ASTImporter::AddToLookupTable(Decl *ToD) {
- if (LookupTable)
-if (auto *ToND = dyn_cast(ToD))
- LookupTable->add(ToND);
+ SharedState->addDeclToLookup(ToD);
}
Expected ASTImporter::ImportImpl(Decl *FromD) {
@@ -7855,6 +7858,12 @@
// Check whether we've already imported this declaration.
Decl *ToD = GetAlreadyImportedOrNull(FromD);
if (ToD) {
+// Already imported (possibly from another TU) and with an error.
+if (auto Error = SharedState->getImportDeclErrorIfAny(ToD)) {
+ setImportDeclError(FromD, *Error);
+ return make_error(*Error);
+}
+
// 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
@@ -7888,27 +7897,39 @@
// traverse of the 'to' context).
auto PosF = ImportedFromDecls.find(ToD);
if (PosF != ImportedFromDecls.end()) {
-
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks for the explanation! It will be good if someone else takes a look at this patch. Comment at: clang/include/clang/AST/ASTImporterSharedState.h:40 + /// never cleared (like ImportedFromDecls). + llvm::DenseMap ImportErrors; + ErrorsInToContext? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62376/new/ https://reviews.llvm.org/D62376 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
martong marked 7 inline comments as done.
martong added inline comments.
Comment at: clang/lib/AST/ASTImporter.cpp:7867
if (PosF != ImportedFromDecls.end()) {
-if (LookupTable)
+if (SharedState->getLookupTable())
if (auto *ToND = dyn_cast(ToD))
a_sidorin wrote:
> I think we can encapsulate these conditions into
> `SharedState::[add/remove]Decl[To/From]Lookup methods.
Good catch, thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62376/new/
https://reviews.llvm.org/D62376
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
martong updated this revision to Diff 206441.
martong marked an inline comment as done.
martong added a comment.
- Encapsulate by adding addDeclToLookup and removeDeclFromLookup to the shared
state
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62376/new/
https://reviews.llvm.org/D62376
Files:
clang/include/clang/AST/ASTImporter.h
clang/include/clang/AST/ASTImporterSharedState.h
clang/include/clang/CrossTU/CrossTranslationUnit.h
clang/lib/AST/ASTImporter.cpp
clang/lib/CrossTU/CrossTranslationUnit.cpp
clang/lib/Frontend/ASTMerge.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
@@ -316,10 +316,11 @@
RedirectingImporterTest() {
Creator = [](ASTContext , FileManager ,
ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
return new RedirectingImporter(ToContext, ToFileManager, FromContext,
FromFileManager, MinimalImport,
- LookupTable);
+ SharedState);
};
}
};
@@ -2888,7 +2889,7 @@
CXXMethodDecl *Method =
FirstDeclMatcher().match(ToClass, MethodMatcher);
ToClass->removeDecl(Method);
- LookupTablePtr->remove(Method);
+ SharedStatePtr->getLookupTable()->remove(Method);
}
ASSERT_EQ(DeclCounter().match(ToClass, MethodMatcher), 0u);
@@ -4723,10 +4724,11 @@
ErrorHandlingTest() {
Creator = [](ASTContext , FileManager ,
ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
return new ASTImporterWithFakeErrors(ToContext, ToFileManager,
FromContext, FromFileManager,
- MinimalImport, LookupTable);
+ MinimalImport, SharedState);
};
}
// In this test we purposely report an error (UnsupportedConstruct) when
@@ -4999,6 +5001,79 @@
EXPECT_TRUE(ImportedOK);
}
+// An error should be set for a class if it had a previous import with an error
+// from another TU.
+TEST_P(ErrorHandlingTest,
+ ImportedDeclWithErrorShouldFailTheImportOfDeclWhichMapToIt) {
+ // We already have a fwd decl.
+ TranslationUnitDecl *ToTU = getToTuDecl(
+ "class X;", Lang_CXX);
+ // Then we import a definition.
+ {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+ void f() { )") + ErroneousStmt + R"( }
+ void ok();
+};
+)",
+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);
+ }
+ // ... but the node had been created.
+ auto *ToXDef = FirstDeclMatcher().match(
+ ToTU, cxxRecordDecl(hasName("X"), isDefinition()));
+ // An error is set for "ToXDef" in the shared state.
+ Optional OptErr =
+ SharedStatePtr->getImportDeclErrorIfAny(ToXDef);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+
+ auto *ToXFwd = FirstDeclMatcher().match(
+ ToTU, cxxRecordDecl(hasName("X"), unless(isDefinition(;
+ // An error is NOT set for the fwd Decl of X in the shared state.
+ OptErr = SharedStatePtr->getImportDeclErrorIfAny(ToXFwd);
+ ASSERT_FALSE(OptErr);
+
+ // Try to import X again but from another TU.
+ {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+ void f() { )") + ErroneousStmt + R"( }
+ void ok();
+};
+)",
+Lang_CXX, "input1.cc");
+
+auto *FromX = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("X")));
+CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+// If we did not save the errors for the "to" context then the below checks
+// would fail, because the lookup finds the fwd Decl of the existing
+// definition in the "to" context. We can reach the existing definition via
+// the found fwd Decl. That existing definition is structurally equivalent
+// (we check only the fields) with this one we want to import, so we return
+
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
martong added a reviewer: balazske.
martong marked 6 inline comments as done.
martong added inline comments.
Comment at: clang/lib/AST/ASTImporter.cpp:7830
if (ToD) {
+// Already imported (possibly from another TU) and with an error.
+if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
a_sidorin wrote:
> I'm not sure if it is safe to compare declarations from different TUs by
> pointers because they belong to different allocators.
In the `SharedState` we keep pointers only from the "to" context.
Comment at: clang/lib/AST/ASTImporter.cpp:7831
+// Already imported (possibly from another TU) and with an error.
+if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+ return make_error(*Error);
a_sidorin wrote:
> getImportDeclErrorIfAny() is usually called for FromD, not for ToD. Is it
> intentional?
`ASTImporter::getImportDeclErrorIfAny()` is different from
`ASTImporterSharedState::getImportDeclErrorIfAny()`.
The former keeps track of the erroneous decls from the "from" context.
In the latter we keep track of those decls (and their error) which are in the
"to" context.
The "to" context is common for all ASTImporter objects in the cross translation
unit context.
They share the same ASTImporterLookupTable object too. Thus the name
ASTImporterSharedState.
Comment at: clang/lib/AST/ASTImporter.cpp:7924
+ if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+return make_error(*Error);
+
a_sidorin wrote:
> I don' think that an import failure from another TU means that the import
> from the current TU will fail.
It should. At least if we map the newly imported decl to an existing but
already messed up decl in the "to" context. Please refer to the added test case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62376/new/
https://reviews.llvm.org/D62376
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
martong updated this revision to Diff 206431.
martong added a comment.
- Set error for FromD if it maps to an existing Decl which has an error set
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62376/new/
https://reviews.llvm.org/D62376
Files:
clang/include/clang/AST/ASTImporter.h
clang/include/clang/AST/ASTImporterSharedState.h
clang/include/clang/CrossTU/CrossTranslationUnit.h
clang/lib/AST/ASTImporter.cpp
clang/lib/CrossTU/CrossTranslationUnit.cpp
clang/lib/Frontend/ASTMerge.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
@@ -316,10 +316,11 @@
RedirectingImporterTest() {
Creator = [](ASTContext , FileManager ,
ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
return new RedirectingImporter(ToContext, ToFileManager, FromContext,
FromFileManager, MinimalImport,
- LookupTable);
+ SharedState);
};
}
};
@@ -2888,7 +2889,7 @@
CXXMethodDecl *Method =
FirstDeclMatcher().match(ToClass, MethodMatcher);
ToClass->removeDecl(Method);
- LookupTablePtr->remove(Method);
+ SharedStatePtr->getLookupTable()->remove(Method);
}
ASSERT_EQ(DeclCounter().match(ToClass, MethodMatcher), 0u);
@@ -4723,10 +4724,11 @@
ErrorHandlingTest() {
Creator = [](ASTContext , FileManager ,
ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
return new ASTImporterWithFakeErrors(ToContext, ToFileManager,
FromContext, FromFileManager,
- MinimalImport, LookupTable);
+ MinimalImport, SharedState);
};
}
// In this test we purposely report an error (UnsupportedConstruct) when
@@ -4999,6 +5001,79 @@
EXPECT_TRUE(ImportedOK);
}
+// An error should be set for a class if it had a previous import with an error
+// from another TU.
+TEST_P(ErrorHandlingTest,
+ ImportedDeclWithErrorShouldFailTheImportOfDeclWhichMapToIt) {
+ // We already have a fwd decl.
+ TranslationUnitDecl *ToTU = getToTuDecl(
+ "class X;", Lang_CXX);
+ // Then we import a definition.
+ {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+ void f() { )") + ErroneousStmt + R"( }
+ void ok();
+};
+)",
+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);
+ }
+ // ... but the node had been created.
+ auto *ToXDef = FirstDeclMatcher().match(
+ ToTU, cxxRecordDecl(hasName("X"), isDefinition()));
+ // An error is set for "ToXDef" in the shared state.
+ Optional OptErr =
+ SharedStatePtr->getImportDeclErrorIfAny(ToXDef);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+
+ auto *ToXFwd = FirstDeclMatcher().match(
+ ToTU, cxxRecordDecl(hasName("X"), unless(isDefinition(;
+ // An error is NOT set for the fwd Decl of X in the shared state.
+ OptErr = SharedStatePtr->getImportDeclErrorIfAny(ToXFwd);
+ ASSERT_FALSE(OptErr);
+
+ // Try to import X again but from another TU.
+ {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+ void f() { )") + ErroneousStmt + R"( }
+ void ok();
+};
+)",
+Lang_CXX, "input1.cc");
+
+auto *FromX = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("X")));
+CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+// If we did not save the errors for the "to" context then the below checks
+// would fail, because the lookup finds the fwd Decl of the existing
+// definition in the "to" context. We can reach the existing definition via
+// the found fwd Decl. That existing definition is structurally equivalent
+// (we check only the fields) with this one we want to import, so we return
+// with the existing definition, which is
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
martong updated this revision to Diff 206429.
martong added a comment.
- Add FIXMEs
- Set error for FromD if it maps to an existing Decl which has an error set
- Add test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62376/new/
https://reviews.llvm.org/D62376
Files:
clang/include/clang/AST/ASTImporter.h
clang/include/clang/AST/ASTImporterSharedState.h
clang/include/clang/CrossTU/CrossTranslationUnit.h
clang/lib/AST/ASTImporter.cpp
clang/lib/CrossTU/CrossTranslationUnit.cpp
clang/lib/Frontend/ASTMerge.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
@@ -316,10 +316,11 @@
RedirectingImporterTest() {
Creator = [](ASTContext , FileManager ,
ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
return new RedirectingImporter(ToContext, ToFileManager, FromContext,
FromFileManager, MinimalImport,
- LookupTable);
+ SharedState);
};
}
};
@@ -2888,7 +2889,7 @@
CXXMethodDecl *Method =
FirstDeclMatcher().match(ToClass, MethodMatcher);
ToClass->removeDecl(Method);
- LookupTablePtr->remove(Method);
+ SharedStatePtr->getLookupTable()->remove(Method);
}
ASSERT_EQ(DeclCounter().match(ToClass, MethodMatcher), 0u);
@@ -4723,10 +4724,11 @@
ErrorHandlingTest() {
Creator = [](ASTContext , FileManager ,
ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
return new ASTImporterWithFakeErrors(ToContext, ToFileManager,
FromContext, FromFileManager,
- MinimalImport, LookupTable);
+ MinimalImport, SharedState);
};
}
// In this test we purposely report an error (UnsupportedConstruct) when
@@ -4999,6 +5001,79 @@
EXPECT_TRUE(ImportedOK);
}
+// An error should be set for a class if it had a previous import with an error
+// from another TU.
+TEST_P(ErrorHandlingTest,
+ ImportedDeclWithErrorShouldFailTheImportOfDeclWhichMapToIt) {
+ // We already have a fwd decl.
+ TranslationUnitDecl *ToTU = getToTuDecl(
+ "class X;", Lang_CXX);
+ // Then we import a definition.
+ {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+ void f() { )") + ErroneousStmt + R"( }
+ void ok();
+};
+)",
+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);
+ }
+ // ... but the node had been created.
+ auto *ToXDef = FirstDeclMatcher().match(
+ ToTU, cxxRecordDecl(hasName("X"), isDefinition()));
+ // An error is set for "ToXDef" in the shared state.
+ Optional OptErr =
+ SharedStatePtr->getImportDeclErrorIfAny(ToXDef);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+
+ auto *ToXFwd = FirstDeclMatcher().match(
+ ToTU, cxxRecordDecl(hasName("X"), unless(isDefinition(;
+ // An error is NOT set for the fwd Decl of X in the shared state.
+ OptErr = SharedStatePtr->getImportDeclErrorIfAny(ToXFwd);
+ ASSERT_FALSE(OptErr);
+
+ // Try to import X again but from another TU.
+ {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+ void f() { )") + ErroneousStmt + R"( }
+ void ok();
+};
+)",
+Lang_CXX, "input1.cc");
+
+auto *FromX = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("X")));
+CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+// If we did not save the errors for the "to" context then the below checks
+// would fail, because the lookup finds the fwd Decl of the existing
+// definition in the "to" context. We can reach the existing definition via
+// the found fwd Decl. That existing definition is structurally equivalent
+// (we check only the fields) with this one we want to import, so we return
+// with the existing
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
martong added a comment. Alexei, Thanks for the review! I have provided test cases for the 3 previous patches on which this one depends on. I will provide additional tests next week for this one, and of course will address the other comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62376/new/ https://reviews.llvm.org/D62376 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
a_sidorin added a comment.
Hi Gabor!
I haven't find the import sequence examples we try to fix these ways in any of
the three patches these change consists of. Could you please provide some (or
point if I missed them)?
Comment at: clang/include/clang/AST/ASTImporterSharedState.h:46
+public:
+ ASTImporterSharedState() {}
+
`= default?`
Comment at: clang/lib/AST/ASTImporter.cpp:7724
void ASTImporter::AddToLookupTable(Decl *ToD) {
- if (LookupTable)
+ if (SharedState->getLookupTable())
if (auto *ToND = dyn_cast(ToD))
```
if (auto* LookupTable = ..._)
...
LookupTable->add();
```
looks a bit cleaner to me.
Comment at: clang/lib/AST/ASTImporter.cpp:7830
if (ToD) {
+// Already imported (possibly from another TU) and with an error.
+if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
I'm not sure if it is safe to compare declarations from different TUs by
pointers because they belong to different allocators.
Comment at: clang/lib/AST/ASTImporter.cpp:7831
+// Already imported (possibly from another TU) and with an error.
+if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+ return make_error(*Error);
getImportDeclErrorIfAny() is usually called for FromD, not for ToD. Is it
intentional?
Comment at: clang/lib/AST/ASTImporter.cpp:7867
if (PosF != ImportedFromDecls.end()) {
-if (LookupTable)
+if (SharedState->getLookupTable())
if (auto *ToND = dyn_cast(ToD))
I think we can encapsulate these conditions into
`SharedState::[add/remove]Decl[To/From]Lookup methods.
Comment at: clang/lib/AST/ASTImporter.cpp:7924
+ if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+return make_error(*Error);
+
I don' think that an import failure from another TU means that the import from
the current TU will fail.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62376/new/
https://reviews.llvm.org/D62376
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st
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.
This is the third step of the full error handling. We store the errors
for the Decls in the "to" context too. For that, however, we have to put
these errors in a shared state (among all the ASTImporter objects which
handle the same "to" context but different "from" contexts).
After a series of imports from different "from" TUs we have a "to" context
which may have erroneous nodes in it. (Remember, the AST is immutable so
there is no way to delete a node once we had created it and we realized
the error later.) All these erroneous nodes are marked in
ASTImporterSharedState::ImportErrors. Clients of the ASTImporter may
use this as an input. E.g. the static analyzer engine may not try to
analyze a function if that is marked as erroneous (it can be queried via
ASTImporterSharedState::getImportDeclErrorIfAny()).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D62376
Files:
clang/include/clang/AST/ASTImporter.h
clang/include/clang/AST/ASTImporterSharedState.h
clang/include/clang/CrossTU/CrossTranslationUnit.h
clang/lib/AST/ASTImporter.cpp
clang/lib/CrossTU/CrossTranslationUnit.cpp
clang/lib/Frontend/ASTMerge.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
@@ -316,10 +316,11 @@
RedirectingImporterTest() {
Creator = [](ASTContext , FileManager ,
ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
return new RedirectingImporter(ToContext, ToFileManager, FromContext,
FromFileManager, MinimalImport,
- LookupTable);
+ SharedState);
};
}
};
@@ -2888,7 +2889,7 @@
CXXMethodDecl *Method =
FirstDeclMatcher().match(ToClass, MethodMatcher);
ToClass->removeDecl(Method);
- LookupTablePtr->remove(Method);
+ SharedStatePtr->getLookupTable()->remove(Method);
}
ASSERT_EQ(DeclCounter().match(ToClass, MethodMatcher), 0u);
Index: clang/unittests/AST/ASTImporterFixtures.h
===
--- clang/unittests/AST/ASTImporterFixtures.h
+++ clang/unittests/AST/ASTImporterFixtures.h
@@ -17,8 +17,8 @@
#include "gmock/gmock.h"
#include "clang/AST/ASTImporter.h"
-#include "clang/AST/ASTImporterLookupTable.h"
#include "clang/Frontend/ASTUnit.h"
+#include "clang/AST/ASTImporterSharedState.h"
#include "DeclMatcher.h"
#include "Language.h"
@@ -26,7 +26,7 @@
namespace clang {
class ASTImporter;
-class ASTImporterLookupTable;
+class ASTImporterSharedState;
class ASTUnit;
namespace ast_matchers {
@@ -77,9 +77,9 @@
public:
/// Allocates an ASTImporter (or one of its subclasses).
- typedef std::function
+ typedef std::function )>
ImporterConstructor;
// The lambda that constructs the ASTImporter we use in this test.
@@ -104,11 +104,13 @@
ImporterConstructor C = ImporterConstructor());
~TU();
-void lazyInitImporter(ASTImporterLookupTable , ASTUnit *ToAST);
-Decl *import(ASTImporterLookupTable , ASTUnit *ToAST,
- Decl *FromDecl);
-QualType import(ASTImporterLookupTable , ASTUnit *ToAST,
-QualType FromType);
+void
+lazyInitImporter(const std::shared_ptr ,
+ ASTUnit *ToAST);
+Decl *import(const std::shared_ptr ,
+ ASTUnit *ToAST, Decl *FromDecl);
+QualType import(const std::shared_ptr ,
+ASTUnit *ToAST, QualType FromType);
};
// We may have several From contexts and related translation units. In each
@@ -120,14 +122,14 @@
// vector is expanding, with the list we won't have these issues.
std::list FromTUs;
- // Initialize the lookup table if not initialized already.
- void lazyInitLookupTable(TranslationUnitDecl *ToTU);
+ // Initialize the shared state if not initialized already.
+ void lazyInitSharedState(TranslationUnitDecl *ToTU);
void lazyInitToAST(Language ToLang, StringRef ToSrcCode, StringRef FileName);
TU *findFromTU(Decl *From);
protected:
- std::unique_ptr LookupTablePtr;
+ std::shared_ptr SharedStatePtr;
public:
// We may have several From context but only one To context.
Index: clang/unittests/AST/ASTImporterFixtures.cpp
===
---
