From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-relay-2.sys.kth.se (smtp-relay-2.sys.kth.se [IPv6:2001:6b0:1:1200:250:56ff:fead:963e]) by sourceware.org (Postfix) with ESMTPS id A46A13858D28 for ; Mon, 6 Dec 2021 10:51:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A46A13858D28 Received: from exdb2.ug.kth.se (exdb2.ug.kth.se [192.168.32.57]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp-relay-2.sys.kth.se (Postfix) with ESMTPS id 4J70b92DjTzPMdp; Mon, 6 Dec 2021 11:51:01 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp-relay-2.sys.kth.se 4J70b92DjTzPMdp Received: from exdb6.ug.kth.se (192.168.32.61) by exdb2.ug.kth.se (192.168.32.57) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.14; Mon, 6 Dec 2021 11:51:01 +0100 Received: from exdb6.ug.kth.se ([192.168.32.61]) by exdb6.ug.kth.se ([192.168.32.61]) with mapi id 15.02.0986.014; Mon, 6 Dec 2021 11:51:01 +0100 From: Petter Tomner To: David Malcolm , Antoni Boucher , "jit@gcc.gnu.org" Subject: SV: SV: SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089] Thread-Topic: SV: SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089] Thread-Index: AQHX4A5BvSF/jycHYEaAgABQ/pvBR6wQ4v/4gApg9oCAAM6UVoACPboAgAcNsj4= Date: Mon, 6 Dec 2021 10:51:00 +0000 Message-ID: <19d99dcab83a4205acc7374731a69697@kth.se> References: <7d6762623f658712a4b802cc3d6524c106affa2b.camel@zoho.com> <62c178b5bb0ab004def3c7869da2b03846e5134c.camel@zoho.com> , , <4287fbb8dd3644849ea57aaf070de870@kth.se>, In-Reply-To: Accept-Language: sv-SE, en-US Content-Language: sv-SE X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [192.168.32.250] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, 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: Mon, 06 Dec 2021 10:51:04 -0000 Hi! Thanks for the review. An updated patch has been submitted to the list. https://gcc.gnu.org/pipermail/jit/2021q4/001409.html Regards, Petter Fr=E5n: David Malcolm Skickat: den 2 december 2021 01:07 Till: Petter Tomner; Antoni Boucher; jit@gcc.gnu.org =C4mne: Re: SV: SV: [PATCH] libgccjit: Add function to set the initial valu= e of a global variable [PR96089] =A0 =20 On Tue, 2021-11-30 at 14:37 +0000, Petter Tomner wrote: > Hi! >=20 > > It sounds like I should review Petter's patch, but obviously I want > > Antoni's input to be sure that it works for the rustc backend. >=20 > He asked you to review my patch and wrote that he would check if it > worked=20 > for his work when back from a vacation, in a follow up mail. Maybe you > missed it: > https://gcc.gnu.org/pipermail/jit/2021q4/001394.html Thanks.=A0 What I mean is that I want to be sure that this approach will work for the rustc backend and gives all the expressiveness that's needed - I'm hoping that by release, GCC 12's libgccjit will have all of the functionality needed by the rustc backend. >=20 > I tidied up the patch and mailed it for review to the list: > https://gcc.gnu.org/pipermail/jit/2021q4/001399.html >=20 > but it is about the same code as you reviewed so your points still > appy, > except that I fixed some docs, whitespace etc and split up the entry > points and > made them compatible with how Antoni used them. I.e. the user can=20 > leave out the fields arg and just have the values being applied in=20 > struct field definition order. >=20 > > It's a size_t in the docs, but an "int" in the header.=A0 Let's make it > > a > > size_t. >=20 > Ye need to sync those. Are you sure you want size_t though? Neither > array or struct types can have more elements/fields than int due to the > entrypoints for making those have int args (which were the reason I=20 > changed to int, forgot to change the docs).=20 I think that was a historical error on my part, and I should have used size_t.=A0 size_t is shorter to type than "unsigned"; it seems that the type's signedness should be unsigned given that < 0 makes no sense. >=20 > > Reading the docs, it seems that this function seems to be doing 3 > > different things. > >=20 > > Would it be better to have 3 different API entrypoints? > >=20 > > Perhaps something like: > >=20 > > extern gcc_jit_rvalue * > > gcc_jit_context_new_struct_constructor (gcc_jit_context *ctxt, > ... > > extern gcc_jit_rvalue * > > gcc_jit_context_new_array_constructor (gcc_jit_context *ctxt, > ... > > extern gcc_jit_rvalue * > > gcc_jit_context_new_union_constructor (gcc_jit_context *ctxt, > >=20 > > Would that be better?=A0 I'm not sure, but I think it's clearer. >=20 > Funnely enough the exact signature I chose. But with > gcc_jit_type instead of gcc_jit_struct. >=20 > Ye it is way clearer. One entrypoint made it too crammed and > the documentation got very confusing. >=20 > > They could potentially share the memento class internally, or be > > split > > out. >=20 > The code is very similair for arrays and UNION are kinda RECORDs with > no offsets, so I guess it makes sense to keep the internal class > together. Whichever is easier from an implementation point of view (it's much easier to change the implementation, compared to the user-facing API). >=20 > > Would "initializer" be better than "constructor"?=A0 (I'm not sure) >=20 > They can be used for assigning to variables at any time, so > initializer would probably be confusing, if people think in C or > C++ terms. Fair enough. >=20 > > > If I remember correctly there need to be alot of folding to not > > > segfault deeper into gcc on > > > expressions that are not one literal, for e.g. pointer arithmetic. > >=20 > > Can we fail to fold to a const?=A0 If so, should this function return > > NULL_TREE instead? (and the callers be adjusted accordingly) >=20 > I could not construct something that failed to fold=20 > "enough" so that varasm.c coulnd't handle it and that also was=20 > TREE_CONSTANT. >=20 > playback::context::global_set_init_rvalue searches for variables > without DECL_INITIAL set (which would make downstream code > unhappy) and checks that the rvalue is also TREE_CONSTANT. >=20 > fold_const_var () just lets things it can't fold through, like fold () > does. > I guess returning NULL_TREE on failing to fold would add alot of > checking everywhere it is used. My goal here is that it shouldn't be possible to generate a crash inside libgccjit via API calls from client code; as noted in=A0 =A0 https://gcc.gnu.org/onlinedocs/jit/internals/index.html#design-notes we should add additional checking to gracefully reject bad inputs rather than crasing. The checking ideally should be within the libgccjit API entrypoints in libgccjit.c, since this is as close as possible to the error; failing that, a good place is within recording::context::validate () in jit-recording.c.=A0 That said, if we can only detect the problem when a tree is generated during playback, then we should at least fail gracefully at that point, rather than crashing. On re-reading what you wrote above, you needed to add a lot of folding to prevent a crash, but it sounds like you might have already added enough, and fixed things by now.=A0 So hopefully the folding code is good enough as-is, and we can revisit this if someone manages to generate crashing inputs. >=20 > > Two fields called "c" here, FWIW. > > But given that these are different field instances, it should > > complain > > about that, also. >=20 > Ye the name of that "c" field doesn't really matter since the the size > check is before the "is this field object one of the field objects that > created > the type"-check. That is checked in: > test-error-ctor-struct-wrong-field-name.c >=20 > I'll fix the other comments too, and prepare a new patch. Thanks!=A0 Please can you crosspost libgccjit patches to both the jit and gcc-patches mailing lists. Dave [...snip...] =