From: Iain Sandoe <iain@sandoe.co.uk>
To: Richard Biener <richard.guenther@gmail.com>
Cc: Jan Hubicka <hubicka@ucw.cz>,
GCC Patches <gcc-patches@gcc.gnu.org>,
Jason Merrill <jason@redhat.com>
Subject: Re: [PATCH] c++, coroutines: Stabilize names of promoted slot vars [PR101118].
Date: Mon, 27 Mar 2023 13:02:34 +0530 [thread overview]
Message-ID: <5EE43D2E-D258-42AF-8498-523930DE7345@sandoe.co.uk> (raw)
In-Reply-To: <CAFiYyc1XKgvnrPuUMoTGkSX=qyRgTN8QRmjjg36YdeMMjiQqFw@mail.gmail.com>
Hi Richard
> On 27 Mar 2023, at 12:48, Richard Biener <richard.guenther@gmail.com> wrote:
>
> On Mon, Mar 27, 2023 at 8:58 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>>
>> Hi Richard,
>> (I’m away from my usual infrastructure, so responses could be slow and testing things
>> could take a while).
>>
>>> On 27 Mar 2023, at 12:10, Richard Biener <richard.guenther@gmail.com> wrote:
>>>
>>> On Sun, Mar 26, 2023 at 6:55 PM Iain Sandoe via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> Tested on x86_64-darwin21, x86-64-linux-gnu
>>>> OK for trunk?
>>>> Iain
>>>>
>>>> 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 a counter instead of the DECL_UID which makes a name
>>>> that is stable between TUs for each frame layout (one per coroutine func).
>>>
>>> I don't see how this avoids clashes across TUs? But are those VAR_DECLs not
>>> local anyway?
>>
>> The reported ODR issue is in the frame type (which is a structure) — it sees two
>> frame layouts with the same types for each field but a different name for the entries
>> that came from the promotion of the slot var (because I used the DECL_UID to generate
>> the field name).
>
> Ah, I see. If it's from the same TU then why do we generate two frame
> layouts with
> the same type in the first place?
They are different TUs.
The frames are generated for coroutine types instantiated from templates
declared in a (boost) header.
(I do not see anything in the testcase header making stuff explicitily inline)
AFAIR the rules this is OK ODR-use-wise ….
>>> I suppose -Wodr diagnostics for DECL_ARTIFICIAL vars are a bit on the
>>> edge as well ...
>>
>> These promoted vars get DECL_VALUE_EXPRs (and as noted above a name to
>> assist in debugging) tying them to the frame entry,
>>
>> .. although I do agree that reporting warnings for compiler-internal stuff is definitely
>> on the edge (ISTR seeing maybe unused reports against such too).
>
> If the two layouts are used to access the same objects you might run
> into TBAA issues.
> But making them appear the same but still separate types won't help that issue
> (but -flto will "fix" it for you then)
… but I wonder if I should be preventing LTO from doing this (perhaps my frame
type needs a uniquing addition, and then we would not care about the differing).
hmm… now I’m not sure that this patch is the right fix .. I’d welcome Jason’s take
on this.
>> Not sure if we have an easy way to tell that the frame type is an internal one tho.
>> Perhaps that needs a DECL_ARTIFICAL - but would that not make it unavailable
>> for debug?
>
> We have TYPE_ARTIFICIAL, artificial-ness and no-debug are generally separate
> (DECL_IGNORED for decls, but I don't think we have anything for types here).
OK .. I can see about adding that too - but probably not for 13.0 (unless that’s the
right fix for the regression, I guess).
Iain
>
> Richard.
>
>>
>> Iain
>>
>>
>>>
>>> Richard.
>>>
>>>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>>>>
>>>> PR c++/101118
>>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>> * coroutines.cc: Add counter for promoted slot vars.
>>>> (flatten_await_stmt): Use slot vars counter instead of DECL_UID
>>>> to generate the frame entry name for promoted target expression
>>>> slot variables.
>>>> (morph_fn_to_coro): Reset the slot vars counter at the start of
>>>> each coroutine function.
>>>> ---
>>>> gcc/cp/coroutines.cc | 8 +++++++-
>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>>> index a2189e43db8..359a5bf46ff 100644
>>>> --- a/gcc/cp/coroutines.cc
>>>> +++ b/gcc/cp/coroutines.cc
>>>> @@ -2726,6 +2726,11 @@ struct var_nest_node
>>>> var_nest_node *else_cl;
>>>> };
>>>>
>>>> +/* This is used to make a stable, but unique-per-function, sequence number for
>>>> + each TARGET_EXPR slot variable that we 'promote' to a frame entry. It needs
>>>> + to be stable because the frame type is visible to LTO ODR checking. */
>>>> +static unsigned tmpno = 0;
>>>> +
>>>> /* This is called for single statements from the co-await statement walker.
>>>> It checks to see if the statement contains any initializers for awaitables
>>>> and if any of these capture items by reference. */
>>>> @@ -2889,7 +2894,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);
>>>> @@ -4374,6 +4379,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))
>>>> --
>>>> 2.37.1 (Apple Git-137.1)
>>
next prev parent reply other threads:[~2023-03-27 7:32 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-26 16:54 [PATCH] c++,coroutines: " Iain Sandoe
2023-03-27 6:40 ` [PATCH] c++, coroutines: " Richard Biener
2023-03-27 6:58 ` Iain Sandoe
2023-03-27 7:18 ` Richard Biener
2023-03-27 7:32 ` Iain Sandoe [this message]
2023-03-28 6:28 ` Richard Biener
2023-03-28 6:57 ` Iain Sandoe
2023-03-28 11:16 ` Iain Sandoe
2023-03-28 11:27 ` Richard Biener
2023-03-29 19:23 ` [PATCH] c++,coroutines: " Jason Merrill
2023-03-30 7:53 ` Iain Sandoe
2023-03-30 20:50 ` Jason Merrill
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5EE43D2E-D258-42AF-8498-523930DE7345@sandoe.co.uk \
--to=iain@sandoe.co.uk \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=jason@redhat.com \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).