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

[erik.pilkington]

Summary

Previously, we didn't mark an array's element type's destructor referenced when it was annotated with no_destroy. This is not correct: we still need the destructor if we need to do any cleanup if an element's constructor throws. This was leading to crashes and linker errors. The fix is just to mark the destructor referenced in the array case.

This leads to an inconsistency with access control: If the array element type's destructor is used, then we really ought check its access. However, that would be a source breaking change. This patch ignores access checking, which is a bit unfortunate, but I think its the best we can do if we're not willing to break source compatibility.

Fixes rdar://48462498

Thanks!
Erik

[rjmccall]

Are you sure these are the right semantics for nodestroy? I think there's a reasonable argument that we should not destroy previous elements of a nodestroy array just because an element constructor throws. It's basically academic for true globals because the exception will terminate the process anyway, and even for thread_locals and static locals (where I believe the exception is theoretically recoverable) it's at least arguable that we should either decline to destroy (possibly causing leaks) or simply call std::terminate.

[jfb]

In D61165#1479937, @rjmccall wrote:

Are you sure these are the right semantics for nodestroy? I think there's a reasonable argument that we should not destroy previous elements of a nodestroy array just because an element constructor throws. It's basically academic for true globals because the exception will terminate the process anyway, and even for thread_locals and static locals (where I believe the exception is theoretically recoverable) it's at least arguable that we should either decline to destroy (possibly causing leaks) or simply call std::terminate.

I think std::terminate makes the most sense. Getting teardown correctly is always tricky, and I'm willing to bet that teardown caused by an exception in construction of an array is even harder and done wrong.

[erik.pilkington]

JF, Michael, and I were talking about this offline, and we think that the right choice of semantics for the static local case is to call the destructors.

struct HoldsResource {
  HoldsResource() { tryToAcquireItMayThrow(); }
  ~HoldsResource() { releaseIt(); }
};

void doSomeThings() {
  try { 
    [[clang::no_destroy]] static HoldsResource MyResources[10];
  } catch (...) {
    /* recover gracefully somehow.... */
  }
}

Here, its possible to call doSomeThings many times, until it actually manages to construct MyResources. Just not calling the dtor doesn't seem right since we'd be leaking resources. Calling terminate doesn't make sense either, since its possible to recover from this and try again or continue. no_destroy doesn't mean don't destroy (lol), it means don't register exit-time dtors, that's why it only applies to static/thread local declarations. @rjmccall: WDYT? This is obviously a pretty narrow edge-case.

[erik.pilkington]

By using no_destroy, you're saying that exit-time destructor doesn't matter because the OS will either reclaim the resources automatically, or its just doesn't matter since the process is going down. I don't think that implies that we can safely ignore the destructor in the middle of the program's execution.

[rjmccall]

I believe at least one of the goals of nodestroy is to allow the type to potentially not provide a destructor at all, so if we're going to implicitly require the destructor anyway in certain situations, we should clearly document that, and we should be aware that we may be making the attribute less useful.

Since I believe the dominant use-case here is a true global, does only requiring the destructor for arrays in the static-local case when exceptions are enabled at least make it acceptable to do proper access checking, or is that still a source-compatibility problem for existing clients?

[erik.pilkington]

In D61165#1480976, @rjmccall wrote:

I believe at least one of the goals of nodestroy is to allow the type to potentially not provide a destructor at all, so if we're going to implicitly require the destructor anyway in certain situations, we should clearly document that, and we should be aware that we may be making the attribute less useful.

Since I believe the dominant use-case here is a true global, does only requiring the destructor for arrays in the static-local case when exceptions are enabled at least make it acceptable to do proper access checking, or is that still a source-compatibility problem for existing clients?

It's hard to say exactly how bad the source compatibility problem is, we haven't been pushing on this much internally (yet), so I doubt we'll run into any issues. There was included in an open-source release though. If we only checked access when we needed it, the sequence of steps to actually break anything is pretty far: you'd have to have a type with a private dtor, used in a static local array, with the attribute, and be compiling with exceptions. I'd actually be quite surprised if we ran into any issues, so I guess we should do the right thing with access checking.

I'll update the patch and the docs.

[rjmccall]

Okay, SGTM.

[erik.pilkington]

Address review comments.

[jfb]

In D61165#1479937, @rjmccall wrote:

Are you sure these are the right semantics for nodestroy? I think there's a reasonable argument that we should not destroy previous elements of a nodestroy array just because an element constructor throws. It's basically academic for true globals because the exception will terminate the process anyway, and even for thread_locals and static locals (where I believe the exception is theoretically recoverable) it's at least arguable that we should either decline to destroy (possibly causing leaks) or simply call std::terminate.

I think std::terminate makes the most sense. Getting teardown correctly is always tricky, and I'm willing to bet that teardown caused by an exception in construction of an array is even harder and done wrong.

clang/include/clang/Basic/AttrDocs.td

3906 ↗

