From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 0753A385840B for ; Tue, 30 Nov 2021 01:34:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0753A385840B Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-424-6gRQvj6JNJKgKaV8_r2rEg-1; Mon, 29 Nov 2021 20:34:33 -0500 X-MC-Unique: 6gRQvj6JNJKgKaV8_r2rEg-1 Received: by mail-qk1-f199.google.com with SMTP id o19-20020a05620a22d300b0046754380e8aso26902214qki.13 for ; Mon, 29 Nov 2021 17:34:33 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=XeTaEzTCARN8V9UPI75laaicvaM/tcN4C12bc4BTo9U=; b=bSx/EYuyKgxptkEHuwN8nGj1yTNMSCTN/pqjmNzE2v29Y5kujRDGr8SLNfTsimewHX iU1LGkUk2w6jXg7bAK3eHIBaB5VqrLutUqOAwsPGV8dQ8YP02rgZIm5gho3uE0ko/gCT B+lyKWvbIX6rvFtDq3zYS9PvL8/Rb2LT3iAPwLKpexKz1I2XZfANvLKYcM8/D+KebUQq CZPjh6kkqZxfiVaTQJuEUo/W36retovEUaN1+48ETg9fVWmVLtZAfZaTkidlj0DH1hn+ pUR8SDOg8qLYQFzEnRbGE8fyRhTAsxWiJVZYr29qBt0aXd9sr2XwptoCIB1jyk+5RHfu hEWw== X-Gm-Message-State: AOAM5311X3eu2OqD+BpNCQHmC8GAVhnk+P92/U3zachBYGQ2cvDSAodm 341K0JikJjNY0T2Pzc/4iflQEAg7ZrY4RiKxwuQfUrD/HLHdhctduV6TpdVpuAR6+UeN1+BvOKc nwCJGN8c= X-Received: by 2002:a05:622a:30b:: with SMTP id q11mr48378555qtw.348.1638236072385; Mon, 29 Nov 2021 17:34:32 -0800 (PST) X-Google-Smtp-Source: ABdhPJwdWnSs/J5V8YZ01OamGR7rrlr2dIikeHpvJw12xtEFFCS+sH6Qwc48s1RhnTBzqTrIywGbIQ== X-Received: by 2002:a05:622a:30b:: with SMTP id q11mr48378530qtw.348.1638236072019; Mon, 29 Nov 2021 17:34:32 -0800 (PST) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id 73sm8727205qkm.94.2021.11.29.17.34.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 Nov 2021 17:34:31 -0800 (PST) Message-ID: Subject: Re: SV: [PATCH] libgccjit: Add function to set the initial value of a global variable [PR96089] From: David Malcolm To: Petter Tomner , Antoni Boucher , "jit@gcc.gnu.org" Date: Mon, 29 Nov 2021 20:34:29 -0500 In-Reply-To: References: <7d6762623f658712a4b802cc3d6524c106affa2b.camel@zoho.com> <62c178b5bb0ab004def3c7869da2b03846e5134c.camel@zoho.com> , User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP, WEIRD_QUOTING autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Nov 2021 01:34:41 -0000 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 för > Antoni Boucher via Gcc-patches > 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 > 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 (base_a); > + recording::array_type *arr_b = > + static_cast (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 (base_a); > + recording::vector_type *arr_b = > + static_cast (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 elements) const; > > + rvalue new_constructor (type type_, > + std::vector &fields, > + std::vector &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 > +#include > + > +#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 > +#include > + > +#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 > +#include > + > +#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 > +#include > + > +#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 > +#include > + > +#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