From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp002.apm-internet.net (smtp002.apm-internet.net [85.119.248.221]) by sourceware.org (Postfix) with ESMTPS id AE0263858D39 for ; Mon, 27 Mar 2023 07:32:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AE0263858D39 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sandoe.co.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=sandoe.co.uk Received: (qmail 47259 invoked from network); 27 Mar 2023 07:32:37 -0000 X-APM-Out-ID: 16799023554724 X-APM-Authkey: 257869/1(257869/1) 8 Received: from unknown (HELO smtpclient.apple) (103.2.133.137) by smtp002.apm-internet.net with SMTP; 27 Mar 2023 07:32:37 -0000 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.2\)) Subject: Re: [PATCH] c++, coroutines: Stabilize names of promoted slot vars [PR101118]. From: Iain Sandoe In-Reply-To: Date: Mon, 27 Mar 2023 13:02:34 +0530 Cc: Jan Hubicka , GCC Patches , Jason Merrill Content-Transfer-Encoding: quoted-printable Message-Id: <5EE43D2E-D258-42AF-8498-523930DE7345@sandoe.co.uk> References: <20230326165447.43628-1-iain@sandoe.co.uk> <74E48D71-C3B8-412C-A819-C5D54719CDC4@sandoe.co.uk> To: Richard Biener X-Mailer: Apple Mail (2.3696.120.41.1.2) X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_COUK,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Richard > On 27 Mar 2023, at 12:48, Richard Biener = wrote: >=20 > On Mon, Mar 27, 2023 at 8:58=E2=80=AFAM Iain Sandoe = wrote: >>=20 >> Hi Richard, >> (I=E2=80=99m away from my usual infrastructure, so responses could be = slow and testing things >> could take a while). >>=20 >>> On 27 Mar 2023, at 12:10, Richard Biener = wrote: >>>=20 >>> On Sun, Mar 26, 2023 at 6:55=E2=80=AFPM Iain Sandoe via Gcc-patches >>> wrote: >>>>=20 >>>> Tested on x86_64-darwin21, x86-64-linux-gnu >>>> OK for trunk? >>>> Iain >>>>=20 >>>> 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. >>>>=20 >>>> 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). >>>=20 >>> I don't see how this avoids clashes across TUs? But are those = VAR_DECLs not >>> local anyway? >>=20 >> The reported ODR issue is in the frame type (which is a structure) = =E2=80=94 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). >=20 > 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 =E2=80=A6. >>> I suppose -Wodr diagnostics for DECL_ARTIFICIAL vars are a bit on = the >>> edge as well ... >>=20 >> These promoted vars get DECL_VALUE_EXPRs (and as noted above a name = to >> assist in debugging) tying them to the frame entry, >>=20 >> .. although I do agree that reporting warnings for compiler-internal = stuff is definitely >> on the edge (ISTR seeing maybe unused reports against such too). >=20 > 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) =E2=80=A6 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). =20 hmm=E2=80=A6 now I=E2=80=99m not sure that this patch is the right fix = .. I=E2=80=99d welcome Jason=E2=80=99s 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? >=20 > 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=E2=80=99s the right fix for the regression, I guess). Iain >=20 > Richard. >=20 >>=20 >> Iain >>=20 >>=20 >>>=20 >>> Richard. >>>=20 >>>> Signed-off-by: Iain Sandoe >>>>=20 >>>> PR c++/101118 >>>>=20 >>>> gcc/cp/ChangeLog: >>>>=20 >>>> * 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(-) >>>>=20 >>>> 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; >>>> }; >>>>=20 >>>> +/* 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 =3D 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 *promoted, >>>> tree init =3D t; >>>> temps_used->add (init); >>>> tree var_type =3D TREE_TYPE (init); >>>> - char *buf =3D xasprintf ("D.%d", DECL_UID (TREE_OPERAND = (init, 0))); >>>> + char *buf =3D xasprintf ("T%03u", tmpno++); >>>> tree var =3D build_lang_decl (VAR_DECL, get_identifier = (buf), var_type); >>>> DECL_ARTIFICIAL (var) =3D true; >>>> free (buf); >>>> @@ -4374,6 +4379,7 @@ morph_fn_to_coro (tree orig, tree *resumer, = tree *destroyer) >>>> { >>>> gcc_checking_assert (orig && TREE_CODE (orig) =3D=3D = FUNCTION_DECL); >>>>=20 >>>> + tmpno =3D 0; >>>> *resumer =3D error_mark_node; >>>> *destroyer =3D error_mark_node; >>>> if (!coro_function_valid_p (orig)) >>>> -- >>>> 2.37.1 (Apple Git-137.1) >>=20