[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
abhinavgaba added inline comments.
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5974
+ SmallVectorImpl ) override {}
+};
+
Overloading `CompleteType(TagDecl*)` is causing a warning:
virtual void
`clang::ExternalASTSource::CompleteType(clang::ObjCInterfaceDecl*)’ was hidden
[-Woverloaded-virtual]`
One way to get rid of it would be to add this here :
private:
using ExternalASTSource::CompleteType;
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdef7c7f60205: [ASTImporter] Fix handling of not defined
FromRecord in ImportContext(...) (authored by shafik).
Herald added projects: clang, LLDB.
Changed prior to commit:
https://reviews.llvm.org/D78000?vs=259703=259747#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+ // Introduce a TypedefNameDecl
+ using Iterator = D *;
+};
+
+struct C {
+ // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+ // the DeclContext of C.
+ // We will invoke ImportContext(...) which should force the From Decl to
+ // be defined if it not already defined. We will then Import the definition
+ // to the To Decl.
+ // This will force processing of the base class of D which allows us to see
+ // base class methods such as dump().
+ D::Iterator iter;
+
+ bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int", result_value="42")
+ }
+};
+
+int main() {
+ C c;
+ D d;
+
+ c.f();
+
+ return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5942,6 +5942,70 @@
return ::testing::ValuesIn(Copy);
}
+struct ImportWithExternalSource : ASTImporterOptionSpecificTestBase {
+ ImportWithExternalSource() {
+Creator = [](ASTContext , FileManager ,
+ ASTContext , FileManager ,
+ bool MinimalImport,
+ const std::shared_ptr ) {
+ return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+ }
+};
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
+ std::vector
+ SourceWithCompletedTagList(std::vector )
+ : CompletedTags(CompletedTags) {}
+ void CompleteType(TagDecl *Tag) override {
+auto *Record = cast(Tag);
+Record->startDefinition();
+Record->completeDefinition();
+CompletedTags.push_back(Tag);
+ }
+ void
+ FindExternalLexicalDecls(const DeclContext *DC,
+ llvm::function_ref IsKindWeWant,
+ SmallVectorImpl ) override {}
+};
+
+TEST_P(ImportWithExternalSource, CompleteRecordBeforeImporting) {
+ // Create an empty TU.
+ TranslationUnitDecl *FromTU = getTuDecl("", Lang_CXX, "input.cpp");
+
+ // Create and add the test ExternalASTSource.
+ std::vector CompletedTags;
+ IntrusiveRefCntPtr source =
+ new SourceWithCompletedTagList(CompletedTags);
+ clang::ASTContext = FromTU->getASTContext();
+ Context.setExternalSource(std::move(source));
+
+ // Create a dummy class by hand with external lexical storage.
+ IdentifierInfo = Context.Idents.get("test_class");
+ auto *Record = CXXRecordDecl::Create(
+ Context, TTK_Class, FromTU, SourceLocation(), SourceLocation(), );
+ Record->setHasExternalLexicalStorage();
+ FromTU->addDecl(Record);
+
+ // Do a minimal import of the created class.
+ EXPECT_EQ(0U, CompletedTags.size());
+ Import(Record, Lang_CXX);
+ EXPECT_EQ(0U, CompletedTags.size());
+
+ // Import the definition of the created class.
+ llvm::Error Err = findFromTU(Record)->Importer->ImportDefinition(Record);
+ EXPECT_FALSE((bool)Err);
+ consumeError(std::move(Err));
+
+ // Make sure the class was completed once.
+ EXPECT_EQ(1U, CompletedTags.size());
+
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
shafik updated this revision to Diff 259703.
shafik marked 2 inline comments as done.
shafik added a comment.
Capitalization fixes.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+ // Introduce a TypedefNameDecl
+ using Iterator = D *;
+};
+
+struct C {
+ // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+ // the DeclContext of C.
+ // We will invoke ImportContext(...) which should force the From Decl to
+ // be defined if it not already defined. We will then Import the definition
+ // to the To Decl.
+ // This will force processing of the base class of D which allows us to see
+ // base class methods such as dump().
+ D::Iterator iter;
+
+ bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int", result_value="42")
+ }
+};
+
+int main() {
+ C c;
+ D d;
+
+ c.f();
+
+ return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5922,6 +5922,70 @@
EXPECT_TRUE(ToA);
}
+struct ImportWithExternalSource : ASTImporterOptionSpecificTestBase {
+ ImportWithExternalSource() {
+Creator = [](ASTContext , FileManager ,
+ ASTContext , FileManager ,
+ bool MinimalImport,
+ const std::shared_ptr ) {
+ return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+ }
+};
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
+ std::vector
+ SourceWithCompletedTagList(std::vector )
+ : CompletedTags(CompletedTags) {}
+ void CompleteType(TagDecl *Tag) override {
+auto *Record = cast(Tag);
+Record->startDefinition();
+Record->completeDefinition();
+CompletedTags.push_back(Tag);
+ }
+ void
+ FindExternalLexicalDecls(const DeclContext *DC,
+ llvm::function_ref IsKindWeWant,
+ SmallVectorImpl ) override {}
+};
+
+TEST_P(ImportWithExternalSource, CompleteRecordBeforeImporting) {
+ // Create an empty TU.
+ TranslationUnitDecl *FromTU = getTuDecl("", Lang_CXX, "input.cpp");
+
+ // Create and add the test ExternalASTSource.
+ std::vector CompletedTags;
+ IntrusiveRefCntPtr source =
+ new SourceWithCompletedTagList(CompletedTags);
+ clang::ASTContext = FromTU->getASTContext();
+ Context.setExternalSource(std::move(source));
+
+ // Create a dummy class by hand with external lexical storage.
+ IdentifierInfo = Context.Idents.get("test_class");
+ auto *Record = CXXRecordDecl::Create(
+ Context, TTK_Class, FromTU, SourceLocation(), SourceLocation(), );
+ Record->setHasExternalLexicalStorage();
+ FromTU->addDecl(Record);
+
+ // Do a minimal import of the created class.
+ EXPECT_EQ(0U, CompletedTags.size());
+ Import(Record, Lang_CXX);
+ EXPECT_EQ(0U, CompletedTags.size());
+
+ // Import the definition of the created class.
+ llvm::Error Err = findFromTU(Record)->Importer->ImportDefinition(Record);
+ EXPECT_FALSE((bool)Err);
+ consumeError(std::move(Err));
+
+ // Make sure the class was completed once.
+ EXPECT_EQ(1U, CompletedTags.size());
+ EXPECT_EQ(Record, CompletedTags.front());
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
DefaultTestValuesForRunOptions, );
@@ -5983,5 +6047,8 @@
INSTANTIATE_TEST_CASE_P(ParameterizedTests,
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
a_sidorin accepted this revision. a_sidorin added a comment. Hello Shafik! The patch looks good, I only have a few stylish nits. Comment at: clang/unittests/AST/ASTImporterTest.cpp:5964 + new SourceWithCompletedTagList(CompletedTags); + clang::ASTContext = FromTU->getASTContext(); + context.setExternalSource(std::move(source)); Context (starting with capital). Comment at: clang/unittests/AST/ASTImporterTest.cpp:5980 + // Import the definition of the created class. + llvm::Error err = findFromTU(Record)->Importer->ImportDefinition(Record); + EXPECT_FALSE((bool)err); Err (starting with capital). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
martong accepted this revision. martong added a comment. LGTM! Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
shafik updated this revision to Diff 259110.
shafik marked 2 inline comments as done.
shafik added a comment.
- Simplifying the changes in `ASTImporter::ImportContext`
- Removing `DC->setHasExternalLexicalStorage(true);` from unit test since we
are checking `getExternalSource()`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+ // Introduce a TypedefNameDecl
+ using Iterator = D *;
+};
+
+struct C {
+ // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+ // the DeclContext of C.
+ // We will invoke ImportContext(...) which should force the From Decl to
+ // be defined if it not already defined. We will then Import the definition
+ // to the To Decl.
+ // This will force processing of the base class of D which allows us to see
+ // base class methods such as dump().
+ D::Iterator iter;
+
+ bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int", result_value="42")
+ }
+};
+
+int main() {
+ C c;
+ D d;
+
+ c.f();
+
+ return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5922,6 +5922,70 @@
EXPECT_TRUE(ToA);
}
+struct ImportWithExternalSource : ASTImporterOptionSpecificTestBase {
+ ImportWithExternalSource() {
+Creator = [](ASTContext , FileManager ,
+ ASTContext , FileManager ,
+ bool MinimalImport,
+ const std::shared_ptr ) {
+ return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+ }
+};
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
+ std::vector
+ SourceWithCompletedTagList(std::vector )
+ : CompletedTags(CompletedTags) {}
+ void CompleteType(TagDecl *Tag) override {
+auto *Record = cast(Tag);
+Record->startDefinition();
+Record->completeDefinition();
+CompletedTags.push_back(Tag);
+ }
+ void
+ FindExternalLexicalDecls(const DeclContext *DC,
+ llvm::function_ref IsKindWeWant,
+ SmallVectorImpl ) override {}
+};
+
+TEST_P(ImportWithExternalSource, CompleteRecordBeforeImporting) {
+ // Create an empty TU.
+ TranslationUnitDecl *FromTU = getTuDecl("", Lang_CXX, "input.cpp");
+
+ // Create and add the test ExternalASTSource.
+ std::vector CompletedTags;
+ IntrusiveRefCntPtr source =
+ new SourceWithCompletedTagList(CompletedTags);
+ clang::ASTContext = FromTU->getASTContext();
+ context.setExternalSource(std::move(source));
+
+ // Create a dummy class by hand with external lexical storage.
+ IdentifierInfo = context.Idents.get("test_class");
+ auto *Record = CXXRecordDecl::Create(
+ context, TTK_Class, FromTU, SourceLocation(), SourceLocation(), );
+ Record->setHasExternalLexicalStorage();
+ FromTU->addDecl(Record);
+
+ // Do a minimal import of the created class.
+ EXPECT_EQ(0U, CompletedTags.size());
+ Import(Record, Lang_CXX);
+ EXPECT_EQ(0U, CompletedTags.size());
+
+ // Import the definition of the created class.
+ llvm::Error err = findFromTU(Record)->Importer->ImportDefinition(Record);
+ EXPECT_FALSE((bool)err);
+ consumeError(std::move(err));
+
+ // Make sure the class was completed once.
+ EXPECT_EQ(1U, CompletedTags.size());
+ EXPECT_EQ(Record, CompletedTags.front());
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests,
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
teemperor added a comment.
Beside Gabors comment I think I'm happy with this. Thanks!
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5939
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
"is completed" -> "it completed"
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5954
+ SmallVectorImpl ) override {
+DC->setHasExternalLexicalStorage(true);
+ }
You can remove this as you changed the check in the ASTImporter to only check
for the existence of an ExternalASTSource.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
martong added a comment.
Thanks for the test Shafik, that is pretty self explanatory!
Comment at: clang/lib/AST/ASTImporter.cpp:8161
if (auto *ToRecord = dyn_cast(ToDC)) {
auto *FromRecord = cast(FromDC);
if (ToRecord->isCompleteDefinition()) {
What if we did this a bit differently? We could simply complete the From type
if it is not completed, before getting into `ImportDefinition`.
```
if (ToRecord->isCompleteDefinition())
return ToDC;
auto *FromRecord = cast(FromDC);
if (FromRecord->hasExternalLexicalStorage() &&
!FromRecord->isCompleteDefinition())
FromRecord->getASTContext().getExternalSource()->CompleteType(
FromRecord);
```
This way we could get rid of the redundant calls of `ImportDefinition`. As we
have now a call for the case when we don't have external storage and for the
case when we have.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
shafik updated this revision to Diff 258084.
shafik added a comment.
- Added unit test
- Modified condition for defining `FromRecord` to whether it has an
`ExternalSource`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+ // Introduce a TypedefNameDecl
+ using Iterator = D *;
+};
+
+struct C {
+ // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+ // the DeclContext of C.
+ // We will invoke ImportContext(...) which should force the From Decl to
+ // be defined if it not already defined. We will then Import the definition
+ // to the To Decl.
+ // This will force processing of the base class of D which allows us to see
+ // base class methods such as dump().
+ D::Iterator iter;
+
+ bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int", result_value="42")
+ }
+};
+
+int main() {
+ C c;
+ D d;
+
+ c.f();
+
+ return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5922,6 +5922,72 @@
EXPECT_TRUE(ToA);
}
+struct ImportWithExternalSource : ASTImporterOptionSpecificTestBase {
+ ImportWithExternalSource() {
+Creator = [](ASTContext , FileManager ,
+ ASTContext , FileManager ,
+ bool MinimalImport,
+ const std::shared_ptr ) {
+ return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+ }
+};
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
+ std::vector
+ SourceWithCompletedTagList(std::vector )
+ : CompletedTags(CompletedTags) {}
+ void CompleteType(TagDecl *Tag) override {
+auto *Record = cast(Tag);
+Record->startDefinition();
+Record->completeDefinition();
+CompletedTags.push_back(Tag);
+ }
+ void
+ FindExternalLexicalDecls(const DeclContext *DC,
+ llvm::function_ref IsKindWeWant,
+ SmallVectorImpl ) override {
+DC->setHasExternalLexicalStorage(true);
+ }
+};
+
+TEST_P(ImportWithExternalSource, CompleteRecordBeforeImporting) {
+ // Create an empty TU.
+ TranslationUnitDecl *FromTU = getTuDecl("", Lang_CXX, "input.cpp");
+
+ // Create and add the test ExternalASTSource.
+ std::vector CompletedTags;
+ IntrusiveRefCntPtr source =
+ new SourceWithCompletedTagList(CompletedTags);
+ clang::ASTContext = FromTU->getASTContext();
+ context.setExternalSource(std::move(source));
+
+ // Create a dummy class by hand with external lexical storage.
+ IdentifierInfo = context.Idents.get("test_class");
+ auto *Record = CXXRecordDecl::Create(
+ context, TTK_Class, FromTU, SourceLocation(), SourceLocation(), );
+ Record->setHasExternalLexicalStorage();
+ FromTU->addDecl(Record);
+
+ // Do a minimal import of the created class.
+ EXPECT_EQ(0U, CompletedTags.size());
+ Decl *Imported = Import(Record, Lang_CXX);
+ EXPECT_EQ(0U, CompletedTags.size());
+
+ // Import the definition of the created class.
+ llvm::Error err = findFromTU(Record)->Importer->ImportDefinition(Record);
+ EXPECT_FALSE((bool)err);
+ consumeError(std::move(err));
+
+ // Make sure the class was completed once.
+ EXPECT_EQ(1U, CompletedTags.size());
+ EXPECT_EQ(Record, CompletedTags.front());
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:8173 + // If a RecordDecl has base classes they won't be imported and we will + // be missing anything that we inherit from those bases. + if (FromRecord->hasExternalLexicalStorage() && teemperor wrote: > I'm not sure how it can be that ASTImporter::CompleteDecl starts but never > finishes the decl as it does both things at once unconditionally? > ``` > lang=c++ > TD->startDefinition(); > TD->setCompleteDefinition(true); > ``` > > FWIW, this whole comment could just be `Ask the external source if this is > actually a complete record that just hasn't been completed yet` or something > like this. I think if we reach this point then it makes a complete sense to > ask the external source for more info. The explanation about the base classes > seems to be just a smaller detail of the whole picture here. `TD->setCompleteDefinition(true);` just sets a flag but it does not import the bases, which it can not do since From is not defined. See `ASTNodeImporter::ImportDefinition(RecordDecl *From, RecordDecl *To, ImportDefinitionKind Kind)` which does this after it does `To->startDefinition();`. So we can't really consider the definition finished if we don't do this. Do you have a better way of describing this situation? I think it is important to describe why we don't use `CompleteDecl` since the other cases do. Comment at: clang/lib/AST/ASTImporter.cpp:8173 + // If a RecordDecl has base classes they won't be imported and we will + // be missing anything that we inherit from those bases. + if (FromRecord->hasExternalLexicalStorage() && martong wrote: > shafik wrote: > > teemperor wrote: > > > I'm not sure how it can be that ASTImporter::CompleteDecl starts but > > > never finishes the decl as it does both things at once unconditionally? > > > ``` > > > lang=c++ > > > TD->startDefinition(); > > > TD->setCompleteDefinition(true); > > > ``` > > > > > > FWIW, this whole comment could just be `Ask the external source if this > > > is actually a complete record that just hasn't been completed yet` or > > > something like this. I think if we reach this point then it makes a > > > complete sense to ask the external source for more info. The explanation > > > about the base classes seems to be just a smaller detail of the whole > > > picture here. > > `TD->setCompleteDefinition(true);` just sets a flag but it does not import > > the bases, which it can not do since From is not defined. See > > `ASTNodeImporter::ImportDefinition(RecordDecl *From, RecordDecl *To, > > ImportDefinitionKind Kind)` which does this after it does > > `To->startDefinition();`. So we can't really consider the definition > > finished if we don't do this. > > > > Do you have a better way of describing this situation? > > > > I think it is important to describe why we don't use `CompleteDecl` since > > the other cases do. > > Ask the external source if this is actually a complete record that just > > hasn't been completed yet > > FWIW this seems to be a recurring pattern, so maybe it would be worth to do > this not just with `RecordDecl`s but with other kind of decls. E.g. an > `EnumDecl` could have an external source and not completed, couldn't it? This is definitely more costly, for the `EnumDecl` case I don't see how we could get in a similar situation since we don't have inheritance. I looked over the `EnumDecl` members and I don't see anything that would require it be defined. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
martong added a comment. Looks okay to me (other than the redundant import code that I have a comment about). > Also this seems to be testable via a Clang unit test, so I think this patch > should have one. Yeah, would be nice to have a Clang unit test. I believe we have the infrastructure for that in place. E.g. in `LLDBLookupTest` we have an lldb specific setup, that could be a good starting point. Comment at: clang/lib/AST/ASTImporter.cpp:8173 + // If a RecordDecl has base classes they won't be imported and we will + // be missing anything that we inherit from those bases. + if (FromRecord->hasExternalLexicalStorage() && teemperor wrote: > I'm not sure how it can be that ASTImporter::CompleteDecl starts but never > finishes the decl as it does both things at once unconditionally? > ``` > lang=c++ > TD->startDefinition(); > TD->setCompleteDefinition(true); > ``` > > FWIW, this whole comment could just be `Ask the external source if this is > actually a complete record that just hasn't been completed yet` or something > like this. I think if we reach this point then it makes a complete sense to > ask the external source for more info. The explanation about the base classes > seems to be just a smaller detail of the whole picture here. > Ask the external source if this is actually a complete record that just > hasn't been completed yet FWIW this seems to be a recurring pattern, so maybe it would be worth to do this not just with `RecordDecl`s but with other kind of decls. E.g. an `EnumDecl` could have an external source and not completed, couldn't it? Comment at: clang/lib/AST/ASTImporter.cpp:8179 + + if (FromRecord->isCompleteDefinition()) +if (Error Err = ASTNodeImporter(*this).ImportDefinition( We could merge this `if` with the `else if` at line 8164, because their body is exactly the same. But to do that, we should move the external storage query and type completion above the enclosing `if` (above line 8162 and just after line 8161). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
shafik updated this revision to Diff 257552.
shafik marked 4 inline comments as done.
shafik added a comment.
Addressing comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
Files:
clang/lib/AST/ASTImporter.cpp
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
Index:
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+ // Introduce a TypedefNameDecl
+ using Iterator = D *;
+};
+
+struct C {
+ // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+ // the DeclContext of C.
+ // We will invoke ImportContext(...) which should force the From Decl to
+ // be defined if it not already defined. We will then Import the definition
+ // to the To Decl.
+ // This will force processing of the base class of D which allows us to see
+ // base class methods such as dump().
+ D::Iterator iter;
+
+ bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int",
result_value="42")
+ }
+};
+
+int main() {
+ C c;
+ D d;
+
+ c.f();
+
+ return 0;
+}
Index:
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8166,7 +8166,20 @@
FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
return std::move(Err);
} else {
- CompleteDecl(ToRecord);
+ // If FromRecord is not defined we need to force it to be.
+ // Simply calling CompleteDecl(...) for a RecordDecl will break some
cases
+ // it will start the definition but we never finish it.
+ // If there are base classes they won't be imported and we will
+ // be missing anything that we inherit from those bases.
+ if (FromRecord->hasExternalLexicalStorage() &&
+ !FromRecord->isCompleteDefinition())
+FromRecord->getASTContext().getExternalSource()->CompleteType(
+FromRecord);
+
+ if (FromRecord->isCompleteDefinition())
+if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+ return std::move(Err);
}
} else if (auto *ToEnum = dyn_cast(ToDC)) {
auto *FromEnum = cast(FromDC);
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+ // Introduce a TypedefNameDecl
+ using Iterator = D *;
+};
+
+struct C {
+ // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+ // the DeclContext of C.
+ // We will invoke ImportContext(...) which should force the From Decl to
+ // be defined if it not already defined. We will then Import the definition
+ // to the To Decl.
+ // This will force processing of the base class of D which allows us to see
+ // base class methods such as dump().
+ D::Iterator iter;
+
+ bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int", result_value="42")
+ }
+};
+
+int main() {
+ C c;
+ D d;
+
+ c.f();
+
+ return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
From what I understand the whole idea here is to just ask the external AST
source to complete the record before we import them? If yes, then this seems
like the right idea to me.
Also this seems to be testable via a Clang unit test, so I think this patch
should have one.
But otherwise this LGTM. Thanks for figuring this out!
Comment at: clang/lib/AST/ASTImporter.cpp:8173
+ // If a RecordDecl has base classes they won't be imported and we will
+ // be missing anything that we inherit from those bases.
+ if (FromRecord->hasExternalLexicalStorage() &&
I'm not sure how it can be that ASTImporter::CompleteDecl starts but never
finishes the decl as it does both things at once unconditionally?
```
lang=c++
TD->startDefinition();
TD->setCompleteDefinition(true);
```
FWIW, this whole comment could just be `Ask the external source if this is
actually a complete record that just hasn't been completed yet` or something
like this. I think if we reach this point then it makes a complete sense to ask
the external source for more info. The explanation about the base classes seems
to be just a smaller detail of the whole picture here.
Comment at: clang/lib/AST/ASTImporter.cpp:8180
+ if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+ FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+return std::move(Err);
I assume we should check here that FromRecord is now a complete definition
before trying to import it's definition?
Comment at:
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp:25
+return true; //%self.expect_expr("DD->dump()", result_type="int",
+ // result_value="42")
+ }
You need to make this a "//%" as otherwise this test fails (which it does right
now).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
shafik added a comment. I am open to different approaches to this problem, this is opening attempt at fixing it but I may be missing other interactions. AFAICT starting the definition of `ToRecord` the way `CompleteDecl(…)` does when we know `FromRecord` is not defined is just broken for the `RecordDecl` case if we have bases. It took me a lot of time to come up with this reproducer from a real life issue debugging `llc`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
shafik created this revision.
shafik added reviewers: martong, balazske, teemperor, a_sidorin.
Herald added a subscriber: rnkovacs.
Herald added a reviewer: a.sidorin.
shafik added a comment.
I am open to different approaches to this problem, this is opening attempt at
fixing it but I may be missing other interactions.
AFAICT starting the definition of `ToRecord` the way `CompleteDecl(…)` does
when we know `FromRecord` is not defined is just broken for the `RecordDecl`
case if we have bases.
It took me a lot of time to come up with this reproducer from a real life issue
debugging `llc`.
In `ImportContext(…)` we may call into `CompleteDecl(…)` which if `FromRecrord`
is not defined will start the definition of a `ToRecord` but from what I can
tell at least one of the paths though here don't ensure we complete the
definition.
For a `RecordDecl` this can be problematic since this means we won’t import
base classes and we won’t have any of the methods or types we inherit from
these bases.
One such path is through `VisitTypedefNameDecl(…)` which is exercised by the
reproducer.
For LLDB expression parsing this results in expressions failing but sometimes
succeeding in subsequent attempts e.g.:
(lldb) p BB->dump()
error: no member named 'dump' in 'B'
(lldb) p BB->dump()
(lldb)
This happens because the first time through `FromRecord` is not defined but in
subsequent attempts through it may be.
https://reviews.llvm.org/D78000
Files:
clang/lib/AST/ASTImporter.cpp
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
Index:
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,36 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+ // Introduce a TypedefNameDecl
+ using Iterator = D *;
+};
+
+struct C {
+ // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+ // the DeclContext of C.
+ // We will invoke ImportContext(...) which should force the From Decl to
+ // be defined if it not already defined. We will then Import the definition
+ // to the To Decl.
+ // This will force processing of the base class of D which allows us to see
+ // base class methods such as dump().
+ D::Iterator iter;
+
+ bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int",
+ // result_value="42")
+ }
+};
+
+int main() {
+ C c;
+ D d;
+
+ c.f();
+
+ return 0;
+}
Index:
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8166,7 +8166,19 @@
FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
return std::move(Err);
} else {
- CompleteDecl(ToRecord);
+ // If FromRecord is not defined we need to force it to be.
+ // Simply calling CompleteDecl(...) for a RecordDecl will break some
cases
+ // it will start the definition but we never finish it.
+ // If a RecordDecl has base classes they won't be imported and we will
+ // be missing anything that we inherit from those bases.
+ if (FromRecord->hasExternalLexicalStorage() &&
+ !FromRecord->isCompleteDefinition())
+FromRecord->getASTContext().getExternalSource()->CompleteType(
+FromRecord);
+
+ if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+ FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+return std::move(Err);
}
} else if (auto *ToEnum = dyn_cast(ToDC)) {
auto *FromEnum = cast(FromDC);
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,36 @@
+struct B {
+ int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+
