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

[David Malcolm <dmalcolm@redhat.com>]

On Thu, 2021-05-27 at 21:51 -0400, Antoni Boucher wrote:
> I chose option A, so everything is a size_t, now.
> I also renamed the dyncast functions.
> Here's the new patch.

Thanks, sorry again about the delays in reviewing your work.

You didn't specify how you tested the patch; are you running the full
jit regression test suite?

This is looking close to done; a few nits inline below:

[...snip...]

> diff --git a/gcc/jit/docs/topics/functions.rst b/gcc/jit/docs/topics/functions.rst
> index b2d9239aa0a..1d20e3045a0 100644
> --- a/gcc/jit/docs/topics/functions.rst
> +++ b/gcc/jit/docs/topics/functions.rst

[...snip...]

> +   The API entrypoints relating to getting info about parameters and return
> +   types:
> +
> +      * :c:func:`gcc_jit_function_get_return_type`
> +
> +      * :c:func:`gcc_jit_function_get_param_count`
> +
> +   were added in :ref:`LIBGCCJIT_ABI_15`; you can test for their presence
> +   using

16, rather than 15.

> +   The API entrypoints related to the reflection API:
> +
> +      * :c:func:`gcc_jit_function_type_get_return_type`
> +

[...snip...]

> +
> +      * :c:func:`gcc_jit_struct_get_field_count`
> +
> +   were added in :ref:`LIBGCCJIT_ABI_15`; you can test for their presence
> +   using

16, rather than 15 again.

> +
> +   .. code-block:: c
> +
> +      #ifdef LIBGCCJIT_HAVE_REFLECTION
> +
> +   .. type:: gcc_jit_case

[...snip...]

> +gcc_jit_type *
> +gcc_jit_function_type_get_param_type (gcc_jit_function_type *function_type,
> +				size_t index)
> +{
> +  RETURN_NULL_IF_FAIL (function_type, NULL, NULL, "NULL function_type");
> +  size_t num_params = function_type->get_param_types ().length ();
> +  gcc::jit::recording::context *ctxt = function_type->m_ctxt;
> +  RETURN_NULL_IF_FAIL_PRINTF3 (index < num_params,
> +			       ctxt, NULL,
> +			       "index of %ld is too large (%s has %ld params)",
> +			       index,
> +			       function_type->get_debug_string (),
> +			       num_params);
> +  return (gcc_jit_type *)function_type->get_param_types ()[index];
> +}
> +

I'm retaining the above, since...

[...snip...]

>  
> +
> +/* 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,
> +			   size_t 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");
> +  RETURN_NULL_IF_FAIL ((int) index < struct_type->get_fields ()->length (),
> +				NULL, NULL, "NULL struct type");

...copy&paste error here; the message for this kind of failure needs
updating.  Do it in a similar way to how you did
gcc_jit_function_type_get_param_type above, using the ctxt of the
struct_type.

[...snip...]

> +#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

16, rather than 15, again.

> +   presence using
> +     #ifdef LIBGCCJIT_HAVE_REFLECTION

[...snip...]

OK for trunk with the above nits fixed, assuming that you have run the
full regression test suite (or do you need help with that?)

I can't remember, sorry, do you have push rights to the gcc git
repository?

Do you have a preference as to which patch you want me to look at next?
Otherwise I'll go through them in the order in
https://github.com/antoyo/rustc_codegen_gcc/tree/master/gcc-patches

Dave