[Lldb-commits] [lldb] 22447a6 - [lldb] Mark the implicit copy constructor as deleted when a move constructor is provided.
Raphael Isemann via lldb-commits
lldb-commits at lists.llvm.org
Mon Jan 20 05:34:34 PST 2020
Author: Raphael Isemann
Date: 2020-01-20T14:34:07+01:00
New Revision: 22447a61d405a9e279c7dad72b342dcc6e8b1b4b
URL: https://github.com/llvm/llvm-project/commit/22447a61d405a9e279c7dad72b342dcc6e8b1b4b
DIFF: https://github.com/llvm/llvm-project/commit/22447a61d405a9e279c7dad72b342dcc6e8b1b4b.diff
LOG: [lldb] Mark the implicit copy constructor as deleted when a move constructor is provided.
Summary:
CXXRecordDecls that have a move constructor but no copy constructor need to
have their implicit copy constructor marked as deleted (see C++11 [class.copy]p7, p18)
Currently we don't do that when building an AST with ClangASTContext which causes
Sema to realise that the AST is malformed and asserting when trying to create an implicit
copy constructor for us in the expression:
```
Assertion failed: ((data().DefaultedCopyConstructorIsDeleted || needsOverloadResolutionForCopyConstructor())
&& "Copy constructor should not be deleted"), function setImplicitCopyConstructorIsDeleted, file include/clang/AST/DeclCXX.h, line 828.
```
In the test case there is a class `NoCopyCstr` that should have its copy constructor marked as
deleted (as it has a move constructor). When we end up trying to tab complete in the
`IndirectlyDeletedCopyCstr` constructor, Sema realises that the `IndirectlyDeletedCopyCstr`
has no implicit copy constructor and tries to create one for us. It then realises that
`NoCopyCstr` also has no copy constructor it could find via lookup. However because we
haven't marked the FieldDecl as having a deleted copy constructor the
`needsOverloadResolutionForCopyConstructor()` returns false and the assert fails.
`needsOverloadResolutionForCopyConstructor()` would return true if during the time we
added the `NoCopyCstr` FieldDecl to `IndirectlyDeletedCopyCstr` we would have actually marked
it as having a deleted copy constructor (which would then mark the copy constructor of
`IndirectlyDeletedCopyCstr ` as needing overload resolution and Sema is happy).
This patch sets the correct mark when we complete our CXXRecordDecls (which is the time when
we know whether a copy constructor has been declared). In theory we don't have to do this if
we had a Sema around when building our debug info AST but at the moment we don't have this
so this has to do the job for now.
Reviewers: shafik
Reviewed By: shafik
Subscribers: aprantl, JDevlieghere, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D72694
Added:
lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/TestDeletingImplicitCopyConstructor.py
lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/main.cpp
Modified:
lldb/source/Symbol/ClangASTContext.cpp
lldb/unittests/Symbol/TestClangASTContext.cpp
Removed:
lldb/packages/Python/lldbsuite/test/commands/expression/completion-crash1/TestCompletionCrash1.py
lldb/packages/Python/lldbsuite/test/commands/expression/completion-crash1/main.cpp
################################################################################
diff --git a/lldb/packages/Python/lldbsuite/test/commands/expression/completion-crash1/TestCompletionCrash1.py b/lldb/packages/Python/lldbsuite/test/commands/expression/completion-crash1/TestCompletionCrash1.py
deleted file mode 100644
index 3f2a6100607d..000000000000
--- a/lldb/packages/Python/lldbsuite/test/commands/expression/completion-crash1/TestCompletionCrash1.py
+++ /dev/null
@@ -1,4 +0,0 @@
-from lldbsuite.test import lldbinline
-from lldbsuite.test import decorators
-
-lldbinline.MakeInlineTest(__file__, globals(), [decorators.skipIf(bugnumber="rdar://53659341")])
diff --git a/lldb/packages/Python/lldbsuite/test/commands/expression/completion-crash1/main.cpp b/lldb/packages/Python/lldbsuite/test/commands/expression/completion-crash1/main.cpp
deleted file mode 100644
index 7b123c0662d3..000000000000
--- a/lldb/packages/Python/lldbsuite/test/commands/expression/completion-crash1/main.cpp
+++ /dev/null
@@ -1,12 +0,0 @@
-namespace std {
-struct a {
- a() {}
- a(a &&);
-};
-template <class> struct au {
- a ay;
- ~au() { //%self.dbg.GetCommandInterpreter().HandleCompletion("e ", len("e "), 0, -1, lldb.SBStringList())
- }
-};
-}
-int main() { std::au<int>{}; }
diff --git a/lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/TestDeletingImplicitCopyConstructor.py b/lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/TestDeletingImplicitCopyConstructor.py
new file mode 100644
index 000000000000..c8308c16011e
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/TestDeletingImplicitCopyConstructor.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
diff --git a/lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/main.cpp b/lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/main.cpp
new file mode 100644
index 000000000000..1387a6326439
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/main.cpp
@@ -0,0 +1,20 @@
+struct NoCopyCstr {
+ NoCopyCstr() {}
+ // No copy constructor but a move constructor means we have an
+ // implicitly deleted copy constructor (C++11 [class.copy]p7, p18).
+ NoCopyCstr(NoCopyCstr &&);
+};
+struct IndirectlyDeletedCopyCstr {
+ // This field indirectly deletes the implicit copy constructor.
+ NoCopyCstr field;
+ // Completing in the constructor or constructing the class
+ // will cause Sema to declare the special members of IndirectlyDeletedCopyCstr.
+ // If we correctly set the deleted implicit copy constructor in NoCopyCstr then this
+ // should have propagated to this record and Clang won't crash.
+ IndirectlyDeletedCopyCstr() { //%self.expect_expr("IndirectlyDeletedCopyCstr x; 1+1", result_type="int", result_value="2")
+ //%self.dbg.GetCommandInterpreter().HandleCompletion("e ", len("e "), 0, -1, lldb.SBStringList())
+ }
+};
+int main() {
+ IndirectlyDeletedCopyCstr{};
+}
diff --git a/lldb/source/Symbol/ClangASTContext.cpp b/lldb/source/Symbol/ClangASTContext.cpp
index ac3bce179d9e..6d2bf3a69d33 100644
--- a/lldb/source/Symbol/ClangASTContext.cpp
+++ b/lldb/source/Symbol/ClangASTContext.cpp
@@ -7783,6 +7783,19 @@ bool ClangASTContext::CompleteTagDeclarationDefinition(
clang::TagDecl *tag_decl = tag_type->getDecl();
if (auto *cxx_record_decl = llvm::dyn_cast<CXXRecordDecl>(tag_decl)) {
+ // If we have a move constructor declared but no copy constructor we
+ // need to explicitly mark it as deleted. Usually Sema would do this for
+ // us in Sema::DeclareImplicitCopyConstructor but we don't have a Sema
+ // when building an AST from debug information.
+ // See also:
+ // C++11 [class.copy]p7, p18:
+ // If the class definition declares a move constructor or move assignment
+ // operator, an implicitly declared copy constructor or copy assignment
+ // operator is defined as deleted.
+ if (cxx_record_decl->hasUserDeclaredMoveConstructor() &&
+ cxx_record_decl->needsImplicitCopyConstructor())
+ cxx_record_decl->setImplicitCopyConstructorIsDeleted();
+
if (!cxx_record_decl->isCompleteDefinition())
cxx_record_decl->completeDefinition();
cxx_record_decl->setHasLoadedFieldsFromExternalStorage(true);
diff --git a/lldb/unittests/Symbol/TestClangASTContext.cpp b/lldb/unittests/Symbol/TestClangASTContext.cpp
index 99ef3ed5bef9..653c9ce01302 100644
--- a/lldb/unittests/Symbol/TestClangASTContext.cpp
+++ b/lldb/unittests/Symbol/TestClangASTContext.cpp
@@ -523,3 +523,89 @@ TEST_F(TestClangASTContext, TestFunctionTemplateInRecordConstruction) {
EXPECT_EQ("foo", func_template->getName());
EXPECT_EQ(clang::AccessSpecifier::AS_public, func_template->getAccess());
}
+
+TEST_F(TestClangASTContext, TestDeletingImplicitCopyCstrDueToMoveCStr) {
+ // We need to simulate this behavior in our AST that we construct as we don't
+ // have a Sema instance that can do this for us:
+ // C++11 [class.copy]p7, p18:
+ // If the class definition declares a move constructor or move assignment
+ // operator, an implicitly declared copy constructor or copy assignment
+ // operator is defined as deleted.
+
+ // Create a record and start defining it.
+ llvm::StringRef class_name = "S";
+ CompilerType t = clang_utils::createRecord(*m_ast, class_name);
+ m_ast->StartTagDeclarationDefinition(t);
+
+ // Create a move constructor that will delete the implicit copy constructor.
+ CompilerType return_type = m_ast->GetBasicType(lldb::eBasicTypeVoid);
+ CompilerType param_type = t.GetRValueReferenceType();
+ CompilerType function_type =
+ m_ast->CreateFunctionType(return_type, ¶m_type, /*num_params*/ 1,
+ /*variadic=*/false, /*quals*/ 0U);
+ bool is_virtual = false;
+ bool is_static = false;
+ bool is_inline = false;
+ bool is_explicit = true;
+ bool is_attr_used = false;
+ bool is_artificial = false;
+ m_ast->AddMethodToCXXRecordType(
+ t.GetOpaqueQualType(), class_name, nullptr, function_type,
+ lldb::AccessType::eAccessPublic, is_virtual, is_static, is_inline,
+ is_explicit, is_attr_used, is_artificial);
+
+ // Complete the definition and check the created record.
+ m_ast->CompleteTagDeclarationDefinition(t);
+ auto *record = llvm::cast<CXXRecordDecl>(ClangUtil::GetAsTagDecl(t));
+ // We can't call defaultedCopyConstructorIsDeleted() as this requires that
+ // the Decl passes through Sema which will actually compute this field.
+ // Instead we check that there is no copy constructor declared by the user
+ // which only leaves a non-deleted defaulted copy constructor as an option
+ // that our record will have no simple copy constructor.
+ EXPECT_FALSE(record->hasUserDeclaredCopyConstructor());
+ EXPECT_FALSE(record->hasSimpleCopyConstructor());
+}
+
+TEST_F(TestClangASTContext, TestNotDeletingUserCopyCstrDueToMoveCStr) {
+ // Tests that we don't delete the a user-defined copy constructor when
+ // a move constructor is provided.
+ // See also the TestDeletingImplicitCopyCstrDueToMoveCStr test.
+ llvm::StringRef class_name = "S";
+ CompilerType t = clang_utils::createRecord(*m_ast, class_name);
+ m_ast->StartTagDeclarationDefinition(t);
+
+ CompilerType return_type = m_ast->GetBasicType(lldb::eBasicTypeVoid);
+ bool is_virtual = false;
+ bool is_static = false;
+ bool is_inline = false;
+ bool is_explicit = true;
+ bool is_attr_used = false;
+ bool is_artificial = false;
+ // Create a move constructor.
+ {
+ CompilerType param_type = t.GetRValueReferenceType();
+ CompilerType function_type =
+ m_ast->CreateFunctionType(return_type, ¶m_type, /*num_params*/ 1,
+ /*variadic=*/false, /*quals*/ 0U);
+ m_ast->AddMethodToCXXRecordType(
+ t.GetOpaqueQualType(), class_name, nullptr, function_type,
+ lldb::AccessType::eAccessPublic, is_virtual, is_static, is_inline,
+ is_explicit, is_attr_used, is_artificial);
+ }
+ // Create a copy constructor.
+ {
+ CompilerType param_type = t.GetLValueReferenceType().AddConstModifier();
+ CompilerType function_type =
+ m_ast->CreateFunctionType(return_type, ¶m_type, /*num_params*/ 1,
+ /*variadic=*/false, /*quals*/ 0U);
+ m_ast->AddMethodToCXXRecordType(
+ t.GetOpaqueQualType(), class_name, nullptr, function_type,
+ lldb::AccessType::eAccessPublic, is_virtual, is_static, is_inline,
+ is_explicit, is_attr_used, is_artificial);
+ }
+
+ // Complete the definition and check the created record.
+ m_ast->CompleteTagDeclarationDefinition(t);
+ auto *record = llvm::cast<CXXRecordDecl>(ClangUtil::GetAsTagDecl(t));
+ EXPECT_TRUE(record->hasUserDeclaredCopyConstructor());
+}
More information about the lldb-commits
mailing list