[SemaCXX] Mark destructor as referenced

[ahatanak]

Summary

If an initializer in a braced-init-list is a C++ class that has a non-trivial destructor, mark the destructor as referenced. This fixes a crash in CodeGenFunction::destroyCXXObject that occurs when it tries to emit a call to the destructor on the stack unwinding path but the CXXRecordDecl of the class doesn't have a CXXDestructorDecl for the destructor.

[rjmccall]

lib/Sema/SemaInit.cpp

7691

If we're actually using the destructor, we can't just look it up and mark it referenced; we have to call CheckDestructorAccess and DiagnoseUseOfDecl.

Walking the syntactic initializers like this seems wrong:

• I don't know if we actually promise to rewrite these expressions to the actual recursive element initialization expression if the expression requires

conversion.

• This is going to miss implicitly-initialized elements, which can happen for a variety of reasons including (1) running out of explicit initializers and (2) designated initializers skipping a field.
• We'll end up with redundant diagnostics from CheckDestructorAccess/DiagnoseUseOfDecl if the expression is an already-materialized temporary.
• We'll end up with unwanted diagnostics from CheckDestructorAccess/DiagnoseUseOfDecl if the expression is a gl-value and actually part of a reference-initialization, where we don't actually use the destructor.

I think the right solution is probably that list-initialization needs to recognize that the initialization of a record needs to be treated as a potential use of the destructor even if we aren't supposed to bind it as a temporary, because once an object is initialized in the local context, there are all sorts of reasons why we might need to call its destructor before the initialization completes/transitions. Basically every call to shouldBindAsTemporary in this file is suspect, because they're all places where we might need to check the destructor use even if we didn't make a temporary.

We seem to get this right in SK_UserConversion where we call shouldDestroyEntity, and the comment there is spot-on — it doesn't make sense for this code to be specific to SK_UserConversion at all. My tentative suggestion would be to (1) make a helper function that does that exact "if should-bind then bind else if should-destroy then destroy" dance and then (2) look at all the should-bind checks and try to use your helper function instead. But I'd really like Richard and/or Doug to weigh in.

test/CodeGenObjCXX/arc-list-init-destruct.mm

2

Does the corresponding C++ test case (replacing Class0 *f; with HasExplicitNonTrivialDestructor f;) not reproduce the problem?

[ahatanak]

Richard and Doug, do you have any thoughts on John's suggestion?

test/CodeGenObjCXX/arc-list-init-destruct.mm

2

I wasn't able to reproduce the problem by changing the type of field 'f' to a C++ class with a non-trivial destructor because, if I make that change, Class1's destructor declaration gets added in Sema::AddImplicitlyDeclaredMembersToClass. I don't fully understand the reason behind it, but Class1's destructor declaration is added when the type of one of its subobject has a user-declared destructor.

[rsmith]

As it happens, the C++ committee fixed the language wording hole here very recently. The new rule can be found here: http://wg21.link/p0968r0#2227
In summary: we should to consider the destructor for all elements of the aggregate to be potentially-invoked.

@rjmccall Were you suggesting that we should also consider the destructor of the aggregate itself to be potentially-invoked, or just the elements? In the new-expression case at least, I don't think the former is permitted.

[rjmccall]

I agree that the new-expression case doesn't use the destructor, and all the other cases of list-initialization presumably use the destructor for the initialized type for separate reasons. Ok.

test/CodeGenObjCXX/arc-list-init-destruct.mm

2

Interesting, alright.

[ahatanak]

In D45898#1104025, @rsmith wrote:

As it happens, the C++ committee fixed the language wording hole here very recently. The new rule can be found here: http://wg21.link/p0968r0#2227
In summary: we should to consider the destructor for all elements of the aggregate to be potentially-invoked.

It doesn't mean that clang should reject the following code, does it?

// This should compile fine as long as 'Deleted7d d7d' is commented out.
struct DeletedDtor { ~DeletedDtor() = delete; };
struct Deleted7d { DeletedDtor a = {}; }; 
//Deleted7d d7d;

I tried making a helper function out of the code in SK_UserConversion and using it in "case SK_ListInitialization". That doesn't seem to work because DiagnoseUseOfDecl rejects the code (with error message "error: attempt to use a deleted function") above even though the destructor isn't needed (because 'd7d' is commented out), so I guess that check should be done in another place.

