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

[David Malcolm <dmalcolm@redhat.com>]

On Mon, 2020-08-03 at 10:07 +0200, Andrea Corallo wrote:
> David Malcolm <dmalcolm@redhat.com> writes:
> 
> > On Fri, 2020-07-24 at 18:05 -0400, David Malcolm via Gcc-patches wrote:
> >
> > [...]
> >
> >> I haven't thought this through in detail, and I'm not sure exactly
> >> how
> >> it would work for arbitrary types, but I thought it worth sharing. 
> >> (For example I can think of nasty issues if we ever want to support
> >> cross-compilation, e.g. where sizeof types or  endianness differs
> >> between host and target).
> >
> > ...which is an argument in favor of retaining the name "blob", perhaps
> > as the name of the argument in the header file e.g.:
> >
> > extern void
> > gcc_jit_global_set_initializer (gcc_jit_lvalue *global,
> >                                  const void *blob,
> >                                  size_t num_bytes);
> >  
> >
> > as a subtle hint to the user that they need to be wary about binary
> > layouts ("here be dragons").
> >
> > [...]
> 
> Hi Dave & all,
> 
> following up this is my take on the implementation of:
> 
> gcc_jit_global_set_initializer (gcc_jit_lvalue *global,
>                                 const void *blob,
>                                 size_t num_bytes);
> 
> 'global' must be an array but in the seek of generality it now supports
> all the various integral types and is not limited to char[].
> 
> As you anticipated the implementation I came up is currently not safe
> for cross-compilation, not sure is requirement tho.
> 
> make check-jit is clean
> 
> Feedback very welcome
> 
> Thanks!

Thanks for the updated patch.  Comments inline below.

>   Andrea
> 
> gcc/jit/ChangeLog
> 
> 2020-08-01  Andrea Corallo  <andrea.corallo@arm.com>
> 
>         * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI tag.
>         * docs/topics/expressions.rst (gcc_jit_global_set_initializer):
>         Document new entry point in section 'Global variables'.
>         * jit-playback.c (global_new_decl, global_finalize_lvalue): New
>         method.
>         (playback::context::new_global): Make use of global_new_decl,
>         global_finalize_lvalue.
>         (load_blob_in_ctor): New template function in use by the
>         following.
>         (playback::context::new_global_initialized): New method.
>         * jit-playback.h (class context): Decl 'new_global_initialized',
>         'global_new_decl', 'global_finalize_lvalue'.
>         (lvalue::set_initializer): Add implementation.
>         * jit-recording.c (recording::memento_of_get_pointer::get_size)
>         (recording::memento_of_get_type::get_size): Add implementation.
>         (recording::global::write_initializer_reproducer): New function in
>         use by 'recording::global::write_reproducer'.
>         (recording::global::replay_into)
>         (recording::global::write_to_dump)
>         (recording::global::write_reproducer): Handle
>         initialized case.
>         * jit-recording.h (class type): Decl 'get_size' and
>         'num_elements'.
>         * libgccjit++.h (class lvalue): Declare new 'set_initializer'
>         method.
>         (class lvalue): Decl 'is_global' and 'set_initializer'.
>         (class class global) Decl 'write_initializer_reproducer'. Add
>         'm_initializer', 'm_initializer_num_bytes' fields.  Implement
>         'set_initializer'.
>         * libgccjit.c (gcc_jit_global_set_initializer): New function.
>         * libgccjit.h (gcc_jit_global_set_initializer): New function
>         declaration.
>         * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag.
> 
> gcc/testsuite/ChangeLog
> 
> 2020-08-01  Andrea Corallo  <andrea.corallo@arm.com>
> 
>         * jit.dg/all-non-failing-tests.h: Add test-blob.c.
>         * jit.dg/test-global-set-initializer.c: New testcase.

[...]

> diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
> index d783ceea51a8..7699dcfd27be 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 arrays of an
                                      
Typo: "arrays" -> "array"

> +   integral type.

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)

[...]

