From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sender3-of-o90.zoho.com (sender3-of-o90.zoho.com [136.143.184.90]) by sourceware.org (Postfix) with ESMTPS id 1D2513858428 for ; Thu, 2 Dec 2021 22:58:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1D2513858428 ARC-Seal: i=1; a=rsa-sha256; t=1638485908; cv=none; d=zohomail.com; s=zohoarc; b=cr/clsFkPBluf4R2Eqd3Fb1HoLF5oI/W6v77uf5zQtbw/+7CnPQeTUzm85TNkHUm0PiRYRU25NBfhEUa1LeAZK0n3UR1tSQ93+3cq3qUHJ7MIT7y7+qtmUc9JjzFmmsJdCHHaRLEs4xcQcOBUtbyrsKBH33KVo6I2sq73+7LM9w= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1638485908; h=Content-Type:Content-Transfer-Encoding:Date:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:To; bh=MMlrn4k+EpaSXnW2aroEZr1r5syClkwtSq4DufjVIps=; b=ndDY6iefjZ9jLZ13YV1g90IO6e2sdchXlGSljpo2bZMDThHJmtruSl7sSYBy+KdlTOl+ymBHDV9ePebk2OWBNVnZYsnpA4JTMiY0O0JQPNWtfzn8NTBd9es9iIIL1cZWacERRWtmHrfViOTtDgV7W2qqoHd+fRCsVCtwSD1pqC4= 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 1638485905319650.6122370822708; Thu, 2 Dec 2021 14:58:25 -0800 (PST) Message-ID: <6fb794c3f9204623a9a4da314dc2965ec502123b.camel@zoho.com> Subject: Re: SV: SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089] From: Antoni Boucher To: David Malcolm , Petter Tomner , "jit@gcc.gnu.org" Date: Thu, 02 Dec 2021 17:58:22 -0500 In-Reply-To: References: <7d6762623f658712a4b802cc3d6524c106affa2b.camel@zoho.com> <62c178b5bb0ab004def3c7869da2b03846e5134c.camel@zoho.com> , , <4287fbb8dd3644849ea57aaf070de870@kth.se> 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=-5.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, 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: Thu, 02 Dec 2021 22:58:38 -0000 Hi Petter! I tried your patch with rustc_codegen_gcc and here's the first issue I got: when initializing a global variable to the address of another global variable, I get the following error: libgccjit.so: error: can't initialize _RNvNvCsMJfZecVzJR_21mini_core_hello_world4main14ANOTHER_STATIC with _RNvCs3Hdp5GKxhqH_9mini_core8A_STATIC since it has no initial value set It looks like I can't take the address of an uninitialized global variable (maybe it's initialized after, but from a quick look, it seems like it's an imported global variable). Also, I'd appreciate if the function gcc_jit_context_new_struct_constructor would not need the fields parameter. After that, I removed this check to see if there was any other error later, but everything was fine! Thanks for your work! Le mercredi 01 d=C3=A9cembre 2021 =C3=A0 19:07 -0500, David Malcolm a =C3= =A9crit=C2=A0: > 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 >=20 > Thanks.=C2=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 > >=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.=C2=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 > > changed to int, forgot to change the docs).=20 >=20 > I think that was a historical error on my part, and I should have > used > size_t.=C2=A0 size_t is shorter to type than "unsigned"; it seems that th= e > type's signedness should be unsigned given that < 0 makes no sense. >=20 > >=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?=C2=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. >=20 > Whichever is easier from an implementation point of view (it's much > easier to change the implementation, compared to the user-facing > API). >=20 > >=20 > > > Would "initializer" be better than "constructor"?=C2=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. >=20 > Fair enough. >=20 > >=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?=C2=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. >=20 > 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=C2=A0 > =C2=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.=C2=A0 That said, if w= e > 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. >=20 > 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.=C2=A0 So hopefully the folding code is > good > enough as-is, and we can revisit this if someone manages to generate > crashing inputs. >=20 >=20 > >=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. >=20 > Thanks!=C2=A0 Please can you crosspost libgccjit patches to both the jit > and > gcc-patches mailing lists. >=20 > Dave >=20 > [...snip...] >=20