Ensure that we delete destructors in the right cases. Specifically:

zygoloid · zygoloid · commit 921bd20dddf5 · 2012-02-26T09:11:52.000Z
- variant members with nontrivial destructors make the containing class's
   destructor deleted
 - check for a virtual destructor after checking for overridden methods in the
   base class(es)
 - check for an inaccessible operator delete for a class with a virtual
   destructor.

Do not try to call an anonymous union field's destructor from the destructor of
the containing class.

llvm-svn: 151483
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
@@ -1062,6 +1062,10 @@ void CodeGenFunction::EnterDtorCleanups(const CXXDestructorDecl *DD,
     QualType::DestructionKind dtorKind = type.isDestructedType();
     if (!dtorKind) continue;
 
+    // Anonymous union members do not have their destructors called.
+    const RecordType *RT = type->getAsUnionType();
+    if (RT && RT->getDecl()->isAnonymousStructOrUnion()) continue;
+
     CleanupKind cleanupKind = getCleanupKind(dtorKind);
     EHStack.pushCleanup<DestroyField>(cleanupKind, field,
                                       getDestroyer(dtorKind),
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -3293,6 +3293,9 @@ Sema::MarkBaseAndMemberDestructorsReferenced(SourceLocation Location,
       continue;
     if (FieldClassDecl->hasIrrelevantDestructor())
       continue;
+    // The destructor for an implicit anonymous union member is never invoked.
+    if (FieldClassDecl->isUnion() && FieldClassDecl->isAnonymousStructOrUnion())
+      continue;
 
     CXXDestructorDecl *Dtor = LookupDestructor(FieldClassDecl);
     assert(Dtor && "No dtor found for FieldClassDecl!");
@@ -4372,31 +4375,33 @@ struct SpecialMemberDeletionInfo {
                                  TQ & Qualifiers::Volatile);
   }
 
+  bool shouldDeleteForClassSubobject(CXXRecordDecl *Class, FieldDecl *Field);
+
   bool shouldDeleteForBase(CXXRecordDecl *BaseDecl, bool IsVirtualBase);
   bool shouldDeleteForField(FieldDecl *FD);
   bool shouldDeleteForAllConstMembers();
 };
 }
 