> --- a/gcc/jit/jit-playback.h
> +++ b/gcc/jit/jit-playback.h
> @@ -111,6 +111,15 @@ public:
>  	      type *type,
>  	      const char *name);
>  
> +  lvalue *
> +  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);
> +
>    template <typename HOST_TYPE>
>    rvalue *
>    new_rvalue_from_const (type *type,
> @@ -266,6 +275,14 @@ private:
>    const char * get_path_s_file () const;
>    const char * get_path_so_file () const;
>  
> +  tree
> +  global_new_decl (location *loc,
> +                   enum gcc_jit_global_kind kind,
> +                   type *type,
> +                   const char *name);
> +  lvalue *
> +  global_finalize_lvalue (tree inner);
> +
>  private:
>  
>    /* Functions for implementing "compile".  */
> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> index 0fddf04da873..52fc92f5928c 100644
> --- a/gcc/jit/jit-playback.c
> +++ b/gcc/jit/jit-playback.c
> @@ -510,14 +510,14 @@ new_function (location *loc,
>    return func;
>  }
>  
> -/* Construct a playback::lvalue instance (wrapping a tree).  */
> +/* In use by new_global and new_global_initialized.  */
>  
> -playback::lvalue *
> +tree
>  playback::context::
> -new_global (location *loc,
> -	    enum gcc_jit_global_kind kind,
> -	    type *type,
> -	    const char *name)
> +global_new_decl (location *loc,
> +		 enum gcc_jit_global_kind kind,
> +		 type *type,
> +		 const char *name)
>  {
>    gcc_assert (type);
>    gcc_assert (name);
> @@ -547,6 +547,15 @@ new_global (location *loc,
>    if (loc)
>      set_tree_location (inner, loc);
>  
> +  return inner;
> +}
> +
> +/* In use by new_global and new_global_initialized.  */
> +
> +playback::lvalue *
> +playback::context::
> +global_finalize_lvalue (tree inner)
> +{
>    varpool_node::get_create (inner);
>  
>    varpool_node::finalize_decl (inner);
> @@ -556,6 +565,90 @@ new_global (location *loc,
>    return new lvalue (this, inner);
>  }
>  
> +/* Construct a playback::lvalue instance (wrapping a tree).  */
> +
> +playback::lvalue *
> +playback::context::
> +new_global (location *loc,
> +	    enum gcc_jit_global_kind kind,
> +	    type *type,
> +	    const char *name)
> +{
> +  tree inner = global_new_decl (loc, kind, type, name);
> +
> +  return global_finalize_lvalue (inner);
> +}
> +
> +/* Fill 'constructor_elements' with the memory content of
> +   'initializer'.  Each element of the initializer is of the size of
> +   type T.  In use by new_global_initialized.*/
> +
> +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);
> +    }
> +}
> +
> +/* 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)

> +
> +  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 ();
> +    }
> +  /* Compare with 'pop_init_level' c-typeck.c:8780.  */
> +  tree ctor = build_constructor (type->as_tree (), constructor_elements);
> +  constructor_elements = NULL;
> +
> +  /* Compare with 'store_init_value' c-typeck.c:7555.  */
> +  DECL_INITIAL (inner) = ctor;
> +
> +  return global_finalize_lvalue (inner);
> +}
> +
>  /* Implementation of the various
>        gcc::jit::playback::context::new_rvalue_from_const <HOST_TYPE>
>     methods.
> diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> index 726b9c4b8371..7a7a61a0e126 100644
> --- a/gcc/jit/jit-recording.h
> +++ b/gcc/jit/jit-recording.h
> @@ -502,6 +502,7 @@ 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;
> +  virtual size_t get_size () { gcc_unreachable (); }

It looks like get_size is only implemented for memento_of_get_pointer
and memento_of_get_type::get_size.
A gcc_unreachable in the base class seems like a "code smell", but it's
probably OK here: I believe the restriction here is that this should
only be called on initializers for globals, and we only set these on
globals of certain types, right?  Please can you capture that
restriction in a comment next to the gcc_unreachable.

>    /* Dynamic casts.  */
>    virtual function_type *dyn_cast_function_type () { return NULL; }
> @@ -534,6 +535,7 @@ public:
>    virtual type *is_array () = 0;
>    virtual bool is_void () const { return false; }
>    virtual bool has_known_size () const { return true; }
> +  virtual int num_elements () { gcc_unreachable (); }

Again, I'm wary of a virtual fn with a gcc_unreachable in the base
class.  Here, I believe it's only implemented for array_type.
Wouldn't this be better as a non-virtual member function of array_type,
and simply cast to the subclass at any caller(s)?

