This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Mark the implicit copy constructor as deleted when a move constructor is provided.
ClosedPublic

Authored by teemperor on Jan 14 2020, 5:08 AM.

Details

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.

Diff Detail

Event Timeline

teemperor created this revision.Jan 14 2020, 5:08 AM
teemperor updated this revision to Diff 237948.Jan 14 2020, 5:13 AM
  • Upload correct revision of the test file.

FWIW this patch makes me realise that we really should have a Sema around when we create the module AST as this would prevent issues like this and saves us from simulating Sema behavior. That's probably yet another large refactoring so I don't have time for that soon-ish but if anyone wants to do this then be my guest.

teemperor updated this revision to Diff 237981.Jan 14 2020, 7:59 AM
  • Readd test files.
shafik accepted this revision.Jan 17 2020, 10:49 AM

LGTM

lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/main.cpp
15

Is this the only way to trigger the assert on the command line?

lldb/source/Symbol/ClangASTContext.cpp
7786

There are other cases where special member functions should be deleted as well. I don't know if they will show up as asserts as well but it is unfortunate that we have to mirror the logic here.

This revision is now accepted and ready to land.Jan 17 2020, 10:49 AM
teemperor marked 2 inline comments as done.Jan 20 2020, 5:17 AM

Thanks for the review!

lldb/packages/Python/lldbsuite/test/commands/expression/deleting-implicit-copy-constructor/main.cpp
15

Actually touching IndirectlyDeletedCopyCstr in any other way also does the trick (but it was found by completing in the constructor). I added an expression that just tries to create an instance which tests the same thing (and I keep the completing around as that's a very cheap test as it doesn't codegen/run anything).

lldb/source/Symbol/ClangASTContext.cpp
7786

Please add them as tests when you have time!

teemperor updated this revision to Diff 239084.Jan 20 2020, 5:18 AM
  • Added a simple expr evaluation as an additional way to trigger the error.
This revision was automatically updated to reflect the committed changes.