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

[Petter Tomner <tomner@kth.se>]

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

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

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

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

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

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

Regards,


Från: David Malcolm <dmalcolm@redhat.com>
Skickat: den 30 november 2021 02:34
Till: Petter Tomner; Antoni Boucher; jit@gcc.gnu.org
Ämne: Re: SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089]
    
On Tue, 2021-11-23 at 10:51 +0000, Petter Tomner wrote:
> Hi!
> 
> Does it work with pointers to other symbols and unions? I don't think
> constant symbols end
> up in the .rdata section unless they are marked for that.
> 
> I did a similar patch that I just dropped in a RFC mail some time ago.
> (See attachment).

Petter and Antoni, thanks for the patches.

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.

Also, Antoni has much more experience than me of creating initializers
in libgccjit.

> 
> 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.
> 
> Regards,
> Petter 
> 
> 
> Från: Gcc-patches <gcc-patches-bounces+tomner=kth.se@gcc.gnu.org> för
> Antoni Boucher via Gcc-patches <gcc-patches@gcc.gnu.org>
> Skickat: den 23 november 2021 03:01
> Till: David Malcolm
> Kopia: jit@gcc.gnu.org; gcc-patches@gcc.gnu.org
> Ämne: Re: [PATCH] libgccjit: Add function to set the initial value of a
> global variable [PR96089]
>     
> Hi David!
> 
> I updated the patch to allow initializing global variables with values
> of type array or struct.
> 
> I also fixed the bug I was talking in my previous message by using the
> following workaround: I create a new memento for the action of setting
> the global variable initial value and as such, both the global variable
> and the initial value are bound to exist when setting the global
> variable initializer.
> Is that workaround good enough?
> (I guess that workaround could be used to fix the same issue that we
> have for inline assembly.)
> 
> Thanks for the review!
> 
> Le vendredi 11 juin 2021 à 16:44 -0400, Antoni Boucher a écrit :
> > David: this one wasn't reviewed yet by you, so you can review it.
> > 
> > Le jeudi 20 mai 2021 à 21:27 -0400, Antoni Boucher a écrit :
> > > Hi.
> > > 
> > > I made this patch to set an arbitrary value to a global variable.
> > > 
> > > This patch suffers from the same issue as inline assembly
> > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100380), i.e. it
> > > segfaults if the `rvalue` is created after the global variable.
> > > It seems to be a design issue so I'm not sure what would be the fix
> > > for
> > > it and having it fixed would allow me to test this new function
> > > much
> > > more and see if things are missing (i.e. it might require a way to
> > > create a constant struct).
> > > See the link above for an explanation of this issue.
> > > 
> > > Thanks for the review.
> > 
> 
>     

> From e6850f3417bc0c35d9712e850e5274117527021c Mon Sep 17 00:00:00 2001
> From: Petter Tomner <tomner@kth.se>
> Date: Sun, 24 Oct 2021 21:13:44 +0200
> Subject: [PATCH 3/5] Add suport for global rvalue initialization and ctors
> 
> This patch adds support for initialization of global variables
> with rvalues and creating constructors for array, struct and
> union types which can be used as rvalues.
>

[...snip...]

Review comments follow inline throughout below...

> diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
> index 30a3b9780f9..3b38cab025c 100644
> --- a/gcc/jit/docs/topics/expressions.rst
> +++ b/gcc/jit/docs/topics/expressions.rst
> @@ -176,6 +176,91 @@ Simple expressions
>     underlying string, so it is valid to pass in a pointer to an on-stack
>     buffer.
>  
> +.. function:: gcc_jit_rvalue *\
> +           gcc_jit_context_new_constructor (gcc_jit_context *ctxt,\
> +                                               gcc_jit_location *loc,\
> +                                            gcc_jit_type *type,\
> +                                            size_t arr_length,\

It's a size_t in the docs, but an "int" in the header.  Let's make it a
size_t.

> +                                            gcc_jit_field **fields,\
> +                                            gcc_jit_rvalue **values)
> +
> +   Create a constructor for an array, union or struct as a rvalue.
> +
> +   Returns NULL on error. The two parameter arrays are copied and
> +   do not have to outlive the context.
> +
> +   ``type`` specifies what the constructor will build.
> +
> +   ``arr_length`` specifies the number of elements in ``values``.
> +   
> +   Constructing structs
> +   """"""""""""""""""""
> +
> +   For a struct, each field in ``fields`` specifies
> +   which field in the struct to set to the corresponding
> +   value in ``values``. ``fields`` and ``values`` are paired by index
> +   and the pair need to have the same unqualified type.
> +
> +   A NULL value in ``values`` is a shorthand for zero initialization
> +   of the corresponding field or array element.
> +
> +   The fields in ``fields`` have to be in definition order, but there
> +   can be gaps. Any field in the struct that is not specified in
> +   ``fields`` will be zeroed.
> +
> +   The fields in ``fields`` need to be the same objects that were used
> +   to create the struct.
> +
> +   ``fields`` need to have the same length as ``values``. 
> +   
> +   If ``arr_length`` is 0, the array parameters will be
> +   ignored and zero initialization will be used.
> +   
> +
> +   Constructing arrays
> +   """""""""""""""""""
> +   
> +   For an array type, the ``fields`` parameter is ignored.
> +
> +   Each value in ``values`` sets the corresponding value in the array.
> +   If the array type itself has more elements than ``values``, the
> +   left-over elements will be zeroed.
> +
> +   Each value in ``values`` need to be the same unqualified type as the
> +   array type's elements' type.
> +
> +   If ``arr_length`` is 0, the array parameters will be
> +   ignored and zero initialization will be used.
> +
> +   Constructing unions
> +   """""""""""""""""""
> +
> +   For unions, ``arr_length`` need to be 1. There need to be one field
> +   in ``fields`` and one value in ``values`` which specified which field
> +   in the union to set to what value. The pair need to have the same
> +   unqualified type.
> +
> +   The field in ``fields`` need to be one of the objects that were used
> +   to create the union.
> +
> +   Remarks
> +   """""""
> +
> +   The constructor rvalue can be used for assignment to locals.
> +   It can be used to initialize global variables with
> +   :func:`gcc_jit_global_set_initializer_rvalue`. It can also be used as a
> +   temporary value for function calls and return values.
> +
> +   The constructor can contain nested constructors. Note that a string
> +   literal rvalue can't be used to construct a char array. It need one
> +   rvalue for each char.
> +
> +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_16`; you can test for its
> +   presense using:
> +
> +   .. code-block:: c
> +     #ifdef LIBGCCJIT_HAVE_CTORS
> +

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,
                                        gcc_jit_location *loc,
                                        gcc_jit_struct *type,
                                        size_t num_fields, // and values
                                        gcc_jit_field **fields,
                                        gcc_jit_rvalue **values);

extern gcc_jit_rvalue *
gcc_jit_context_new_array_constructor (gcc_jit_context *ctxt,
                                       gcc_jit_location *loc,
                                       gcc_jit_type *element_type,
                                       size_t num_values,
                                       gcc_jit_rvalue **values);

extern gcc_jit_rvalue *
gcc_jit_context_new_union_constructor (gcc_jit_context *ctxt,
                                       gcc_jit_location *loc,
                                       gcc_jit_type *union_type,
                                       gcc_jit_field *field,
                                       gcc_jit_rvalue *value);

Would that be better?  I'm not sure, but I think it's clearer.

They could potentially share the memento class internally, or be split
out.

Would "initializer" be better than "constructor"?  (I'm not sure)


[...snip...]

> +/* Flags for global variables class. For when the playback of the
> +   global need to know what will happen to it later. */
> +enum global_var_flags
> +{
> +  GLOBAL_VAR_FLAGS_NONE = 0,
> +  GLOBAL_VAR_FLAGS_WILL_BE_RVAL_INIT = 1,
> +  GLOBAL_VAR_FLAGS_WILL_BE_BLOB_INIT = 2,
> +};
> +
>  } // namespace gcc::jit
>  
>  } // namespace gcc
> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> index 1f4dc31a1c1..71f0e7283ba 100644
> --- a/gcc/jit/jit-playback.c
> +++ b/gcc/jit/jit-playback.c
> @@ -107,6 +107,43 @@ namespace jit {
>   Playback.
>   **********************************************************************/
>  
> +/* Fold a readonly non-volatile variable with an initial constant value,
> +   to that value.
> +
> +   Otherwise return the argument unchanged.
> +
> +   This fold is needed for setting a variable's DECL_INITIAL to the value
> +   of a const variable. The c-frontend does this in its own special
> +   fold(), so we lift this part out and do it explicitly where there is a
> +   potential for variables to be used as rvalues. */
> +static tree
> +fold_const_var (tree node)
> +{
> +  /* See c_fully_fold_internal in c-fold.c and decl_constant_value_1
> +     in c-typeck.c */
> +  if (VAR_P (node)
> +      && TREE_READONLY (node)
> +      && !TREE_THIS_VOLATILE (node)
> +      && DECL_INITIAL (node) != NULL_TREE
> +      /* "This is invalid if initial value is not constant.
> +       If it has either a function call, a memory reference,
> +       or a variable, then re-evaluating it could give different
> +       results." */
> +      && TREE_CONSTANT (DECL_INITIAL (node)))
> +    {
> +      tree ret = DECL_INITIAL (node);
> +      /* "Avoid unwanted tree sharing between the initializer and current
> +       function's body where the tree can be modified e.g. by the
> +       gimplifier."  */
> +      if (TREE_STATIC (node))
> +     ret = unshare_expr (ret);
> +
> +      return ret;
> +    }
> +
> +  return node;
> +}

You mentioned above:

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

[...snip...]

> +void
> +playback::context::
> +global_set_init_rvalue (lvalue* variable,
> +                     rvalue* init)
> +{
> +  tree inner = variable->as_tree ();
> +
> +  /* We need to fold all expressions as much as possible. The code
> +     for a DECL_INITIAL only handles some operations,
> +     etc addition, minus, 'address of'. See output_addressed_constants()
> +     in varasm.c  */
> +  tree init_tree = init->as_tree ();
> +  tree folded = fold_const_var (init_tree);
> +
> +  /* Find any VAR_DECL without DECL_INITIAL set.
> +     Assume that any ..._CST is OK to save some CPU.
> +     Handle CONSTRUCTORs explicitly to avoid tree walks
> +     on array inits consisting of only ..._CSTs. */
> +  tree sinner = NULL_TREE;
> +
> +  if (TREE_CODE (folded) == CONSTRUCTOR)
> +    {
> +      unsigned HOST_WIDE_INT idx;
> +      tree elt;
> +      FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (folded), idx, elt)
> +     {
> +       if (!CONSTANT_CLASS_P (elt))
> +         sinner = walk_tree (&elt, validate_var_has_init, NULL, NULL);
> +       if (sinner != NULL_TREE)
> +         break;
> +     }
> +    }
> +  else if (!CONSTANT_CLASS_P (folded))
> +    sinner = walk_tree (&folded, validate_var_has_init, NULL, NULL);
> +
> +  if (sinner != NULL_TREE)
> +    {
> +      tree name = DECL_NAME (inner);
> +      tree rvname = DECL_NAME (sinner);
> +      add_error (NULL,
> +              "can't initialize %s with %s since it has no "
> +              "initial value set",

What is "it" in this message?  Is it "the latter"?

> +              name != NULL_TREE ? IDENTIFIER_POINTER (name) : NULL,
> +              rvname != NULL_TREE ? IDENTIFIER_POINTER (rvname) : NULL);
> +      return;
> +    }
> +
> +  if (!TREE_CONSTANT (folded))
> +    {
> +      tree name = DECL_NAME (inner);
> +
> +      add_error (NULL,
> +              "init rvalue for the global variable %s does not seem"
> +              " to be constant",

Maybe reword to
  "unable to convert initial value for the global variable %s to a compile-time constant"
or somesuch?

[...snip...]

> +enum strip_flags {
> +  STRIP_FLAG_NONE,
> +  STRIP_FLAG_ARR,
> +  STRIP_FLAG_VEC
> +};
> +
> +/* Strips type down to array, vector or base type (whichever comes first)
> +
> +   Also saves 'ptr_depth' and sets 'flags' for array or vector types */
> +static
> +recording::type *
> +strip_and_count (recording::type *type_to_strip,

Can the type be made const?

> +              int &ptr_depth,
> +              strip_flags &flags)
> +{
> +  recording::type *t = type_to_strip;
> +
> +  while (true)
> +    {
> +      if (!t)
> +     gcc_unreachable (); /* Should only happen on corrupt input */
> +
> +      recording::type *pointed_to_type = t->is_pointer();
> +      if (pointed_to_type != NULL)
> +     {
> +       ptr_depth++;
> +       t = pointed_to_type;
> +       continue;
> +     }
> +
> +      recording::type *array_el = t->is_array ();
> +      if (array_el != NULL)
> +     {
> +       flags = STRIP_FLAG_ARR;
> +       break;
> +     }
> +
> +      recording::type *vec = t->dyn_cast_vector_type ();
> +      if (vec != NULL)
> +     {
> +       flags = STRIP_FLAG_VEC;
> +       break;
> +     }
> +
> +      /* unqualified() returns 'this' on base types */
> +      recording::type *next = t->unqualified ();
> +      if (next == t)
> +     {
> +       break;
> +     }
> +      t = next;
> +    }
> +
> +  return t;
> +}
> +
> +/* Strip qualifiers and count pointer depth, returning true
> +   if the types' base type and pointer depth are
> +   the same, otherwise false.
> +
> +   For array and vector types the number of element also
> +   has to match.
> +
> +   Do not call this directly. Call 'types_kinda_same' */
> +bool
> +types_kinda_same_internal (recording::type *a, recording::type *b)

Likewise, can these types be made const?

> +{
> +  int ptr_depth_a = 0;
> +  int ptr_depth_b = 0;
> +  recording::type *base_a;
> +  recording::type *base_b;
> +
> +  strip_flags flags_a = STRIP_FLAG_NONE;
> +  strip_flags flags_b = STRIP_FLAG_NONE;
> +
> +  base_a = strip_and_count (a, ptr_depth_a, flags_a);
> +  base_b = strip_and_count (b, ptr_depth_b, flags_b);
> +
> +  if (ptr_depth_a != ptr_depth_b)
> +    return false;
> +
> +  if (base_a == base_b)
> +    return true;
> +
> +  if (flags_a != flags_b)
> +    return false;
> +
> +  /* If the "base type" is an array or vector we might need to
> +     check deeper. */
> +  if (flags_a == STRIP_FLAG_ARR)
> +    {
> +      recording::array_type *arr_a =
> +     static_cast<recording::array_type*> (base_a);
> +      recording::array_type *arr_b =
> +     static_cast<recording::array_type*> (base_b);
> +
> +      if (arr_a->num_elements () != arr_b->num_elements ())
> +     return false;
> +
> +      /* is_array returns element type */
> +      recording::type *el_a = arr_a->is_array ();
> +      recording::type *el_b = arr_b->is_array ();
> +
> +      if (el_a == el_b)
> +     return true;
> +
> +      return types_kinda_same_internal (el_a, el_b);
> +    }
> +  if (flags_a == STRIP_FLAG_VEC)
> +    {
> +      recording::vector_type *arr_a =
> +     static_cast<recording::vector_type*> (base_a);
> +      recording::vector_type *arr_b =
> +     static_cast<recording::vector_type*> (base_b);
> +
> +      if (arr_a->get_num_units () != arr_b->get_num_units ())
> +     return false;
> +
> +      recording::type *el_a = arr_a->get_element_type ();
> +      recording::type *el_b = arr_b->get_element_type ();
> +
> +      if (el_a == el_b)
> +     return true;
> +
> +      return types_kinda_same_internal (el_a, el_b);
> +    }
> +
> +  return false;
> +}
> +
> +recording::type *
> +strip_outer_qualifiers (recording::type *type)

...and again, can the return type and input type be const?

> +{
> +  while (true)
> +    {
> +      if (!type)
> +     gcc_unreachable (); /* Should only happen on corrupt input */
> +
> +      /* unqualified() returns 'this' on base types, vector, arrays and
> +      pointers. */
> +      recording::type *next = type->unqualified ();
> +      if (next == type)
> +     {
> +       break;
> +     }
> +      type = next;
> +    }
> +
> +  return type;
> +}
> +
> +recording::type *
> +get_stripped_subtype (recording::type *type)

...and again.

> +{
> +  recording::type *stripped = strip_outer_qualifiers (type);
> +  recording::type *subtype;
> +
> +  if ((subtype = stripped->is_pointer()))
> +    return strip_outer_qualifiers (subtype);
> +
> +  if ((subtype = stripped->is_array ()))
> +    return strip_outer_qualifiers (subtype);
> +
> +  if ((subtype = stripped->dyn_cast_vector_type ()))
> +    return strip_outer_qualifiers (subtype);
> +
> +  return NULL;
> +}
> +
>  } // namespace gcc::jit
>

[...snip...]

> diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
> index b94cdc85c8e..019172854a7 100644
> --- a/gcc/jit/libgccjit++.h
> +++ b/gcc/jit/libgccjit++.h
> @@ -206,6 +206,11 @@ namespace gccjit
>      rvalue new_rvalue (type vector_type,
>                       std::vector<rvalue> elements) const;
>  
> +    rvalue new_constructor (type type_,
> +                         std::vector<field> &fields,
> +                         std::vector<rvalue> &values,
> +                         location loc = location ());

Similar considerations as per the C API.

[...snip...]

>  /* Set an initial value for a global, which must be an array of
> diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> index 4022dbb6fbc..c5a1afdeacf 100644
> --- a/gcc/jit/libgccjit.map
> +++ b/gcc/jit/libgccjit.map
> @@ -208,9 +208,11 @@ LIBGCCJIT_ABI_15 {
>  
>  LIBGCCJIT_ABI_16 {
>    global:
> +    gcc_jit_context_new_constructor;
>      gcc_jit_context_new_rvalue_from_complex_double;
>      gcc_jit_context_new_rvalue_from_complex_long_double;
> -    gcc_jit_context_set_bool_enable_complex_types;
>      gcc_jit_context_new_rvalue_from_long_double;
>      gcc_jit_context_new_rvalue_from_long_long;
> +    gcc_jit_context_set_bool_enable_complex_types;
> +    gcc_jit_global_set_initializer_rvalue;
>  } LIBGCCJIT_ABI_15;

LIBGCCJIT_ABI_16 is in trunk, so we'll need a new ID for this.

> \ No newline at end of file
> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index 8416b312bad..26f45fe0811 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -211,6 +211,13 @@
>  #undef create_code
>  #undef verify_code
>  
> +/* test-global-init-rvalue.c */
> +#define create_code create_code_global_init_rvalue
> +#define verify_code verify_code_global_init_rvalue
> +#include "test-global-init-rvalue.c"
> +#undef create_code
> +#undef verify_code
> +
>  /* test-global-set-initializer.c */
>  #define create_code create_code_global_set_initializer
>  #define verify_code verify_code_global_set_initializer
> @@ -232,6 +239,13 @@
>  #undef create_code
>  #undef verify_code
>  
> +/* test-local-init-rvalue.c */
> +#define create_code create_code_local_init_rvalue
> +#define verify_code verify_code_local_init_rvalue
> +#include "test-local-init-rvalue.c"
> +#undef create_code
> +#undef verify_code
> +
>  /* test-long-names.c */
>  #define create_code create_code_long_names
>  #define verify_code verify_code_long_names

You should add entries for the above to the "testcases" array at the
bottom of the file.

Thanks for all the error-handling test coverage...

> diff --git a/gcc/testsuite/jit.dg/test-error-ctor-struct-too-big.c b/gcc/testsuite/jit.dg/test-error-ctor-struct-too-big.c
> new file mode 100644
> index 00000000000..a66b894dfde
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-error-ctor-struct-too-big.c
> @@ -0,0 +1,86 @@
> +/*
> +
> +  Test that the proper error is triggered when we build a ctor
> +  for an struct type, but have too many fields.
> +
> +*/
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +#include "harness.h"
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> +  gcc_jit_type *int_type = gcc_jit_context_get_type (ctxt,
> +    GCC_JIT_TYPE_INT);
> +  
> +  gcc_jit_field *b1 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "a");
> +  gcc_jit_field *b2 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "b");
> +  gcc_jit_field *b3 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "c");
> +  gcc_jit_field *fields_b[] = {b1, b2, b3};
> +
> +  gcc_jit_type *struct_bar_type =
> +    gcc_jit_struct_as_type (
> +      gcc_jit_context_new_struct_type (ctxt,
> +                                    0,
> +                                    "bar",
> +                                    3,
> +                                    fields_b));
> +  
> +  gcc_jit_field *b11 = gcc_jit_context_new_field (ctxt,
> +                                               0,
> +                                               int_type,
> +                                               "a");
> +  gcc_jit_field *b22 = gcc_jit_context_new_field (ctxt,
> +                                               0,
> +                                               int_type,
> +                                               "b");
> +  gcc_jit_field *b33 = gcc_jit_context_new_field (ctxt,
> +                                               0,
> +                                               int_type,
> +                                               "c");
> +  gcc_jit_field *b44 = gcc_jit_context_new_field (ctxt,
> +                                               0,
> +                                               int_type,
> +                                               "c");

Two fields called "c" here, FWIW.
But given that these are different field instances, it should complain
about that, also.

> +
> +  gcc_jit_field *fields_ctor[] = {b11, b22, b33, b44};
> +  gcc_jit_rvalue *values[] = {0,0,0,0};
> +
> +  gcc_jit_rvalue *ctor = gcc_jit_context_new_constructor
> +    (ctxt, 0,
> +     struct_bar_type,
> +     4,
> +     fields_ctor,
> +     values);
> +
> +  CHECK_VALUE (ctor, NULL);
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> +  /* Ensure that the bad API usage prevents the API giving a bogus
> +     result back.  */
> +  CHECK_VALUE (result, NULL);
> +
> +  /* Verify that the correct error message was emitted. */
> +  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> +                   "gcc_jit_context_new_constructor: more fields in "
> +                   "constructor than in the target struct or union");
> +  CHECK_STRING_VALUE (gcc_jit_context_get_last_error (ctxt),
> +                   "gcc_jit_context_new_constructor: more fields in "
> +                   "constructor than in the target struct or union");
> +}
> diff --git a/gcc/testsuite/jit.dg/test-error-ctor-struct-wrong-field-name.c b/gcc/testsuite/jit.dg/test-error-ctor-struct-wrong-field-name.c
> new file mode 100644
> index 00000000000..2b31b2fc123
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-error-ctor-struct-wrong-field-name.c
> @@ -0,0 +1,83 @@
> +/*
> +
> +  Test that the proper error is triggered when we build a ctor
> +  for an struct type, but has the name wrong on a field.
> +
> +*/
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +#include "harness.h"
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> +  gcc_jit_type *int_type = gcc_jit_context_get_type (ctxt,
> +    GCC_JIT_TYPE_INT);
> +
> +  gcc_jit_field *b1 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "a");
> +  gcc_jit_field *b2 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "b");
> +  gcc_jit_field *b3 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "c");
> +  gcc_jit_field *b4 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "d");
> +  gcc_jit_field *b5 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "d");

Should this be named "e"?

> +  gcc_jit_field *fields_b[] = {b1, b2, b3, b4, b5};
> +
> +  gcc_jit_type *struct_bar_type =
> +    gcc_jit_struct_as_type (
> +      gcc_jit_context_new_struct_type (ctxt,
> +                                    0,
> +                                    "bar",
> +                                    5,
> +                                    fields_b));
> +
> +
> +  gcc_jit_field *b44 = gcc_jit_context_new_field (ctxt,
> +                                               0,
> +                                               int_type,
> +                                               "d");

Maybe call this "something_else" for clarity?

> +
> +  gcc_jit_field *fields_ctor[] = {b1, b2, b44, b5};
> +  gcc_jit_rvalue *values[] = {0,0,0,0};
> +
> +  gcc_jit_rvalue *ctor = gcc_jit_context_new_constructor
> +    (ctxt, 0,
> +     struct_bar_type,
> +     4,
> +     fields_ctor,
> +     values);
> +
> +  CHECK_VALUE (ctor, NULL);
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> +  /* Ensure that the bad API usage prevents the API giving a bogus
> +     result back.  */
> +  CHECK_VALUE (result, NULL);
> +
> +  /* Verify that the correct error message was emitted. */
> +  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> +                   "gcc_jit_context_new_constructor: field at index 2, was "
> +                   "not used when creating the union or struct (d)");
> +  CHECK_STRING_VALUE (gcc_jit_context_get_last_error (ctxt),
> +                   "gcc_jit_context_new_constructor: field at index 2, was "
> +                   "not used when creating the union or struct (d)");

Ideally the error message should contain both the struct/union name,
and the bad field name
e.g.:
  gcc_jit_context_new_constructor: field at index 2 (d) is not an
element of struct "bar"


> diff --git a/gcc/testsuite/jit.dg/test-error-ctor-struct-wrong-type.c b/gcc/testsuite/jit.dg/test-error-ctor-struct-wrong-type.c
> new file mode 100644
> index 00000000000..987b6b8fcbd
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-error-ctor-struct-wrong-type.c
> @@ -0,0 +1,76 @@
> +/*
> +
> +  Test that the proper error is triggered when we build a ctor
> +  for an struct type, but has the type wrong on a field.
> +
> +*/
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +#include "harness.h"
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> +  gcc_jit_type *int_type = gcc_jit_context_get_type (ctxt,
> +    GCC_JIT_TYPE_INT);
> +  gcc_jit_type *float_type = gcc_jit_context_get_type (ctxt,
> +    GCC_JIT_TYPE_FLOAT);
> +
> +  gcc_jit_field *b1 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "a");
> +  gcc_jit_field *b2 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "b");
> +  gcc_jit_field *b3 = gcc_jit_context_new_field (ctxt,
> +                                              0,
> +                                              int_type,
> +                                              "c");
> +  gcc_jit_field *fields_b[] = {b1, b2, b3};
> +
> +  gcc_jit_type *struct_bar_type =
> +    gcc_jit_struct_as_type (
> +      gcc_jit_context_new_struct_type (ctxt,
> +                                    0,
> +                                    "bar",
> +                                    3,
> +                                    fields_b));
> +  gcc_jit_rvalue *frv = gcc_jit_context_new_rvalue_from_double (ctxt,
> +                                                             float_type,
> +                                                             12);
> +
> +  gcc_jit_field *fields_ctor[] = {b2};
> +  gcc_jit_rvalue *values[] = {frv};
> +
> +  gcc_jit_rvalue *ctor = gcc_jit_context_new_constructor
> +    (ctxt, 0,
> +     struct_bar_type,
> +     1,
> +     fields_ctor,
> +     values);
> +
> +  CHECK_VALUE (ctor, NULL);
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> +  /* Ensure that the bad API usage prevents the API giving a bogus
> +     result back.  */
> +  CHECK_VALUE (result, NULL);
> +
> +  /* Verify that the correct error message was emitted. */
> +  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> +                   "gcc_jit_context_new_constructor: value and field not "
> +                   "the same unqualified type, "
> +                   "at index 0 (field type: int)(value type: float)");
> +  CHECK_STRING_VALUE (gcc_jit_context_get_last_error (ctxt),
> +                   "gcc_jit_context_new_constructor: value and field not "
> +                   "the same unqualified type, "
> +                   "at index 0 (field type: int)(value type: float)");

This is good; even better would be to also give the name of the field
and name of the struct.


[...snip...]

> diff --git a/gcc/testsuite/jit.dg/test-error-global-allready-init.c b/gcc/testsuite/jit.dg/test-error-global-allready-init.c
> new file mode 100644
> index 00000000000..7eaf182029d
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-error-global-allready-init.c
> @@ -0,0 +1,46 @@
> +/*
> +
> +  Test that we can't set the initializer on a global twice.
> +
> +*/
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +#include "harness.h"
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> +  gcc_jit_type *int_type = gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> +
> +  gcc_jit_lvalue *bar =  gcc_jit_context_new_global (
> +    ctxt, NULL,
> +    GCC_JIT_GLOBAL_EXPORTED,
> +    int_type,
> +    "global_lvalueinit_int_0");
> +
> +  gcc_jit_global_set_initializer_rvalue (
> +    bar,
> +    gcc_jit_context_new_rvalue_from_int (ctxt, int_type, 1));
> +  gcc_jit_global_set_initializer_rvalue (
> +    bar,
> +    gcc_jit_context_new_rvalue_from_int (ctxt, int_type, 1));
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> +  /* Ensure that the bad API usage prevents the API giving a bogus
> +     result back.  */
> +  CHECK_VALUE (result, NULL);
> +
> +  /* Verify that the correct error message was emitted. */
> +  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> +                   "gcc_jit_global_set_initializer_rvalue: global variable "
> +                   "allready initialized: global_lvalueinit_int_0");

"allready" -> "already" (in the implementation, of course)

> +  CHECK_STRING_VALUE (gcc_jit_context_get_last_error (ctxt),
> +                   "gcc_jit_global_set_initializer_rvalue: global variable "
> +                   "allready initialized: global_lvalueinit_int_0");
> +}

[...snip...]

> diff --git a/gcc/testsuite/jit.dg/test-error-global-init-too-small-array.c b/gcc/testsuite/jit.dg/test-error-global-init-too-small-array.c
> new file mode 100644
> index 00000000000..137bae6c6b5
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-error-global-init-too-small-array.c
> @@ -0,0 +1,64 @@
> +/*
> +
> +  Test that the proper error is triggered when we initialize
> +  a global with another non-const global's rvalue.
> +
> +  Using gcc_jit_global_set_initializer_rvalue()
> +
> +*/
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +#include "harness.h"
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{ /* float foo[1] = {1,2}; */
> +
> +  gcc_jit_type *float_type = gcc_jit_context_get_type (ctxt,
> +    GCC_JIT_TYPE_FLOAT);
> +  gcc_jit_type *arr_type = gcc_jit_context_new_array_type (ctxt,
> +                                                        0,
> +                                                        float_type,
> +                                                        1);
> +  gcc_jit_rvalue *rval_1 = gcc_jit_context_new_rvalue_from_int (
> +    ctxt, float_type, 1);
> +  gcc_jit_rvalue *rval_2 = gcc_jit_context_new_rvalue_from_int (
> +    ctxt, float_type, 2);
> +
> +  gcc_jit_rvalue *values[] = {rval_1, rval_2};
> +
> +  gcc_jit_rvalue *ctor = gcc_jit_context_new_constructor (ctxt,
> +                                                       0,
> +                                                       arr_type,
> +                                                       2,
> +                                                       0,
> +                                                       values);
> +  if (!ctor)
> +    return;
> +  
> +  gcc_jit_lvalue *foo =  gcc_jit_context_new_global (
> +    ctxt, NULL,
> +    GCC_JIT_GLOBAL_EXPORTED,
> +    arr_type,
> +    "global_floatarr_12");
> +  gcc_jit_global_set_initializer_rvalue (foo, ctor);
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> +  /* Ensure that the bad API usage prevents the API giving a bogus
> +     result back.  */
> +  CHECK_VALUE (result, NULL);
> +
> +  /* Verify that the correct error message was emitted. */
> +  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> +                   "gcc_jit_context_new_constructor: array constructor has"
> +                   " more values than the array type's length");
> +  CHECK_STRING_VALUE (gcc_jit_context_get_last_error (ctxt),
> +                   "gcc_jit_context_new_constructor: array constructor has"
> +                   " more values than the array type's length");
> +}

Ideally would also tell the user the specific values.

[...snip...]

Thanks again for the patches; hope this is constructive.  As noted
above, I'm keen on hearing Antoni's opinion of the patch.

Dave