(On Diff #197453)

You probably want a try/catch here to illustrate why exceptions can matter.

[erik.pilkington]

Add a try/catch block to the docs.

[rjmccall]

Hmm. You know, there's another case where the destructor can be called even for a non-array: if constructing the object requires a temporary, I believe an exception thrown from that temporary's destructor is supposed to go back and destroy the variable. (This is, sadly, not entirely clear under the standard since the object is not automatic.) Now, Clang does not actually get this correct — we abort the construction, but we don't destroy the variable — but (1) we should fix that someday and (2) in the meantime, we shouldn't implement something which will be a problem when we go to fix that.

This doesn't affect non-locals because there the exception will call std::terminate() as specified in [except.terminate]p1.

clang/include/clang/Basic/AttrDocs.td

3893 ↗

(On Diff #197477)

Missing semicolon.

[erik.pilkington]

In D61165#1485514, @rjmccall wrote:

Hmm. You know, there's another case where the destructor can be called even for a non-array: if constructing the object requires a temporary, I believe an exception thrown from that temporary's destructor is supposed to go back and destroy the variable. (This is, sadly, not entirely clear under the standard since the object is not automatic.) Now, Clang does not actually get this correct — we abort the construction, but we don't destroy the variable — but (1) we should fix that someday and (2) in the meantime, we shouldn't implement something which will be a problem when we go to fix that.

Just to be clear, you're thinking of something like this:

struct S {
  S();
  ~S() noexcept(false) { throw 0; }
};

struct G {
  G(const S &) noexcept;
  ~G();
};

int main() { G g = S(); }

Which clang just generates straight-line code for, even in -fexceptions mode. This seems wrong.

call void @_ZN1SC1Ev(%struct.S* %2)
call void @_ZN1GC1ERK1S(%struct.G* %1, %struct.S* dereferenceable(1) %2) #4
call void @_ZN1SD1Ev(%struct.S* %2)
call void @_ZN1GD1Ev(%struct.G* %1) #4

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). 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.

[rjmccall]

In 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, 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. 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).

[erik.pilkington]

In D61165#1487328, @rjmccall wrote:

In 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 :)

[rjmccall]

In D61165#1488553, @erik.pilkington wrote:

In D61165#1487328, @rjmccall wrote:

In 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.

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.

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.

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

Actually, cross-thread isn't a problem because the C++ runtime already serializes all initialization attempts.

[erik.pilkington]

In D61165#1488657, @rjmccall wrote:

In D61165#1488553, @erik.pilkington wrote:

In D61165#1487328, @rjmccall wrote:

In 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.

[rjmccall]

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.

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".

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.

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.

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.

That's a good point, but there really can't be *that* many uses of this feature yet, especially in exceptions-enabled code. We can fix semantic mistakes.

[erik.pilkington]

In 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.

[rjmccall]

In D61165#1490417, @erik.pilkington wrote:

In 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 :)

Well, but you're assuming that the standard is just using two different words for the same concept, often in close proximity. That's probably against some canon of interpretation.

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.

Outside of aggregate initialization, yes. For aggregate initialization, GCC appears to treat the object as fully initialized as soon as its last member is constructed; until that point, no destructors are run for fully-constructed previous members, which is obviously a bug at some level.

I guess my constructed-vs-initialized rule would naturally suggest that exceptions thrown by full-expression destructors for the last member's initializer cause all the members to be individually destroyed without invoking the aggregate's destructor. (Somewhat oddly, aggregates can have user-defined destructors, so this is a detectable difference.) This is a little weird.

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.

Yes, I acknowledged way upthread that this is a bug.

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 think drawing that distinction is a necessary precursor to applying my rule.

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.

I would also be interested in getting Richard's opinion on this.

[ldionne]

