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

["adrian.perl at web dot de" <gcc-bugzilla@gcc.gnu.org>]

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.