[Bug c++/98401] New: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[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]

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.

[Bug c++/98401] Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[brandt.milo2 at gmail dot com]

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

[Bug c++/98401] Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[dpsicilia at gmail dot com]

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.

[Bug c++/98401] Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[davidledger at live dot com.au]

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

[Bug c++/98401] Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[victor.burckel at gmail dot com]

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

[Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[pigman46 at gmail dot com]

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.

[Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[brandt.milo2 at gmail dot com]

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.

[Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[brandt.milo2 at gmail dot com]

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.

[Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[brandt.milo2 at gmail dot com]

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

[Bug c++/98401] coroutines: Temporaries passed to co_await sometimes cause an extraneous call to destructor at incorrect address

[iains at gcc dot gnu.org]

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.