From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id C4A47385841D for ; Thu, 2 Dec 2021 00:07:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C4A47385841D Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-599-vd5xq84TOoKOkk8MJeKEtQ-1; Wed, 01 Dec 2021 19:07:21 -0500 X-MC-Unique: vd5xq84TOoKOkk8MJeKEtQ-1 Received: by mail-qv1-f72.google.com with SMTP id g15-20020a0562141ccf00b003cada9e7e2fso34509899qvd.1 for ; Wed, 01 Dec 2021 16:07:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=oevByqCJDsmbpFKFOC1IP+YiqOYnnUsvaNrKyjt7524=; b=6nT7wlEw5NWgIhV7Y+5vlSuVxneCfLKInGmOOYe6K099129HWrZfxD5Ffuy6jOWSYB zZG4LTdMV/ivAUL3gpxDKts8m9P54l8n+oZqC/35+tL/3i2JIcItcfv3fvVgELU/3rik foX8Rxyp+A8DX92iV6TgGq9dtGCPrU4VUyYHnDjmK/P5iu1fzYUwkraiiM+BATF3DlnZ 0w0X9w/3MMDQ/LBA/ePeN7ukPJEyCgkZqya3h6L6vN9o2Ky3kc01Z5V8nOwMEoFiKDE1 SXawA2t4J/wrrFtmPtf7UVowu60NVo4023uZJL5C1pU4C97d1gJl60Er7bAYbOq/aFbK 3gJg== X-Gm-Message-State: AOAM533oFK3t6YPJ5fv+r8N5C6iMcAky0ImDDLF8Tfn6fxd7OtwoyfKJ DcGUCh8RlSGWXR1JTffRMUZ56AhF8HMGaZr3muvm9156kYeduqdrVSu1rmiDWLXBaIr4OQO4zsd tsobAJr8= X-Received: by 2002:a0c:c784:: with SMTP id k4mr10089057qvj.1.1638403640977; Wed, 01 Dec 2021 16:07:20 -0800 (PST) X-Google-Smtp-Source: ABdhPJw0UJNL/tbXHIh6Ca7xn2AG5KXVG5ve3MjjYjRx9rXLN3azVMMg1M1W/w3hr/BDUAKKmzVk0w== X-Received: by 2002:a0c:c784:: with SMTP id k4mr10089022qvj.1.1638403640723; Wed, 01 Dec 2021 16:07:20 -0800 (PST) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id a24sm608038qtp.95.2021.12.01.16.07.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Dec 2021 16:07:20 -0800 (PST) Message-ID: Subject: Re: SV: SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089] From: David Malcolm To: Petter Tomner , Antoni Boucher , "jit@gcc.gnu.org" Date: Wed, 01 Dec 2021 19:07:18 -0500 In-Reply-To: <4287fbb8dd3644849ea57aaf070de870@kth.se> References: <7d6762623f658712a4b802cc3d6524c106affa2b.camel@zoho.com> <62c178b5bb0ab004def3c7869da2b03846e5134c.camel@zoho.com> , , <4287fbb8dd3644849ea57aaf070de870@kth.se> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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 00:07:27 -0000 On Tue, 2021-11-30 at 14:37 +0000, Petter Tomner wrote: > Hi! > > > 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. > > He asked you to review my patch and wrote that he would check if it > worked > 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. 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. > > I tidied up the patch and mailed it for review to the list: > https://gcc.gnu.org/pipermail/jit/2021q4/001399.html > > 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 > leave out the fields arg and just have the values being applied in > struct field definition order. > > > It's a size_t in the docs, but an "int" in the header.  Let's make it > > a > > size_t. > > 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). I think that was a historical error on my part, and I should have used size_t. size_t is shorter to type than "unsigned"; it seems that the type's signedness should be unsigned given that < 0 makes no sense. > > > Reading the docs, it seems that this function seems to be doing 3 > > different things. > > > > Would it be better to have 3 different API entrypoints? > > > > Perhaps something like: > > > > 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, > > > > Would that be better?  I'm not sure, but I think it's clearer. > > Funnely enough the exact signature I chose. But with > gcc_jit_type instead of gcc_jit_struct. > > Ye it is way clearer. One entrypoint made it too crammed and > the documentation got very confusing. > > > They could potentially share the memento class internally, or be > > split > > out. > > 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). > > > Would "initializer" be better than "constructor"?  (I'm not sure) > > 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. > > > > 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. > > > > Can we fail to fold to a const?  If so, should this function return > > NULL_TREE instead? (and the callers be adjusted accordingly) > > I could not construct something that failed to fold > "enough" so that varasm.c coulnd't handle it and that also was > TREE_CONSTANT. > > 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. > > 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  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. 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. So hopefully the folding code is good enough as-is, and we can revisit this if someone manages to generate crashing inputs. > > > Two fields called "c" here, FWIW. > > But given that these are different field instances, it should > > complain > > about that, also. > > 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 > > I'll fix the other comments too, and prepare a new patch. Thanks! Please can you crosspost libgccjit patches to both the jit and gcc-patches mailing lists. Dave [...snip...]