[ahatanak]

Implement the new rule defined here: http://wg21.link/p0968r0#2227. Produce a diagnostic if a class is initialized with aggregate initialization and one of its element's destructor is not accessible.

[rjmccall]

LGTM, but I'd like Richard to sign off, too.

[ahatanak]

ping

[ahatanak]

ping

[rsmith]

Does this check destructors of nested aggregate initializations in the case where brace elision occurs? I don't think just checking the top level of the SK_ListInitialization is enough. Perhaps it would make more sense to mark the dtors used from InitListChecker (in non-VerifyOnly mode).

[ahatanak]

Check if destructors are accessible from InitListChecker's constructor. Add test cases for designated initializer and brace elision.

[ahatanak]

ping

[rsmith]

lib/Sema/SemaInit.cpp

829

You also need to check base classes (since C++17, aggregates can have base classes).

829–833

I think this now checks too much: only those subobjects for which we formed an InitListExpr should be checked. Testcase:

struct A { friend struct B; private: ~A(); };
struct B { B(); A a; };
struct C { B b; };
C c = { B(); };

Here, we must not check that A's destructor is accessible from the context of C's initialization.

I think the best thing to do is probably to move the call to this function into InitListChecker::CheckStructUnionTypes, and make it just check one level.

830

Do we need a getBaseElementType() here? (What if the member is of array-of-class type?)

[ahatanak]

Move the call that checks the element's destructor into InitListChecker::CheckStructUnionTypes.

[rsmith]

lib/Sema/SemaInit.cpp

1827–1829

Usual Clang convention is to return true on error.

1862

Hmm, I don't think we considered this case when working on the relevant DR. It doesn't make much sense to check the destructors of the inactive union members. I'll take this back to WG21, but let's go with the wording as-is for now.

1869

It's a minor thing, but I think it'd be preferable to point the diagnostic at the relevant init list element, or at the close brace if the initializer was omitted.

[ahatanak]

Point the diagnostic at either the relevant init list element or at the close brace.

[ahatanak]

lib/Sema/SemaInit.cpp

1827–1829

I also renamed the function to hasAccessibleDestructor to make it clearer what the function does.

1869

The function that checks the destructor (hasAccessibleDestructor) is called in five different places now.

[orivej]

The committed test does not crash Clang 7, but the following test does, yet it compiles without any warnings by the current Clang trunk thanks to this fix.

struct A { ~A(); };
struct B : A {};
struct C { C();  B b; };
struct D { C c, d; };
D f() { return {}; }

[rjmccall]

Always worth adding more tests. Mind writing that one up as a commit?

[orivej]

Actually, arc-list-init-destruct.mm crashes Clang 7 with the same backtrace as this test case, and Clang trunk generates similar assembly (to the the point that I could essentially copy the // CHECK comments from the .mm test to a .cpp test), so I'm not sure if adding a .cpp test is valuable…

[rjmccall]

Yeah, if we weren't exercising the code in our test suite, that would be one thing, but I think a language-mode-specific test probably isn't too valuable.

[orivej]

I have noticed that this change breaks seemingly valid code:

class A { protected: ~A(); };
struct B : A {};
B f() { return B(); }
B g() { return {}; }

f compiles, but g fails with temporary of type 'A' has protected destructor. (g++ 8.2 compiles this file.)

[rjmccall]

In D45898#1324625, @orivej wrote:

I have noticed that this change breaks seemingly valid code:

class A { protected: ~A(); };
struct B : A {};
B f() { return B(); }
B g() { return {}; }

f compiles, but g fails with temporary of type 'A' has protected destructor. (g++ 8.2 compiles this file.)

We talked about this in the analysis for https://reviews.llvm.org/D53860, and Richard decided that it is indeed invalid under the standard.

As a language designer I'm not sure that's a good language rule, but it's the realm of a standard defect, not a compiler bug.

lib/Sema/SemaInit.cpp

InitListChecker::InitListChecker(Sema &S, const InitializedEntity &Entity,
   : SemaRef(S), VerifyOnly(VerifyOnly),
     TreatUnavailableAsInvalid(TreatUnavailableAsInvalid) {
   // FIXME: Check that IL isn't already the semantic form of some other
   // InitListExpr. If it is, we'd create a broken AST.
 
   hadError = false;
 
   FullyStructuredList =
       getStructuredSubobjectInit(IL, 0, T, nullptr, 0, IL->getSourceRange());
   CheckExplicitInitList(Entity, IL, T, FullyStructuredList,
                         /*TopLevelObject=*/true);
 
   if (!hadError && !VerifyOnly) {
     bool RequiresSecondPass = false;
     FillInEmptyInitializations(Entity, FullyStructuredList, RequiresSecondPass,
                                /*OuterILE=*/nullptr, /*OuterIndex=*/0);
     if (RequiresSecondPass && !hadError)
       FillInEmptyInitializations(Entity, FullyStructuredList,
                                  RequiresSecondPass, nullptr, 0);
   }
 }
SemaRef.Diag(InitExpr->getBeginLoc(), FlexArrayDiag)
         << InitExpr->getBeginLoc();
     SemaRef.Diag(Field->getLocation(), diag::note_flexible_array_member)
       << Field;
   }
 
   return FlexArrayDiag != diag::ext_flexible_array_init;
 }
 
