SV: SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089]

[Antoni Boucher]

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...]
>