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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D61165#1490417 <https://reviews.llvm.org/D61165#1490417>, @erik.pilkington wrote:

> In D61165#1490168 <https://reviews.llvm.org/D61165#1490168>, @rjmccall wrote:
>
> > I think the intuitive rule is that initialization is complete when the full-expression performing the initialization is complete because that's the normal unit of sequencing.  Note that the standard does use both "constructed" and "initialized" in different contexts, although this might just be an editorial choice.
>
>
> I think it would be quite subtle if the standard was deliberately splitting this particular hair by implicitly creating a "constructed, but not initialized" state of an object, without calling that out anywhere :)


If by "constructed" you mean "a constructor has finished", then we need to split this hair. Consider:

  struct B { ~B(); };
  struct A {
    A() {}
    A(B &&) : A() { throw 0; }
  };
  void f() {
    static A a = B();
  }

At the point when the exception is thrown, a constructor for `a` has completed, but its initialization is not complete.

[except.ctor]/3 and /4 lay out the rules:

"""
If the initialization or destruction of an object other than by delegating constructor is terminated by an
exception, the destructor is invoked for each of the object’s direct subobjects and, for a complete object,
virtual base class subobjects, whose initialization has completed (9.3) and whose destructor has not yet begun
execution, except that in the case of destruction, the variant members of a union-like class are not destroyed.
[Note: If such an object has a reference member that extends the lifetime of a temporary object, this ends
the lifetime of the reference member, so the lifetime of the temporary object is effectively not extended.
— end note] The subobjects are destroyed in the reverse order of the completion of their construction. Such
destruction is sequenced before entering a handler of the function-try-block of the constructor or destructor,
if any.

If the compound-statement of the function-body of a delegating constructor for an object exits via an
exception, the object’s destructor is invoked. Such destruction is sequenced before entering a handler of the
function-try-block of a delegating constructor for that object, if any.
"""

The above wording seems to suggest that the initialization of an object of class type completes when its outermost constructor finishes (at least for the case of initialization by constructor). And indeed all the other wording I can find that has some bearing on when an object is deemed initialized suggests that option 1 is correct, and in general that the initialization of a variable is complete when the initialization full-expression ends, which is before the destructors for any temporaries run. (Those destructor calls are separate full-expressions that happen afterwards; see [intro.execution]/5.)

Following the standard literally (and in particular the rule that destruction of temporaries is a separate full-expression and so -- presumably -- not part of the initialization of the variable), I think we must conclude that a temporary whose destructor throws does *not* destroy the static-storage duration variable (its initialization already completed). For the purposes of this patch, I think that means we never need a destructor for the type of a `[[no_destroy]]` variable.

I've mailed the core reflector to ask for clarification, but my reading is that (1) is the intended model.



================
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:
----------------
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.


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

https://reviews.llvm.org/D61165