[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3897-3900
+This works in almost all cases, but if ``no_destroy`` is applied to a ``static``
+or ``thread_local`` local builtin array variable and exceptions are enabled, the
+destructor is still needed to perform cleanup if the construction of an element
+of the array throws. For instance:
----------------
rsmith wrote:
> I think this is the wrong way to think about and describe this. `[[no_destroy]]` removes the need for the type of the variable to have a usable destructor. But all immediate subobjects still need usable destructors, in case an exception is thrown during the object's construction. This is then identical to the constraints on `new T` -- subobjects of `T` (including array elements) still need to be destructible, but `T` itself does not.
Sure, that makes sense. The new patch rephrases this.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13119
+  // variable is static local array and exceptions are enabled, since then we
+  // need to clean up the elements.
+  if (VD->isNoDestroy(getASTContext()) &&
----------------
rjmccall wrote:
> This isn't "emitting" the destructor, it's "using" it.  Also, the comment should make it clear that this is about aggregate initialization in general, and it should contain a FIXME if there's part of that rule we're not implementing correctly.
Are you alluding to the CodeGen bug? That seems unrelated to this function. 


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+  if (VD->isNoDestroy(getASTContext()) &&
+      !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+        VD->isStaticLocal()))
     return;
----------------
rjmccall wrote:
> rsmith wrote:
> > Hmm, perhaps it would be cleaner if the destructor for array elements were required by the initialization of the array, instead of here. That's how this works for struct aggregate initialization (see `InitListChecker::CheckStructUnionTypes`), and (indirectly) how it works for initialization by constructor, and so on. And it reflects the fact that it's the initialization process that needs the array element destructor, not the destruction of the variable (which never happens).
> > 
> > For the case of aggregate initialization of an array, we appear to fail to implement [dcl.init.aggr]/8:
> > 
> > """
> > The destructor for each element of class type is potentially invoked (11.3.6) from the context where the aggregate initialization occurs. [Note: This provision ensures that destructors can be called for fully-constructed subobjects in case an exception is thrown (14.2). — end note]
> > """
> > 
> > (But there doesn't appear to be any corresponding wording for default / value initialization of arrays. See also the special case at the end of `BuildCXXNewExpr`.)
> That makes sense to me.  The default/value initialization case seems like an obvious oversight in the standard, but I think it might be a distinction without a difference: the special cases for base-or-member initializers and new-expressions might cover literally every situation where initialization doesn't come with an adjacent need for non-exceptional destruction.
Sure, done. Moving this to initialization time seems like a nice cleanup.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61165/new/

https://reviews.llvm.org/D61165