On Tue, 2021-05-25 at 20:19 -0400, Antoni Boucher wrote:
> @David: PING
>
> As far as I know, the only remaining question is about using
> `ssize_t`
> for the return type of some functions.
> Here's why I use this type:
>
> That seemed off to return NULL for the functions returning a
> size_t to indicate an error. So I changed it to return -1 (and return
> type to ssize_t). Is that the proper way to indicate an error?
>
> Once I know the answer for this error handling question, I'll fix the
> types.
>
> Thanks!
I think the things that accept params for indexes should be size_t
(i.e. unsigned).
As for the error-handling, I've been going back and forth.
The various return of -1 in the patch all occur when NULL is passed in
as the gcc_jit_foo*, for the various foo. Unfortunately this means that
we can't get at a jit context to emit the error on, so jit_error is
called with a NULL context, and hence merely prints to stderr; it can't
set state on any jit context.
Presumably if the code is passing in NULL for one of these things, then
something's gone wrong already.
Option (A): returning a size_t (unsigned):
- No way to indicate error if the user passes in NULL, or a bogus
value, beyond output on stderr; we have to defensively return 0, which
the user's code is likely to gracefully handle
Option (B): returning a ssize_t:
- Can return -1 if the user passes in NULL or a bogus value. But if
the user doesn't check for -1 and blithely casts to unsigned (e.g.
allocating a buffer), their code is likely to explode with a very large
allocation.
Consider e.g. gcc_jit_function_type_get_param_count. If the user uses
gcc_jit_type_is_function_ptr_type to dynamically cast some type to
function_type and forgets to check, and passes in NULL, then...
...with option (A), it returns 0, as if the function has no params;
eventually a type-checking error occurs.
...with option (B), it returns -1, and the user who didn't check before
has to check again
So I think I prefer option (A), but I'm willing to be convinced on the
alternative.
I also now think that the dynamic cast functions (e.g.
gcc_jit_type_is_function_ptr_type) should be "_dyncast_" rather than
"_is_" (e.g. gcc_jit_type_dyncast_function_ptr_type) to make it clear
that this is a dynamic cast that can fail, and to contrast them with
the "_as_" functions which are static casts.
Hope this makes sense and is constructive.
Dave
>
> Le jeudi 13 mai 2021 à 17:30 -0400, David Malcolm a écrit :
> > On Tue, 2020-11-03 at 17:13 -0500, Antoni Boucher wrote:
> > > I was missing a check in gcc_jit_struct_get_field, I added it in
> > > this
> > > new patch.
> > >
> >
> > Sorry about the long delay in reviewing this patch.
> >
> > The main high-level points are:
> > - currently the get_*_count functions return "ssize_t" - why? Only
> > unsigned values are meaningful; shouldn't they return "size_t"
> > instead?
> >
> > - the various "lookup by index" functions take "int" i.e. signed, but
> > only >= 0 is meaningful. I think it makes sense to make them take
> > size_t instead.
> >
> > Sorry if we covered that before in the review, it's been a while.
> >
> > Various nitpicks inline below...
> >
> > [...snip...]
> >
> > > diff --git a/gcc/jit/docs/topics/compatibility.rst
> > > b/gcc/jit/docs/topics/compatibility.rst
> > > index 6bfa101ed71..236e5c72d81 100644
> > > --- a/gcc/jit/docs/topics/compatibility.rst
> > > +++ b/gcc/jit/docs/topics/compatibility.rst
> > > @@ -226,3 +226,44 @@ entrypoints:
> > > --------------------
> > > ``LIBGCCJIT_ABI_14`` covers the addition of
> > > :func:`gcc_jit_global_set_initializer`
> > > +
> > > +.. _LIBGCCJIT_ABI_15:
> > > +
> > > +``LIBGCCJIT_ABI_15``
> > > +--------------------
> > > +``LIBGCCJIT_ABI_15`` covers the addition of reflection functions
> > > via
> > > API
> > > +entrypoints:
> >
> > This needs updating, as I used LIBGCCJIT_ABI_15 for inline asm.
> >
> > [...snip...]
> >
> > > diff --git a/gcc/jit/docs/topics/functions.rst
> > > b/gcc/jit/docs/topics/functions.rst
> > > index eb40d64010e..aa6de87282d 100644
> > > --- a/gcc/jit/docs/topics/functions.rst
> > > +++ b/gcc/jit/docs/topics/functions.rst
> > > @@ -171,6 +171,16 @@ Functions
> > > underlying string, so it is valid to pass in a pointer to an
> > > on-
> > > stack
> > > buffer.
> > >
> > > +.. function:: ssize_t \
> > > + gcc_jit_function_get_param_count (gcc_jit_function
> > > *func)
> > > +
> > > + Get the number of parameters of the function.
> > > +
> > > +.. function:: gcc_jit_type \*
> > > + gcc_jit_function_get_return_type (gcc_jit_function
> > > *func)
> > > +
> > > + Get the return type of the function.
> >
> > As noted before, this doesn't yet document all the new entrypoints; I
> > think you wanted to hold off until all the details were thrashed out,
> > but hopefully we're close.
> >
> > The documentation for an entrypoint should specify which ABI it was
> > added in.
> >
> > [...snip...]
> >
> > > +/* Public entrypoint. See description in libgccjit.h.
> > > +
> > > + After error-checking, the real work is done by the
> > > + gcc::jit::recording::type::is_struct method, in
> > > + jit-recording.c. */
> > > +
> > > +gcc_jit_struct *
> > > +gcc_jit_type_is_struct (gcc_jit_type *type)
> > > +{
> > > + RETURN_NULL_IF_FAIL (type, NULL, NULL, "NULL type");
> > > + gcc::jit::recording::struct_ *struct_type = type->is_struct ();
> > > + return (gcc_jit_struct *)struct_type;
> > > +}
> > > +
> > > +/* Public entrypoint. See description in libgccjit.h.
> > > +
> > > + After error-checking, the real work is done by the
> > > + gcc::jit::recording::vector_type::get_num_units method, in
> > > + jit-recording.c. */
> > > +
> > > +ssize_t
> > > +gcc_jit_vector_type_get_num_units (gcc_jit_vector_type
> > > *vector_type)
> > > +{
> > > + RETURN_VAL_IF_FAIL (vector_type, -1, NULL, NULL, "NULL
> > > vector_type");
> > > + return vector_type->get_num_units ();
> > > +}
> > > +
> > > +/* Public entrypoint. See description in libgccjit.h.
> > > +
> > > + After error-checking, the real work is done by the
> > > + gcc::jit::recording::vector_type::get_element_type method, in
> > > + jit-recording.c. */
> > > +
> > > +gcc_jit_type *
> > > +gcc_jit_vector_type_get_element_type (gcc_jit_vector_type
> > > *vector_type)
> > > +{
> > > + RETURN_NULL_IF_FAIL (vector_type, NULL, NULL, "NULL
> > > vector_type");
> > > + return (gcc_jit_type *)vector_type->get_element_type ();
> > > +}
> > > +
> > > +/* Public entrypoint. See description in libgccjit.h.
> > > +
> > > + After error-checking, the real work is done by the
> > > + gcc::jit::recording::type::unqualified method, in
> > > + jit-recording.c. */
> > > +
> > > +gcc_jit_type *
> > > +gcc_jit_type_unqualified (gcc_jit_type *type)
> > > +{
> > > + RETURN_NULL_IF_FAIL (type, NULL, NULL, "NULL type");
> > > +
> > > + return (gcc_jit_type *)type->unqualified ();
> > > +}
> > > +
> > > +/* Public entrypoint. See description in libgccjit.h.
> > > +
> > > + After error-checking, the real work is done by the
> > > + gcc::jit::recording::type::dyn_cast_function_type method, in
> > > + jit-recording.c. */
> > > +
> > > +gcc_jit_function_type *
> > > +gcc_jit_type_is_function_ptr_type (gcc_jit_type *type)
> > > +{
> > > + RETURN_NULL_IF_FAIL (type, NULL, NULL, "NULL type");
> > > + gcc::jit::recording::type *func_ptr_type = type->dereference ();
> > > + RETURN_NULL_IF_FAIL (func_ptr_type, NULL, NULL, "NULL type");
> > > + gcc::jit::recording::function_type *func_type =
> > > + func_ptr_type->dyn_cast_function_type ();
> > > + RETURN_NULL_IF_FAIL (func_type, NULL, NULL, "NULL type");
> >
> > I notice that the other new "*_is_*" functions don't fail if the
> > dyncast returns NULL, whereas this one does.
> >
> > RETURN_NULL_IF_FAIL calls jit_error; do we really want that? It
> > seems
> > more consistent to have it return NULL without an error for the case
> > where "type" isn't a function ptr type.
> >
> > > +
> > > + return (gcc_jit_function_type *)func_type;
> > > +}
> > > +
> > > +/* Public entrypoint. See description in libgccjit.h.
> > > +
> > > + After error-checking, the real work is done by the
> > > + gcc::jit::recording::function_type::get_return_type method, in
> > > + jit-recording.c. */
> > > +
> > > +gcc_jit_type *
> > > +gcc_jit_function_type_get_return_type (gcc_jit_function_type
> > > *function_type)
> > > +{
> > > + RETURN_NULL_IF_FAIL (function_type, NULL, NULL, "NULL
> > > struct_type");
> >
> > Wrong error message; should be "NULL function_type".
> >
> >
> > > + return (gcc_jit_type *)function_type->get_return_type ();
> > > +}
> > > +
> > > +/* Public entrypoint. See description in libgccjit.h.
> > > +
> > > + After error-checking, the real work is done by the
> > > + gcc::jit::recording::function_type::get_param_types method, in
> > > + jit-recording.c. */
> > > +
> > > +ssize_t
> > > +gcc_jit_function_type_get_param_count (gcc_jit_function_type
> > > *function_type)
> > > +{
> > > + RETURN_VAL_IF_FAIL (function_type, -1, NULL, NULL, "NULL
> > > struct_type");
> >
> > Wrong error message; should be "NULL function_type".
> >
> > > + return function_type->get_param_types ().length ();
> > > +}
> > > +
> > > +/* Public entrypoint. See description in libgccjit.h.
> > > +
> > > + After error-checking, the real work is done by the
> > > + gcc::jit::recording::function_type::get_param_types method, in
> > > + jit-recording.c. */
> > > +
> > > +gcc_jit_type *
> > > +gcc_jit_function_type_get_param_type (gcc_jit_function_type
> > > *function_type,
> > > + int index)
> > > +{
> > > + RETURN_NULL_IF_FAIL (function_type, NULL, NULL, "NULL
> > > struct_type");
> >
> > Wrong error message; should be "NULL function_type".
> >
> >
> > > + int num_params = function_type->get_param_types ().length ();
> > > + gcc::jit::recording::context *ctxt = function_type->m_ctxt;
> >
> > "index" should either be unsigned, or we need to check here for >= 0.
> >
> > > + RETURN_NULL_IF_FAIL_PRINTF3 (index < num_params,
> > > + ctxt, NULL,
> > > + "index of %d is too large (%s has %d
> > > params)",
> > > + index,
> > > + function_type->get_debug_string (),
> > > + num_params);
> > > + return (gcc_jit_type *)function_type->get_param_types ()[index];
> > > +}
> > > +
> > > /* Public entrypoint. See description in libgccjit.h.
> > >
> > > After error-checking, the real work is done by the
> > > @@ -731,6 +930,36 @@ gcc_jit_struct_set_fields (gcc_jit_struct
> > > *struct_type,
> > > (gcc::jit::recording::field **)fields);
> > > }
> > >
> > > +
> > > +/* Public entrypoint. See description in libgccjit.h.
> > > +
> > > + After error-checking, the real work is done by the
> > > + gcc::jit::recording::fields::get_field method in
> > > + jit-recording.c. */
> > > +extern gcc_jit_field *
> > > +gcc_jit_struct_get_field (gcc_jit_struct *struct_type,
> > > + int index)
> > > +{
> > > + RETURN_NULL_IF_FAIL (struct_type, NULL, NULL, "NULL struct
> > > type");
> > > + RETURN_NULL_IF_FAIL (struct_type->get_fields (), NULL, NULL,
> > > "NULL
> > > struct fields");
> >
> > "index" should either be unsigned, or we need to check here for >= 0.
> >
> > > + RETURN_NULL_IF_FAIL (index < struct_type->get_fields ()->length
> > > (), NULL,
> > > + NULL, "NULL struct type");
> > > + return (gcc_jit_field *)struct_type->get_fields ()->get_field
> > > (index);
> > > +}
> >
> > [...snip...]
> >
> > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > index 7134841bb07..db4ccffb41c 100644
> > > --- a/gcc/jit/libgccjit.h
> > > +++ b/gcc/jit/libgccjit.h
> > > @@ -96,6 +96,12 @@ typedef struct gcc_jit_field gcc_jit_field;
> > > the layout for, or an opaque type. */
> > > typedef struct gcc_jit_struct gcc_jit_struct;
> > >
> > > +/* A gcc_jit_function_type encapsulates a function type. */
> > > +typedef struct gcc_jit_function_type gcc_jit_function_type;
> > > +
> > > +/* A gcc_jit_vector_type encapsulates a vector type. */
> > > +typedef struct gcc_jit_vector_type gcc_jit_vector_type;
> > > +
> >
> > Please add these to the ASCII art inheritance diagram immediately
> > above:
> > typedef struct gcc_jit_object gcc_jit_object;
> >
> > [...snip...]
> >
> > > /* Unions work similarly to structs. */
> > > extern gcc_jit_type *
> > > gcc_jit_context_new_union_type (gcc_jit_context *ctxt,
> > > @@ -1518,6 +1533,78 @@ gcc_jit_version_minor (void);
> > > extern int
> > > gcc_jit_version_patchlevel (void);
> > >
> > > +#define LIBGCCJIT_HAVE_REFLECTION
> > > +
> > > +/* Reflection functions to get the number of parameters, return
> > > type
> > > of
> > > + a function and whether a type is a bool from the C API.
> > > +
> > > + This API entrypoint was added in LIBGCCJIT_ABI_15; you can test
> > > for its
> > > + presence using
> > > + #ifdef LIBGCCJIT_HAVE_REFLECTION
> > > +*/
> > > +/* Get the return type of a function. */
> > > +extern gcc_jit_type *
> > > +gcc_jit_function_get_return_type (gcc_jit_function *func);
> > > +
> > > +/* Get the number of params of a function. */
> > > +extern ssize_t
> > > +gcc_jit_function_get_param_count (gcc_jit_function *func);
> >
> > You're using ssize_t here, why? Wouldn't size_t be better?
> > The count can't be negative.
> >
> > Should we use size_t for indexes, rather than "int"?
> >
> > (this happens in various places e.g.
> > gcc_jit_function_type_get_param_count and
> > gcc_jit_vector_type_get_num_units)
> >
> > [...snip...]
> >
> > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > index a6e67e781a4..720b6ca286f 100644
> > > --- a/gcc/jit/libgccjit.map
> > > +++ b/gcc/jit/libgccjit.map
> > > @@ -192,3 +192,24 @@ LIBGCCJIT_ABI_14 {
> > > global:
> > > gcc_jit_global_set_initializer;
> > > } LIBGCCJIT_ABI_13;
> > > +
> > > +LIBGCCJIT_ABI_15 {
> > > + global:
> > > + gcc_jit_function_get_return_type;
> > > + gcc_jit_function_get_param_count;
> > > + gcc_jit_function_type_get_return_type;
> > > + gcc_jit_function_type_get_param_count;
> > > + gcc_jit_function_type_get_param_type;
> > > + gcc_jit_type_unqualified;
> > > + gcc_jit_type_is_array;
> > > + gcc_jit_type_is_bool;
> > > + gcc_jit_type_is_function_ptr_type;
> > > + gcc_jit_type_is_integral;
> > > + gcc_jit_type_is_pointer;
> > > + gcc_jit_type_is_vector;
> > > + gcc_jit_vector_type_get_element_type;
> > > + gcc_jit_vector_type_get_num_units;
> > > + gcc_jit_struct_get_field;
> > > + gcc_jit_type_is_struct;
> > > + gcc_jit_struct_get_field_count;
> > > +} LIBGCCJIT_ABI_14;
> >
> > As noted above, this will need to be LIBGCCJIT_ABI_16 as I used
> > LIBGCCJIT_ABI_15 for inline asm.
> >
> > [...snip...]
> >
> > > diff --git a/gcc/testsuite/jit.dg/test-reflection.c
> > > b/gcc/testsuite/jit.dg/test-reflection.c
> > > new file mode 100644
> > > index 00000000000..b3cf2942f3a
> > > --- /dev/null
> > > +++ b/gcc/testsuite/jit.dg/test-reflection.c
> > > @@ -0,0 +1,89 @@
> > > +#include <stdlib.h>
> > > +#include <stdio.h>
> > > +
> > > +#include "libgccjit.h"
> > > +
> > > +#include "harness.h"
> > > +
> > > +void
> > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > +{
> > > + /* Do nothing. */
> > > +}
> > > +
> > > +void
> > > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> > > +{
> > > + /* Get the built-in functions. */
> > > + gcc_jit_function *builtin_sin =
> > > + gcc_jit_context_get_builtin_function (ctxt, "sin");
> > > +
> > > + CHECK_VALUE (gcc_jit_function_get_param_count(builtin_sin), 1);
> > > +
> > > + gcc_jit_type *double_type =
> > > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_DOUBLE);
> > > + CHECK_VALUE (gcc_jit_function_get_return_type(builtin_sin),
> > > double_type);
> > > + CHECK (!gcc_jit_type_is_integral(double_type));
> > > +
> > > + gcc_jit_type *bool_type =
> > > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_BOOL);
> > > + CHECK (gcc_jit_type_is_bool(bool_type));
> > > + CHECK (!gcc_jit_type_is_integral(bool_type));
> > > +
> > > + gcc_jit_type *aligned_bool_type =
> > > + gcc_jit_type_get_aligned(gcc_jit_context_get_type (ctxt,
> > > GCC_JIT_TYPE_BOOL), 8);
> > > + CHECK (gcc_jit_type_is_bool(aligned_bool_type));
> > > + CHECK (bool_type != aligned_bool_type);
> > > + CHECK_VALUE (gcc_jit_type_unqualified(aligned_bool_type),
> > > bool_type);
> > > +
> > > + CHECK_VALUE
> > > (gcc_jit_type_unqualified(gcc_jit_type_get_const(bool_type)),
> > > bool_type);
> > > + CHECK_VALUE
> > > (gcc_jit_type_unqualified(gcc_jit_type_get_volatile(bool_type)),
> > > bool_type);
> > > +
> > > + gcc_jit_type *int64 =
> > > + gcc_jit_context_get_int_type(ctxt, 8, 1);
> > > + CHECK (gcc_jit_type_is_integral(int64));
> > > + gcc_jit_type *uint64 =
> > > + gcc_jit_context_get_int_type(ctxt, 8, 0);
> > > + CHECK (gcc_jit_type_is_integral(uint64));
> > > + gcc_jit_type *int8 =
> > > + gcc_jit_context_get_int_type(ctxt, 1, 1);
> > > + CHECK (gcc_jit_type_is_integral(int8));
> > > + gcc_jit_type *uint8 =
> > > + gcc_jit_context_get_int_type(ctxt, 1, 0);
> > > + CHECK (gcc_jit_type_is_integral(uint8));
> > > +
> > > + CHECK (!gcc_jit_type_is_vector(double_type));
> > > + gcc_jit_type *vec_type = gcc_jit_type_get_vector (double_type,
> > > 4);
> > > + gcc_jit_vector_type *vector_type =
> > > gcc_jit_type_is_vector(vec_type);
> > > + CHECK (vector_type);
> > > + CHECK (vec_type != double_type);
> > > + CHECK_VALUE (gcc_jit_vector_type_get_element_type(vector_type),
> > > double_type);
> > > + CHECK_VALUE (gcc_jit_vector_type_get_num_units(vector_type), 4);
> > > +
> > > + CHECK (!gcc_jit_type_is_pointer(double_type));
> > > + CHECK_VALUE
> > > (gcc_jit_type_is_pointer(gcc_jit_type_get_pointer(double_type)),
> > > double_type);
> > > +
> > > + gcc_jit_type* params[2] = {int8, uint64};
> > > + gcc_jit_type *function_ptr_type =
> > > gcc_jit_context_new_function_ptr_type(ctxt, NULL, int64, 2, params,
> > > 0);
> > > + gcc_jit_function_type *function_type =
> > > gcc_jit_type_is_function_ptr_type (function_ptr_type);
> > > + CHECK (function_type);
> > > + int param_count =
> > > gcc_jit_function_type_get_param_count(function_type);
> > > + CHECK_VALUE (param_count, 2);
> > > + gcc_jit_type *return_type =
> > > gcc_jit_function_type_get_return_type(function_type);
> > > + CHECK_VALUE (return_type, int64);
> > > + gcc_jit_type *param1 =
> > > gcc_jit_function_type_get_param_type(function_type, 0);
> > > + CHECK_VALUE (param1, int8);
> > > + gcc_jit_type *param2 =
> > > gcc_jit_function_type_get_param_type(function_type, 1);
> > > + CHECK_VALUE (param2, uint64);
> > > +
> > > + gcc_jit_field *field1 = gcc_jit_context_new_field (ctxt, NULL,
> > > uint64, "field1");
> > > + gcc_jit_field *field2 = gcc_jit_context_new_field (ctxt, NULL,
> > > double_type, "field2");
> > > + gcc_jit_field *fields[2] = { field1, field2 };
> > > + gcc_jit_struct *struct_type = gcc_jit_context_new_struct_type
> > > (ctxt, NULL, "testStruct", 2, fields);
> > > + CHECK_VALUE (gcc_jit_struct_get_field_count(struct_type), 2);
> > > + CHECK_VALUE (gcc_jit_struct_get_field(struct_type, 0), field1);
> > > + CHECK_VALUE (gcc_jit_struct_get_field(struct_type, 1), field2);
> > > + gcc_jit_struct *struct_ty =
> > > gcc_jit_type_is_struct(gcc_jit_struct_as_type(struct_type));
> > > + CHECK_VALUE (struct_ty, struct_type);
> >
> >
> > I think this is missing test coverage for the various
> > gcc_jit_type_is_*
> > entrypoints gracefully returning NULL when the type passed to them is
> > wrong (e.g. a non-struct type passed to gcc_jit_type_is_struct, etc)
> >
> > Hope this is constructive; sorry again about the delay in reviewing
> > it.
> > Dave
> >
> >
>
>
