public inbox for gcc-bugs@sourceware.org help / color / mirror / Atom feed
* [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address @ 2020-12-20 16:39 brandt.milo2 at gmail dot com 2020-12-20 16:40 ` [Bug c++/98401] " brandt.milo2 at gmail dot com ` (8 more replies) 0 siblings, 9 replies; 10+ messages in thread From: brandt.milo2 at gmail dot com @ 2020-12-20 16:39 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 Bug ID: 98401 Summary: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address Product: gcc Version: 10.2.0 Status: UNCONFIRMED Severity: normal Priority: P3 Component: c++ Assignee: unassigned at gcc dot gnu.org Reporter: brandt.milo2 at gmail dot com Target Milestone: --- Created attachment 49811 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49811&action=edit main file I was writing some code using coroutines the other day and started noticing that the destructor of a std::string was crashing in certain situations. The problem seems to arise in gcc's handling of passing a temporary to co_await, but it's a bit finicky when exactly this arises. I found a minimal working example, which is attached (both the original .cpp and the post-pre-processed .ii file, although only standard library headers are #include'd). It does the following: (1) Defines a class lifetime_tester that prints something whenever its constructed, assigned, or destructed. It also has a single member that must, by all rights, always equal this to demonstrate some further perplexing behavior. (2) Defines a struct tag whose single element is a lifetime_tester. (3) Defines the simplest possible coroutine, which just runs the code without suspending and has a single await_transform member accepting a tag&& and outputting the address and value of the member of that reference. (4) A coroutine which passes a temporary of type tag. (5) An invocation of that coroutine in main(). The expected output of this program (and the output I get when compiled with clang++ and the experimental parts of libc++) would be something like: Constructed at 0x1ba32e0 Poked at 0x1ba32e0 with ptr to 0x1ba32e0 Destructed at 0x1ba32e0 End of coroutine body. Where a lifetime_tester is constructed at some address, observed at that address, then destructed. However, when I compile the code with g++-10 -std=c++20 -fcoroutines main.cpp -o program where g++-10 --version gives g++-10 (Ubuntu 10.2.0-5ubuntu1~20.04) 10.2.0. I get some very surprising output: Constructed at 0x560814d21ee0 Poked at 0x560814d21ed8 with ptr to 0x560814d21ee0 Destructed at 0x560814d21ed8 Destructed at 0x560814d21ee0 End of coroutine body. The constructor is called at some address, but then when we look at the temporary passed to co_await, its address is 8 bytes higher than where the constructor was called (yet it somehow has a member pointing to the original address). Then, the destructor is called at this higher address and then called again at the correct address. The difference between the two addresses is consistent between runs. Various perturbations of the code yield varying results: * If we change the signature of await_transform to auto await_transform(tag), the bug remains. * If we construct a local variable of type tag in the coroutine, then pass it to co_await (possibly calling std::move on it), the program behaves correctly, regardless of whether we pass by rvalue or lvalue reference or by value. * If we remove the struct tag and just pass lifetime_tester, the program behaves as expected. * If we change the struct to be struct tag { int x; lifetime_tester tester; }; the incorrect address appears 8 bytes lower than the original one instead of 8 bytes higher. Similarly if we did struct tag{ lifetime_tester tester; int x; }; * If we leave the struct tag as it originally was, but add more elements to lifetime_tester (e.g. add a member char padding[1024]), the difference between the addresses increases - and seems to always equal -sizeof(lifetime_tester) bytes. I also tried compiling the master branch of the gcc git repository and noticed the same behavior when compiling the program (--version gives g++ (GCC) 11.0.0 20201219 (experimental)). The same behavior happens if I try the code on godbolt on various recent gcc branches (https://godbolt.org/z/fx6YPb) - though, oddly, selecting x86-64 gcc 10.2 on the compiler explorer produces a different incorrect output: Constructed at 0x21cbed8 Poked at 0x21cbed8 with ptr to 0x21cbed8 End of coroutine body. Where the destructor is not called on the temporary at all. Attached is the original main.cpp file which can be compiled with the command listed earlier. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/98401] Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com @ 2020-12-20 16:40 ` brandt.milo2 at gmail dot com 2021-01-09 19:56 ` dpsicilia at gmail dot com ` (7 subsequent siblings) 8 siblings, 0 replies; 10+ messages in thread From: brandt.milo2 at gmail dot com @ 2020-12-20 16:40 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 --- Comment #1 from Milo Brandt <brandt.milo2 at gmail dot com> --- Created attachment 49812 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49812&action=edit A zipped copy of the preprocessed source (main.ii) - which is slightly larger than the 1000 KB file size limit alone ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/98401] Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com 2020-12-20 16:40 ` [Bug c++/98401] " brandt.milo2 at gmail dot com @ 2021-01-09 19:56 ` dpsicilia at gmail dot com 2021-06-10 14:28 ` davidledger at live dot com.au ` (6 subsequent siblings) 8 siblings, 0 replies; 10+ messages in thread From: dpsicilia at gmail dot com @ 2021-01-09 19:56 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 David Sicilia <dpsicilia at gmail dot com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dpsicilia at gmail dot com --- Comment #2 from David Sicilia <dpsicilia at gmail dot com> --- I believe I've just run into this as well. In some cases (which I don't know how to describe) when I co_await on a temporary I get an error that manifests as a sanitizer error (in my build): "runtime error: member call on misaligned address 0x00000095c55e for type 'struct optional', which requires 8 byte alignment." When I store the awaitable first in a local variable, then co_await on that, the issue goes away. Unfortunately I have not managed to produce a minimal reproducer yet, so not sure how much this helps, but it would seem that gcc's handling of co_wait'ing on temporaries needs to be looked at. Also want to point out that with Milo Brandt's reproducer, it has different output between gcc 10.2 and gcc trunk: gcc 10.2: https://godbolt.org/z/rPdY9f gcc trunk: https://godbolt.org/z/Y1adoq The gcc 10.2 output looks correct to me, so it could be a regression? That said, I observed my issue on gcc 10.2 (not trunk), so it may not be a regression. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/98401] Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com 2020-12-20 16:40 ` [Bug c++/98401] " brandt.milo2 at gmail dot com 2021-01-09 19:56 ` dpsicilia at gmail dot com @ 2021-06-10 14:28 ` davidledger at live dot com.au 2021-06-23 8:55 ` victor.burckel at gmail dot com ` (5 subsequent siblings) 8 siblings, 0 replies; 10+ messages in thread From: davidledger at live dot com.au @ 2021-06-10 14:28 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 David Ledger <davidledger at live dot com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |davidledger at live dot com.au --- Comment #3 from David Ledger <davidledger at live dot com.au> --- I'm seeing the same double destructor: https://godbolt.org/z/8r8oGG4z5 This appears to have many equivilent: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100611 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/98401] Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com ` (2 preceding siblings ...) 2021-06-10 14:28 ` davidledger at live dot com.au @ 2021-06-23 8:55 ` victor.burckel at gmail dot com 2021-11-06 5:07 ` [Bug c++/98401] coroutines: " pigman46 at gmail dot com ` (4 subsequent siblings) 8 siblings, 0 replies; 10+ messages in thread From: victor.burckel at gmail dot com @ 2021-06-23 8:55 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 Victor Burckel <victor.burckel at gmail dot com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor.burckel at gmail dot com --- Comment #4 from Victor Burckel <victor.burckel at gmail dot com> --- I'm also seeing the same behavior, destructor of lambda captures seems to get called twice https://godbolt.org/z/zxnhM3x47 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com ` (3 preceding siblings ...) 2021-06-23 8:55 ` victor.burckel at gmail dot com @ 2021-11-06 5:07 ` pigman46 at gmail dot com 2021-12-28 4:02 ` brandt.milo2 at gmail dot com ` (3 subsequent siblings) 8 siblings, 0 replies; 10+ messages in thread From: pigman46 at gmail dot com @ 2021-11-06 5:07 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 Michael Theall <pigman46 at gmail dot com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pigman46 at gmail dot com --- Comment #5 from Michael Theall <pigman46 at gmail dot com> --- This is also happening for me. In my case i have a promise_type defined by coroutine_traits. This promise_type contains only a shared_ptr, which is constructed via make_shared on default and move constructors, and the copy constructor is deleted. a promise_type x is default-constructed and then immediately another promise_type y is move-constructed from x but with the wrong reference. I'll call it x'. promise_type contains a shared_ptr which has the same reference in both x and x' although x' is never constructed, and the shared_ptr has a refcount of 1. So now we have x with its original shared_ptr 'a'. We have x' with some unspecified shared_ptr 'b' (it was swapped or destroyed when moved to y). We have y also with shared_ptr 'a'. The refcount of 'a' at this point is still 1. Then, by the time my coroutine returns, all of x, x', and y are destructed, which means shared_ptr 'a' is destructed too many times. shared_ptr 'b' is fine. If I assign the awaitable to a local variable and then co_await that instead, the problem does not occur. In no cases does the problem occur with clang. Same behavior on GCC 11.2.1 x86_64 with -std=c++20 and GCC 11.1.0 aarch64 with -std=gnu++20. The problem occurs regardless of optimization level and whether LTO is enabled, if that makes any difference. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com ` (4 preceding siblings ...) 2021-11-06 5:07 ` [Bug c++/98401] coroutines: " pigman46 at gmail dot com @ 2021-12-28 4:02 ` brandt.milo2 at gmail dot com 2021-12-28 4:03 ` brandt.milo2 at gmail dot com ` (2 subsequent siblings) 8 siblings, 0 replies; 10+ messages in thread From: brandt.milo2 at gmail dot com @ 2021-12-28 4:02 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 --- Comment #6 from Milo Brandt <brandt.milo2 at gmail dot com> --- Created attachment 52074 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52074&action=edit A proposed testcase for the original bug. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com ` (5 preceding siblings ...) 2021-12-28 4:02 ` brandt.milo2 at gmail dot com @ 2021-12-28 4:03 ` brandt.milo2 at gmail dot com 2021-12-28 4:04 ` brandt.milo2 at gmail dot com 2021-12-28 10:03 ` iains at gcc dot gnu.org 8 siblings, 0 replies; 10+ messages in thread From: brandt.milo2 at gmail dot com @ 2021-12-28 4:03 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 --- Comment #7 from Milo Brandt <brandt.milo2 at gmail dot com> --- Created attachment 52075 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52075&action=edit A proposed testcase for a more subtle variant of the same bug. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com ` (6 preceding siblings ...) 2021-12-28 4:03 ` brandt.milo2 at gmail dot com @ 2021-12-28 4:04 ` brandt.milo2 at gmail dot com 2021-12-28 10:03 ` iains at gcc dot gnu.org 8 siblings, 0 replies; 10+ messages in thread From: brandt.milo2 at gmail dot com @ 2021-12-28 4:04 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 --- Comment #8 from Milo Brandt <brandt.milo2 at gmail dot com> --- I was trying to fix this and, from my work, I have a precise diagnosis of the bug and a hack that *mostly* fixes things, but leaves a more subtle bug unfixed. Debugging this is getting over my head, but the descriptions below should suffice to describe fairly precisely how the bug operates and what behavior needs to be changed to patch it - perhaps someone more capable with this codebase will find this helpful. Apologies for the length of this comment, but the issue is very particular. The bugs reported here arise from the following behavior of the code that morphs functions to coroutines: when a temporary is created by aggregate initialization and has its lifetime extended across a suspension point, the members of the temporary are constructed out of place, then effectively memcpy'd into the slots for the members of the constructed aggregate. After the suspend point, both the temporary itself and the members that were constructed out of place are destroyed. This is incorrect behavior. The expected behavior is that the temporary gets its lifetime extended, but that its members are initialized in place. Only the temporary itself should be cleaned up. The current behavior can lead to serious problems in generated code - for instance, if the aggregate initialized struct had a unique_ptr as a member, the current behavior will delete that pointer twice, almost certainly causing correct code to crash at runtime when compiled with gcc. As a particular example, if we had "struct Pair { X x; Y y; };" and wished to write a line of code such as "Pair{}, co_await /*awaitable*/;", the existing code allocates a slot in the coroutine frame for values of type Pair, X, and Y. It initializes the slots for X and Y via constructor calls, then copies the memory from these slots to the members of the Pair slot, then call destructors for each of the three promoted temporaries. This behavior is implemented in gcc/cp/coroutines.cc and principally involves the functions maybe_promote_temps and flatten_await_stmt, which are responsible for finding temporaries in expressions involving co_await and promoting them to values in the coroutine's frame. At present, to flatten a sub-expression such as Pair{} appearing in a co_await statement, it will detect three expressions of interest: one representing the Pair itself, then two as sub-nodes of the CONSTRUCT node for that Pair, corresponding to the two members. These are identified by find_interesting_subtree, which calls tmp_target_expr_p, which selects TARGET_EXPRs that are artificial (from the compiler) and which are not associated to a named variable. All three TARGET_EXPRs meet these criteria, and hence get promoted, even though only the outermost one actually *should* be promoted. This can largely be fixed by adding the following lines to tmp_target_expr_p: + if (TREE_CODE (TREE_OPERAND (t, 1)) == AGGR_INIT_EXPR) + return false; Adding these lines doesn't harm any existing tests and resolves the problem reported here - they ensure that a temporary that is aggregate initialized, but has its lifetime extended over a suspension point will have its members constructed in place and not have their destructors called spuriously. This is kind of a hack, but it does fix the problem reported in this thread and doesn't seem to create any new problems. However, this patch fails to fix a more subtle bug that is intimately tied to the one it fixes: if there is a suspension point *during* aggregate initialization, the initializers for the aggregate will be executed in the wrong order even though the standard guarantees their execution order will be from first to last. For instance, code such as "Pair{.x{}, .y=(co_await std::suspend_always{}, 1)};" acts incorrectly by first calling the awaiter's await_ready and await_suspend functions, then, upon resumption, constructing the .x member via X::X(), *then* calling the awaiter's await_resume, constructing .y, and calling Pair::~Pair(). I believe that the only correct behavior for this line is to construct .x via X::X(), then call await_ready and await_suspend, then, upon resumption, calling await_resume immediately, then constructing the .y member and calling Pair::~Pair() to clean up (the only difference being that X::X() must be called before await_ready). If the coroutine is destroyed instead, we should destroy the .x member via X::~X() (or, similarly, we must call X::~X() if the call to Y::Y(int) throws an exception after resumption). An example that prints out the ordering of these functions is here: https://godbolt.org/z/dz1818naf (though note that none of gcc, clang nor MSVC correctly handle this code). I believe that the correct way to patch this problem is as follows: 1. The function flatten_await_stmt must look specifically for CONSTRUCTOR nodes in the tree and, if any initializer other than the first contains a co_await statement, this CONSTRUCTOR node must somehow be rewritten into a series of constructions of each individual element (e.g. replacing the above construction by something similar to "new (&p->x) X{}, new (&p->y) Y{co_await std::suspend_always{}, 1}, ..." where p is a Pair* pointing to the allocation of the Pair value in the coroutine's frame - except with extra complexity to handle exceptions during construction). 2. Whenever a transformation as in (1) occurs, the destroyer must be amended to delete the members of a partially constructed aggregate type. The destroyer should not destroy the aggregate itself until it has finished construction. 3. Any AGGR_INIT_EXPRs appearing as the initializer for a TARGET_EXPR in a CONSTRUCTOR (which I believe is the only place they *can* appear in a coroutine body? - if they can appear elsewhere, those situations should also be examined for similar bugs) should not be promoted except through the special procedure from (1). I believe this strategy would fix the bad behavior identified here, but it looks quite difficult to actually implement - at least without a strong knowledge of gcc internals that I simply do not have. I've attached two testcases that I would propose as minimal examples of the incorrect behavior; the first deals merely with the problem of extending the lifetime of aggregates. The second deals with aggregate construction that is interrupted by a suspension point. The two line patch proposed above would fix the first case, but not the second. The outline for a more proper patch would fix both at once (and make the two line patch redundant). ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com ` (7 preceding siblings ...) 2021-12-28 4:04 ` brandt.milo2 at gmail dot com @ 2021-12-28 10:03 ` iains at gcc dot gnu.org 8 siblings, 0 replies; 10+ messages in thread From: iains at gcc dot gnu.org @ 2021-12-28 10:03 UTC (permalink / raw) To: gcc-bugs https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401 --- Comment #9 from Iain Sandoe <iains at gcc dot gnu.org> --- thanks for your analysis; the problem is indeed clear. I have some work in progress that I expect to produce for a fix for this - it is just a question of finding time to progress it. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-12-28 10:03 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-20 16:39 [Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address brandt.milo2 at gmail dot com 2020-12-20 16:40 ` [Bug c++/98401] " brandt.milo2 at gmail dot com 2021-01-09 19:56 ` dpsicilia at gmail dot com 2021-06-10 14:28 ` davidledger at live dot com.au 2021-06-23 8:55 ` victor.burckel at gmail dot com 2021-11-06 5:07 ` [Bug c++/98401] coroutines: " pigman46 at gmail dot com 2021-12-28 4:02 ` brandt.milo2 at gmail dot com 2021-12-28 4:03 ` brandt.milo2 at gmail dot com 2021-12-28 4:04 ` brandt.milo2 at gmail dot com 2021-12-28 10:03 ` iains at gcc dot gnu.org
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).