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

Fri May 3 09:49:40 PDT 2019

erik.pilkington added a comment.

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

> In D61165#1488553 <https://reviews.llvm.org/D61165#1488553>, @erik.pilkington wrote:
>
> > 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.
>
>
> Conceptually yes, but formally no.  The standard *could* write this rule as "all currently-initialized subobjects must be separately destroyed when an exception aborts initialization of the containing aggregate, including constructor bodies and aggregate initialization", but it doesn't actually do so; instead it has specific rules covering the behavior when an exception is thrown out of the body of a constructor, and those rules simply do not apply to aggregate initialization.

Right, I was just trying to draw an analogy. Can you be more specific about the inconsistency you mentioned above? What objects with static storage duration have to be destroyed when an exception is thrown? Just subobjects of static aggregates that had their initialization aborted by an exception? If so, that obviously doesn't seem inconsistent.

> Even if it did, though, that wouldn't tell us how to resolve this because this is fundamentally about when exactly the initialization of an object is "complete", which doesn't seem to be clearly defined in the standard.  There's a rule for when a *constructor* is complete, but among other things, not all initializations involve constructors.

Yeah, I agree that this is the fundamental problem here, and that the standard isn't much help. I'm trying to understand why you think this choice of semantics is better (or at least, good enough to make us hedge our bets here at the cost of keeping this feature simple and useful). I'm not seeing the aggregate initialization inconsistency you mentioned above. If the standard wanted us to call the destructor before the object's initialization was formally complete, then that seems like something they would mention. Other implementations (well, GCC) seem to have made the opposite decision, which I think makes more intuitive sense.

>>> 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 think the simplest rule would be to say that the destructor must still be accessible for static or thread-local locals and that it'll be used in certain cases when initialization is aborted.

If we did this the source compat problem would be a lot less theoretical.

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

https://reviews.llvm.org/D61165