teemperor created this revision.
teemperor added reviewers: shafik, labath.
Herald added subscribers: lldb-commits, JDevlieghere, abidh, aprantl.
Herald added a project: LLDB.
I noticed this strange line in `ASTImporterDelegate::ImportDefinitionTo` which
doesn't make a lot of sense:
to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
It forcibly sets the imported TagDecl to be defined if the source TagDecl was
defined. This doesn't make any
sense as in this code we already forced the ASTImporter to import the
definition so this should always be
a no-op.
Turns out this is hiding two bugs:
1. The way we handle forward declarations in the debug info that might be
completed later is that we import them and then mark them as having external
lexical storage. This makes Clang ask for the definition later when it needs it
(at which point we hopefully have the definition around and can complete it).
However, this is currently not completing the forward decls with external
storage but instead creates a duplicated decl in the target AST which is then
defined. The forward decl is kept incomplete after the import and we just
forcibly make it a definition of the record without any content with our
workaround. The TestSharedLib* tests is only passing because of this.
2. Minimal import of lambdas is broken and never imports the definition it
seems. That appears to be a bug in the ASTImporter which gives the definition
of lambda's some special treatment. TestLambdas.py is actually broken but is
passing because of this workaround.
This patch fixes the first bug by forcing the ASTImporter to import to the
target forward declaration. We can't
delete the workaround as the second bug is still around but that will be a
follow up review for the ASTImporter.
However it will get rid of all the duplicated RecordDecls that are in our
expression AST that are strangely defined
but don't have any of the fields they are supposed to have.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D73345
Files:
lldb/source/Symbol/ClangASTImporter.cpp
lldb/unittests/Symbol/TestClangASTImporter.cpp
Index: lldb/unittests/Symbol/TestClangASTImporter.cpp
===================================================================
--- lldb/unittests/Symbol/TestClangASTImporter.cpp
+++ lldb/unittests/Symbol/TestClangASTImporter.cpp
@@ -88,6 +88,33 @@
EXPECT_EQ(origin.decl, source.record_decl);
}
+TEST_F(TestClangASTImporter, CompleteFwdDeclWithOtherOrigin) {
+ // Create an AST with a full type that is defined.
+ clang_utils::SourceASTWithRecord source_with_definition;
+
+ // Create an AST with a type thst is only a forward declaration with the
+ // same name as the one in the the other source.
+ std::unique_ptr<TypeSystemClang> fwd_decl_source = clang_utils::createAST();
+ CompilerType fwd_decl_type = clang_utils::createRecord(
+ *fwd_decl_source, source_with_definition.record_decl->getName());
+
+ // Create a target and import the forward decl.
+ std::unique_ptr<TypeSystemClang> target = clang_utils::createAST();
+ ClangASTImporter importer;
+ CompilerType imported = importer.CopyType(*target, fwd_decl_type);
+ ASSERT_TRUE(imported.IsValid());
+ EXPECT_FALSE(imported.IsDefined());
+
+ // Now complete the forward decl with the definition from the other source.
+ // This should define the decl and give it the fields of the other origin.
+ clang::TagDecl *imported_tag_decl = ClangUtil::GetAsTagDecl(imported);
+ importer.CompleteTagDeclWithOrigin(imported_tag_decl,
+ source_with_definition.record_decl);
+ ASSERT_TRUE(imported.IsValid());
+ EXPECT_TRUE(imported.IsDefined());
+ EXPECT_EQ(1U, imported.GetNumFields());
+}
+
TEST_F(TestClangASTImporter, DeportDeclTagDecl) {
// Tests that the ClangASTImporter::DeportDecl completely copies TagDecls.
clang_utils::SourceASTWithRecord source;
Index: lldb/source/Symbol/ClangASTImporter.cpp
===================================================================
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -908,6 +908,14 @@
void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
clang::Decl *to, clang::Decl *from) {
+ // We might have a forward declaration from a shared library that we
+ // gave external lexical storage so that Clang asks us about the full
+ // definition when it needs it. In this case the ASTImporter isn't aware
+ // that the forward decl from the shared library is the actual import
+ // target but would create a second declaration that would then be defined.
+ // We want that 'to' is actually complete after this function so let's
+ // tell the ASTImporter that 'to' was imported from 'from'.
+ MapImported(from, to);
ASTImporter::Imported(from, to);
/*
Index: lldb/unittests/Symbol/TestClangASTImporter.cpp
===================================================================
--- lldb/unittests/Symbol/TestClangASTImporter.cpp
+++ lldb/unittests/Symbol/TestClangASTImporter.cpp
@@ -88,6 +88,33 @@
EXPECT_EQ(origin.decl, source.record_decl);
}
+TEST_F(TestClangASTImporter, CompleteFwdDeclWithOtherOrigin) {
+ // Create an AST with a full type that is defined.
+ clang_utils::SourceASTWithRecord source_with_definition;
+
+ // Create an AST with a type thst is only a forward declaration with the
+ // same name as the one in the the other source.
+ std::unique_ptr<TypeSystemClang> fwd_decl_source = clang_utils::createAST();
+ CompilerType fwd_decl_type = clang_utils::createRecord(
+ *fwd_decl_source, source_with_definition.record_decl->getName());
+
+ // Create a target and import the forward decl.
+ std::unique_ptr<TypeSystemClang> target = clang_utils::createAST();
+ ClangASTImporter importer;
+ CompilerType imported = importer.CopyType(*target, fwd_decl_type);
+ ASSERT_TRUE(imported.IsValid());
+ EXPECT_FALSE(imported.IsDefined());
+
+ // Now complete the forward decl with the definition from the other source.
+ // This should define the decl and give it the fields of the other origin.
+ clang::TagDecl *imported_tag_decl = ClangUtil::GetAsTagDecl(imported);
+ importer.CompleteTagDeclWithOrigin(imported_tag_decl,
+ source_with_definition.record_decl);
+ ASSERT_TRUE(imported.IsValid());
+ EXPECT_TRUE(imported.IsDefined());
+ EXPECT_EQ(1U, imported.GetNumFields());
+}
+
TEST_F(TestClangASTImporter, DeportDeclTagDecl) {
// Tests that the ClangASTImporter::DeportDecl completely copies TagDecls.
clang_utils::SourceASTWithRecord source;
Index: lldb/source/Symbol/ClangASTImporter.cpp
===================================================================
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -908,6 +908,14 @@
void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
clang::Decl *to, clang::Decl *from) {
+ // We might have a forward declaration from a shared library that we
+ // gave external lexical storage so that Clang asks us about the full
+ // definition when it needs it. In this case the ASTImporter isn't aware
+ // that the forward decl from the shared library is the actual import
+ // target but would create a second declaration that would then be defined.
+ // We want that 'to' is actually complete after this function so let's
+ // tell the ASTImporter that 'to' was imported from 'from'.
+ MapImported(from, to);
ASTImporter::Imported(from, to);
/*
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
