[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#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 :)

As far as I see it, we have three options here:

1. Claim that the object is formally initialized when the constructor returns

This appears to be what GCC implements.

2. Claim that the object is formally initialized when the full-expression ends, and if a temporary throws don't call the destructor because the object isn't initialized.

This is what Clang implements today, but seems wrong.

3. Claim that the object is formally initialized when the full-expression ends, but if a temporary throws call the destructor because the constructor ran.

This seems weird to me. If destroying temporaries is an indivisible part of the initialization of an object, then we shouldn't be able to call the destructor, because the initialization of the object didn't succeed. (I mean, assuming there isn't a distinction between constructed and initialized)

> I see it as inconsistent that a destructor for a temporary can abort the initialization of an automatic object but not a static one. I have seen programs that build idioms reliant on this kind of destructor ordering, e.g. as a way to commit / abort a "transaction".

If we assert rule 1, then there isn't any inconsistency. The destructor of the temporary would never abort the initialization of an object when the object was initialized by a constructor, regardless of whether the object was static or automatic. The object would be considered initialized and destructed whenever appropriate for it's storage duration. I guess it would theoretically break that "transaction" pattern, buts its not like we or any other compiler actually supported that on static locals to begin with. We don't even do the "right thing" for automatics there, since we don't `invoke` the temporary destructor (see the IR I posted above).

> there really can't be *that* many uses of this feature yet, especially in exceptions-enabled code. We can fix semantic mistakes.

Yeah, if we assert rule 3 then we I guess we should just do this, rather than try to determine whether we need the dtor for the static local in Sema.

Anyways, I think I've layed out my thinking here as clearly as I can. If you still think that 3 is right here, then I'm happy to defer to you (although it would be quite nice if @rsmith chimed in too). I'm happy to implement whatever the right thing for CodeGen to do here is too.


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

https://reviews.llvm.org/D61165