Page MenuHomePhabricator

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

Authored by erik.pilkington on Apr 25 2019, 5:47 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2019, 5:47 PM

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 added a comment.Apr 26 2019, 9:50 AM

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.

jfb added a subscriber: Bigcheese.Apr 26 2019, 9:51 AM

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.

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.

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 planned changes to this revision.Apr 26 2019, 2:53 PM

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.

Address review comments.

jfb added a comment.Apr 30 2019, 3:35 PM

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.

Add a try/catch block to the docs.

jfb accepted this revision.Apr 30 2019, 4:19 PM
This revision is now accepted and ready to land.Apr 30 2019, 4:19 PM

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.

rjmccall requested changes to this revision.Apr 30 2019, 4:46 PM
This revision now requires changes to proceed.Apr 30 2019, 4:46 PM

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.

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

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

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.

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.

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.

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.

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

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

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.

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

  1. 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 added a subscriber: ldionne.May 3 2019, 3:57 PM

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

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.

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.

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?

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.

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 added a comment.May 6 2019, 5:38 PM

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.

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.

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 added a comment.May 7 2019, 1:03 PM

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

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.

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 added inline comments.May 7 2019, 3:26 PM
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 added inline comments.May 7 2019, 7:57 PM
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 marked 6 inline comments as done.

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

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 added inline comments.May 9 2019, 4:06 PM
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 marked 2 inline comments as done.May 9 2019, 4:14 PM
erik.pilkington added inline comments.
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 added inline comments.May 9 2019, 4:34 PM
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 marked an inline comment as done.

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 accepted this revision.May 9 2019, 5:51 PM

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.

This revision is now accepted and ready to land.May 9 2019, 5:51 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2019, 10:50 AM