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

[David Malcolm <dmalcolm@redhat.com>]

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