public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
From: Petter Tomner <tomner@kth.se>
To: Antoni Boucher <bouanto@zoho.com>,
	David Malcolm <dmalcolm@redhat.com>,
	"jit@gcc.gnu.org" <jit@gcc.gnu.org>
Subject: SV: SV: SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089]
Date: Mon, 6 Dec 2021 10:56:15 +0000	[thread overview]
Message-ID: <cac2afb5eb8e41aaa13ae93a13567a61@kth.se> (raw)
In-Reply-To: <6fb794c3f9204623a9a4da314dc2965ec502123b.camel@zoho.com>

Hi Antoni!
          
> 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 the TREE_CONSTANT
check. Also added a testcase for it. Updated patch: https://gcc.gnu.org/pipermail/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ån: Antoni Boucher <bouanto@zoho.com>
Skickat: den 2 december 2021 23:58
Till: David Malcolm; Petter Tomner; jit@gcc.gnu.org
Ämne: Re: SV: SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089]
    
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écembre 2021 à 19:07 -0500, David Malcolm a écrit :
> 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...]
> 

    

  reply	other threads:[~2021-12-06 10:56 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-21  1:27 Antoni Boucher
2021-06-11 20:44 ` Antoni Boucher
2021-11-23  2:01   ` Antoni Boucher
2021-11-23 10:51     ` SV: " Petter Tomner
2021-11-23 14:10       ` Antoni Boucher
2021-11-24 13:38         ` SV: " Petter Tomner
2021-11-30  1:34       ` David Malcolm
2021-11-30 14:37         ` SV: " Petter Tomner
2021-12-02  0:07           ` David Malcolm
2021-12-02 22:58             ` Antoni Boucher
2021-12-06 10:56               ` Petter Tomner [this message]
2021-12-06 10:51             ` SV: " Petter Tomner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cac2afb5eb8e41aaa13ae93a13567a61@kth.se \
    --to=tomner@kth.se \
    --cc=bouanto@zoho.com \
    --cc=dmalcolm@redhat.com \
    --cc=jit@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).