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

This revision was automatically updated to reflect the committed changes. Closed by commit rL360446: [Sema] Mark array element destructors referenced during initialization (authored by epilk, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

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

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/test/SemaCXX/aggregate-initialization.cpp:199 void test0() { -auto *y = new Y {}; // expected-error {{temporary of type

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

erik.pilkington added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122 + if (VD->isNoDestroy(getASTContext()) && + !(VD->getType()->isArrayType() && getLangOpts().Exceptions && +VD->isStaticLocal())) return; rjmccall

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

erik.pilkington updated this revision to Diff 198944. erik.pilkington marked an inline comment as done. erik.pilkington added a comment. Rename `hasAccessibleDestructor`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61165/new/ https://reviews.llvm.org/D61165 Files:

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122 + if (VD->isNoDestroy(getASTContext()) && + !(VD->getType()->isArrayType() && getLangOpts().Exceptions && +VD->isStaticLocal())) return; erik.pilkington

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

erik.pilkington marked 2 inline comments as done. erik.pilkington added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122 + if (VD->isNoDestroy(getASTContext()) && + !(VD->getType()->isArrayType() && getLangOpts().Exceptions && +

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

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3915 +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. }]; Probably worth

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

erik.pilkington updated this revision to Diff 198916. erik.pilkington marked 6 inline comments as done. erik.pilkington added a comment. 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

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

erik.pilkington added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3897-3900 +This works in almost all cases, but if ``no_destroy`` is applied to a ``static`` +or ``thread_local`` local builtin array variable and exceptions are enabled, the +destructor is

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122 + if (VD->isNoDestroy(getASTContext()) && + !(VD->getType()->isArrayType() && getLangOpts().Exceptions && +VD->isStaticLocal())) return; rsmith wrote: >

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

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122 + if (VD->isNoDestroy(getASTContext()) && + !(VD->getType()->isArrayType() && getLangOpts().Exceptions && +VD->isStaticLocal())) return; Hmm, perhaps it would

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

rjmccall added a comment. 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. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13119 + //

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

rjmccall added a comment. In D61165#1494001 , @rsmith wrote: > In D61165#1492830 , @rjmccall wrote: > > > In D61165#1492781 , @rsmith wrote: > > > > > For the purposes of

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

rsmith added a comment. In D61165#1492830 , @rjmccall wrote: > In D61165#1492781 , @rsmith wrote: > > > (Those destructor calls are separate full-expressions that happen > > afterwards; see [intro.execution]/5.) >

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

rjmccall added a comment. In D61165#1492781 , @rsmith wrote: > In D61165#1490417 , @erik.pilkington > wrote: > > > In D61165#1490168 , @rjmccall > > wrote: > > > > > I

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

rjmccall added a comment. In D61165#1492780 , @dexonsmith wrote: > In D61165#1492724 , @rjmccall wrote: > > > In D61165#1492582 , > > @erik.pilkington wrote: > > > > >

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

rsmith added a comment. 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

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

dexonsmith added a comment. 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

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

rjmccall added a comment. 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

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

erik.pilkington added a comment. 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 >

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

erik.pilkington added a comment. 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

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

rjmccall added a comment. 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

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

ldionne added a comment. 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

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

rjmccall added a comment. 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

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

erik.pilkington added a comment. 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.

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

rjmccall added a comment. >>> 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,

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

erik.pilkington added a comment. In D61165#1488657 , @rjmccall wrote: > In D61165#1488553 , @erik.pilkington > wrote: > > > In D61165#1487328 , @rjmccall > > wrote: > > >

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

rjmccall added a comment. In D61165#1488553 , @erik.pilkington wrote: > In D61165#1487328 , @rjmccall wrote: > > > In D61165#1487100 , > > @erik.pilkington wrote: > > > >

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

erik.pilkington added a comment. 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

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

rjmccall added a comment. 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

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

erik.pilkington added a comment. 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

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

rjmccall added a comment. 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,

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

erik.pilkington updated this revision to Diff 197477. erik.pilkington added a comment. Add a try/catch block to the docs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61165/new/ https://reviews.llvm.org/D61165 Files: clang/include/clang/Basic/AttrDocs.td

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

jfb added a comment. 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

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

erik.pilkington updated this revision to Diff 197453. erik.pilkington added a comment. Address review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61165/new/ https://reviews.llvm.org/D61165 Files: clang/include/clang/Basic/AttrDocs.td clang/lib/CodeGen/CGDeclCXX.cpp

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

rjmccall added a comment. Okay, SGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61165/new/ https://reviews.llvm.org/D61165 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

erik.pilkington planned changes to this revision. erik.pilkington added a comment. 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

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

rjmccall added a comment. 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

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

erik.pilkington added a comment. 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

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

erik.pilkington added a comment. 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() {

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

jfb added a comment. 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

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

rjmccall added a comment. 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

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

erik.pilkington created this revision. erik.pilkington added reviewers: rjmccall, rsmith, jfb. Herald added subscribers: dexonsmith, jkorous. Herald added a project: clang. Previously, we didn't mark an array's element type's destructor referenced when it was annotated with no_destroy. This is