+/// Check if the type of a class element has an accessible destructor.
+///
+/// Aggregate initialization requires a class element's destructor be
+/// accessible per 11.6.1 [dcl.init.aggr]:
+///
+/// The destructor for each element of class type is potentially invoked
+/// (15.4 [class.dtor]) from the context where the aggregate initialization
+/// occurs.
+static bool hasAccessibleDestructor(QualType ElementType, SourceLocation Loc,
+                                    Sema &SemaRef) {
+  auto *CXXRD = ElementType->getAsCXXRecordDecl();
+  if (!CXXRD)
+    return false;
+
+  CXXDestructorDecl *Destructor = SemaRef.LookupDestructor(CXXRD);
+  SemaRef.CheckDestructorAccess(Loc, Destructor,
+                                SemaRef.PDiag(diag::err_access_dtor_temp)
+                                    << ElementType);
+  SemaRef.MarkFunctionReferenced(Loc, Destructor);
+  if (SemaRef.DiagnoseUseOfDecl(Destructor, Loc))
+    return true;
+  return false;
+}
+
 void InitListChecker::CheckStructUnionTypes(
     const InitializedEntity &Entity, InitListExpr *IList, QualType DeclType,
     CXXRecordDecl::base_class_range Bases, RecordDecl::field_iterator Field,
     bool SubobjectIsDesignatorContext, unsigned &Index,
     InitListExpr *StructuredList, unsigned &StructuredIndex,
     bool TopLevelObject) {
   RecordDecl *structDecl = DeclType->getAs<RecordType>()->getDecl();
 
   // If the record is invalid, some of it's members are invalid. To avoid
   // confusion, we forgo checking the intializer for the entire record.
   if (structDecl->isInvalidDecl()) {
     // Assume it was supposed to consume a single initializer.
     ++Index;
     hadError = true;
     return;
   }
 
   if (DeclType->isUnionType() && IList->getNumInits() == 0) {
     RecordDecl *RD = DeclType->getAs<RecordType>()->getDecl();
 
+    if (!VerifyOnly)
+      for (FieldDecl *FD : RD->fields()) {
+        QualType ET = SemaRef.Context.getBaseElementType(FD->getType());
+        if (hasAccessibleDestructor(ET, IList->getEndLoc(), SemaRef)) {
+          hadError = true;
+          return;
+        }
+      }
+
     // If there's a default initializer, use it.
     if (isa<CXXRecordDecl>(RD) && cast<CXXRecordDecl>(RD)->hasInClassInitializer()) {
       if (VerifyOnly)
         return;
       for (RecordDecl::field_iterator FieldEnd = RD->field_end();
            Field != FieldEnd; ++Field) {
         if (Field->hasInClassInitializer()) {
           StructuredList->setInitializedFieldInUnion(*Field);
if (DeclType->isUnionType() && IList->getNumInits() == 0) {
     return;
   }
 
   bool InitializedSomething = false;
 
   // If we have any base classes, they are initialized prior to the fields.
   for (auto &Base : Bases) {
     Expr *Init = Index < IList->getNumInits() ? IList->getInit(Index) : nullptr;
-    SourceLocation InitLoc = Init ? Init->getBeginLoc() : IList->getEndLoc();
 
     // Designated inits always initialize fields, so if we see one, all
     // remaining base classes have no explicit initializer.
     if (Init && isa<DesignatedInitExpr>(Init))
       Init = nullptr;
 
+    SourceLocation InitLoc = Init ? Init->getBeginLoc() : IList->getEndLoc();
     InitializedEntity BaseEntity = InitializedEntity::InitializeBase(
         SemaRef.Context, &Base, false, &Entity);
     if (Init) {
       CheckSubElementType(BaseEntity, IList, Base.getType(), Index,
                           StructuredList, StructuredIndex);
       InitializedSomething = true;
     } else if (VerifyOnly) {
       CheckEmptyInitializable(BaseEntity, InitLoc);
     }
+
+    if (!VerifyOnly)
+      if (hasAccessibleDestructor(Base.getType(), InitLoc, SemaRef)) {
+        hadError = true;
+        return;
+      }
   }
 
   // If structDecl is a forward declaration, this loop won't do
   // anything except look at designated initializers; That's okay,
   // because an error should get printed out elsewhere. It might be
   // worthwhile to skip over the rest of the initializer, though.
   RecordDecl *RD = DeclType->getAs<RecordType>()->getDecl();
   RecordDecl::field_iterator FieldEnd = RD->field_end();
   bool CheckForMissingFields =
     !IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts());
+  bool HasDesignatedInit = false;
 
   while (Index < IList->getNumInits()) {
     Expr *Init = IList->getInit(Index);
+    SourceLocation InitLoc = Init->getBeginLoc();
 
     if (DesignatedInitExpr *DIE = dyn_cast<DesignatedInitExpr>(Init)) {
       // If we're not the subobject that matches up with the '{' for
       // the designator, we shouldn't be handling the
       // designator. Return immediately.
       if (!SubobjectIsDesignatorContext)
         return;
 
+      HasDesignatedInit = true;
+
       // Handle this designated initializer. Field will be updated to
       // the next field that we'll be initializing.
       if (CheckDesignatedInitializer(Entity, IList, DIE, 0,
                                      DeclType, &Field, nullptr, Index,
                                      StructuredList, StructuredIndex,
                                      true, TopLevelObject))
         hadError = true;
+      else if (!VerifyOnly) {
+        // Find the field named by the designated initializer.
+        RecordDecl::field_iterator F = RD->field_begin();
+        while (std::next(F) != Field)
+          ++F;
+        QualType ET = SemaRef.Context.getBaseElementType(F->getType());
+        if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
+          hadError = true;
+          return;
+        }
+      }
 
       InitializedSomething = true;
 
       // Disable check for missing fields when designators are used.
       // This matches gcc behaviour.
       CheckForMissingFields = false;
       continue;
     }
else
           *Field, IList->getInit(Index)->getBeginLoc());
     if (InvalidUse) {
       ++Index;
       ++Field;
       hadError = true;
       continue;
     }
 
+    if (!VerifyOnly) {
+      QualType ET = SemaRef.Context.getBaseElementType(Field->getType());
+      if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
+        hadError = true;
+        return;
+      }
+    }
+
     InitializedEntity MemberEntity =
       InitializedEntity::InitializeMember(*Field, &Entity);
     CheckSubElementType(MemberEntity, IList, Field->getType(), Index,
                         StructuredList, StructuredIndex);
     InitializedSomething = true;
 
     if (DeclType->isUnionType() && !VerifyOnly) {
       // Initialize the first field within the union.
if (VerifyOnly && Field != FieldEnd && !DeclType->isUnionType() &&
     for (; Field != FieldEnd && !hadError; ++Field) {
       if (!Field->isUnnamedBitfield() && !Field->hasInClassInitializer())
         CheckEmptyInitializable(
             InitializedEntity::InitializeMember(*Field, &Entity),
             IList->getEndLoc());
     }
   }
 
+  // Check that the types of the remaining fields have accessible destructors.
+  if (!VerifyOnly) {
+    // If the initializer expression has a designated initializer, check the
+    // elements for which a designated initializer is not provided too.
+    RecordDecl::field_iterator I = HasDesignatedInit ? RD->field_begin()
+                                                     : Field;
+    for (RecordDecl::field_iterator E = RD->field_end(); I != E; ++I) {
+      QualType ET = SemaRef.Context.getBaseElementType(I->getType());
+      if (hasAccessibleDestructor(ET, IList->getEndLoc(), SemaRef)) {
+        hadError = true;
+        return;
+      }
+    }
+  }
+
   if (Field == FieldEnd || !Field->getType()->isIncompleteArrayType() ||
       Index >= IList->getNumInits())
     return;
 
   if (CheckFlexibleArrayInit(Entity, IList->getInit(Index), *Field,
                              TopLevelObject)) {
     hadError = true;
     ++Index;
case SK_ListInitialization: {
       }
 
       InitListExpr *StructuredInitList =
           PerformInitList.getFullyStructuredList();
       CurInit.get();
       CurInit = shouldBindAsTemporary(InitEntity)
           ? S.MaybeBindToTemporary(StructuredInitList)
           : StructuredInitList;
       break;
     }
 
     case SK_ConstructorInitializationFromList: {
       if (checkAbstractType(Step->Type))
         return ExprError();
 
       // When an initializer list is passed for a parameter of type "reference
       // to object", we don't get an EK_Temporary entity, but instead an

test/CodeGenObjCXX/arc-list-init-destruct.mm

+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.13.0 -std=c++1z -fobjc-arc -fobjc-exceptions -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[V0:.*]] = type opaque
+// CHECK: %[[STRUCT_CLASS1:.*]] = type { %[[V0]]* }
+
+@interface Class0;
+@end
+
+struct Class1 {
+  Class0 *f;
+};
+
+struct Container {
+  Class1 a;
+  bool b;
+};
+
+bool getBool() {
+  return false;
+}
+
+Class0 *g;
+
+// CHECK: define {{.*}} @_Z4testv()
+// CHECK: invoke zeroext i1 @_Z7getBoolv()
+// CHECK: landingpad { i8*, i32 }
+// CHECK: call void @_ZN6Class1D1Ev(%[[STRUCT_CLASS1]]* %{{.*}})
+// CHECK: br label
+
+// CHECK: define linkonce_odr void @_ZN6Class1D1Ev(
+
+Container test() {
+  return {{g}, getBool()};
+}

test/SemaCXX/aggregate-initialization.cpp

 #pragma clang diagnostic pop
 }
 
 namespace HugeArraysUseArrayFiller {
   // All we're checking here is that initialization completes in a reasonable
   // amount of time.
   struct A { int n; int arr[1000 * 1000 * 1000]; } a = {1, {2}};
 }
+
+namespace ElementDestructor {
+  // The destructor for each element of class type is potentially invoked
+  // (15.4 [class.dtor]) from the context where the aggregate initialization
+  // occurs. Produce a diagnostic if an element's destructor isn't accessible.
+
+  class X { int f; ~X(); }; // expected-note {{implicitly declared private here}}
+  struct Y { X x; };
+
+  void test0() {
+    auto *y = new Y {}; // expected-error {{temporary of type 'ElementDestructor::X' has private destructor}}
+  }
+
+  struct S0 { int f; ~S0() = delete; }; // expected-note 3 {{'~S0' has been explicitly marked deleted here}}
+  struct S1 { S0 s0; int f; };
+
+  S1 test1() {
+    auto *t = new S1 { .f = 1 }; // expected-error {{attempt to use a deleted function}}
+    return {2}; // expected-error {{attempt to use a deleted function}}
+  }
+
+  // Check if the type of an array element has a destructor.
+  struct S2 { S0 a[4]; };
+
+  void test2() {
+    auto *t = new S2 {1,2,3,4}; // expected-error {{attempt to use a deleted function}}
+  }
+
+#if __cplusplus >= 201703L
+  namespace BaseDestructor {
+     struct S0 { int f; ~S0() = delete; }; // expected-note {{'~S0' has been explicitly marked deleted here}}
+
+    // Check destructor of base class.
+    struct S3 : S0 {};
+
+    void test3() {
+      S3 s3 = {1}; // expected-error {{attempt to use a deleted function}}
+    }
+  }
+#endif
+
+  // A's destructor doesn't have to be accessible from the context of C's
+  // initialization.
+  struct A { friend struct B; private: ~A(); };
+  struct B { B(); A a; };
+  struct C { B b; };
+  C c = { B() };
+}