Re: [PATCH] libgccjit: add some reflection functions in the jit C api

[David Malcolm <dmalcolm@redhat.com>]

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