[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 a comment.

In D61165#1487328 <https://reviews.llvm.org/D61165#1487328>, @rjmccall wrote:

> In D61165#1487100 <https://reviews.llvm.org/D61165#1487100>, @erik.pilkington wrote:
>
> > It seems like the most common sense interpretation here is to just treat the initialization of `G` as completed at the point when the constructor finishes (this appears to be what GCC implements: https://wandbox.org/permlink/R3C9pPhoT4efAdL1).
>
>
> Your example doesn't actually demonstrate that that's what GCC implements because it doesn't try to execute the declaration twice.  But you're right, GCC does appear to consider the initialization complete as soon as the static object's constructor returns normally.  On the other hand, GCC gets the array case here wrong: if a static local has array type, and a destructor for a temporary required by the first element initializer throws, then it does not destroy the element but also (correctly) does not mark the variable as fully initialized, and so a second attempt to run the initializer will simply construct a new object on top of an already-constructed one.  This is arguably correct under the standard — the first array element is not a previously-constructed object of automatic duration — but I hope it's obvious that that's a defect.
>
> > So if it was static it would just get destroyed at exit-time, and therefore should be disable-able with `no_destroy`. If the standard implies that we should be doing something else, then we should do that, but I can't seem to find any reference to the rule you're describing.
>
> Like I said, this is a poorly-specified part of the standard, because at least *some* objects with static storage duration have to be destroyed when an exception is thrown (because of aggregate initialization), but the standard wants to pretend that this isn't true.  I think that allowing temporary destructors to cancel initialization uniformly across all kinds of objects is the most consistent rule,


That's only true for subobjects of an enclosing aggregate before that aggregate's initialization is complete though, right? So it doesn't seem like that much of an inconsistency, just mimicking what we would be doing if an exception was thrown in, say, the body of the ctor before the object's initialization is completed. Maybe I'm misunderstanding?

> but if the standard wants to use a rule that non-aggregate initialization of static and thread-local locals is complete as soon as the constructor (or assignment) completes, as opposed to the end of the full-expression, that's also implementable.

So what should that path forward be here? I'd really like to get this crash fixed soon. If we want to consider a static local no_destroy dtor potentially-invoked in Sema if the initializer has a temporary with a throwing dtor, then we could do that, but it'd be unfortunate for 98/03 where I believe a dtor isn't noexcept by default, so we'd have to assume the worst. I guess it'd be easier to change our minds in the future if we treat the dtor as potentially-invoked, but I'm not really seeing the argument that we shouldn't just use this rule.

> I guess it would also technically be feasible for repeated attempts at initialization to just pick up after the last subobject to be successfully constructed, although that would be really obnoxious to actually implement (and would require an ABI change for static local aggregates with vague linkage).

Yeah, that seems quite strange, especially if initialization got picked up in another thread :)


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

https://reviews.llvm.org/D61165