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

[David Malcolm <dmalcolm@redhat.com>]

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