-/// Check whether we should delete a special member function due to the class
-/// having a particular direct or virtual base class.
-bool SpecialMemberDeletionInfo::shouldDeleteForBase(CXXRecordDecl *BaseDecl,
-                                                    bool IsVirtualBase) {
-  // C++11 [class.copy]p23:
-  // -- for the move assignment operator, any direct or indirect virtual
-  //    base class.
-  if (CSM == Sema::CXXMoveAssignment && IsVirtualBase)
-    return true;
-
+/// Check whether we should delete a special member function due to having a
+/// direct or virtual base class or static data member of class type M.
+bool SpecialMemberDeletionInfo::shouldDeleteForClassSubobject(
+    CXXRecordDecl *Class, FieldDecl *Field) {
   // C++11 [class.ctor]p5, C++11 [class.copy]p11, C++11 [class.dtor]p5:
   // -- any direct or virtual base class [...] has a type with a destructor
   //    that is deleted or inaccessible
   if (!IsAssignment) {
-    CXXDestructorDecl *BaseDtor = S.LookupDestructor(BaseDecl);
-    if (BaseDtor->isDeleted())
+    CXXDestructorDecl *Dtor = S.LookupDestructor(Class);
+    if (Dtor->isDeleted())
       return true;
-    if (S.CheckDestructorAccess(Loc, BaseDtor, S.PDiag())
-          != Sema::AR_accessible)
+    if (S.CheckDestructorAccess(Loc, Dtor, S.PDiag()) != Sema::AR_accessible)
+      return true;
+
+    // C++11 [class.dtor]p5:
+    // -- X is a union-like class that has a variant member with a non-trivial
+    //    destructor
+    if (CSM == Sema::CXXDestructor && Field && Field->getParent()->isUnion() &&
+        !Dtor->isTrivial())
       return true;
   }
 
@@ -4410,50 +4415,58 @@ bool SpecialMemberDeletionInfo::shouldDeleteForBase(CXXRecordDecl *BaseDecl,
   //    overload resolution, as applied to B's corresponding special member,
   //    results in an ambiguity or a function that is deleted or inaccessible
   //    from the defaulted special member
+  // FIXME: in-class initializers should be handled here
   if (CSM != Sema::CXXDestructor) {
-    Sema::SpecialMemberOverloadResult *SMOR = lookupIn(BaseDecl);
+    Sema::SpecialMemberOverloadResult *SMOR = lookupIn(Class);
     if (!SMOR->hasSuccess())
       return true;
 
-    CXXMethodDecl *BaseMember = SMOR->getMethod();
+    CXXMethodDecl *Member = SMOR->getMethod();
+    // A member of a union must have a trivial corresponding special member.
+    if (Field && Field->getParent()->isUnion() && !Member->isTrivial())
+      return true;
+
     if (IsConstructor) {
-      CXXConstructorDecl *BaseCtor = cast<CXXConstructorDecl>(BaseMember);
-      if (S.CheckConstructorAccess(Loc, BaseCtor, BaseCtor->getAccess(),
-                                   S.PDiag()) != Sema::AR_accessible)
+      CXXConstructorDecl *Ctor = cast<CXXConstructorDecl>(Member);
+      if (S.CheckConstructorAccess(Loc, Ctor, Ctor->getAccess(), S.PDiag())
+            != Sema::AR_accessible)
         return true;
 
       // -- for the move constructor, a [...] direct or virtual base class with
       //    a type that does not have a move constructor and is not trivially
       //    copyable.
-      if (IsMove && !BaseCtor->isMoveConstructor() &&
-          !BaseDecl->isTriviallyCopyable())
+      if (IsMove && !Ctor->isMoveConstructor() && !Class->isTriviallyCopyable())
         return true;
     } else {
       assert(IsAssignment && "unexpected kind of special member");
-      if (S.CheckDirectMemberAccess(Loc, BaseMember, S.PDiag())
+      if (S.CheckDirectMemberAccess(Loc, Member, S.PDiag())
             != Sema::AR_accessible)
         return true;
 
       // -- for the move assignment operator, a direct base class with a type
       //    that does not have a move assignment operator and is not trivially
       //    copyable.
-      if (IsMove && !BaseMember->isMoveAssignmentOperator() &&
-          !BaseDecl->isTriviallyCopyable())
+      if (IsMove && !Member->isMoveAssignmentOperator() &&
+          !Class->isTriviallyCopyable())
         return true;
     }
   }
 
-  // C++11 [class.dtor]p5:
-  // -- for a virtual destructor, lookup of the non-array deallocation function
-  //    results in an ambiguity or in a function that is deleted or inaccessible
-  if (CSM == Sema::CXXDestructor && MD->isVirtual()) {
-    FunctionDecl *OperatorDelete = 0;
-    DeclarationName Name =
-      S.Context.DeclarationNames.getCXXOperatorName(OO_Delete);
-    if (S.FindDeallocationFunction(Loc, MD->getParent(), Name,
-                                   OperatorDelete, false))
-      return true;
-  }
+  return false;
+}
+
+/// Check whether we should delete a special member function due to the class
+/// having a particular direct or virtual base class.
+bool SpecialMemberDeletionInfo::shouldDeleteForBase(CXXRecordDecl *BaseDecl,
+                                                    bool IsVirtualBase) {
+  // C++11 [class.copy]p23:
+  // -- for the move assignment operator, any direct or indirect virtual
+  //    base class.
+  if (CSM == Sema::CXXMoveAssignment && IsVirtualBase)
+    return true;
+
+  if (shouldDeleteForClassSubobject(BaseDecl, 0))
+    return true;
 
   return false;
 }
@@ -4500,58 +4513,14 @@ bool SpecialMemberDeletionInfo::shouldDeleteForField(FieldDecl *FD) {
                                          UE = FieldRecord->field_end();
            UI != UE; ++UI) {
         QualType UnionFieldType = S.Context.getBaseElementType(UI->getType());
-        CXXRecordDecl *UnionFieldRecord =
-          UnionFieldType->getAsCXXRecordDecl();
 
         if (!UnionFieldType.isConstQualified())
           AllVariantFieldsAreConst = false;
 
-        if (UnionFieldRecord) {
-          // FIXME: Checking for accessibility and validity of this
-          //        destructor is technically going beyond the
-          //        standard, but this is believed to be a defect.
-          if (!IsAssignment) {
-            CXXDestructorDecl *FieldDtor = S.LookupDestructor(UnionFieldRecord);
-            if (FieldDtor->isDeleted())
-              return true;
-            if (S.CheckDestructorAccess(Loc, FieldDtor, S.PDiag()) !=
-                Sema::AR_accessible)
-              return true;
-            if (!FieldDtor->isTrivial())
-              return true;
-          }
-
-          // FIXME: in-class initializers should be handled here
-          if (CSM != Sema::CXXDestructor) {
-            Sema::SpecialMemberOverloadResult *SMOR =
-                lookupIn(UnionFieldRecord);
-            // FIXME: Checking for accessibility and validity of this
-            //        corresponding member is technically going beyond the
-            //        standard, but this is believed to be a defect.
-            if (!SMOR->hasSuccess())
-              return true;
-
-            CXXMethodDecl *FieldMember = SMOR->getMethod();
-            // A member of a union must have a trivial corresponding
-            // special member.
-            if (!FieldMember->isTrivial())
-              return true;
-
-            if (IsConstructor) {
-              CXXConstructorDecl *FieldCtor =
-                  cast<CXXConstructorDecl>(FieldMember);
-              if (S.CheckConstructorAccess(Loc, FieldCtor,
-                                           FieldCtor->getAccess(),
-                                           S.PDiag()) != Sema::AR_accessible)
-              return true;
-            } else {
-              assert(IsAssignment && "unexpected kind of special member");
-              if (S.CheckDirectMemberAccess(Loc, FieldMember, S.PDiag())
-                  != Sema::AR_accessible)
-                return true;
-            }
-          }
-        }
+        CXXRecordDecl *UnionFieldRecord = UnionFieldType->getAsCXXRecordDecl();
+        if (UnionFieldRecord &&
+            shouldDeleteForClassSubobject(UnionFieldRecord, *UI))
+          return true;
       }
 
       // At least one member in each anonymous union must be non-const
@@ -4564,9 +4533,9 @@ bool SpecialMemberDeletionInfo::shouldDeleteForField(FieldDecl *FD) {
       return false;
     }
 
-    // Unless we're doing assignment, the field's destructor must be
-    // accessible and not deleted.
-    if (!IsAssignment) {
+    // When checking a constructor, the field's destructor must be accessible
+    // and not deleted.
+    if (IsConstructor) {
       CXXDestructorDecl *FieldDtor = S.LookupDestructor(FieldRecord);
       if (FieldDtor->isDeleted())
         return true;
@@ -4578,13 +4547,18 @@ bool SpecialMemberDeletionInfo::shouldDeleteForField(FieldDecl *FD) {
     // Check that the corresponding member of the field is accessible,
     // unique, and non-deleted. We don't do this if it has an explicit
     // initialization when default-constructing.
-    if (CSM != Sema::CXXDestructor &&
-        !(CSM == Sema::CXXDefaultConstructor && FD->hasInClassInitializer())) {
+    if (!(CSM == Sema::CXXDefaultConstructor && FD->hasInClassInitializer())) {
       Sema::SpecialMemberOverloadResult *SMOR = lookupIn(FieldRecord);
       if (!SMOR->hasSuccess())
         return true;
 
       CXXMethodDecl *FieldMember = SMOR->getMethod();
+
+      // We need the corresponding member of a union to be trivial so that
+      // we can safely process all members simultaneously.
+      if (inUnion() && !FieldMember->isTrivial())
+        return true;
+
       if (IsConstructor) {
         CXXConstructorDecl *FieldCtor = cast<CXXConstructorDecl>(FieldMember);
         if (S.CheckConstructorAccess(Loc, FieldCtor, FieldCtor->getAccess(),
@@ -4597,6 +4571,13 @@ bool SpecialMemberDeletionInfo::shouldDeleteForField(FieldDecl *FD) {
         if (IsMove && !FieldCtor->isMoveConstructor() &&
             !FieldRecord->isTriviallyCopyable())
           return true;
+      } else if (CSM == Sema::CXXDestructor) {
+        CXXDestructorDecl *FieldDtor = S.LookupDestructor(FieldRecord);
+        if (FieldDtor->isDeleted())
+          return true;
+        if (S.CheckDestructorAccess(Loc, FieldDtor, S.PDiag()) !=
+            Sema::AR_accessible)
+          return true;
       } else {
         assert(IsAssignment && "unexpected kind of special member");
         if (S.CheckDirectMemberAccess(Loc, FieldMember, S.PDiag())
@@ -4610,14 +4591,6 @@ bool SpecialMemberDeletionInfo::shouldDeleteForField(FieldDecl *FD) {
             !FieldRecord->isTriviallyCopyable())
           return true;
       }
-
-      // We need the corresponding member of a union to be trivial so that
-      // we can safely copy them all simultaneously.
-      // FIXME: Note that performing the check here (where we rely on the lack
-      // of an in-class initializer) is technically ill-formed. However, this
-      // seems most obviously to be a bug in the standard.
-      if (inUnion() && !FieldMember->isTrivial())
-        return true;
     }
   } else if (CSM == Sema::CXXDefaultConstructor && !inUnion() &&
              FieldType.isConstQualified() && !FD->hasInClassInitializer()) {
@@ -4652,6 +4625,8 @@ bool Sema::ShouldDeleteSpecialMember(CXXMethodDecl *MD, CXXSpecialMember CSM) {
   if (!LangOpts.CPlusPlus0x || RD->isInvalidDecl())
     return false;
 
+  // FIXME: Provide the ability to diagnose why a special member was deleted.
+
   // C++11 [expr.lambda.prim]p19:
   //   The closure type associated with a lambda-expression has a
   //   deleted (8.4.3) default constructor and a deleted copy
@@ -4660,6 +4635,18 @@ bool Sema::ShouldDeleteSpecialMember(CXXMethodDecl *MD, CXXSpecialMember CSM) {
       (CSM == CXXDefaultConstructor || CSM == CXXCopyAssignment))
     return true;
 
+  // C++11 [class.dtor]p5:
+  // -- for a virtual destructor, lookup of the non-array deallocation function
+  //    results in an ambiguity or in a function that is deleted or inaccessible
+  if (CSM == Sema::CXXDestructor && MD->isVirtual()) {
+    FunctionDecl *OperatorDelete = 0;
+    DeclarationName Name =
+      Context.DeclarationNames.getCXXOperatorName(OO_Delete);
+    if (FindDeallocationFunction(MD->getLocation(), MD->getParent(), Name,
+                                 OperatorDelete, false))
+      return true;
+  }
+
   // For an anonymous struct or union, the copy and assignment special members
   // will never be used, so skip the check. For an anonymous union declared at
   // namespace scope, the constructor and destructor are used.
@@ -4672,8 +4659,6 @@ bool Sema::ShouldDeleteSpecialMember(CXXMethodDecl *MD, CXXSpecialMember CSM) {
 
   SpecialMemberDeletionInfo SMI(*this, MD, CSM);
 
-  // FIXME: We should put some diagnostic logic right into this function.
-
   for (CXXRecordDecl::base_class_iterator BI = RD->bases_begin(),
                                           BE = RD->bases_end(); BI != BE; ++BI)
     if (!BI->isVirtual() &&
@@ -7216,11 +7201,11 @@ CXXDestructorDecl *Sema::DeclareImplicitDestructor(CXXRecordDecl *ClassDecl) {
   // This could be uniqued if it ever proves significant.
   Destructor->setTypeSourceInfo(Context.getTrivialTypeSourceInfo(Ty));
 
+  AddOverriddenMethods(ClassDecl, Destructor);
+
   if (ShouldDeleteSpecialMember(Destructor, CXXDestructor))
     Destructor->setDeletedAsWritten();
-  
-  AddOverriddenMethods(ClassDecl, Destructor);
-  
+
   return Destructor;
 }
 
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
@@ -1666,9 +1666,13 @@ bool Sema::FindAllocationOverload(SourceLocation StartLoc, SourceRange Range,
 
       Args[i] = Result.takeAs<Expr>();
     }
+
     Operator = FnDecl;
-    CheckAllocationAccess(StartLoc, Range, R.getNamingClass(), Best->FoundDecl,
-                          Diagnose);
+
+    if (CheckAllocationAccess(StartLoc, Range, R.getNamingClass(),
+                              Best->FoundDecl, Diagnose) == AR_inaccessible)
+      return true;
+
     return false;
   }
 
@@ -1895,8 +1899,10 @@ bool Sema::FindDeallocationFunction(SourceLocation StartLoc, CXXRecordDecl *RD,
       return true;
     }
 
-    CheckAllocationAccess(StartLoc, SourceRange(), Found.getNamingClass(),
-                          Matches[0], Diagnose);
+    if (CheckAllocationAccess(StartLoc, SourceRange(), Found.getNamingClass(),
+                              Matches[0], Diagnose) == AR_inaccessible)
+      return true;
+
     return false;
 
   // We found multiple suitable operators;  complain about the ambiguity.
diff --git a/clang/test/CXX/special/class.dtor/p5-0x.cpp b/clang/test/CXX/special/class.dtor/p5-0x.cpp