I've only been lurking but FWIW (1) above makes the most sense to me, unless the Standard clearly draws a distinction between *constructed* and *initialized* in the way that was described, in which case (3) is the right approach. However, I would wait for at least a CWG issue to be filed to clarify the intent of the standard before adopting (3), otherwise it seems like we're adopting a slightly surprising behavior (and also one that's different from GCC) on a presumption of intent.

So for now I'd personally go with (1) and consider it a bugfix if the Standard decides to clarify intent in a way that (3) is the right thing to do -- we'll already have to change stuff anyway if that happens.

Also, I would personally be keen on potentially breaking source compatibility by doing access checking, as it's not clear to me at all that this is going to cause any actual breakage in the real world given the age and narrowness of the attribute.

Just my .02

[rjmccall]

The flip side of that argument is that (1) there aren't very many users right now and (2) it's much easier to start conservative and then weaken the rule than it will be to strengthen it later. It really isn't acceptable to just turn off access/use-checking for the destructor, so if we get trapped by the choice we make here, we'll end up having to either leak or call std::terminate.

[erik.pilkington]

In D61165#1490608, @rjmccall wrote:

The flip side of that argument is that (1) there aren't very many users right now and (2) it's much easier to start conservative and then weaken the rule than it will be to strengthen it later. It really isn't acceptable to just turn off access/use-checking for the destructor, so if we get trapped by the choice we make here, we'll end up having to either leak or call std::terminate.

I agree that'd we'd be much better off if we had to change our minds and relax the requirement here. Though we haven't been pushing on this, I would disagree with the point that there aren't many users, this was included in an open source release, an Xcode release, and there was a wg21 paper about it. That paper is currently the first result on google for the search "disabling static destructors". Hedging our bets here is an option, but I'd really like to avoid it if we can.

[erik.pilkington]

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

[rjmccall]

In D61165#1492582, @erik.pilkington wrote:

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

That's talking about constructor bodies.

[dexonsmith]

In D61165#1492724, @rjmccall wrote:

In D61165#1492582, @erik.pilkington wrote:

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

That's talking about constructor bodies.

Sure, but that's all that dcl.int suggests happens during initialization. Skipping over paragraphs that don't apply:

http://eel.is/c++draft/dcl.init#17: The semantics of initializers are as follows.

• http://eel.is/c++draft/dcl.init#17.6: [I]f the destination type is a (possibly cv-qualified) class type:
  • http://eel.is/c++draft/dcl.init#17.6.2: [I]f the initialization is direct-initialization, or if it is copy-initialization where the cv-unqualified version of the source type is the same class as, or a derived class of, the class of the destination, constructors are considered. The applicable constructors are enumerated ([over.match.ctor]), and the best one is chosen through overload resolution ([over.match]). Then:
    • http://eel.is/c++draft/dcl.init#17.6.2.1: If overload resolution is successful, the selected constructor is called to initialize the object, with the initializer expression or expression-list as its argument(s).

Which I read as saying, "the selected constructor is the thing that initializes the object". But, IIUC, you're saying the object isn't necessarily initialized when the selected constructor returns. I can't find any text that would support your interpretation.

Let me argue from the other direction as well:

http://eel.is/c++draft/dcl.init#21: An object whose initialization has completed is deemed to be constructed, [...]

IIUC, you're suggesting that there could be a "constructed, but not yet initialized" state, but the definition of "constructed" seems to refute that.

[rsmith]

In D61165#1490417, @erik.pilkington wrote:

In 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.

clang/include/clang/Basic/AttrDocs.td

3897–3900 ↗

(On Diff #197477)

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.

[rjmccall]

In D61165#1492780, @dexonsmith wrote:

In D61165#1492724, @rjmccall wrote:

In D61165#1492582, @erik.pilkington wrote:

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

That's talking about constructor bodies.

Sure, but that's all that dcl.int suggests happens during initialization. Skipping over paragraphs that don't apply:

http://eel.is/c++draft/dcl.init#17: The semantics of initializers are as follows.

• http://eel.is/c++draft/dcl.init#17.6: [I]f the destination type is a (possibly cv-qualified) class type:
  • http://eel.is/c++draft/dcl.init#17.6.2: [I]f the initialization is direct-initialization, or if it is copy-initialization where the cv-unqualified version of the source type is the same class as, or a derived class of, the class of the destination, constructors are considered. The applicable constructors are enumerated ([over.match.ctor]), and the best one is chosen through overload resolution ([over.match]). Then:
    • http://eel.is/c++draft/dcl.init#17.6.2.1: If overload resolution is successful, the selected constructor is called to initialize the object, with the initializer expression or expression-list as its argument(s).

We've been talking about two things in this thread:

1. We've talked about aggregate initialization, which is a kind of list-initialization, for which this paragraph does not apply.
2. We've talked about temporaries in the initializer, which are not part of the constructor body and are definitely not ordered before the destruction of subobjects within the constructor when an exception is thrown.

That's why I don't think that section is determinative.

[rjmccall]

In D61165#1492781, @rsmith wrote:

In D61165#1490417, @erik.pilkington wrote:

In 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.)

Huh? The destruction of temporaries at the end of a full-expression is definitely still part of the full-expression. [class.temporary]p4: Temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created. [intro.execution] clarifies that all cases of individual initializers are full-expressions.

For the purposes of this patch, I think that means we never need a destructor for the type of a [[no_destroy]] variable.

Arrays and other subobjects of an aggregate initializaton, unless applying the standard "literally" implies the obviously perverse result that we don't destroy subobjects in such cases.

[rsmith]

In D61165#1492830, @rjmccall wrote:

In D61165#1492781, @rsmith wrote:

(Those destructor calls are separate full-expressions that happen afterwards; see [intro.execution]/5.)

Huh? The destruction of temporaries at the end of a full-expression is definitely still part of the full-expression. [class.temporary]p4: Temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created. [intro.execution] clarifies that all cases of individual initializers are full-expressions.

Sorry, I confused myself. You're right, of course. But I don't think that makes a material difference to anything else.

For the purposes of this patch, I think that means we never need a destructor for the type of a [[no_destroy]] variable.

Arrays and other subobjects of an aggregate initializaton, unless applying the standard "literally" implies the obviously perverse result that we don't destroy subobjects in such cases.

Yes, but you need the destructors for those subobjects as a side-condition of the initialization, irrespective of what kind of object that initialization is initializing. I don't think that's got anything to do with [[no_destroy]]. I think it remains the case that you never need a destructor for the type of a [[no_destroy]] variable.

So far the opinion of folks on the core reflector has unanimously been that (1) is the right model. And I think that makes sense: it would be somewhat strange for the initialization of a complete object to be considered complete after temporaries are destroyed, but the initializations of its subobjects to be considered complete before the temporaries are destroyed.

[rjmccall]

In D61165#1494001, @rsmith wrote:

In D61165#1492830, @rjmccall wrote:

In D61165#1492781, @rsmith wrote:

For the purposes of this patch, I think that means we never need a destructor for the type of a [[no_destroy]] variable.

Arrays and other subobjects of an aggregate initializaton, unless applying the standard "literally" implies the obviously perverse result that we don't destroy subobjects in such cases.

Yes, but you need the destructors for those subobjects as a side-condition of the initialization, irrespective of what kind of object that initialization is initializing. I don't think that's got anything to do with [[no_destroy]]. I think it remains the case that you never need a destructor for the type of a [[no_destroy]] variable.

Okay, so the rule should be that [[no_destroy]] never requires the destructor for the exact type of the variable, but it might require the destructors of sub-object types of a static local that uses aggregate initialization when exceptions are enabled, which necessarily includes the element type of an array.

So far the opinion of folks on the core reflector has unanimously been that (1) is the right model. And I think that makes sense: it would be somewhat strange for the initialization of a complete object to be considered complete after temporaries are destroyed, but the initializations of its subobjects to be considered complete before the temporaries are destroyed.

Okay. I'm not sure that's the decision I would make, but I can accept it, and it certainly simplifies the model in some ways.

[rjmccall]

All of the IRGen changes in this patch are unnecessary according to the model we've worked out, right? The fix is just to mark the destructor as still used when we're constructing an array.

clang/lib/Sema/SemaDeclCXX.cpp

13119 ↗

(On Diff #197477)

This isn't "emitting" the destructor, it's "using" it. Also, the comment should make it clear that this is about aggregate initialization in general, and it should contain a FIXME if there's part of that rule we're not implementing correctly.

[rsmith]

clang/lib/Sema/SemaDeclCXX.cpp

13121–13122 ↗

(On Diff #197477)

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.)

[rjmccall]

clang/lib/Sema/SemaDeclCXX.cpp

13121–13122 ↗

(On Diff #197477)

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.

[erik.pilkington]

Address review comments. Also remove the special case where we wouldn't check a destructor for an array in -fno-exceptions mode. This seems inconsistent with both how we're treating -fno-exceptions in general, and inconsistent with other cases of aggregate initialization (i.e. we still check struct members here).

In D61165#1494250, @rjmccall wrote:

All of the IRGen changes in this patch are unnecessary according to the model we've worked out, right? The fix is just to mark the destructor as still used when we're constructing an array.

Yeah, the IRGen changes were to stop clang from referencing a non-existant dtor for a global no_destroy array, but if we're using it regardless then its not necessary. New patch removes it.

clang/include/clang/Basic/AttrDocs.td

3897–3900 ↗

(On Diff #197477)

Sure, that makes sense. The new patch rephrases this.

clang/lib/Sema/SemaDeclCXX.cpp

13119 ↗

(On Diff #197477)

Are you alluding to the CodeGen bug? That seems unrelated to this function.

13121–13122 ↗

(On Diff #197477)

Sure, done. Moving this to initialization time seems like a nice cleanup.

[rjmccall]

clang/include/clang/Basic/AttrDocs.td

3915 ↗

(On Diff #198916)

Probably worth adding somewhere in here that is only required if exceptions are enabled, since disabling exceptions is a pretty common configuration.

clang/lib/Sema/SemaDeclCXX.cpp

13121–13122 ↗

(On Diff #197477)

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?

clang/lib/Sema/SemaInit.cpp

6328 ↗

(On Diff #198916)

There's no way that the right semantics are to return failure if the type has an accessible destructor. :) Looks like the function is misnamed. Also, it should also be named something that makes it clear that it's more than a predicate, it's actually checking access to / marking the use of the destructor. I know this is an existing function, but could you take care of that?

[erik.pilkington]

clang/lib/Sema/SemaDeclCXX.cpp

13121–13122 ↗

(On Diff #197477)

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).

clang/lib/Sema/SemaInit.cpp

6328 ↗

(On Diff #198916)

Yeah, the name doesn't make much sense. I'll update it...

[rjmccall]

clang/lib/Sema/SemaDeclCXX.cpp

13121–13122 ↗

(On Diff #197477)

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?

[erik.pilkington]

Rename hasAccessibleDestructor.

clang/lib/Sema/SemaDeclCXX.cpp

13121–13122 ↗

(On Diff #197477)

Sure, thats included in test/SemaCXX/no_destroy.cpp. Pretty much every test is running in -fno-exceptions mode, since thats the default in -cc1 for some reason. All the more reason to keep the two modes as similar as possible I guess :)

[rjmccall]

LGTM.

clang/test/SemaCXX/aggregate-initialization.cpp

199 ↗

(On Diff #198944)

Feel free to address this in a follow-up, but this seems like a regression.

cfe/trunk/include/clang/Basic/AttrDocs.td

 
 def NoDestroyDocs : Documentation {
   let Category = DocCatVariable;
   let Content = [{
 The ``no_destroy`` attribute specifies that a variable with static or thread
 storage duration shouldn't have its exit-time destructor run. Annotating every
 static and thread duration variable with this attribute is equivalent to
 invoking clang with -fno-c++-static-destructors.
+
+If a variable is declared with this attribute, clang doesn't access check or
+generate the type's destructor. If you have a type that you only want to be
+annotated with ``no_destroy``, you can therefore declare the destructor private:
+
+.. code-block:: c++
+
+  struct only_no_destroy {
+    only_no_destroy();
+  private:
+    ~only_no_destroy();
+  };
+
+  [[clang::no_destroy]] only_no_destroy global; // fine!
+
+Note that destructors are still required for subobjects of aggregates annotated
+with this attribute. This is because previously constructed subobjects need to
+be destroyed if an exception gets thrown before the initialization of the
+complete object is complete. For instance:
+
+.. code-block::c++
+
+  void f() {
+    try {
+      [[clang::no_destroy]]
+      static only_no_destroy array[10]; // error, only_no_destroy has a private destructor.
+    } catch (...) {
+      // Handle the error
+    }
+  }
+
+Here, if the construction of `array[9]` fails with an exception, `array[0..8]`
+will be destroyed, so the element's destructor needs to be accessible.
   }];
 }
 
 def UninitializedDocs : Documentation {
   let Category = DocCatVariable;
   let Content = [{
 The command-line parameter ``-ftrivial-auto-var-init=*`` can be used to
 initialize trivial automatic stack variables. By default, trivial automatic

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

void Sema::FinalizeVarWithDestructor(VarDecl *VD, const RecordType *Record) {
   if (ClassDecl->isInvalidDecl()) return;
   if (ClassDecl->hasIrrelevantDestructor()) return;
   if (ClassDecl->isDependentContext()) return;
 
   if (VD->isNoDestroy(getASTContext()))
     return;
 
   CXXDestructorDecl *Destructor = LookupDestructor(ClassDecl);
+
+  // If this is an array, we'll require the destructor during initialization, so
+  // we can skip over this. We still want to emit exit-time destructor warnings
+  // though.
+  if (!VD->getType()->isArrayType()) {
     MarkFunctionReferenced(VD->getLocation(), Destructor);
     CheckDestructorAccess(VD->getLocation(), Destructor,
                           PDiag(diag::err_access_dtor_var)
-                        << VD->getDeclName()
-                        << VD->getType());
+                              << VD->getDeclName() << VD->getType());
     DiagnoseUseOfDecl(Destructor, VD->getLocation());
+  }
 
   if (Destructor->isTrivial()) return;
   if (!VD->hasGlobalStorage()) return;
 
   // Emit warning for non-trivial dtor in global scope (a real global,
   // class-static, function-static).
   Diag(VD->getLocation(), diag::warn_exit_time_destructor);
 

cfe/trunk/lib/Sema/SemaExprCXX.cpp

if (OperatorNew) {
     MarkFunctionReferenced(StartLoc, OperatorNew);
   }
   if (OperatorDelete) {
     if (DiagnoseUseOfDecl(OperatorDelete, StartLoc))
       return ExprError();
     MarkFunctionReferenced(StartLoc, OperatorDelete);
   }
 
-  // C++0x [expr.new]p17:
-  //   If the new expression creates an array of objects of class type,
-  //   access and ambiguity control are done for the destructor.
-  QualType BaseAllocType = Context.getBaseElementType(AllocType);
-  if (ArraySize && !BaseAllocType->isDependentType()) {
-    if (const RecordType *BaseRecordType = BaseAllocType->getAs<RecordType>()) {
-      if (CXXDestructorDecl *dtor = LookupDestructor(
-              cast<CXXRecordDecl>(BaseRecordType->getDecl()))) {
-        MarkFunctionReferenced(StartLoc, dtor);
-        CheckDestructorAccess(StartLoc, dtor,
-                              PDiag(diag::err_access_dtor)
-                                << BaseAllocType);
-        if (DiagnoseUseOfDecl(dtor, StartLoc))
-          return ExprError();
-      }
-    }
-  }
-
   return CXXNewExpr::Create(Context, UseGlobal, OperatorNew, OperatorDelete,
                             PassAlignment, UsualArrayDeleteWantsSize,
                             PlacementArgs, TypeIdParens, ArraySize, initStyle,
                             Initializer, ResultType, AllocTypeInfo, Range,
                             DirectInitRange);
 }
 
 /// Checks that a type is suitable as the allocated type

cfe/trunk/lib/Sema/SemaInit.cpp

if (numEltsInit != maxElements) {
     if (!VerifyOnly)
       SemaRef.Diag(IList->getBeginLoc(),
                    diag::err_vector_incorrect_num_initializers)
           << (numEltsInit < maxElements) << maxElements << numEltsInit;
     hadError = true;
   }
 }
 
+/// Check if the type of a class element has an accessible destructor, and marks
+/// it referenced. Returns true if we shouldn't form a reference to the
+/// destructor.
+///
+/// Aggregate initialization requires a class element's destructor be
+/// accessible per 11.6.1 [dcl.init.aggr]:
+///
+/// The destructor for each element of class type is potentially invoked
+/// (15.4 [class.dtor]) from the context where the aggregate initialization
+/// occurs.
+static bool checkDestructorReference(QualType ElementType, SourceLocation Loc,
+                                     Sema &SemaRef) {
+  auto *CXXRD = ElementType->getAsCXXRecordDecl();
+  if (!CXXRD)
+    return false;
+
+  CXXDestructorDecl *Destructor = SemaRef.LookupDestructor(CXXRD);
+  SemaRef.CheckDestructorAccess(Loc, Destructor,
+                                SemaRef.PDiag(diag::err_access_dtor_temp)
+                                << ElementType);
+  SemaRef.MarkFunctionReferenced(Loc, Destructor);
+  return SemaRef.DiagnoseUseOfDecl(Destructor, Loc);
+}
+
 void InitListChecker::CheckArrayType(const InitializedEntity &Entity,
                                      InitListExpr *IList, QualType &DeclType,
                                      llvm::APSInt elementIndex,
                                      bool SubobjectIsDesignatorContext,
                                      unsigned &Index,
                                      InitListExpr *StructuredList,
                                      unsigned &StructuredIndex) {
   const ArrayType *arrayType = SemaRef.Context.getAsArrayType(DeclType);
 
+  if (!VerifyOnly) {
+    if (checkDestructorReference(arrayType->getElementType(),
+                                 IList->getEndLoc(), SemaRef)) {
+      hadError = true;
+      return;
+    }
+  }
+
   // Check for the special-case of initializing an array with a string.
   if (Index < IList->getNumInits()) {
     if (IsStringInit(IList->getInit(Index), arrayType, SemaRef.Context) ==
         SIF_None) {
       // We place the string literal directly into the resulting
       // initializer list. This is the only place where the structure
       // of the structured initializer list doesn't match exactly,
       // because doing so would involve allocating one character
SemaRef.Diag(InitExpr->getBeginLoc(), FlexArrayDiag)
         << InitExpr->getBeginLoc();
     SemaRef.Diag(Field->getLocation(), diag::note_flexible_array_member)
       << Field;
   }
 
   return FlexArrayDiag != diag::ext_flexible_array_init;
 }
 
-/// Check if the type of a class element has an accessible destructor.
-///
-/// Aggregate initialization requires a class element's destructor be
-/// accessible per 11.6.1 [dcl.init.aggr]:
-///
-/// The destructor for each element of class type is potentially invoked
-/// (15.4 [class.dtor]) from the context where the aggregate initialization
-/// occurs.
-static bool hasAccessibleDestructor(QualType ElementType, SourceLocation Loc,
-                                    Sema &SemaRef) {
-  auto *CXXRD = ElementType->getAsCXXRecordDecl();
-  if (!CXXRD)
-    return false;
-
-  CXXDestructorDecl *Destructor = SemaRef.LookupDestructor(CXXRD);
-  SemaRef.CheckDestructorAccess(Loc, Destructor,
-                                SemaRef.PDiag(diag::err_access_dtor_temp)
-                                    << ElementType);
-  SemaRef.MarkFunctionReferenced(Loc, Destructor);
-  if (SemaRef.DiagnoseUseOfDecl(Destructor, Loc))
-    return true;
-  return false;
-}
-
 void InitListChecker::CheckStructUnionTypes(
     const InitializedEntity &Entity, InitListExpr *IList, QualType DeclType,
     CXXRecordDecl::base_class_range Bases, RecordDecl::field_iterator Field,
     bool SubobjectIsDesignatorContext, unsigned &Index,
     InitListExpr *StructuredList, unsigned &StructuredIndex,
     bool TopLevelObject) {
   RecordDecl *structDecl = DeclType->getAs<RecordType>()->getDecl();
 
   // If the record is invalid, some of it's members are invalid. To avoid
   // confusion, we forgo checking the intializer for the entire record.
   if (structDecl->isInvalidDecl()) {
     // Assume it was supposed to consume a single initializer.
     ++Index;
     hadError = true;
     return;
   }
 
   if (DeclType->isUnionType() && IList->getNumInits() == 0) {
     RecordDecl *RD = DeclType->getAs<RecordType>()->getDecl();
 
     if (!VerifyOnly)
       for (FieldDecl *FD : RD->fields()) {
         QualType ET = SemaRef.Context.getBaseElementType(FD->getType());
-        if (hasAccessibleDestructor(ET, IList->getEndLoc(), SemaRef)) {
+        if (checkDestructorReference(ET, IList->getEndLoc(), SemaRef)) {
           hadError = true;
           return;
         }
       }
 
     // If there's a default initializer, use it.
     if (isa<CXXRecordDecl>(RD) && cast<CXXRecordDecl>(RD)->hasInClassInitializer()) {
       if (VerifyOnly)
if (Init) {
       CheckSubElementType(BaseEntity, IList, Base.getType(), Index,
                           StructuredList, StructuredIndex);
       InitializedSomething = true;
     } else if (VerifyOnly) {
       CheckEmptyInitializable(BaseEntity, InitLoc);
     }
 
     if (!VerifyOnly)
-      if (hasAccessibleDestructor(Base.getType(), InitLoc, SemaRef)) {
+      if (checkDestructorReference(Base.getType(), InitLoc, SemaRef)) {
         hadError = true;
         return;
       }
   }
 
   // If structDecl is a forward declaration, this loop won't do
   // anything except look at designated initializers; That's okay,
   // because an error should get printed out elsewhere. It might be
if (DesignatedInitExpr *DIE = dyn_cast<DesignatedInitExpr>(Init)) {
                                      true, TopLevelObject))
         hadError = true;
       else if (!VerifyOnly) {
         // Find the field named by the designated initializer.
         RecordDecl::field_iterator F = RD->field_begin();
         while (std::next(F) != Field)
           ++F;
         QualType ET = SemaRef.Context.getBaseElementType(F->getType());
-        if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
+        if (checkDestructorReference(ET, InitLoc, SemaRef)) {
           hadError = true;
           return;
         }
       }
 
       InitializedSomething = true;
 
       // Disable check for missing fields when designators are used.
if (InvalidUse) {
       ++Index;
       ++Field;
       hadError = true;
       continue;
     }
 
     if (!VerifyOnly) {
       QualType ET = SemaRef.Context.getBaseElementType(Field->getType());
-      if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
+      if (checkDestructorReference(ET, InitLoc, SemaRef)) {
         hadError = true;
         return;
       }
     }
 
     InitializedEntity MemberEntity =
       InitializedEntity::InitializeMember(*Field, &Entity);
     CheckSubElementType(MemberEntity, IList, Field->getType(), Index,
void InitListChecker::CheckStructUnionTypes(
   // Check that the types of the remaining fields have accessible destructors.
   if (!VerifyOnly) {
     // If the initializer expression has a designated initializer, check the
     // elements for which a designated initializer is not provided too.
     RecordDecl::field_iterator I = HasDesignatedInit ? RD->field_begin()
                                                      : Field;
     for (RecordDecl::field_iterator E = RD->field_end(); I != E; ++I) {
       QualType ET = SemaRef.Context.getBaseElementType(I->getType());
-      if (hasAccessibleDestructor(ET, IList->getEndLoc(), SemaRef)) {
+      if (checkDestructorReference(ET, IList->getEndLoc(), SemaRef)) {
         hadError = true;
         return;
       }
     }
   }
 
   if (Field == FieldEnd || !Field->getType()->isIncompleteArrayType() ||
       Index >= IList->getNumInits())
PerformConstructorInitialization(Sema &S,
   if (CurInit.isInvalid())
     return ExprError();
 
   // Only check access if all of that succeeded.
   S.CheckConstructorAccess(Loc, Constructor, Step.Function.FoundDecl, Entity);
   if (S.DiagnoseUseOfDecl(Step.Function.FoundDecl, Loc))
     return ExprError();
 
+  if (const ArrayType *AT = S.Context.getAsArrayType(Entity.getType()))
+    if (checkDestructorReference(S.Context.getBaseElementType(AT), Loc, S))
+      return ExprError();
+
   if (shouldBindAsTemporary(Entity))
     CurInit = S.MaybeBindToTemporary(CurInit.get());
 
   return CurInit;
 }
 
 namespace {
 enum LifetimeKind {

cfe/trunk/test/CXX/expr/expr.unary/expr.new/p17.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 class ctor {
   ctor(); // expected-note{{implicitly declared private here}}
 };
 
 class dtor {
   ~dtor(); // expected-note 3 {{implicitly declared private here}}
 };
 
 void test() {
   new ctor[0]; // expected-error{{calling a private constructor of class 'ctor'}}
-  new dtor[0]; // expected-error{{calling a private destructor of class 'dtor'}}
-  new dtor[3]; // expected-error{{calling a private destructor of class 'dtor'}}
-  new dtor[3][3]; // expected-error{{calling a private destructor of class 'dtor'}}
+  new dtor[0]; // expected-error{{temporary of type 'dtor' has private destructor}}
+  new dtor[3]; // expected-error{{temporary of type 'dtor' has private destructor}}
+  new dtor[3][3]; // expected-error{{temporary of type 'dtor' has private destructor}}
 }

cfe/trunk/test/CodeGenCXX/no_destroy.cpp

-// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,NO_EXCEPTIONS
+// RUN: %clang_cc1 -fexceptions %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,EXCEPTIONS
 
 struct NonTrivial {
   ~NonTrivial();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] NonTrivial nt1;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] thread_local NonTrivial nt2;
 
 struct NonTrivial2 {
   ~NonTrivial2();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
 NonTrivial2 nt21;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN11NonTrivial2D1Ev
 thread_local NonTrivial2 nt22;
 
+// CHECK-LABEL: define void @_Z1fv
 void f() {
   // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
   static NonTrivial2 nt21;
   // CHECK: _tlv_atexit{{.*}}_ZN11NonTrivial2D1Ev
   thread_local NonTrivial2 nt22;
 }
 
+// CHECK-LABEL: define void @_Z1gv
+void g() {
+  // CHECK-NOT: __cxa_atexit
+  [[clang::no_destroy]] static NonTrivial2 nt21;
+  // CHECK-NOT: _tlv_atexit
+  [[clang::no_destroy]] thread_local NonTrivial2 nt22;
+}
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] NonTrivial nt3;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] thread_local NonTrivial nt4;
+
+
+struct NonTrivial3 {
+  NonTrivial3();
+  ~NonTrivial3();
+};
+
+[[clang::no_destroy]] NonTrivial3 arr[10];
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// EXCEPTIONS: call void @_ZN11NonTrivial3D1Ev
+// NO_EXCEPTIONS-NOT: call void @_ZN11NonTrivial3D1Ev
+// CHECK-NOT: call i32 @__cxa_atexit
+
+void h() {
+  [[clang::no_destroy]] static NonTrivial3 slarr[10];
+}
+
+// CHECK-LABEL: define void @_Z1hv
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// EXCEPTIONS: call void @_ZN11NonTrivial3D1Ev
+// NO_EXCEPTIONS-NOT: call void @_ZN11NonTrivial3D1Ev
+// CHECK-NOT: call i32 @__cxa_atexit
+
+void i() {
+  [[clang::no_destroy]] thread_local NonTrivial3 tlarr[10];
+}
+
+// CHECK-LABEL: define void @_Z1iv
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// EXCEPTIONS: call void @_ZN11NonTrivial3D1Ev
+// NO_EXCEPTIONS-NOT: call void @_ZN11NonTrivial3D1Ev
+// CHECK-NOT: _tlv_atexit

cfe/trunk/test/SemaCXX/no_destroy.cpp

-// RUN: %clang_cc1 -DNO_DTORS -fno-c++-static-destructors -verify %s
-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -DNO_EXCEPTIONS -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -DNO_EXCEPTIONS -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -fexceptions -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -fexceptions -verify %s
 
 struct SecretDestructor {
 #ifndef NO_DTORS
   // expected-note@+2 4 {{private}}
 #endif
-private: ~SecretDestructor(); // expected-note 2 {{private}}
+private: ~SecretDestructor(); // expected-note + {{private}}
 };
 
 SecretDestructor sd1;
 thread_local SecretDestructor sd2;
 void locals() {
   static SecretDestructor sd3;
   thread_local SecretDestructor sd4;
 }

 [[clang::always_destroy]] [[clang::no_destroy]] int p; // expected-error{{'no_destroy' and 'always_destroy' attributes are not compatible}} // expected-note{{here}}
 [[clang::no_destroy]] [[clang::always_destroy]] int p2; // expected-error{{'always_destroy' and 'no_destroy' attributes are not compatible}} // expected-note{{here}}
 
 [[clang::always_destroy]] void f() {} // expected-warning{{'always_destroy' attribute only applies to variables}}
 struct [[clang::no_destroy]] DoesntApply {};  // expected-warning{{'no_destroy' attribute only applies to variables}}
 
 [[clang::no_destroy(0)]] int no_args; // expected-error{{'no_destroy' attribute takes no arguments}}
 [[clang::always_destroy(0)]] int no_args2; // expected-error{{'always_destroy' attribute takes no arguments}}
+
+// expected-error@+1 {{temporary of type 'SecretDestructor' has private destructor}}
+SecretDestructor arr[10];
+
+void local_arrays() {
+  // expected-error@+1 {{temporary of type 'SecretDestructor' has private destructor}}
+  static SecretDestructor arr2[10];
+  // expected-error@+1 {{temporary of type 'SecretDestructor' has private destructor}}
+  thread_local SecretDestructor arr3[10];
+}
+
+struct Base {
+  ~Base();
+};
+struct Derived1 {
+  Derived1(int);
+  Base b;
+};
+struct Derived2 {
+  Derived1 b;
+};
+
+void dontcrash() {
+  [[clang::no_destroy]] static Derived2 d2[] = {0, 0};
+}
+
+[[clang::no_destroy]] Derived2 d2[] = {0, 0};