>    bool is_numeric () const
>    {
> @@ -569,6 +571,8 @@ public:
>  
>    type *dereference () FINAL OVERRIDE;
>  
> +  size_t get_size () FINAL OVERRIDE;
> +
>    bool accepts_writes_from (type *rtype) FINAL OVERRIDE
>    {
>      if (m_kind == GCC_JIT_TYPE_VOID_PTR)
> @@ -610,6 +614,8 @@ public:
>  
>    type *dereference () FINAL OVERRIDE { return m_other_type; }
>  
> +  size_t get_size () FINAL OVERRIDE;
> +
>    bool accepts_writes_from (type *rtype) FINAL OVERRIDE;
>  
>    void replay_into (replayer *r) FINAL OVERRIDE;
> @@ -755,6 +761,7 @@ class array_type : public type
>    bool is_bool () const FINAL OVERRIDE { return false; }
>    type *is_pointer () FINAL OVERRIDE { return NULL; }
>    type *is_array () FINAL OVERRIDE { return m_element_type; }
> +  int num_elements () FINAL OVERRIDE { return m_num_elements; }
>  
>    void replay_into (replayer *) FINAL OVERRIDE;
>  
> @@ -1107,6 +1114,8 @@ public:
>  
>    const char *access_as_rvalue (reproducer &r) OVERRIDE;
>    virtual const char *access_as_lvalue (reproducer &r);
> +  virtual bool is_global () const { return false; }
> +  virtual void set_initializer (const void *, size_t) { gcc_unreachable (); }

Similar comments here.

>  class param : public lvalue
> @@ -1335,8 +1344,23 @@ public:
>  
>    void write_to_dump (dump &d) FINAL OVERRIDE;
>  
> +  virtual bool is_global () const FINAL OVERRIDE { return true; }

No need to add "virtual" on a function that's labelled FINAL and/or
OVERRIDE.

> +  virtual void
> +  set_initializer (const void *initializer,
> +                   size_t num_bytes) FINAL OVERRIDE
> +  {
> +    if (m_initializer)
> +      free (m_initializer);
> +    m_initializer = xmalloc (num_bytes);
> +    memcpy (m_initializer, initializer, num_bytes);
> +    m_initializer_num_bytes = num_bytes;
> +  }

As noted above, could this be non-virtual, and use casting instead?

I can't remember if this class has a dtor, but presumably we should
also be freeing m_initializer there.

>  private:
>    string * make_debug_string () FINAL OVERRIDE { return m_name; }
> +  template <typename T>
> +  void write_initializer_reproducer (const char *id, reproducer &r);
>    void write_reproducer (reproducer &r) FINAL OVERRIDE;
>    enum precedence get_precedence () const FINAL OVERRIDE
>    {
> @@ -1346,6 +1370,8 @@ private:
>  private:
>    enum gcc_jit_global_kind m_kind;
>    string *m_name;
> +  void *m_initializer = NULL;
> +  size_t m_initializer_num_bytes = 0;
>  };

Are we still targetting C++98, and are these initializers compatible
with it?


>  template <typename HOST_TYPE>
> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> index b73cd76a0a02..f97de00f63c3 100644
> --- a/gcc/jit/jit-recording.c
> +++ b/gcc/jit/jit-recording.c

[...]

> @@ -4440,9 +4510,26 @@ recording::global::write_to_dump (dump &d)
>        d.write ("extern ");
>        break;
>      }
> -  d.write ("%s %s;\n",
> +
> +  d.write ("%s %s",
>  	   m_type->get_debug_string (),
>  	   get_debug_string ());
> +
> +  if (!m_initializer)
> +    {
> +      d.write (";\n");
> +      return;
> +    }
> +
> +  d.write ("=\n  { ");
> +  const char *p = (const char *)m_initializer;

Should be "const unsigned char"?

> +  for (size_t i = 0; i < m_initializer_num_bytes; i++)
> +    {
> +      d.write ("0x%x, ", p[i]);
> +      if (i && !(i % 64))
> +	d.write ("\n    ");
> +    }
> +  d.write ("};\n");
>  }
>  
>  /* A table of enum gcc_jit_global_kind values expressed in string
> @@ -4454,6 +4541,27 @@ static const char * const global_kind_reproducer_strings[] = {
>    "GCC_JIT_GLOBAL_IMPORTED"
>  };
>  
> +template <typename T>
> +void
> +recording::global::write_initializer_reproducer (const char *id, reproducer &r)
> +{
> +  const char *init_id = r.make_tmp_identifier ("init_for", this);
> +  r.write ("  %s %s[] =\n    {",
> +	   m_type->dereference ()->get_debug_string (),
> +	   init_id);
> +
> +  const T *p = (const T *)m_initializer;
> +  for (size_t i = 0; i < m_initializer_num_bytes / sizeof (T); i++)
> +    {
> +      r.write ("%lu, ", (uint64_t)p[i]);

Does this assume that "%lu" matches uint64_t?



> +      if (i && !(i % 64))
> +	r.write ("\n    ");
> +    }
> +  r.write ("};\n");
> +  r.write ("  gcc_jit_global_set_initializer (%s, %s, sizeof (%s));\n",
> +	   id, init_id, init_id);
> +}
> +
>  /* Implementation of recording::memento::write_reproducer for globals. */
>  
>  void
> @@ -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 ();
> +      }
>  }
>  
>  /* The implementation of the various const-handling classes:
> diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
> index 69e67766640c..1b9ef1a5db98 100644
> --- a/gcc/jit/libgccjit++.h
> +++ b/gcc/jit/libgccjit++.h
> @@ -488,6 +488,7 @@ namespace gccjit
>  			 location loc = location ());
>  
>      rvalue get_address (location loc = location ());
> +    lvalue set_initializer (const void *blob, size_t num_bytes);
>    };
>  
>    class param : public lvalue
> @@ -1737,6 +1738,15 @@ lvalue::get_address (location loc)
>  					     loc.get_inner_location ()));
>  }
>  
> +inline lvalue
> +lvalue::set_initializer (const void *blob, size_t num_bytes)
> +{
> +  gcc_jit_global_set_initializer (get_inner_lvalue (),
> +                                  blob,
> +                                  num_bytes);
> +  return *this;
> +}

(Again the return type)

>  // class param : public lvalue
>  inline param::param () : lvalue () {}
>  inline param::param (gcc_jit_param *inner)
> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> index 1c5a12e9c015..08b855230f6a 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_global_set_initializer
> +
> +/* Set a static initializer for a global 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);
> +
>  /* Upcasting.  */
>  extern gcc_jit_object *
>  gcc_jit_lvalue_as_object (gcc_jit_lvalue *lvalue);
> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> index 3d04f6db3aff..80c5aa6ac115 100644
> --- a/gcc/jit/libgccjit.c
> +++ b/gcc/jit/libgccjit.c
> @@ -1117,6 +1117,42 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt,
>    return (gcc_jit_lvalue *)ctxt->new_global (loc, kind, type, name);
>  }
>  
> +/* Public entrypoint.  See description in libgccjit.h.
> +
> +   After error-checking, the real work is done by the
> +   gcc::jit::recording::global::set_initializer method, in
> +   jit-recording.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 (global, NULL, NULL, "NULL global");
> +  RETURN_NULL_IF_FAIL (blob, NULL, NULL, "NULL blob");
> +  RETURN_NULL_IF_FAIL_PRINTF1 (global->is_global (), NULL, NULL,
> +			       "global \"%s\" not a global",
                                ^^^^^^
                               "lvalue \"%s\" is not a global",
surely, as it's not a global.


> +			       global->get_debug_string ());
> +  gcc::jit::recording::type *lval_type = global->get_type ();
> +  RETURN_NULL_IF_FAIL_PRINTF1 (lval_type->is_array (), NULL, NULL,
> +			       "global \"%s\" not an array",

"not" -> "is not".

> +			       global->get_debug_string ());
> +  RETURN_NULL_IF_FAIL_PRINTF1 (lval_type->dereference ()->is_int (), NULL, NULL,
> +			       "global \"%s\" not an array of integral type",

"not" -> "is not".

> +			       global->get_debug_string ());
> +  RETURN_NULL_IF_FAIL_PRINTF1 (
> +    lval_type->dereference ()->get_size() * lval_type->num_elements ()
> +    == num_bytes,
> +    NULL, NULL,
> +    "global \"%s\" and initializer don't match size",

Would be more user-friendly to specify the two values in the error
message, though off the top of my head I don't remember the format code
for size_t.


> +    global->get_debug_string ());
> +
> +  global->set_initializer (blob, num_bytes);
> +
> +  return global;
> +}
> +

[...]

Thanks again for the patch; hope this is constructive
Dave