David Malcolm <dmalc...@redhat.com> writes: > Thanks for the patch. > > Is this entrypoint only needed for the ahead-of-time use case? If the > client code is instead going to do an in-memory compilation, then I > believe it can simply build the buffer of data in memory and expose it > to the jitted code via gcc_jit_context_new_rvalue_from_ptr.
Hi Dave, correct, if you can leak pointers into the generated code this entry point is likely to be unnecessary. > It occurred to me that the entrypoint is combining two things: > - creating a global char[] > - creating an initializer for that global > > which got me wondering if we should instead have a way to add > initializers for globals. > > My first thought was something like: > > gcc_jit_context_new_global_with_initializer > > which would be like gcc_jit_context_new_global but would have an > additional gcc_jit_rvalue *init_value param? > The global would have to be of kind GCC_JIT_GLOBAL_EXPORTED or > GCC_JIT_GLOBAL_INTERNAL, not GCC_JIT_GLOBAL_IMPORTED. > > Alternatively, maybe it would be better to have > > gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > gcc_jit_rvalue *init_val); > > to make the API more flexible. > > But even if we had this, we'd still need some way to create the rvalue > for that initial value. Also, maybe there ought to be a distinction > between rvalues that can vary at runtime vs those that can be computed > at compile-time (and are thus suitable for use in static > initialization). > > I suspect you may have gone through the same thought process and come > up with a simpler approach. (I'm mostly just "thinking out loud" > here, sorry if it isn't very coherent). Yes I had kind of similar thoughs. Ideally would be good to have a generic solution, the complication is that as you mentioned not every rvalue is suitable for initializing every global, but rather the opposite. My fear was that the space to be covered would be non trivial for a robust solution in this case. Also I believe we currently have no way to express in libgccjit rvalues an array with some content, so how to use this as initializer? Perhaps also we should have a new type gcc_jit_initializer? On the other hand I thought that for simple things like integers the problem is tipically already solved with an assignment in some init code (infact I think so far nobody complained) while the real standing limitation is for blobs (perhaps I'm wrong). And in the end if you can stuff some data in, you can use it later for any scope. Another "hybrid" solution would be to have specific entry point for each type of the subset we want to allow for static initialization. This way we still control the creation of the initializer internally so it's safe. In this view this blob entry point would be just one of these (probably with a different name like 'gcc_jit_context_new_glob_init_char_array'). > Should it be a "const char[]" rather than just a "char[]"? Good question, I believe depends on the usecase so I left out the const. Perhaps should be a parameter of the entry point. > One specific nit about the patch: in compatibility.rst, there should be > a: > > .. _LIBGCCJIT_ABI_14: > > label before the heading. Ops > Hope this is constructive > Dave Sure it is. Thanks Andrea
