Thanks for your reviews. I attached the new patch to this email. See answers below: 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? For those, I had this question in a previous email: 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. > - 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. That's fixed in the new patch. > 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. This was updated for the new patch. > [...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. The documentation was added in the new patch. > [...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. This is fixed in the patch. > > > + > > +  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". Fixed in the patch. > > > > +  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". Fixed in the patch > > > +  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". Fixed in the patch. > > > > +  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. Fixed in the patch. > > > +  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; Done. > > [...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. As explained aboved, it is to indicate an error. > > 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. Fixed. > > [...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 > > +#include > > + > > +#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) I added tests for those. > > Hope this is constructive; sorry again about the delay in reviewing > it. > Dave > >