From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sender4-pp-o91.zoho.com (sender4-pp-o91.zoho.com [136.143.188.91]) by sourceware.org (Postfix) with ESMTPS id 549B83858D28; Sat, 4 Dec 2021 04:37:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 549B83858D28 ARC-Seal: i=1; a=rsa-sha256; t=1638592653; cv=none; d=zohomail.com; s=zohoarc; b=GQP89GMiaet1yibTD4hzkb500X222W4CwdlzIiWqgSrb21Rfbz5uVNAjYHl7tPPo5XbRnq9QSeaRm2Ai235YwY2tzm75ljjIuJw/jDK5oz1EfUJ9kADxvcIIhKCygAEOIM8vpbhZsVtCbvOVkW3ssjL3AsBsZGP2QbAVsCTaY08= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1638592653; h=Content-Type:Content-Transfer-Encoding:Date:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:To; bh=MeO9Y/FgIRGeb79y5hjdjUzh51JKBEAIBeGKLzuSV0c=; b=OGQvvbKierGDAk6wwJ4Jv1U8YqK/uK/tbyOLbAP9v5zGNE59agSDdORpxciAfSZSAHQNPxfsDUZUGkSpmjjvMfkt+dqtpWmVWWzwg34goNmYgjrWNep68lNyw+UgdaqSqtfnDDvyK71XnMa99WxmntNRu8s7prfNjgfWHYrq7S0= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=zoho.com; spf=pass smtp.mailfrom=bouanto@zoho.com; dmarc=pass header.from= Received: from [192.168.1.174] (38.87.11.6 [38.87.11.6]) by mx.zohomail.com with SMTPS id 1638592652084168.46430391057106; Fri, 3 Dec 2021 20:37:32 -0800 (PST) Message-ID: Subject: Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067] From: Antoni Boucher To: David Malcolm , jit@gcc.gnu.org, gcc-patches@gcc.gnu.org Date: Fri, 03 Dec 2021 23:37:30 -0500 In-Reply-To: <8e5c4b562f6c232e592c0a585777cf325321bf54.camel@zoho.com> References: <212418d2a4b81cfff1e125ddb12d0b4d10d8404a.camel@zoho.com> <3abfd5cf6546892f25a01c1772121c848487a196.camel@zoho.com> <45c7ea23708a4ff029c0d8711f809230e90ea057.camel@redhat.com> <8e5c4b562f6c232e592c0a585777cf325321bf54.camel@zoho.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.1 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-ZohoMailClient: External X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, BODY_8BITS, 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.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 04 Dec 2021 04:37:38 -0000 David: PING In case you missed it, that's the last patch left to review for now. Le dimanche 21 novembre 2021 =C3=A0 16:44 -0500, Antoni Boucher a =C3=A9cri= t=C2=A0: > Thanks for the review! > I updated the patch. >=20 > See notes below. >=20 > Le samedi 20 novembre 2021 =C3=A0 13:50 -0500, David Malcolm a =C3=A9crit= =C2=A0: > > On Sat, 2021-11-20 at 11:27 -0500, Antoni Boucher wrote: > > > Hi. > > > Here's the updated patch. > > > Thanks for the review! > >=20 > > Thanks for the updated patch... > >=20 > > >=20 > > > Le jeudi 20 mai 2021 =C3=A0 16:24 -0400, David Malcolm a =C3=A9crit= =C2=A0: > > > > On Mon, 2021-05-17 at 21:02 -0400, Antoni Boucher via Jit > > > > wrote: > > > > > Hello. > > > > > This patch fixes the issue with using atomic builtins in > > > > > libgccjit. > > > > > Thanks to review it. > > > >=20 > > > > [...snip...] > > > > =C2=A0 > > > > > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit- > > > > > recording.c > > > > > index 117ff70114c..de876ff9fa6 100644 > > > > > --- a/gcc/jit/jit-recording.c > > > > > +++ b/gcc/jit/jit-recording.c > > > > > @@ -2598,8 +2598,18 @@ > > > > > recording::memento_of_get_pointer::accepts_writes_from (type > > > > > *rtype) > > > > > =C2=A0=C2=A0=C2=A0=C2=A0 return false; > > > > > =C2=A0 > > > > > =C2=A0=C2=A0 /* It's OK to assign to a (const T *) from a (T *).= =C2=A0 */ > > > > > -=C2=A0 return m_other_type->unqualified () > > > > > -=C2=A0=C2=A0=C2=A0 ->accepts_writes_from (rtype_points_to); > > > > > +=C2=A0 if (m_other_type->unqualified () > > > > > +=C2=A0=C2=A0=C2=A0 ->accepts_writes_from (rtype_points_to)) { > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return true; > > > > > +=C2=A0 } > > > > > + > > > > > +=C2=A0 /* It's OK to assign to a (volatile const T *) from a > > > > > (volatile > > > > > const T *). */ > > > > > +=C2=A0 if (m_other_type->unqualified ()->unqualified () > > > > > +=C2=A0=C2=A0=C2=A0 ->accepts_writes_from (rtype_points_to->unqua= lified ())) > > > > > { > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return true; > > > > > +=C2=A0 } > > > >=20 > > > > Presumably you need this to get the atomic builtins working? > > > >=20 > > > > If I'm reading the above correctly, the new test doesn't > > > > distinguish > > > > between the 3 different kinds of qualifiers (aligned, volatile, > > > > and > > > > const), it merely tries to strip some of them off. > > > >=20 > > > > It's not valid to e.g.=C2=A0assign to a (aligned T *) from a (const > > > > T > > > > *). > > > >=20 > > > > Maybe we need an internal enum to discriminate between > > > > different > > > > subclasses of decorated_type? > >=20 > > I'm still concerned about this case, my reading of the updated > > patch > > is > > that this case is still not quite correctly handled (see notes > > below). > > I don't think we currently have test coverage for assignment to > > e.g. > > (aligned T *) from a (const T*); I feel that it should be an error, > > without an explicit cast. > >=20 > > Please can you add a testcase for this? >=20 > Done. >=20 > >=20 > > If you want to go the extra mile, given that this is code created > > through an API, you could have a testcase that iterates through all > > possible combinations of qualifiers (for both source and > > destination > > pointer), and verifies that libgccjit at least doesn't crash on > > them > > (and hopefully does the right thing on each one)=C2=A0 :/ > >=20 > > (perhaps doing each one in a different gcc_jit_context) > >=20 > > Might be nice to update test-fuzzer.c for the new qualifiers; I > > don't > > think I've touched it in a long time. >=20 > Done. >=20 > >=20 > > [...snip...] > >=20 > > > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > > > index 4a994fe7094..60aaba2a246 100644 > > > --- a/gcc/jit/jit-recording.h > > > +++ b/gcc/jit/jit-recording.h > > > @@ -545,6 +545,8 @@ public: > > > =C2=A0=C2=A0 virtual bool is_float () const =3D 0; > > > =C2=A0=C2=A0 virtual bool is_bool () const =3D 0; > > > =C2=A0=C2=A0 virtual type *is_pointer () =3D 0; > > > +=C2=A0 virtual type *is_volatile () { return NULL; } > > > +=C2=A0 virtual type *is_const () { return NULL; } > > > =C2=A0=C2=A0 virtual type *is_array () =3D 0; > > > =C2=A0=C2=A0 virtual struct_ *is_struct () { return NULL; } > > > =C2=A0=C2=A0 virtual bool is_void () const { return false; } > > > @@ -687,6 +689,13 @@ public: > > > =C2=A0=C2=A0 /* Strip off the "const", giving the underlying type.=C2= =A0 */ > > > =C2=A0=C2=A0 type *unqualified () FINAL OVERRIDE { return m_other_typ= e; } > > > =C2=A0 > > > +=C2=A0 virtual bool is_same_type_as (type *other) > > > +=C2=A0 { > > > +=C2=A0=C2=A0=C2=A0 return m_other_type->is_same_type_as (other->is_c= onst ()); > > > +=C2=A0 } > >=20 > > What happens if other_is_const () returns NULL, and > > =C2=A0 m_other_type->is_same_type_as () > > tries to call a vfunc on it... >=20 > Fixed. >=20 > >=20 > > > + > > > +=C2=A0 virtual type *is_const () { return m_other_type; } > > > + > > > =C2=A0=C2=A0 void replay_into (replayer *) FINAL OVERRIDE; > > > =C2=A0 > > > =C2=A0private: > > > @@ -701,9 +710,16 @@ public: > > > =C2=A0=C2=A0 memento_of_get_volatile (type *other_type) > > > =C2=A0=C2=A0 : decorated_type (other_type) {} > > > =C2=A0 > > > +=C2=A0 virtual bool is_same_type_as (type *other) > > > +=C2=A0 { > > > +=C2=A0=C2=A0=C2=A0 return m_other_type->is_same_type_as (other->is_v= olatile > > > ()); > > > +=C2=A0 } > >=20 > > ...with similar considerations here. > >=20 > > i.e. is it possible for the user to create combinations of > > qualifiers > > that lead to a vfunc call with NULL "this" (and thus a segfault?) > >=20 > > > + > > > =C2=A0=C2=A0 /* Strip off the "volatile", giving the underlying type.= =C2=A0 */ > > > =C2=A0=C2=A0 type *unqualified () FINAL OVERRIDE { return m_other_typ= e; } > > > =C2=A0 > > > +=C2=A0 virtual type *is_volatile () { return m_other_type; } > > > + > > > =C2=A0=C2=A0 void replay_into (replayer *) FINAL OVERRIDE; > > > =C2=A0 > >=20 > > Hope this is constructive > > Dave > >=20 >=20