[Bug c++/101118] New: coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[Bug c++/101118] New: coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[nilsgladitz at gmail dot com]

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

            Bug ID: 101118
           Summary: coroutines: unexpected ODR warning for coroutine frame
                    type in LTO builds
           Product: gcc
           Version: 11.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: nilsgladitz at gmail dot com
  Target Milestone: ---

Created attachment 51033
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51033&action=edit
Testcase source files

The attached sources files are reduced from a C++ project using boost::asio and
coroutines.

I don't fully understand it but compiling them with vanilla GCC 11.1.0 like
this:
g++ -std=c++2a -fPIC -fcoroutines -Wall -flto -fno-fat-lto-objects -shared
foo.cpp bar.cpp

unexpectedly produces the following diagnostic:
boost.hpp:81:3: warning: type ‘struct
_ZN5boost4asio1gIiFviEE2abIiEENS0_9awaitableIiiEET_.frame’ violates the C++ One
Definition Rule [-Wodr]
   81 |   }
      |   ^
boost.hpp:81:3: note: a different type is defined in another translation unit
   81 |   }
      |   ^
boost.hpp:79:34: note: the first difference of corresponding definitions is
field ‘__D.9984.3.4’
   79 |   template <typename k> static w ab(k ai) {
      |                                  ^
boost.hpp:79:34: note: a field with different name is defined in another
translation unit
   79 |   template <typename k> static w ab(k ai) {
      |

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic, lto
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |iains at gcc dot gnu.org

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
It's not exactly clear what these fields are but I suppose it's not just the
names that differ.

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

--- Comment #2 from Iain Sandoe <iains at gcc dot gnu.org> ---
hmm. 

__D.9984.3.4 means that this is a frame variable that is a 'promoted' temporary
(promoted because its lifetime had to be extended across a suspend point by
copying it into the frame).

So, I am supposing that this issue is because such a temporary would almost
certainly not have the same DECL_UID (from which the __D.xxxx is created) in
different TUs.

Which seems to imply that we need to find a way to disambiguate the frame
variable names for LTO.  This is, I would expect, another case of something
with local visibility becoming globally visible for LTO (but that's
supposition, not analysed).

the coros testsuite runs torture tests thus:
....
PASS: g++.dg/coroutines/torture/alloc-00-gro-on-alloc-fail.C   -O2 -flto
-flto-partition=none  (test for excess errors)
PASS: g++.dg/coroutines/torture/alloc-00-gro-on-alloc-fail.C   -O2 -flto
-flto-partition=none  execution test
PASS: g++.dg/coroutines/torture/alloc-00-gro-on-alloc-fail.C   -O2 -flto  (test
for excess errors)
PASS: g++.dg/coroutines/torture/alloc-00-gro-on-alloc-fail.C   -O2 -flto 
execution test

so there is some coverage of LTO - but not multiple TUs :(.

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[nilsgladitz at gmail dot com]

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

--- Comment #3 from Nils Gladitz <nilsgladitz at gmail dot com> ---
Thanks for looking into this!

Any idea what the potential implications are?
I assume I can't just ignore the warning as this will likely break code?
When I turn off LTO the diagnostic will go away but the ODR violations are
still there; could they still break something?

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Nils Gladitz from comment #3)
> Thanks for looking into this!

just speculation so far ...

> Any idea what the potential implications are?

Not yet.

> I assume I can't just ignore the warning as this will likely break code?

My expectations at present is that the front end actions on a coroutine are
expected to produce functions and a frame type that are TU-local.

At this point, it's not clear whether it is ever valid to merge these using LTO
(and I don't think that was specifically addressed in any of my discussions in
WG21 or with implementers on other compilers).

> When I turn off LTO the diagnostic will go away but the ODR violations are
> still there; could they still break something?

I *think* the "ODR violation" is to do with not giving the frame types unique
names per TU so that there is a [probably invalid] attempt to merge them.

.. but, as noted above, I guess we should consider carefully if the frames can
ever be considered mergeable (which would imply that the actor and destroyer
functions were also).

If they are _never_ validly mergeable, then the ODR violation is not "real" but
the consequence of a naming scheme that is not good enough.  As per my
expectations when implementing that is the case (i.e. it should be OK).

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[john at drouhard dot dev]

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

--- Comment #5 from John Drouhard <john at drouhard dot dev> ---
Has there been any progress toward resolution for this? We've been trying to
use coroutines in our project but we require LTO for performance reasons, so
this is holding us back.

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[hubicka at gcc dot gnu.org]

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

--- Comment #6 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
I am not really expert on coroutines. But this seems to be a type (not a
declaration we globalize during LTO) generated internally by the front-end. 
The name __D.9984.3.4 looks like it has a global counter in it.
ODR types are supposed to be literaly the same across units.

One way to silence this would be to not make these types ODR types since they
are not the usual C++ types anyway.
I wonder if  this is a part of the cross-unit API that is supposed to be same?
I.e. is the coroutine in one compilation unit visible from the other?

If so, perhaps the only problem is hte global counter 9984 and if the counts
were generated internally for each such type, the ODR handling wlll be happy.

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

--- Comment #7 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #6)

from me there has been no progress on anything co-routines related, for a while
- I of not have any resources to work on it.

> I am not really expert on coroutines. But this seems to be a type (not a
> declaration we globalize during LTO) generated internally by the front-end. 
> The name __D.9984.3.4 looks like it has a global counter in it.
> ODR types are supposed to be literaly the same across units.
> 
> One way to silence this would be to not make these types ODR types since
> they are not the usual C++ types anyway.
> I wonder if  this is a part of the cross-unit API that is supposed to be
> same? I.e. is the coroutine in one compilation unit visible from the other?
> 
> If so, perhaps the only problem is hte global counter 9984 and if the counts
> were generated internally for each such type, the ODR handling wlll be happy.

the synthesised functions (actor, destroy) are intended to be TU-local.
the ramp function is what remains of the user's original function after the
coroutine body is outlined - so that has the original signature of the user's
function.

We do use counters to generate local symbol names for frame-promoted
temporaries, so I suppose that there is a possibility that the name(s) that are
intended to be TU-local become visible across TUs in LTO; but those should be
the names of coroutine frame entries (i.e. fields in a structure, not
themselves global symbols).

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[hubicka at ucw dot cz]

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

--- Comment #8 from Jan Hubicka <hubicka at ucw dot cz> ---
> 
> the synthesised functions (actor, destroy) are intended to be TU-local.
> the ramp function is what remains of the user's original function after the
> coroutine body is outlined - so that has the original signature of the user's
> function.
> 
> We do use counters to generate local symbol names for frame-promoted
> temporaries, so I suppose that there is a possibility that the name(s) that are
> intended to be TU-local become visible across TUs in LTO; but those should be
> the names of coroutine frame entries (i.e. fields in a structure, not
> themselves global symbols).

It is the type that we get warning on. So if it is never used to pass
data between translation units, the correct solution would be to put it
to the anoymous namespace.
(or avoid producing ODR name completely but that would lead to worse
code)

Honza

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

--- Comment #9 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #8)
> > 
> > the synthesised functions (actor, destroy) are intended to be TU-local.
> > the ramp function is what remains of the user's original function after the
> > coroutine body is outlined - so that has the original signature of the user's
> > function.
> > 
> > We do use counters to generate local symbol names for frame-promoted
> > temporaries, so I suppose that there is a possibility that the name(s) that are
> > intended to be TU-local become visible across TUs in LTO; but those should be
> > the names of coroutine frame entries (i.e. fields in a structure, not
> > themselves global symbols).
> 
> It is the type that we get warning on. 

The frame type itself .. I see...

> So if it is never used to pass
> data between translation units, the correct solution would be to put it
> to the anoymous namespace.

... OK .. that makes sense; the frame is type-erased for calls from other TUs,
so that this seems the right solution.

> (or avoid producing ODR name completely but that would lead to worse
> code)

Yeah, we need the name for the outlined function(s) so that there is
opportunity to inline there,

> 
> Honza

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

--- Comment #10 from Iain Sandoe <iains at gcc dot gnu.org> ---
Hmm... maybe I am being too hasty here.

If the coroutine has a definition in a header, then the coroutine frame type
_should_ be the same for each instance of it.  So maybe this is actually
reporting a genuine ODR violation?

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

--- Comment #11 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Iain Sandoe from comment #10)
> Hmm... maybe I am being too hasty here.
> 
> If the coroutine has a definition in a header, then the coroutine frame type
> _should_ be the same for each instance of it.  So maybe this is actually
> reporting a genuine ODR violation?

ah (I still misread) ... so the reported error is that the frame type *is* the
same (which is correct) but the field names differ (which is probably a
consequence of using static counters to produce them).  So we need a stable way
to generate the field names.

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-03-03

--- Comment #12 from Iain Sandoe <iains at gcc dot gnu.org> ---
Actually, comment #3 has it mostly :

here is the frame layout from the test case:

FRAME type decl
boost::asio::g<int,
void(|ds|int)>::_ZN5boost4asio1gIiFviEE2abIiEENS0_9awaitableIiiEET_.Frame
   {
     void (*)(|ds|boost::asio::g<int,
void(|ds|int)>::_ZN5boost4asio1gIiFviEE2abIiEENS0_9awaitableIiiEET_.Frame*)_Coro_resume_fn)(|ds|boost::asio::g<int,
void(|ds|int)>::_ZN5boost4asio1gIiFviEE2abIiEENS0_9awaitableIiiEET_.Frame*)
    void (*)(|ds|boost::asio::g<int,
void(|ds|int)>::_ZN5boost4asio1gIiFviEE2abIiEENS0_9awaitableIiiEET_.Frame*)_Coro_destroy_fn)(|ds|boost::asio::g<int,
void(|ds|int)>::_ZN5boost4asio1gIiFviEE2abIiEENS0_9awaitableIiiEET_.Frame*)
    boost::asio::d::promise_type _Coro_promise
     std::__n4861::coroutine_handle<boost::asio::d::z>_Coro_self_handle
     int ai
     short unsigned int _Coro_resume_index
     bool _Coro_frame_needs_free
     bool _Coro_initial_await_resume_called
     std::__n4861::suspend_always Is_1_1
     boost::asio::d::ae::await_transform::y Aw0_2_3
     boost::asio::g<int, void(|ds|int)>::ab::._anon_5 D.10232_2_3 
     boost::asio::d::ae::final_suspend::u Fs_1_4
  }

in the second case, 

     boost::asio::g<int, void(|ds|int)>::ab::._anon_5 D.10233_2_3

This is because I have used the DECL_UID as a convenient way to generate a
unique nane that is also very handy for debugging since it refers back to the
original function.

However, DECL_UIDs are not stable between TUs, obviously, since it depends on
how many there are before the specific case.

So .. for promotion of target expression temporaries to frame vars, one of:
 - a) we need to find a different way to name them
 - b) if it is permissible for an unnamed decl to have a DECL_VALUE_EXPR ()
