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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+  if (VD->isNoDestroy(getASTContext()) &&
+      !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+        VD->isStaticLocal()))
     return;
----------------
erik.pilkington wrote:
> rjmccall wrote:
> > erik.pilkington wrote:
> > > 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.
> > Hmm.  What I just mentioned for the documentation is relevant here, right?  We only need to check access to subobject destructors if exceptions are enabled, so this attempt to avoid duplicate diagnostics might leave us not flagging the use if exceptions are disabled.
> > 
> > Well, maybe the check isn't actually restricted that way, hmm.  Shouldn't it be?
> I thought we tried in general to keep `-fno-exceptions` as similar to normal C++ as possible, rather than invent new language rules for it (even when they make sense).
There's at least one other place where `-fno-exceptions` changes the language rules: we don't look for `operator delete` in a new-expression.  It does look like that's all I was remembering, though, and that we don't generally apply that same principle to destructors, so feel free to ignore my comment.

Still, could you make sure there's a test case that tests that we check access to the destructor of an array variable even when exceptions are disabled, since that's the interesting corner case created by this new condition?


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

https://reviews.llvm.org/D61165