[Bug c++/99576] New: [coroutines] desctructor of a temporary called too early within co_await expression

[Bug c++/99576] New: [coroutines] desctructor of a temporary called too early within co_await expression

[lemourin at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

            Bug ID: 99576
           Summary: [coroutines] desctructor of a temporary called too
                    early within co_await expression
           Product: gcc
           Version: 10.2.1
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: lemourin at gmail dot com
  Target Milestone: ---

```
#if __has_include(<coroutine>)
#include <coroutine>
namespace stdx {
using std::coroutine_handle;
using std::noop_coroutine;
using std::suspend_always;
using std::suspend_never;
}  // namespace stdx
#elif __has_include(<experimental/coroutine>)
#include <experimental/coroutine>
namespace stdx {
using std::experimental::coroutine_handle;
using std::experimental::noop_coroutine;
using std::experimental::suspend_always;
using std::experimental::suspend_never;
}  // namespace stdx
#else
#error "coroutines unsupported"
#endif
#include <exception>
#include <iostream>
#include <utility>

class Task {
 public:
  struct promise_type {
    struct final_awaitable {
      bool await_ready() noexcept { return false; }
      auto await_suspend(stdx::coroutine_handle<promise_type> coro) noexcept {
        return coro.promise().continuation;
      }
      void await_resume() noexcept {}
    };
    Task get_return_object() {
      return Task(stdx::coroutine_handle<promise_type>::from_promise(*this));
    }
    stdx::suspend_always initial_suspend() { return {}; }
    final_awaitable final_suspend() noexcept { return {}; }
    void unhandled_exception() { std::terminate(); }
    void return_void() {}

    stdx::coroutine_handle<void> continuation = stdx::noop_coroutine();
  };

  Task(const Task&) = delete;
  Task(Task&& other) noexcept
      : handle_(std::exchange(other.handle_, nullptr)) {}
  Task& operator=(const Task&) = delete;
  Task& operator=(Task&& other) noexcept {
    handle_ = std::exchange(other.handle_, nullptr);
    return *this;
  }
  ~Task() {
    if (handle_) {
      handle_.destroy();
    }
  }

  bool await_ready() const { return false; }
  auto await_suspend(stdx::coroutine_handle<void> continuation) {
    handle_.promise().continuation = continuation;
    return handle_;
  }
  void await_resume() {}

 private:
  explicit Task(stdx::coroutine_handle<promise_type> handle)
      : handle_(handle) {}

  stdx::coroutine_handle<promise_type> handle_;
};

struct RunTask {
  struct promise_type {
    RunTask get_return_object() { return {}; }
    stdx::suspend_never initial_suspend() { return {}; }
    stdx::suspend_never final_suspend() noexcept { return {}; }
    void return_void() {}
    void unhandled_exception() { std::terminate(); }
  };
};

struct Foo {
  Foo() { std::cerr << "Foo()\n"; }
  ~Foo() { std::cerr << "~Foo()\n"; }
};

Task DoAsync() {
  std::cerr << "START TASK\n";
  co_return co_await [foo = Foo{}]() -> Task {
    std::cerr << "IN LAMBDA\n";
    co_return;
  }();
}

RunTask Main() { co_await DoAsync(); }

int main() { Main(); }
```

gcc outputs:
`
START TASK
Foo()
~Foo()
IN LAMBDA
`

while clang and msvc output:
`
START TASK
Foo()
IN LAMBDA
~Foo()
`

Is this code undefined behavior? Or is clang and msvc wrong?

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[nilsgladitz at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

Nils Gladitz <nilsgladitz at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nilsgladitz at gmail dot com

--- Comment #1 from Nils Gladitz <nilsgladitz at gmail dot com> ---
I am seeing issues with lambdas and coroutines that I can't quite explain yet.
Specifically reference counting objects capture copied (e.g. std::shared_ptr)
seemed to decrease their reference count more often than they should.

Found this issue which didn't quite seem to match but trying Paweł Wegner's
test case I see something that may explain some weirdness (haven't fully
digested the test case or disregarded the possibility that it is my fault
though).

With vanilla gcc 10.2.0 I am seeing what Paweł observed:

START TASK
Foo()
~Foo()
IN LAMBDA

With vanilla 10.3.0 and 11.1.0 I however see the following:

START TASK
Foo()
IN LAMBDA
~Foo()
~Foo()

i.e. the Foo destructor now gets called later but apparently twice

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[nilsgladitz at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #2 from Nils Gladitz <nilsgladitz at gmail dot com> ---
I opened bug 100611 for what I described above; assuming this is related but
distinct.

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[davidledger at live dot com.au]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

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

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[victor.burckel at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- 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++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-10-01
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[adrian.perl at web dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

Adrian Perl <adrian.perl at web dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |adrian.perl at web dot de

--- Comment #5 from Adrian Perl <adrian.perl at web dot de> ---
Created attachment 53963
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53963&action=edit
Test applications

I also noticed this behaviour in gcc 11.2 and could reproduce it in all version
since 10.2 (even in the current trunk).

Since this issues has existed for some time now, I had a look at the code
myself and possibly found a fix. Please be aware that I am a complete novice in
regards to the gcc-sourcecode.

As the example posted by David Ledger (https://godbolt.org/z/8r8oGG4z5) was the
simplest one I could find, I used it to debug the issue.

Having a look at the generated tree using -fdump-tree-all-graph I found out
that the destructor of A gets indeed called twice, first by the destructor of
class Awaitable and then once more in a finally-clause.
...
                 resume.4:;
                  <<cleanup_point <<< Unknown tree: expr_stmt
                    Awaitable::await_resume (&Aw0) >>>>>;
                }
              finally
                {
                  Awaitable::~Awaitable (&Aw0);
                }
            }
          finally
            {
              A::~A (&D.58267);
            }
...
The finally clauses are generated by the maybe_promote_temps-function in
coroutines.cc, which modifies the lifetime-management of temporaries in
co_await statements in order to keep them alive across a suspension point. For
this purpose it recursivly searches all initializers to the right of the
co_await expression, using the helper functions flatten_await_stmt and
find_interesting_subtree.
In this recursion it finds the initialization of the Awaitable and also the
initialization of its member. So both the temporary Awaitable-instance and its
member get promoted, which leads to the generation of a (correct) destructor
call for the Awaitable and the (incorrect) call for its member.

So the error seems to be in the recursion, which also finds and promotes
initializations in sub-scopes like the constructor. 

Therefore I modified the check for relevant subexpressions to exclude trees
below constructor calls, as can be seen in this patch:

Patch:
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 01a3e831ee5..349b68ea239 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2684,6 +2684,10 @@ find_interesting_subtree (tree *expr_p, int *dosub, void
*d)
          return expr;
        }
     }
+  else if (TREE_CODE(expr) == CONSTRUCTOR)
+    {
+      *dosub = 0; /* We don't need to consider this any further.  */
+    }
   else if (tmp_target_expr_p (expr)
           && !p->temps_used->contains (expr))
     {
-- 
2.34.1

I checked the results of the patched compiler using most of the examples posted
in this and related bug reports (99576, 100611, 102217, 101244, 101976) and
always got the correct results, also matching the output of clang. I have
attached these test applications.

I also noticed that the incorrect behaviour is gone as soon as the Awaitable
has a user defined constructor. This likely moves the initalization of the
Awaitable to a seperate tree which does not get evaluated by
find_interesting_subtree. I checked the .dot-files and there was indeed an
additional tree for the constructor when it is defined by the user.

It would be great if someone could have a look at the patch, as I am unsure if
it could have any unforseen sideeffects.
Another (better?) fix for this issue could be to always generate the
constructor-tree.

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #6 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Adrian Perl from comment #5)
> Created attachment 53963 [details]

thanks for the analysis and the patch.

> Patch:
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 01a3e831ee5..349b68ea239 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -2684,6 +2684,10 @@ find_interesting_subtree (tree *expr_p, int *dosub,
> void *d)
>  	  return expr;
>  	}
>      }
> +  else if (TREE_CODE(expr) == CONSTRUCTOR)
> +    {
> +      *dosub = 0; /* We don't need to consider this any further.  */
> +    }

small nit - the { } are not needed here ^

>    else if (tmp_target_expr_p (expr)
>  	   && !p->temps_used->contains (expr))
>      {
> -- 
> 2.34.1

Your reasoning (I know that several of the outstanding issues are related to
'promotion' of temporaries to the frame) and the patch seem reasonable.  

Have you bootstrapped the compiler with the changes and run the whole of the
testsuite? (I do agree that this change should only affect coroutines, but we
still do the full check too).

The test-cases would need to be added to the gcc/testsuite/g++.dg/coroutines
(with names that reflect the PRs that they test).

once you have things to that stage, then please post the patch to
gcc-patches@gcc.gnu.org (along with the test cases added), and we can go from
there.

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[adrian.perl at web dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #7 from Adrian Perl <adrian.perl at web dot de> ---
Bootstrapping was successfull and the tests are currently running. Some of the
tests have failed, but they don't seem to be related to coroutines. Should I
test twice, with and without the patch, in order to see the actual impact? 

Most of the test-applications attached and posted in the bug reports only log
to stdout/stderr. Should I rewrite them and add assertions?

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #8 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Adrian Perl from comment #7)
Again, thanks for working on this.

> Bootstrapping was successfull and the tests are currently running. Some of
> the tests have failed, but they don't seem to be related to coroutines.

On most platforms, at any given point in the cycle, there will be failing
tests...

> Should I test twice, with and without the patch, in order to see the actual
> impact? 

... so ,yes, that is the usual approac.
there are some tools that help you compare the results.

I personally use:
${src}/contrib/test_summary > summ.txt 

and then the same in the patched build tree ...
... and then compare the two summ.txt files side-by side using diff -> less.

(it is important that the same number of tests are executed of course, so the
numbers are relevant as well as the pass/fail).  Your number for g++ should be
bigger by the ones added.

> Most of the test-applications attached and posted in the bug reports only
> log to stdout/stderr. Should I rewrite them and add assertions?

The usual mechanism is to do a test and to abort()*** on fail [an assert is
equivalent]. The reason for the abort() rather than, say, returning non-zero
from main is that when tests are conducted on remote embedded platforms, the
return value from main() might not be available.


***
For c++ tests, the extra of pulling in <cassert> or <cstdlib> is likely not
significant, but if you want to avoid that (it can make debugging easier to
reduce the number of headers pulled in) .. then you can use __builtin_abort();

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[adrian.perl at web dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #9 from Adrian Perl <adrian.perl at web dot de> ---
Thanks for the advice.

I hope you meant __builtin_trap() as I can't find a __builtin_abort() function.

I have now written test applications for all relevant bug reports (99576,
100611, 101976, 101367). I also verified that it fixes 107288, but did not add
a test as it requires boost asio.

Unfortunately I was wrong that the patch will fix 102217 and 101244. They use
similar examples but also the ternary operator, which still leads to an invalid
statement error when used in co_awaits.

I will send the patch together with the testfiles as soon as the testsuite has
finished. Is it normal that it takes more than 6 hours to complete?

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #10 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Adrian Perl from comment #9)
> Thanks for the advice.
> 
> I hope you meant __builtin_trap() as I can't find a __builtin_abort()
> function.

hmm .. I meant __builtin_abort () ... it is widely used in the testsuite for
the reasons mentioned (try grepping for it in gcc/testsuite/gcc.dg to see some
examples).

> I have now written test applications for all relevant bug reports (99576,
> 100611, 101976, 101367). 

great!

> I also verified that it fixes 107288, but did not add a test as it requires boost asio.

The way to deal with cases like that is to take the .ii file (so that the
dependencies on external headers are removed) and then reduce it to something
usable as a test.  Such reductions vary in difficulty (using tools like c-vise
or creduce can help, sometimes it's possible to do it manually too).  [I'm not
asking you to do this right now, but mentioning that this is the approach used
in such cases].

> Unfortunately I was wrong that the patch will fix 102217 and 101244. They
> use similar examples but also the ternary operator, which still leads to an
> invalid statement error when used in co_awaits.

Yes, this is a different problem for which I have some work in progress, but
not ready for publication just yet.

> I will send the patch together with the testfiles as soon as the testsuite
> has finished. Is it normal that it takes more than 6 hours to complete?

depends on your hardware .. my fastest box takes about 2 hours, my slowest
nearly a week :) .. so long as you are using "-jN" on the make line where N ≈
the number of threads your hardware will accommodate, that's about the best you
can do.

If you plan on working more with GCC there is also the option to get an account
on the "compile farm" which gives you access to more platform versions and some
quite powerful hardware.

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[adrian.perl at web dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #11 from Adrian Perl <adrian.perl at web dot de> ---
Yeah, my mistake. My IDE failed to look up the function and a short search on
the internet revealed only builtin_trap
(https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html)

You just saved me hours with the -j hint! I assumed it was not applicable as it
is not used in the guide (https://gcc.gnu.org/contribute.html).

Thanks

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[adrian.perl at web dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #12 from Adrian Perl <adrian.perl at web dot de> ---
I have sent the patch and tests to gcc-patches@gcc.gnu.org

Thanks for the guidance

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #13 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Adrian Perl from comment #12)
> I have sent the patch and tests to gcc-patches@gcc.gnu.org

As noted there, the patch causes regressions in more complex library code (e.g.
some tests in folly) where CONSTRUCTOR trees might have nested await
expressions.

I mentioned that the real problem is to determine if a temporary should be
promoted - and, in the case of PR95736, the issue is that we promote a
temporary for a target expression that is subsequently elided; we can see this
clearly if we print the object addresses in the test case:

START TASK
 Foo() 0x600001004071
IN LAMBDA
~Foo() 0x600001004070  << corresponds to the elided target expression.
~Foo() 0x600001004071


so this:
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index ce7cf971e03..1bad509233c 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2830,6 +2830,7 @@ find_interesting_subtree (tree *expr_p, int *dosub, void
*d)
        }
     }
   else if (tmp_target_expr_p (expr)
+          && !TARGET_EXPR_ELIDING_P (expr)
           && !p->temps_used->contains (expr))
     {
       p->entry = expr_p;


fixes that specific problem - but regresses other test cases (no detailed
analysis yet). Needs more thought ....

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #14 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Iain Sandoe from comment #13)
> (In reply to Adrian Perl from comment #12)
> > I have sent the patch and tests to gcc-patches@gcc.gnu.org
> 
> As noted there, the patch causes regressions in more complex library code
> (e.g. some tests in folly) where CONSTRUCTOR trees might have nested await
> expressions.
> 
> I mentioned that the real problem is to determine if a temporary should be
> promoted - and, in the case of PR95736, the issue is that we promote a
> temporary for a target expression that is subsequently elided; we can see
> this clearly if we print the object addresses in the test case:
> 
> START TASK
>  Foo() 0x600001004071
> IN LAMBDA
> ~Foo() 0x600001004070  << corresponds to the elided target expression.
> ~Foo() 0x600001004071
> 
> 
> so this:
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index ce7cf971e03..1bad509233c 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -2830,6 +2830,7 @@ find_interesting_subtree (tree *expr_p, int *dosub,
> void *d)
>         }
>      }
>    else if (tmp_target_expr_p (expr)
> +          && !TARGET_EXPR_ELIDING_P (expr)
>            && !p->temps_used->contains (expr))
>      {
>        p->entry = expr_p;
> 
> 
> fixes that specific problem - but regresses other test cases (no detailed
> analysis yet). Needs more thought ....

Turns out the regression was caused by another patch; given Jason's comment on
the gcc-patches thread, this seems worth testing more widely.

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.5

--- Comment #15 from Iain Sandoe <iains at gcc dot gnu.org> ---
revised patch
https://gcc.gnu.org/pipermail/gcc-patches/2022-December/607776.html

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

--- Comment #16 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain D Sandoe <iains@gcc.gnu.org>:

https://gcc.gnu.org/g:58a7b1e354530d8dfe7d8fb859c8b8b5a9140f1f

commit r13-4479-g58a7b1e354530d8dfe7d8fb859c8b8b5a9140f1f
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Wed Nov 30 17:05:56 2022 +0000

    coroutines: Do not promote temporaries that will be elided.

    We usually need to 'promote' (i.e. save to the coroutine frame) any
temporary
    variable that is in a target expression that must persist across an await
    expression.  However, if the TE is just used as a direct initializer for
    another object it will be elided - and we should not promote it since that
    would lead to a DTOR call for something that is never constructed.

    Since we now have a mechanism to tell if TEs will be elided, use that.

    Although the PRs referenced initially appear to be different issues, they
all
    stem from this.

    Co-Authored-By: Adrian Perl <adrian.perl@web.de>
    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

            PR c++/100611
            PR c++/101367
            PR c++/101976
            PR c++/99576

    gcc/cp/ChangeLog:

            * coroutines.cc (find_interesting_subtree): Do not promote
temporaries
            that are only used as direct initializers for some other object.

    gcc/testsuite/ChangeLog:

            * g++.dg/coroutines/pr100611.C: New test.
            * g++.dg/coroutines/pr101367.C: New test.
            * g++.dg/coroutines/pr101976.C: New test.
            * g++.dg/coroutines/pr99576_1.C: New test.
            * g++.dg/coroutines/pr99576_2.C: New test.

[Bug c++/99576] [coroutines] destructor of a temporary called too early within co_await expression

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |13.0
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed in GCC 13, not a regression(?)