[Bug c++/105426] New: [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[Bug c++/105426] New: [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[avi at scylladb dot com]

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

            Bug ID: 105426
           Summary: [wrong-code][regression][coroutines] range-for
                    temporaries are not persisted in coroutines
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: avi at scylladb dot com
  Target Milestone: ---

The attached reproducer, compiled with:


    g++ --std=c++20 coroutine-unpersisted-range-for-temporary.cc -g -O3


generates correct results for gcc before 15a176a833f and for clang, but
incorrect results in 15a176a833f and later.

[Bug c++/105426] [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[avi at scylladb dot com]

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

--- Comment #1 from Avi Kivity <avi at scylladb dot com> ---
Created attachment 52898
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52898&action=edit
reproducer

[Bug c++/105426] [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[iains at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |iains at gcc dot gnu.org
   Last reconfirmed|                            |2022-04-28
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #2 from Iain Sandoe <iains at gcc dot gnu.org> ---
So the fix for PR105287 was too ambitious - and reveals that we are missing to
name some variables that should be promoted.

Rather than delay for a second fix, the proposal would be a partial reversion
of r12-8308-g15a176a833f23e, like so (which would be more suitable for the
branch).  This does fix the new issue (and still fixes the analyser one) in my
local testing.


diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 551ddc9cc41..2e393b2cddc 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -3973,6 +3973,9 @@ register_local_var_uses (tree *stmt, int *do_subtree,
void *d)
          else if (lvname != NULL_TREE)
            buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
                             lvd->nest_depth, lvd->bind_indx);
+         else
+           buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
+                            lvd->bind_indx);
          /* TODO: Figure out if we should build a local type that has any
             excess alignment or size from the original decl.  */
          if (buf)

[Bug c++/105426] [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
possibly combining IDENTIFIER_POINTER with DECL_UID might be easier for
debugging things later

[Bug c++/105426] [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 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:3d8d093e820b10a4b4b2af8949a368377c0888cb

commit r13-28-g3d8d093e820b10a4b4b2af8949a368377c0888cb
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Thu Apr 28 20:06:29 2022 +0100

    c++, coroutines: Partial reversion of r12-8308-g15a176a833f23e [PR105426].

    The changes to fix PR 105287 included a tightening of the constraints on
which
    variables are promoted to frame copies.  This has exposed that we are
failing
    to name some variables that should be promoted.

    We avoid the use of DECL_UID to build anonymous symbols since that might
not
    be stable for -fcompare-debug.

    The long-term fix is to address the cases where the naming has been missed,
    but for the short-term (and for the GCC-12 branch) backing out the
additional
    constraint is proposed.

    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

            PR c++/105426

    gcc/cp/ChangeLog:

            * coroutines.cc (register_local_var_uses): Allow promotion of
unnamed
            temporaries to coroutine frame copies.

[Bug c++/105426] [12/13 Regression] [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |12.0
   Target Milestone|---                         |12.0
            Summary|[wrong-code][regression][co |[12/13 Regression]
                   |routines] range-for         |[wrong-code][regression][co
                   |temporaries are not         |routines] range-for
                   |persisted in coroutines     |temporaries are not
                   |                            |persisted in coroutines
      Known to work|                            |11.3.0

[Bug c++/105426] [12/13 Regression] [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[iains at gcc dot gnu.org]

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

--- Comment #5 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> possibly combining IDENTIFIER_POINTER with DECL_UID might be easier for
> debugging things later

it does make debugging easier (one reason I was using decl_uid) but Jakub
points out that this might not be stable for -fcompare-debug, so we've used a
sequence number.

[Bug c++/105426] [12/13 Regression] [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:7cc5a20ba3f05a783fb75762cfb77ccb571285ab

commit r12-8319-g7cc5a20ba3f05a783fb75762cfb77ccb571285ab
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Thu Apr 28 20:06:29 2022 +0100

    c++, coroutines: Partial reversion of r12-8308-g15a176a833f23e [PR105426].

    The changes to fix PR 105287 included a tightening of the constraints on
which
    variables are promoted to frame copies.  This has exposed that we are
failing
    to name some variables that should be promoted.

    We avoid the use of DECL_UID to build anonymous symbols since that might
not
    be stable for -fcompare-debug.

    The long-term fix is to address the cases where the naming has been missed,
    but for the short-term (and for the GCC-12 branch) backing out the
additional
    constraint is proposed.

    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

            PR c++/105426

    gcc/cp/ChangeLog:

            * coroutines.cc (register_local_var_uses): Allow promotion of
unnamed
            temporaries to coroutine frame copies.

    (cherry picked from commit 3d8d093e820b10a4b4b2af8949a368377c0888cb)

[Bug c++/105426] [12/13 Regression] [wrong-code][regression][coroutines] range-for temporaries are not persisted in coroutines

[iains at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from Iain Sandoe <iains at gcc dot gnu.org> ---
so fixed on gcc-12 and 13