labath created this revision. labath added reviewers: teemperor, shafik. Herald added a project: LLDB. teemperor accepted this revision. teemperor added a comment. This revision is now accepted and ready to land.
To summarize IRC: I think the underlying cause here is that we get the Imported callback from the ASTImporter and then recursively start a completely new import process from that. I don't think that's actually supported by the ASTImporter so we probably should do the same thing we do elsewhere and queue all decls that need to be imported (see `CompleteTagDeclsScope`). But this improves the overall situation so this should land until this is properly fixed. D73024 <https://reviews.llvm.org/D73024> seems to have fixed one set crash, but it introduced another. Namely, if a class contains a covariant method returning itself, the logic in MaybeCompleteReturnType could cause us to attempt a recursive import, which would result in an assertion failure in clang::DeclContext::removeDecl. For some reason, this only manifested itself if the class contained at least two member variables, and the class itself was imported as a result of a recursive covariant import. This patch fixes the crash by not attempting to import classes which are already completed in MaybeCompleteReturnType. However, it's not clear to me if this is the right fix, or if this should be handled automatically by functions lower in the stack. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D76840 Files: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py lldb/test/API/lang/cpp/covariant-return-types/main.cpp Index: lldb/test/API/lang/cpp/covariant-return-types/main.cpp =================================================================== --- lldb/test/API/lang/cpp/covariant-return-types/main.cpp +++ lldb/test/API/lang/cpp/covariant-return-types/main.cpp @@ -28,6 +28,23 @@ OtherDerived& getOtherRef() override { return other_derived; } }; +// A regression test for a class with at least two members containing a +// covariant function, which is referenced through another covariant function. +struct BaseWithMembers { + int a = 42; + int b = 47; + virtual BaseWithMembers *get() { return this; } +}; +struct DerivedWithMembers: BaseWithMembers { + DerivedWithMembers *get() override { return this; } +}; +struct ReferencingBase { + virtual BaseWithMembers *getOther() { return new BaseWithMembers(); } +}; +struct ReferencingDerived: ReferencingBase { + DerivedWithMembers *getOther() { return new DerivedWithMembers(); } +}; + int main() { Derived derived; Base base; @@ -36,5 +53,7 @@ (void)base_ptr_to_derived->getRef(); (void)base_ptr_to_derived->getOtherPtr(); (void)base_ptr_to_derived->getOtherRef(); + + ReferencingDerived referencing_derived; return 0; // break here } Index: lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py =================================================================== --- lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py +++ lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py @@ -38,3 +38,5 @@ self.expect_expr("derived.getOtherRef().value()", result_summary='"derived"') self.expect_expr("base_ptr_to_derived->getOtherRef().value()", result_summary='"derived"') self.expect_expr("base.getOtherRef().value()", result_summary='"base"') + + self.expect_expr("referencing_derived.getOther()->get()->a", result_value='42') Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp =================================================================== --- lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp +++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp @@ -997,6 +997,8 @@ clang::RecordDecl *rd = return_type->getPointeeType()->getAsRecordDecl(); if (!rd) return; + if (rd->getDefinition()) + return; importer.CompleteTagDecl(rd); }
Index: lldb/test/API/lang/cpp/covariant-return-types/main.cpp =================================================================== --- lldb/test/API/lang/cpp/covariant-return-types/main.cpp +++ lldb/test/API/lang/cpp/covariant-return-types/main.cpp @@ -28,6 +28,23 @@ OtherDerived& getOtherRef() override { return other_derived; } }; +// A regression test for a class with at least two members containing a +// covariant function, which is referenced through another covariant function. +struct BaseWithMembers { + int a = 42; + int b = 47; + virtual BaseWithMembers *get() { return this; } +}; +struct DerivedWithMembers: BaseWithMembers { + DerivedWithMembers *get() override { return this; } +}; +struct ReferencingBase { + virtual BaseWithMembers *getOther() { return new BaseWithMembers(); } +}; +struct ReferencingDerived: ReferencingBase { + DerivedWithMembers *getOther() { return new DerivedWithMembers(); } +}; + int main() { Derived derived; Base base; @@ -36,5 +53,7 @@ (void)base_ptr_to_derived->getRef(); (void)base_ptr_to_derived->getOtherPtr(); (void)base_ptr_to_derived->getOtherRef(); + + ReferencingDerived referencing_derived; return 0; // break here } Index: lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py =================================================================== --- lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py +++ lldb/test/API/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py @@ -38,3 +38,5 @@ self.expect_expr("derived.getOtherRef().value()", result_summary='"derived"') self.expect_expr("base_ptr_to_derived->getOtherRef().value()", result_summary='"derived"') self.expect_expr("base.getOtherRef().value()", result_summary='"base"') + + self.expect_expr("referencing_derived.getOther()->get()->a", result_value='42') Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp =================================================================== --- lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp +++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp @@ -997,6 +997,8 @@ clang::RecordDecl *rd = return_type->getPointeeType()->getAsRecordDecl(); if (!rd) return; + if (rd->getDefinition()) + return; importer.CompleteTagDecl(rd); }
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits