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 [130.237.32.40]) by sourceware.org (Postfix) with ESMTPS id 6A9833857823 for ; Mon, 6 Dec 2021 10:56:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6A9833857823 Received: from exdb3.ug.kth.se (exdb3.ug.kth.se [192.168.32.58]) (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 4J70jD2XLszPMdp; Mon, 6 Dec 2021 11:56:16 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp-relay-2.sys.kth.se 4J70jD2XLszPMdp Received: from exdb6.ug.kth.se (192.168.32.61) by exdb3.ug.kth.se (192.168.32.58) 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:56:16 +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:56:16 +0100 From: Petter Tomner To: Antoni Boucher , David Malcolm , "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/4gApg9oCAAM6UVoACPboAgAF/EgCABY7p6w== Date: Mon, 6 Dec 2021 10:56:15 +0000 Message-ID: References: <7d6762623f658712a4b802cc3d6524c106affa2b.camel@zoho.com> <62c178b5bb0ab004def3c7869da2b03846e5134c.camel@zoho.com> , , <4287fbb8dd3644849ea57aaf070de870@kth.se> , <6fb794c3f9204623a9a4da314dc2965ec502123b.camel@zoho.com> In-Reply-To: <6fb794c3f9204623a9a4da314dc2965ec502123b.camel@zoho.com> 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=-4.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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:56:20 -0000 Hi Antoni! =20 > when initializing a global variable to the address of another global > variable, I get the following error: Oh ye that need to work. I removed that check since it is redundant with th= e TREE_CONSTANT check. Also added a testcase for it. Updated patch: https://gcc.gnu.org/pip= ermail/jit/2021q4/001409.html > Also, I'd appreciate if the function > gcc_jit_context_new_struct_constructor would not need the fields > parameter. If the fields parameter is set to NULL the values will be applied in struct= definition order. Did you try that? Regards, Petter Fr=E5n: Antoni Boucher Skickat: den 2 december 2021 23:58 Till: David Malcolm; Petter Tomner; 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 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=E9cembre 2021 =E0 19:07 -0500, David Malcolm a =E9crit=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.=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.=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.=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 > >=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. >=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"?=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?=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=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. >=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.=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!=A0 Please can you crosspost libgccjit patches to both the jit > and > gcc-patches mailing lists. >=20 > Dave >=20 > [...snip...] >=20 =