From 33b1f634e074835a1b49c23d2b7674161fef1762 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Mon, 4 Nov 2013 04:26:14 +0000 Subject: [PATCH] Issue a diagnostic if an implicitly-defined move assignment operator would move the same virtual base class multiple times (and the move assignment is used, and the move assignment for the virtual base is not trivial). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@193977 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticSemaKinds.td | 7 ++ lib/Sema/SemaDeclCXX.cpp | 109 ++++++++++++++++-- .../special/class.copy/implicit-move-def.cpp | 2 +- test/CXX/special/class.copy/implicit-move.cpp | 55 +++++++++ 4 files changed, 162 insertions(+), 11 deletions(-) diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index c3c89a7e52..c1c701db08 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -6054,6 +6054,13 @@ def err_out_of_line_default_deletes : Error< "defaulting this %select{default constructor|copy constructor|move " "constructor|copy assignment operator|move assignment operator|destructor}0 " "would delete it after its first declaration">; +def warn_vbase_moved_multiple_times : Warning< + "defaulted move assignment operator of %0 will move assign virtual base " + "class %1 multiple times">, InGroup>; +def note_vbase_moved_here : Note< + "%select{%1 is a virtual base class of base class %2 declared here|" + "virtual base class %1 declared here}0">; + def ext_implicit_exception_spec_mismatch : ExtWarn< "function previously declared with an %select{explicit|implicit}0 exception " "specification redeclared with an %select{implicit|explicit}0 exception " diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp index 45b03badb2..8b04f8dfcf 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -9627,6 +9627,94 @@ CXXMethodDecl *Sema::DeclareImplicitMoveAssignment(CXXRecordDecl *ClassDecl) { return MoveAssignment; } +/// Check if we're implicitly defining a move assignment operator for a class +/// with virtual bases. Such a move assignment might move-assign the virtual +/// base multiple times. +static void checkMoveAssignmentForRepeatedMove(Sema &S, CXXRecordDecl *Class, + SourceLocation CurrentLocation) { + assert(!Class->isDependentContext() && "should not define dependent move"); + + // Only a virtual base could get implicitly move-assigned multiple times. + // Only a non-trivial move assignment can observe this. We only want to + // diagnose if we implicitly define an assignment operator that assigns + // two base classes, both of which move-assign the same virtual base. + if (Class->getNumVBases() == 0 || Class->hasTrivialMoveAssignment() || + Class->getNumBases() < 2) + return; + + llvm::SmallVector Worklist; + typedef llvm::DenseMap VBaseMap; + VBaseMap VBases; + + for (CXXRecordDecl::base_class_iterator BI = Class->bases_begin(), + BE = Class->bases_end(); + BI != BE; ++BI) { + Worklist.push_back(&*BI); + while (!Worklist.empty()) { + CXXBaseSpecifier *BaseSpec = Worklist.pop_back_val(); + CXXRecordDecl *Base = BaseSpec->getType()->getAsCXXRecordDecl(); + + // If the base has no non-trivial move assignment operators, + // we don't care about moves from it. + if (!Base->hasNonTrivialMoveAssignment()) + continue; + + // If there's nothing virtual here, skip it. + if (!BaseSpec->isVirtual() && !Base->getNumVBases()) + continue; + + // If we're not actually going to call a move assignment for this base, + // or the selected move assignment is trivial, skip it. + Sema::SpecialMemberOverloadResult *SMOR = + S.LookupSpecialMember(Base, Sema::CXXMoveAssignment, + /*ConstArg*/false, /*VolatileArg*/false, + /*RValueThis*/true, /*ConstThis*/false, + /*VolatileThis*/false); + if (!SMOR->getMethod() || SMOR->getMethod()->isTrivial() || + !SMOR->getMethod()->isMoveAssignmentOperator()) + continue; + + if (BaseSpec->isVirtual()) { + // We're going to move-assign this virtual base, and its move + // assignment operator is not trivial. If this can happen for + // multiple distinct direct bases of Class, diagnose it. (If it + // only happens in one base, we'll diagnose it when synthesizing + // that base class's move assignment operator.) + CXXBaseSpecifier *&Existing = + VBases.insert(std::make_pair(Base->getCanonicalDecl(), BI)) + .first->second; + if (Existing && Existing != BI) { + S.Diag(CurrentLocation, diag::warn_vbase_moved_multiple_times) + << Class << Base; + S.Diag(Existing->getLocStart(), diag::note_vbase_moved_here) + << (Base->getCanonicalDecl() == + Existing->getType()->getAsCXXRecordDecl()->getCanonicalDecl()) + << Base << Existing->getType() << Existing->getSourceRange(); + S.Diag(BI->getLocStart(), diag::note_vbase_moved_here) + << (Base->getCanonicalDecl() == + BI->getType()->getAsCXXRecordDecl()->getCanonicalDecl()) + << Base << BI->getType() << BaseSpec->getSourceRange(); + + // Only diagnose each vbase once. + Existing = 0; + } + } else { + // Only walk over bases that have defaulted move assignment operators. + // We assume that any user-provided move assignment operator handles + // the multiple-moves-of-vbase case itself somehow. + if (!SMOR->getMethod()->isDefaulted()) + continue; + + // We're going to move the base classes of Base. Add them to the list. + for (CXXRecordDecl::base_class_iterator BI = Base->bases_begin(), + BE = Base->bases_end(); + BI != BE; ++BI) + Worklist.push_back(&*BI); + } + } + } +} + void Sema::DefineImplicitMoveAssignment(SourceLocation CurrentLocation, CXXMethodDecl *MoveAssignOperator) { assert((MoveAssignOperator->isDefaulted() && @@ -9656,16 +9744,9 @@ void Sema::DefineImplicitMoveAssignment(SourceLocation CurrentLocation, // are assigned, in the order in which they were declared in the class // definition. - // FIXME: Issue a warning if our implicit move assignment operator will move - // from a virtual base more than once. For instance, given: - // - // struct A { A &operator=(A&&); }; - // struct B : virtual A {}; - // struct C : virtual A {}; - // struct D : B, C {}; - // - // If the move assignment operator of D is synthesized, we should warn, - // because the A vbase will be moved from multiple times. + // Issue a warning if our implicit move assignment operator will move + // from a virtual base more than once. + checkMoveAssignmentForRepeatedMove(*this, ClassDecl, CurrentLocation); // The statements that form the synthesized function body. SmallVector Statements; @@ -9692,6 +9773,14 @@ void Sema::DefineImplicitMoveAssignment(SourceLocation CurrentLocation, bool Invalid = false; for (CXXRecordDecl::base_class_iterator Base = ClassDecl->bases_begin(), E = ClassDecl->bases_end(); Base != E; ++Base) { + // C++11 [class.copy]p28: + // It is unspecified whether subobjects representing virtual base classes + // are assigned more than once by the implicitly-defined copy assignment + // operator. + // FIXME: Do not assign to a vbase that will be assigned by some other base + // class. For a move-assignment, this can result in the vbase being moved + // multiple times. + // Form the assignment: // static_cast(this)->Base::operator=(static_cast(other)); QualType BaseType = Base->getType().getUnqualifiedType(); diff --git a/test/CXX/special/class.copy/implicit-move-def.cpp b/test/CXX/special/class.copy/implicit-move-def.cpp index 5c54aea124..5696d1f579 100644 --- a/test/CXX/special/class.copy/implicit-move-def.cpp +++ b/test/CXX/special/class.copy/implicit-move-def.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -emit-llvm -o - -std=c++11 %s | FileCheck -check-prefix=CHECK-ASSIGN %s +// FIXME: %clang_cc1 -emit-llvm -o - -std=c++11 %s | FileCheck -check-prefix=CHECK %s // RUN: %clang_cc1 -emit-llvm -o - -std=c++11 %s | FileCheck -check-prefix=CHECK-ASSIGN %s // RUN: %clang_cc1 -emit-llvm -o - -std=c++11 %s | FileCheck -check-prefix=CHECK-CTOR %s diff --git a/test/CXX/special/class.copy/implicit-move.cpp b/test/CXX/special/class.copy/implicit-move.cpp index 5c3a6027cd..23ecf2e7d9 100644 --- a/test/CXX/special/class.copy/implicit-move.cpp +++ b/test/CXX/special/class.copy/implicit-move.cpp @@ -243,6 +243,61 @@ namespace DR1402 { (void)CopyAndMove(cm); // expected-error {{deleted}} cm = cm; // expected-error {{deleted}} } + + namespace VbaseMove { + struct A {}; + struct B { B &operator=(B&&); }; + struct C { C &operator=(const C&); }; + struct D { B b; }; + + template + struct E : virtual T {}; + + template + struct E : virtual T { E &operator=(E&&); }; + + template + struct F : + E, // expected-note-re 2{{'[BD]' is a virtual base class of base class 'E<}} + E {}; // expected-note-re 2{{'[BD]' is a virtual base class of base class 'E<}} + + template + struct G : E, E {}; + + template + struct H : E, E {}; + + template + struct I : E, T {}; + + template + struct J : + E, // expected-note-re 2{{'[BD]' is a virtual base class of base class 'E<}} + virtual T {}; // expected-note-re 2{{virtual base class '[BD]' declared here}} + + template void move(T t) { t = static_cast(t); } + // expected-warning-re@-1 4{{defaulted move assignment operator of .* will move assign virtual base class '[BD]' multiple times}} + template void move(F); + template void move(F); // expected-note {{in instantiation of}} + template void move(F); + template void move(F); // expected-note {{in instantiation of}} + template void move(G); + template void move(G); + template void move(G); + template void move(G); + template void move(H); + template void move(H); + template void move(H); + template void move(H); + template void move(I); + template void move(I); + template void move(I); + template void move(I); + template void move(J); + template void move(J); // expected-note {{in instantiation of}} + template void move(J); + template void move(J); // expected-note {{in instantiation of}} + } } namespace PR12625 { -- 2.40.0