SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089]
David Malcolm
dmalcolm@redhat.com
Tue Nov 30 01:34:29 GMT 2021
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
More information about the Jit
mailing list