From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com [IPv6:2a00:1450:4864:20::22e]) by sourceware.org (Postfix) with ESMTPS id 2CED43858D39 for ; Tue, 28 Mar 2023 06:29:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2CED43858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lj1-x22e.google.com with SMTP id s20so11384670ljp.1 for ; Mon, 27 Mar 2023 23:29:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1679984949; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=wQ7SaYWehL5eWNRRZJ3DOHvUZZKPNQlNUCYGOlb71+w=; b=aS/8rhANawVblHYE9tHN8QyZPA5kATvTeQ+WAlHE1CBbQHkqitn8pe7uo2sEf+vY7Z oAy3JZnvVvbXfFZhw/Qqe4NO+x7GjI2CzAo7E+/WBKWm3k4wd8zplVq/o2iNACBn5q48 4U9IqO5ILWnufqkBsymIIbPngyMFSNYrpmj/IsUUwBCPeGNmqD2x1/gMsSARg8wfwYW2 Jx8stAJPWfdN4qYl4s4GAPiGnUE3XXRNqiVPyhM7ZA5mW4hxY1x/w9CCVV2TBu16nbp1 xLWh3r3dNMV/yNtjRAwveXWaxSA+kP6Nv7JqffVhUfIp21CpT/IS15i6owDnXTmucAHF LsGQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679984949; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=wQ7SaYWehL5eWNRRZJ3DOHvUZZKPNQlNUCYGOlb71+w=; b=6yClCRQgsuW7gvsXL4xHiPzClvBOj/sWVx6s/pkqcjduRKgXXNT30geCtc8rPFM3hC ISRU+6DD/Vfii9lT0AH+CaVKlxF2kFOJIsIyEAvEjPK4dMtdFFrkqIZF/ivopT5FzR1K BOvovXFYwsPU5SJd56lVcW2/sMODfSmdyPOIEOPNLqsOPiVwi4/HtptznF5mSYcd9lfl wihglLFP4g4iiMnzdOATaVPT4IPYtUcEf1B82qCWs8wpC+nDLzNd2IT5I/1S89VU69tV yLRelG9hP6GgX0H1Yp6Sq0TfHkpmBiq6QjXtixlpxOQf2XBUqUKBmQSCQ/B6IkL/HVWF SPfw== X-Gm-Message-State: AAQBX9cRJqOF8aqClOTb/g1C1WvakRieNd4ZWkN/I4Ou43F1hxckzyXz nSCTevVNkKq41wXMltsp1ge9BLjUUNzGztn6fF0= X-Google-Smtp-Source: AKy350Zk/PP9Er3OCsery2QNHNJE111F1aZoackZu8RcrcDekrwlvzq1CfOvj4UAgvJCoalMSbfko0gtHMpAJBVYhW4= X-Received: by 2002:a2e:9792:0:b0:2a3:fc8:711b with SMTP id y18-20020a2e9792000000b002a30fc8711bmr4309391lji.10.1679984949442; Mon, 27 Mar 2023 23:29:09 -0700 (PDT) MIME-Version: 1.0 References: <20230326165447.43628-1-iain@sandoe.co.uk> <74E48D71-C3B8-412C-A819-C5D54719CDC4@sandoe.co.uk> <5EE43D2E-D258-42AF-8498-523930DE7345@sandoe.co.uk> In-Reply-To: <5EE43D2E-D258-42AF-8498-523930DE7345@sandoe.co.uk> From: Richard Biener Date: Tue, 28 Mar 2023 08:28:29 +0200 Message-ID: Subject: Re: [PATCH] c++, coroutines: Stabilize names of promoted slot vars [PR101118]. To: Iain Sandoe Cc: Jan Hubicka , GCC Patches , Jason Merrill Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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: On Mon, Mar 27, 2023 at 9:32=E2=80=AFAM Iain Sandoe wro= te: > > Hi Richard > > > On 27 Mar 2023, at 12:48, Richard Biener w= rote: > > > > On Mon, Mar 27, 2023 at 8:58=E2=80=AFAM Iain Sandoe = wrote: > >> > >> Hi Richard, > >> (I=E2=80=99m 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 = wrote: > >>> > >>> On Sun, Mar 26, 2023 at 6:55=E2=80=AFPM Iain Sandoe via Gcc-patches > >>> 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 fr= ame) it > >>>> is given a frame entry name. This was based on the DECL_UID for slo= t vars. > >>>> However, when LTO is used, the names from multiple TUs become visibl= e at the > >>>> same time, and the DECL_UIDs usually differ between units. This lea= ds 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_DE= CLs not > >>> local anyway? > >> > >> 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). > > > > 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 in= line) > AFAIR the rules this is OK ODR-use-wise =E2=80=A6. And I only now see the =3D 0 assignment to the static so I suppose the vars= will indeed be consistent across compilations. Still you are building a VAR_DECL here, not a FIELD_DECL. I also wonder why you cannot simply use the original name of the TARGET_EXPR decl but have to invent a new one? If the TARGET_EXPR decl was nameless so can the new one be? > >>> 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 tha= t issue > > (but -flto will "fix" it for you then) > > =E2=80=A6 but I wonder if I should be preventing LTO from doing this (per= haps my frame > type needs a uniquing addition, and then we would not care about the diff= ering). > > 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 inte= rnal one tho. > >> Perhaps that needs a DECL_ARTIFICAL - but would that not make it unava= ilable > >> for debug? > > > > We have TYPE_ARTIFICIAL, artificial-ness and no-debug are generally sep= arate > > (DECL_IGNORED for decls, but I don't think we have anything for types h= ere). > > 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 > > > > > Richard. > > > >> > >> Iain > >> > >> > >>> > >>> Richard. > >>> > >>>> Signed-off-by: Iain Sandoe > >>>> > >>>> PR c++/101118 > >>>> > >>>> gcc/cp/ChangeLog: > >>>> > >>>> * coroutines.cc: Add counter for promoted slot vars. > >>>> (flatten_await_stmt): Use slot vars counter instead of DECL_UI= D > >>>> to generate the frame entry name for promoted target expressio= n > >>>> slot variables. > >>>> (morph_fn_to_coro): Reset the slot vars counter at the start o= f > >>>> 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 entr= y. It needs > >>>> + to be stable because the frame type is visible to LTO ODR checki= ng. */ > >>>> +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 aw= aitables > >>>> 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 (i= nit, 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, tr= ee *destroyer) > >>>> { > >>>> gcc_checking_assert (orig && TREE_CODE (orig) =3D=3D FUNCTION_DECL)= ; > >>>> > >>>> + 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) > >> >