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

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2020-09-01 at 21:01 -0400, Antoni Boucher via Jit wrote:
> Hello.
> This WIP patch implements new reflection functions in the C API as 
> mentioned in bug 96889.
> I'm looking forward for feedbacks on this patch.
> It's WIP because I'll probably add a few more reflection functions.
> Thanks.

Sorry about the belated review, looks like I missed this one.

At a high level, it seems reasonable.

Do you have a copyright assignment in place for GCC contributions?
See https://gcc.gnu.org/contribute.html

[...]
 
> diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
> index bb3387fa583..7e786194ded 100644
> --- a/gcc/jit/docs/topics/compatibility.rst
> +++ b/gcc/jit/docs/topics/compatibility.rst
> @@ -219,3 +219,14 @@ entrypoints:
>    * :func:`gcc_jit_version_minor`
>  
>    * :func:`gcc_jit_version_patchlevel`
> +
> +.. _LIBGCCJIT_ABI_14:
> +
> +``LIBGCCJIT_ABI_14``
> +--------------------
> +``LIBGCCJIT_ABI_14`` covers the addition of reflection functions via API
> +entrypoints:
> +
> +  * :func:`gcc_jit_function_get_return_type`
> +
> +  * :func:`gcc_jit_function_get_param_count`

This will now need bumping to 15; 14 covers the addition of
gcc_jit_global_set_initializer.

[...]

> +/* Public entrypoint.  See description in libgccjit.h.
> +
> +   After error-checking, the real work is done by the
> +   gcc::jit::recording::function::get_return_type method, in
> +   jit-recording.h.  */
> +
> +gcc_jit_type *
> +gcc_jit_function_get_return_type (gcc_jit_function *func)
> +{

This one is missing a:
  RETURN_NULL_IF_FAIL (func, NULL, NULL, "NULL function");


> +    return (gcc_jit_type *)func->get_return_type ();
> +}

[...]

> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> index 1c5a12e9c01..6999ce25ca2 100644
> --- a/gcc/jit/libgccjit.h
> +++ b/gcc/jit/libgccjit.h

[...]

> @@ -1503,6 +1511,22 @@ gcc_jit_version_minor (void);
>  extern int
>  gcc_jit_version_patchlevel (void);
>  
> +#define LIBGCCJIT_HAVE_gcc_jit_function_reflection
> +
> +/* Reflection functions to get the number of parameters and return types of
> +   a function from the C API.

"return type", is better grammar, I think, given that "function" is singular.

> +
> +   "vec_type" should be a vector type, created using gcc_jit_type_get_vector.

This line about "vec_type" seems to be a leftover from a copy&paste.


> +   This API entrypoint was added in LIBGCCJIT_ABI_14; you can test for its
> +   presence using
> +     #ifdef LIBGCCJIT_HAVE_gcc_jit_function_reflection

Version number will need bumping, as mentioned above.

[...]

> diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> index 6137dd4b4b0..b28f81a7a32 100644
> --- a/gcc/jit/libgccjit.map
> +++ b/gcc/jit/libgccjit.map
> @@ -186,4 +186,10 @@ LIBGCCJIT_ABI_13 {
>      gcc_jit_version_major;
>      gcc_jit_version_minor;
>      gcc_jit_version_patchlevel;
> -} LIBGCCJIT_ABI_12;
> \ No newline at end of file
> +} LIBGCCJIT_ABI_12;
> +
> +LIBGCCJIT_ABI_14 {
> +  global:
> +    gcc_jit_function_get_return_type;
> +    gcc_jit_function_get_param_count;
> +} LIBGCCJIT_ABI_13;

Likewise.

[...]

Otherwise looks good to me.

Bonus points for adding C++ bindings (and docs for them), but I don't
know of anyone using the C++ bindings.

Dave