Re: [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point

[David Malcolm <dmalcolm@redhat.com>]

On Wed, 2020-06-03 at 12:11 +0200, Andrea Corallo wrote:
> Hi all,
> 
> I'd like to submit this patch to extend libgccjit to allow for
> storing
> binary blobs (equivalent to initialized char arrays).
> 
> The use-case is when libgccjit is used as a back-end for dynamic
> programming languages.  In this case is often necessary to store
> serialized objects into the compilation unit as support to the
> generated code.
> 
> The proposed entry point is the following:
> 
> gcc_jit_lvalue *
> gcc_jit_context_new_blob (gcc_jit_context *ctxt,
> 			  gcc_jit_location *loc,
> 			  enum gcc_jit_global_kind kind,
> 			  const void *ptr,
> 			  size_t size,
> 			  const char *name);
> 
> This create a special kind of libgccjit global that will be
> initialized
> with the memory content pointed by 'ptr'.
> 
> I've added a testcase and the regression is clean.  I've also tested
> it with the libgccjit based Emacs disabling the current workaround we
> have
> for this.
> 
> Feedback is very welcome.

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.

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

Should it be a "const char[]" rather than just a "char[]"?

One specific nit about the patch: in compatibility.rst, there should be
a:

.. _LIBGCCJIT_ABI_14:

label before the heading.

Hope this is constructive
Dave


>   Andrea
> 
> gcc/jit/ChangeLog
> 
> 2020-06-02  Andrea Corallo  <andrea.corallo@arm.com>
> 
> 	* docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI
> tag.
> 
> 	* docs/topics/expressions.rst (Binary Blobs): New section
> 	documenting gcc_jit_context_new_blob.
> 
> 	* jit-common.h: Document class blob.
> 
> 	* jit-playback.c (playback::context::global_new_decl)
> 	(playback::context::global_finalize_lvalue): New methods.
> 	(playback::context::new_global): Make use of global_new_decl,
> 	global_finalize_lvalue.
> 	(playback::context::new_blob): New method.
> 
> 	* jit-playback.h (new_blob, global_new_decl): New method
> 	declarations.
> 
> 	* jit-recording.c (recording::context::new_blob)
> 	(recording::blob::replay_into)
> 	(recording::global::write_qualifier_to_dump): New methods.
> 	(recording::global::write_to_dump): Use
> write_qualifier_to_dump.
> 	(recording::blob::write_to_dump): New method.
> 
> 	* jit-recording.h (class blob): New class.
> 	(class global): Have m_kind and m_name as protected.
> 	(class global): Remove FINAL qualifier to replay_into and
> 	write_to_dump.
> 	(class global): New method write_qualifier_to_dump decl.
> 	(class context): New method new_blob decl.
> 
> 	* libgccjit++.h (context::new_blob): New method.
> 
> 	* libgccjit.c (gcc_jit_lvalue_as_rvalue): New function.
> 
> 	* libgccjit.h (gcc_jit_context_new_blob): New function
> 	declaration.
> 	(LIBGCCJIT_HAVE_gcc_jit_context_new_blob): New macro.
> 
> 	* libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag.
> 
> gcc/testsuite/ChangeLog
> 
> 2020-06-02  Andrea Corallo  <andrea.corallo@arm.com>
> 
> 	* jit.dg/all-non-failing-tests.h: Add test-blob.c.
> 
> 	* jit.dg/test-blob.c: New test.
>