perhaps the process can be adjusted to avoid naming these cases (although it
does assist in debuggability).

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[hubicka at ucw dot cz]

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

--- Comment #13 from Jan Hubicka <hubicka at ucw dot cz> ---
> So .. for promotion of target expression temporaries to frame vars, one of:
>  - a) we need to find a different way to name them
I think we can just count number of fields within a given frame type?

Honza

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

--- Comment #14 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #13)
> > So .. for promotion of target expression temporaries to frame vars, one of:
> >  - a) we need to find a different way to name them
> I think we can just count number of fields within a given frame type?

yeah I made a hack that did this (and resolves this PR) but I'd think we can
find something neater, I'd like to c++-ify the sources some more, and create a
class to manage the frame... ( maybe for GCC14 now ).

-----


diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 7f8cbc3d95e..bb65d41ce18 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -271,6 +271,8 @@ static GTY(()) tree coro_traits_templ;
 static GTY(()) tree coro_handle_templ;
 static GTY(()) tree void_coro_handle_type;

+static unsigned tmpno = 0;
+
 /* ================= Parse, Semantics and Type checking ================= */

 /* This initial set of routines are helper for the parsing and template
@@ -2882,7 +2884,7 @@ flatten_await_stmt (var_nest_node *n, hash_set<tree>
*promoted,
          tree init = t;
          temps_used->add (init);
          tree var_type = TREE_TYPE (init);
-         char *buf = xasprintf ("D.%d", DECL_UID (TREE_OPERAND (init, 0)));
+         char *buf = xasprintf ("T%03u", tmpno++);
          tree var = build_lang_decl (VAR_DECL, get_identifier (buf),
var_type);
          DECL_ARTIFICIAL (var) = true;
          free (buf);
@@ -4367,6 +4369,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree
*destroyer)
 {
   gcc_checking_assert (orig && TREE_CODE (orig) == FUNCTION_DECL);

+  tmpno = 0;
   *resumer = error_mark_node;
   *destroyer = error_mark_node;
   if (!coro_function_valid_p (orig))

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[hubicka at ucw dot cz]

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

--- Comment #15 from Jan Hubicka <hubicka at ucw dot cz> ---
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101118
> 
> --- Comment #14 from Iain Sandoe <iains at gcc dot gnu.org> ---
> (In reply to Jan Hubicka from comment #13)
> > > So .. for promotion of target expression temporaries to frame vars, one of:
> > >  - a) we need to find a different way to name them
> > I think we can just count number of fields within a given frame type?
> 
> yeah I made a hack that did this (and resolves this PR) but I'd think we can
> find something neater, I'd like to c++-ify the sources some more, and create a
> class to manage the frame... ( maybe for GCC14 now ).
Thanks!
Since this is really part of the ABI, I wonder why it is not covered by
the IA-64 C++ ABI?  Was it ever updated for coroutines?

Honza

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

--- Comment #16 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #15)
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101118
> > 
> > --- Comment #14 from Iain Sandoe <iains at gcc dot gnu.org> ---
> > (In reply to Jan Hubicka from comment #13)
> > > > So .. for promotion of target expression temporaries to frame vars, one of:
> > > >  - a) we need to find a different way to name them
> > > I think we can just count number of fields within a given frame type?
> > 
> > yeah I made a hack that did this (and resolves this PR) but I'd think we can
> > find something neater, I'd like to c++-ify the sources some more, and create a
> > class to manage the frame... ( maybe for GCC14 now ).
> Thanks!
> Since this is really part of the ABI, I wonder why it is not covered by
> the IA-64 C++ ABI?  Was it ever updated for coroutines?


There is a coroutines ABI agreed between the "vendors" I have editorship of the
draft - and I suppose it needs to be included in the Itanium (and MSVC) ABI
docs.

However, the only part of the ABI that is described by this is the type-erased
interface (there is never a need for the body of the coroutine, and detailed
frame layout) to be interpreted by a coroutine body from a second compiler. **
other than the portion of the frame that contains the pointers to resume and
destroy **.

The actual code is TU-local (and must remain that way even under LTO)

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[cvs-commit at gcc dot gnu.org]

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

--- Comment #17 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:fc4cde2e6aa4d6ebdf7f70b7b4359fb59a1915ae

commit r13-6964-gfc4cde2e6aa4d6ebdf7f70b7b4359fb59a1915ae
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Thu Mar 30 13:14:23 2023 +0530

    c++,coroutines: Stabilize names of promoted slot vars [PR101118].

    When we need to 'promote' a value (i.e. store it in the coroutine frame) it
    is given a frame entry name.  This was based on the DECL_UID for slot vars.
    However, when LTO is used, the names from multiple TUs become visible at
the
    same time, and the DECL_UIDs usually differ between units.  This leads to a
    "ODR mismatch" warning for the frame type.

    The fix here is to use the current promoted temporaries count to produce
    the name, this is stable between TUs and computed per coroutine.

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

            PR c++/101118

    gcc/cp/ChangeLog:

            * coroutines.cc (flatten_await_stmt): Use the current count of
            promoted temporaries to build a unique name for the frame entries.

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[cvs-commit at gcc dot gnu.org]

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

--- Comment #18 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:b7e75cdb218f25708b8b1aa3f4b138d88187491f

commit r12-9501-gb7e75cdb218f25708b8b1aa3f4b138d88187491f
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Thu Mar 30 13:14:23 2023 +0530

    c++,coroutines: Stabilize names of promoted slot vars [PR101118].

    When we need to 'promote' a value (i.e. store it in the coroutine frame) it
    is given a frame entry name.  This was based on the DECL_UID for slot vars.
    However, when LTO is used, the names from multiple TUs become visible at
the
    same time, and the DECL_UIDs usually differ between units.  This leads to a
    "ODR mismatch" warning for the frame type.

    The fix here is to use the current promoted temporaries count to produce
    the name, this is stable between TUs and computed per coroutine.

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

            PR c++/101118

    gcc/cp/ChangeLog:

            * coroutines.cc (flatten_await_stmt): Use the current count of
            promoted temporaries to build a unique name for the frame entries.

    (cherry picked from commit fc4cde2e6aa4d6ebdf7f70b7b4359fb59a1915ae)

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:72f004746d87f01e5e3872af3214e3fa1b48dfa8

commit r11-10788-g72f004746d87f01e5e3872af3214e3fa1b48dfa8
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Thu Mar 30 13:14:23 2023 +0530

    c++,coroutines: Stabilize names of promoted slot vars [PR101118].

    When we need to 'promote' a value (i.e. store it in the coroutine frame) it
    is given a frame entry name.  This was based on the DECL_UID for slot vars.
    However, when LTO is used, the names from multiple TUs become visible at
the
    same time, and the DECL_UIDs usually differ between units.  This leads to a
    "ODR mismatch" warning for the frame type.

    The fix here is to use the current promoted temporaries count to produce
    the name, this is stable between TUs and computed per coroutine.

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

            PR c++/101118

    gcc/cp/ChangeLog:

            * coroutines.cc (flatten_await_stmt): Use the current count of
            promoted temporaries to build a unique name for the frame entries.

    (cherry picked from commit fc4cde2e6aa4d6ebdf7f70b7b4359fb59a1915ae)

[Bug c++/101118] coroutines: unexpected ODR warning for coroutine frame type in LTO builds

[iains at gcc dot gnu.org]

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

--- Comment #20 from Iain Sandoe <iains at gcc dot gnu.org> ---
leaving open, I think this might also be needed on 10.x