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

[David Malcolm <dmalcolm@redhat.com>]

On Wed, 2020-08-19 at 09:17 +0200, Andrea Corallo wrote:
> David Malcolm <dmalcolm@redhat.com> writes:
> 
> > Thanks for the updated patch.  Comments inline below.
> 
> Hi Dave,
> 
> sorry for the late reply.

Likewise, sorry.

[...]

> > Why the non-void return type?  Looking at libgccjit.c I see it returns
> > "global" if it succeeds, or NULL if it fails.  Wouldn't it be better to
> > simply have void return type, and rely on the normaly error-handling
> > mechanisms?
> > Or is this inspired by the inline asm patch? (for PR 87291)
> 
> The idea is that this way the user could also create the global,
> initialize it and directly use it like:
> 
> foo (gcc_jit_global_set_initializer (gcc_jit_context_new_global (...), ...))
> 
> I left it that way, let me know if you prefer otherwise.

I don't have a strong preference here, so let's go with what you've
written.

[...]

> >> +/* Construct an initialized playback::lvalue instance (wrapping a
> >> +   tree).  */
> >> +
> >> +playback::lvalue *
> >> +playback::context::
> >> +new_global_initialized (location *loc,
> >> +                    enum gcc_jit_global_kind kind,
> >> +                    type *type,
> >> +                        size_t element_size,
> >> +                    size_t initializer_num_elem,
> >> +                    const void *initializer,
> >> +                    const char *name)
> >> +{
> >> +  tree inner = global_new_decl (loc, kind, type, name);
> >> +
> >> +  static vec<constructor_elt, va_gc> *constructor_elements;
> >
> > Why the use of a function-level static variable here, and why va_gc?
> > Wouldn't an auto_vec<constructor_elt> be cleaner?
> > Ah: is it because of the call to build_constructor?
> > If so, can the "static" be removed and replaced with an "= NULL;"
> > initializer?
> >
> > (I'm very wary of function-level static variables, as they're a place
> > when state can "hide" between multiple in-process invocations of the
> > compiler)
> 
> Sorry that's due to my ignorance on how GCC GC works, I thought GC roots
> had to be static, fixed.

FWIW I don't think this is a GC root; the only GTY-marked roots in
gcc/jit are those in dummy-frontend.c

Looking at gcc/jit/notes.txt, this playback code is called in the "No
GC in here" region within jit_langhook_parse_file.

[...]

> diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
> index d783ceea51a..28f81be0060 100644
> --- a/gcc/jit/docs/topics/expressions.rst
> +++ b/gcc/jit/docs/topics/expressions.rst
> @@ -582,6 +582,27 @@ Global variables
>        referring to it.  Analogous to using an "extern" global from a
>        header file.
>  
> +.. function:: gcc_jit_lvalue *\
> +              gcc_jit_global_set_initializer (gcc_jit_lvalue *global,\
> +                                              const void *blob,\
> +                                              size_t num_bytes)
> +
> +   Set an initializer for an object using the memory content pointed
> +   by ``blob`` for ``num_bytes``.  ``global`` must be an array of an
> +   integral type.
> +
> +   The parameter ``blob`` must be non-NULL. The call copies the memory
> +   pointed by ``blob`` for ``num_bytes`` bytes, so it is valid to pass
> +   in a pointer to an on-stack buffer.  The content will be stored in
> +   the compilation unit and used as initialization value of the array.

Please document here that the return value is "global".

[...]

> +template<typename T>
> +static void
> +load_blob_in_ctor (vec<constructor_elt, va_gc> *&constructor_elements,
> +		   size_t num_elem,
> +		   const void *initializer)
> +{
> +  /* Loosely based on 'output_init_element' c-typeck.c:9691.  */
> +  const T *p = (const T *)initializer;
> +  tree node = make_unsigned_type (BITS_PER_UNIT * sizeof (T));
> +  for (size_t i = 0; i < num_elem; i++)
> +    {
> +      constructor_elt celt =
> +	{ build_int_cst (long_unsigned_type_node, i),
> +	  build_int_cst (node, p[i]) };
> +      vec_safe_push (constructor_elements, celt);

In theory it would be slightly quicker to grow the vector once, and use
vec_quick_push inside the loop, but given how much allocation we're
doing it may be lost in the noise.

> +    }
> +}
> +
> +/* Construct an initialized playback::lvalue instance (wrapping a
> +   tree).  */
> +
> +playback::lvalue *
> +playback::context::
> +new_global_initialized (location *loc,
> +			enum gcc_jit_global_kind kind,
> +			type *type,
> +                        size_t element_size,
> +			size_t initializer_num_elem,
> +			const void *initializer,
> +			const char *name)
> +{
> +  tree inner = global_new_decl (loc, kind, type, name);
> +
> +  vec<constructor_elt, va_gc> *constructor_elements = NULL;
> +
> +  switch (element_size)
> +    {
> +    case 1:
> +      load_blob_in_ctor<uint8_t> (constructor_elements, initializer_num_elem,
> +				  initializer);
> +      break;
> +    case 2:
> +      load_blob_in_ctor<uint16_t> (constructor_elements, initializer_num_elem,
> +				   initializer);
> +      break;
> +    case 4:
> +      load_blob_in_ctor<uint32_t> (constructor_elements, initializer_num_elem,
> +				   initializer);
> +      break;
> +    case 8:
> +      load_blob_in_ctor<uint64_t> (constructor_elements, initializer_num_elem,
> +				   initializer);
> +      break;
> +    default:
> +      gcc_unreachable ();

Is there a way to hit this gcc_unreachable?
If I'm reading it right, presumably this is only going to be called on
the result of get_size, and this can only return the sizes covered by
the cases, right?  If so, please add a comment to this effect.

[...]

> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> index b73cd76a0a0..39510097c6f 100644
> --- a/gcc/jit/jit-recording.c
> +++ b/gcc/jit/jit-recording.c
> @@ -2175,6 +2175,57 @@ recording::type::access_as_type (reproducer &r)
>    return r.get_identifier (this);
>  }
>  
> +/* Override of default implementation of
> +   recording::type::get_size.
> +
> +   Return the size in bytes.  This is in use for global
> +   initialization.  */
> +
> +size_t
> +recording::memento_of_get_type::get_size ()
> +{
> +  int size;
> +  switch (m_kind)
> +    {
> +    case GCC_JIT_TYPE_VOID:
> +      return 0;
> +    case GCC_JIT_TYPE_BOOL:
> +    case GCC_JIT_TYPE_CHAR:
> +    case GCC_JIT_TYPE_SIGNED_CHAR:
> +    case GCC_JIT_TYPE_UNSIGNED_CHAR:
> +      return 1;
> +    case GCC_JIT_TYPE_SHORT:
> +    case GCC_JIT_TYPE_UNSIGNED_SHORT:
> +      size = SHORT_TYPE_SIZE;
> +      break;
> +    case GCC_JIT_TYPE_INT:
> +    case GCC_JIT_TYPE_UNSIGNED_INT:
> +      size = INT_TYPE_SIZE;
> +      break;
> +    case GCC_JIT_TYPE_LONG:
> +    case GCC_JIT_TYPE_UNSIGNED_LONG:
> +      size = LONG_TYPE_SIZE;
> +      break;
> +    case GCC_JIT_TYPE_LONG_LONG:
> +    case GCC_JIT_TYPE_UNSIGNED_LONG_LONG:
> +      size = LONG_LONG_TYPE_SIZE;
> +      break;
> +    case GCC_JIT_TYPE_FLOAT:
> +      size = FLOAT_TYPE_SIZE;
> +      break;
> +    case GCC_JIT_TYPE_DOUBLE:
> +      size = DOUBLE_TYPE_SIZE;
> +      break;
> +    case GCC_JIT_TYPE_LONG_DOUBLE:
> +      size = LONG_DOUBLE_TYPE_SIZE;
> +      break;
> +    default:
> +      gcc_unreachable ();
> +    }
> +
> +  return size / BITS_PER_UNIT;
> +}
> +
>  /* Implementation of pure virtual hook recording::type::dereference for
>     recording::memento_of_get_type.  */
>  

[...]

> @@ -4472,6 +4580,25 @@ recording::global::write_reproducer (reproducer &r)
>      global_kind_reproducer_strings[m_kind],
>      r.get_identifier_as_type (get_type ()),
>      m_name->get_debug_string ());
> +
> +  if (m_initializer)
> +    switch (m_type->dereference ()->get_size ())
> +      {
> +      case 1:
> +	write_initializer_reproducer<uint8_t> (id, r);
> +	break;
> +      case 2:
> +	write_initializer_reproducer<uint16_t> (id, r);
> +	break;
> +      case 4:
> +	write_initializer_reproducer<uint32_t> (id, r);
> +	break;
> +      case 8:
> +	write_initializer_reproducer<uint64_t> (id, r);
> +	break;
> +      default:
> +	gcc_unreachable ();

Similar comments as above.

> diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> index 726b9c4b837..aa38e606a8d 100644
> --- a/gcc/jit/jit-recording.h
> +++ b/gcc/jit/jit-recording.h
> @@ -502,6 +502,12 @@ public:
>       This will return NULL if it's not valid to dereference this type.
>       The caller is responsible for setting an error.  */
>    virtual type *dereference () = 0;
> +  /* Get the type size in bytes.
> +
> +     This is implemented only for memento_of_get_type and
> +     memento_of_get_pointer as is use for initializing globals of

"as is use" -> "as it is used"

[...]

> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> index 3d04f6db3af..bc45bd80b2c 100644
> --- a/gcc/jit/libgccjit.c
> +++ b/gcc/jit/libgccjit.c
[...]
> +extern gcc_jit_lvalue *
> +gcc_jit_global_set_initializer (gcc_jit_lvalue *global,
> +				const void *blob,
> +				size_t num_bytes)
> +{
[...]
> +  RETURN_NULL_IF_FAIL_PRINTF3 (
> +    lvalue_size == num_bytes, NULL, NULL,
> +    "global \"%s\" of size %zu don't match initializer of size %zu",
> +    global->get_debug_string (), lvalue_size, num_bytes);

Please reword (following the pattern of the other "mismatching" errors
in libgccjit.c) to:

  "mismatching sizes:"
  " global \"%s\" has size %zu whereas initializer has size %zu",
  global->get_debug_string (), lvalue_size, num_bytes);

[...]

> +  reinterpret_cast <gcc::jit::recording::global *> (global)
> +    ->set_initializer (blob, num_bytes);

Was there a reason for using reinterpret_cast here, rather than
static_cast?  The file uses static_cast for this kind of thing
throughout, so I'd prefer that here.

[...]

> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> index 1c5a12e9c01..3cbcbef2693 100644
> --- a/gcc/jit/libgccjit.h
> +++ b/gcc/jit/libgccjit.h
> @@ -788,6 +788,20 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt,
>  			    gcc_jit_type *type,
>  			    const char *name);
>  
> +#define LIBGCCJIT_HAVE_gcc_jit_global_set_initializer
> +
> +/* Set a static initializer for a global return the global itself.

Please reword to:

   /* Set an initial value for a global, which must be an array of
      integral type.  Return the global itself.

> +
> +   This API entrypoint was added in LIBGCCJIT_ABI_14; you can test for its
> +   presence using
> +     #ifdef LIBGCCJIT_HAVE_gcc_jit_global_set_initializer
> +*/
> +
> +extern gcc_jit_lvalue *
> +gcc_jit_global_set_initializer (gcc_jit_lvalue *global,
> +				const void *blob,
> +				size_t num_bytes);
> +

[...]

OK for master with these nits fixed, assuming your usual testing.

Thanks!
Dave