Hi.
Thanks for the review. See the comments below.
I attached the updated patch.

On Thu, Oct 15, 2020 at 05:52:33PM -0400, David Malcolm wrote:
>On Thu, 2020-10-15 at 13:39 -0400, Antoni Boucher wrote:
>> Thanks. I updated the patch with these changes.
>
>Thanks for patch; review below.  Sorry if it seems excessively nitpicky
>in places.
>
>> 2020-09-1  Antoni Boucher  <bouanto@zoho.com>
>>
>>     gcc/jit/
>>             PR target/96889
>>             * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI tag.
>
>15 now.
>
>>             * docs/topics/functions.rst: Add documentation for the
>>             functions gcc_jit_function_get_return_type and
>>             gcc_jit_function_get_param_count
>>             * libgccjit.c: New functions:
>>               * 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_int;
>>               * 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;
>
>This isn't valid ChangeLog format; it will fail the git hooks.

Is there any way to run that locally to check the format automatically?

>
>>             * libgccjit.h
>
>Likewise.
>
>>             * jit-recording.h: New functions (is_struct and is_vector)
>>             * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag.
>
>15 now.
>
>>
>>     gcc/testsuite/
>>             PR target/96889
>>             * jit.dg/all-non-failing-tests.h: Add test-reflection.c.
>>             * jit.dg/test-reflection.c: New test.
>
>[...]
>
>
>> diff --git a/gcc/jit/docs/topics/functions.rst b/gcc/jit/docs/topics/functions.rst
>> index eb40d64010e..9819c28cda2 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::  size_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.
>> +
>
>The documentation part of the patch is incomplete: it hasn't been
>updated to add all the new entrypoints.

Indeed, I wanted some feedback before I write the documentation. I'll 
write it soon.

>Also, the return type of gcc_jit_function_get_param_count is
>inconsistent (size_t above, but ssize_t below).
>
>
>> diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
>> index 30e37aff387..525b8bc921d 100644
>> --- a/gcc/jit/jit-recording.h
>> +++ b/gcc/jit/jit-recording.h
>> @@ -538,7 +538,9 @@ public:
>>    virtual bool is_bool () const = 0;
>>    virtual type *is_pointer () = 0;
>>    virtual type *is_array () = 0;
>> +  virtual struct_ *is_struct () { return NULL; }
>
>Can't you use dyn_cast_struct for this?

We could, but wouldn't that be inconvenient for the user to call 
gcc_jit_type_unqualified every time they want to call a function such as 
gcc_jit_type_is_struct?
(I don't mind if you prefer to make the call to gcc_jit_type_unqualified 
explicit in user code.)

>Or is this about looking through decorated_type? e.g. for const and
>volatile variants?
>
>I guess my question is, what is the purpose of gcc_jit_type_is_struct?

The purpose is to be able to get information about the fields of the 
struct, if the type is a struct.

>
>>    virtual bool is_void () const { return false; }
>> +  virtual vector_type *is_vector () { return NULL; }
>
>Likewise, can't you use dyn_cast_vector_type for this?

Same as for is_struct().

>>    virtual bool has_known_size () const { return true; }
>>
>>    bool is_numeric () const
>> @@ -595,6 +597,8 @@ public:
>>    bool is_bool () const FINAL OVERRIDE;
>>    type *is_pointer () FINAL OVERRIDE { return dereference (); }
>>    type *is_array () FINAL OVERRIDE { return NULL; }
>> +  vector_type *is_vector () FINAL OVERRIDE { return NULL; }
>> +  struct_ *is_struct () FINAL OVERRIDE { return NULL; }
>
>Likewise, and this is redundant, as it's merely copying the base class
>implementation.
>
>>    bool is_void () const FINAL OVERRIDE { return m_kind == GCC_JIT_TYPE_VOID; }
>>
>>  public:
>> @@ -629,6 +633,8 @@ public:
>>    bool is_bool () const FINAL OVERRIDE { return false; }
>>    type *is_pointer () FINAL OVERRIDE { return m_other_type; }
>>    type *is_array () FINAL OVERRIDE { return NULL; }
>> +  vector_type *is_vector () FINAL OVERRIDE { return NULL; }
>> +  struct_ *is_struct () FINAL OVERRIDE { return NULL; }
>
>Likewise.
>
>
>> @@ -655,6 +661,7 @@ public:
>>    bool is_bool () const FINAL OVERRIDE { return m_other_type->is_bool (); }
>>    type *is_pointer () FINAL OVERRIDE { return m_other_type->is_pointer (); }
>>    type *is_array () FINAL OVERRIDE { return m_other_type->is_array (); }
>> +  struct_ *is_struct () FINAL OVERRIDE { return m_other_type->is_struct (); }
>
>Aha: with a decorated type you look through the decoration.
>
>>  protected:
>>    type *m_other_type;
>> @@ -737,6 +744,8 @@ public:
>>
>>    void replay_into (replayer *) FINAL OVERRIDE;
>>
>> +  vector_type *is_vector () FINAL OVERRIDE { return this; }
>> +
>>  private:
>>    string * make_debug_string () FINAL OVERRIDE;
>>    void write_reproducer (reproducer &r) FINAL OVERRIDE;
>> @@ -765,6 +774,8 @@ class array_type : public type
>>    bool is_bool () const FINAL OVERRIDE { return false; }
>>    type *is_pointer () FINAL OVERRIDE { return NULL; }
>>    type *is_array () FINAL OVERRIDE { return m_element_type; }
>> +  vector_type *is_vector () FINAL OVERRIDE { return NULL; }
>> +  struct_ *is_struct () FINAL OVERRIDE { return NULL; }
>
>Are these redundant?
>
>>    int num_elements () { return m_num_elements; }
>>
>>    void replay_into (replayer *) FINAL OVERRIDE;
>> @@ -799,6 +810,8 @@ public:
>>    bool is_bool () const FINAL OVERRIDE { return false; }
>>    type *is_pointer () FINAL OVERRIDE { return NULL; }
>>    type *is_array () FINAL OVERRIDE { return NULL; }
>> +  vector_type *is_vector () FINAL OVERRIDE { return NULL; }
>> +  struct_ *is_struct () FINAL OVERRIDE { return NULL; }
>
>Likewise.
>
>>    void replay_into (replayer *) FINAL OVERRIDE;
>>
>> @@ -912,6 +925,7 @@ public:
>>    bool is_bool () const FINAL OVERRIDE { return false; }
>>    type *is_pointer () FINAL OVERRIDE { return NULL; }
>>    type *is_array () FINAL OVERRIDE { return NULL; }
>> +  vector_type *is_vector () FINAL OVERRIDE { return NULL; }
>
>Likewise.
>
>>    bool has_known_size () const FINAL OVERRIDE { return m_fields != NULL; }
>>
>> @@ -943,6 +957,8 @@ public:
>>
>>    const char *access_as_type (reproducer &r) FINAL OVERRIDE;
>>
>> +  struct_ *is_struct () FINAL OVERRIDE { return this; }
>> +
>>  private:
>>    string * make_debug_string () FINAL OVERRIDE;
>>    void write_reproducer (reproducer &r) FINAL OVERRIDE;
>> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
>> index a00aefc7108..8e6a9e85ff0 100644
>> --- a/gcc/jit/libgccjit.c
>> +++ b/gcc/jit/libgccjit.c
>> @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "config.h"
>>  #include "system.h"
>>  #include "coretypes.h"
>> +#include "tm.h"
>
>Why is this included?  It's not mentioned in the ChangeLog.

Leftover. I removed it.

>>  #include "timevar.h"
>>  #include "typed-splay-tree.h"
>>  #include "cppbuiltin.h"
>> @@ -60,6 +61,14 @@ struct gcc_jit_struct : public gcc::jit::recording::struct_
>>  {
>>  };
>>
>> +struct gcc_jit_function_type : public gcc::jit::recording::function_type
>> +{
>> +};
>> +
>> +struct gcc_jit_vector_type : public gcc::jit::recording::vector_type
>> +{
>> +};
>> +
>
>FWIW these aren't mentioned in the ChangeLog either.
>
>>  struct gcc_jit_field : public gcc::jit::recording::field
>>  {
>>  };
>> @@ -510,6 +519,197 @@ gcc_jit_type_get_volatile (gcc_jit_type *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.  */
>> +
>> +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");
>> +  int num_params = function_type->get_param_types ().length ();
>> +  gcc::jit::recording::context *ctxt = function_type->m_ctxt;
>
>Should check that index >= 0 as well (or make it unsigned).
>
>Given that gcc_jit_function_get_param_count returns ssize_t, should
>that be the type of "index"?

For those, I had this question in a previous email:

By the way, that seemed off to return NULL for the function 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 
checks and types.

>> +  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];
>> +}
>
>[...]
>
>> +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");
>
>Similar comments as per gcc_jit_function_type_get_param_type above.
>Should check here that index is in range (>=0 and < num fields).

Same as for gcc_jit_function_type_get_param_type.

>> +  return (gcc_jit_field *)struct_type->get_fields ()->get_field (index);
>> +}
>> +
>> +/* Public entrypoint.  See description in libgccjit.h.
>> +
>> +   After error-checking, this calls the trivial
>> +   gcc::jit::recording::struct_::get_fields method in
>> +   jit-recording.h.  */
>> +
>> +ssize_t
>> +gcc_jit_struct_get_field_count (gcc_jit_struct *struct_type)
>> +{
>> +  RETURN_VAL_IF_FAIL (struct_type, -1, NULL, NULL, "NULL struct type");
>> +  return struct_type->get_fields ()->length ();
>> +}
>> +
>
>[...]
>
>> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
>> index 7134841bb07..6056d00a3ea 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;
>> +
>>  /* A gcc_jit_function encapsulates a function: either one that you're
>>     creating yourself, or a reference to one that you're dynamically
>>     linking to within the rest of the process.  */
>> @@ -586,6 +592,61 @@ gcc_jit_type_get_const (gcc_jit_type *type);
>>  extern gcc_jit_type *
>>  gcc_jit_type_get_volatile (gcc_jit_type *type);
>>
>> +/* Given type "T", return TRUE if the type is an array.  */
>> +extern gcc_jit_type *
>> +gcc_jit_type_is_array (gcc_jit_type *type);
>
>The comment is unclear; does this actually return the type vs NULL?
>
>As noted above, I think we need to think about what happens on
>qualified types; is stripping them the responsibility of the caller?
>Should it be?  (I'm thinking "yes", but presumably all this is
>motivated by your rust hacking).

As said above, I don't mind if you prefer to make that a responsibility 
of the caller. I just thought it was more convenient.

>Also, there isn't a type "T" here; the param name is "type", and there
>isn't a supporting example using "T" as a placeholder.
>Maybe just lose the 'Given type "T", ' prefix here and below (unless
>giving a example).
>
>> +/* Given type "T", return TRUE if the type is a bool.  */
>> +extern int
>> +gcc_jit_type_is_bool (gcc_jit_type *type);
>
>I'd prefer "return non-zero" over "return TRUE" (here, and in various
>places below).
>
>Does this check for the "bool" type i.e.:
>  /* C++'s bool type; also C99's "_Bool" type, aka "bool" if using
>     stdbool.h.  */
>  GCC_JIT_TYPE_BOOL,
>?
>
>> +/* Given type "T", return the function type if it is one.  */
>
>..."or NULL".
>
>> +extern gcc_jit_function_type *
>> +gcc_jit_type_is_function_ptr_type (gcc_jit_type *type);
>> +
>> +/* Given a function type, return its return type.  */
>> +extern gcc_jit_type *
>> +gcc_jit_function_type_get_return_type (gcc_jit_function_type *function_type);
>> +
>> +/* Given a function type, return its number of parameters.  */
>> +extern ssize_t
>> +gcc_jit_function_type_get_param_count (gcc_jit_function_type *function_type);
>> +
>> +/* Given a function type, return the type of the specified parameter.  */
>> +extern gcc_jit_type *
>> +gcc_jit_function_type_get_param_type (gcc_jit_function_type *function_type,
>> +				int index);
>> +
>> +/* Given type "T", return the type pointed by the pointer type
>> + * or NULL if it's not a pointer.  */
>> +extern gcc_jit_type *
>> +gcc_jit_type_is_pointer (gcc_jit_type *type);
>> +
>> +/* Given type "T", return TRUE if the type is an int.  */
>> +extern int
>> +gcc_jit_type_is_int (gcc_jit_type *type);
>
>I'm not sure I understand what this function does, which suggests it's
>misnamed.  The current name suggests that it checks for the C "int"
>type, whereas I believe it actually checks for an integral
>type.  Should it be:
>
>extern int
>gcc_jit_type_is_integral (gcc_jit_type *type);
>
>?

Yes.

>What's the output?  Do you merely need nonzero/zero for boolean, or
>information on the width of the type/signedness, or whatnot?  (perhaps
>have some out params for extracting width/signedness for when it
>returns nonzero?)

At some point, I wanted to add that, but I gave up (I don't remember 
why).

>Looking at the testcase, it looks like it currently returns false on
>the _Bool type.  Is that correct?

Yes. There's a gcc_jit_type_is_bool function to check for that.

>> +/* Given type "T", return the unqualified type, i.e. for a
>> + * decorated type, return the type it wraps.  */
>> +extern gcc_jit_type *
>> +gcc_jit_type_unqualified (gcc_jit_type *type);
>
>"decorated" is a term used inside the implementation, but not in the
>user-facing docs.
>
>How about:
>
>  /* Given a type, return the unqualified type, removing "const",
>"volatile"
>     and alignment qualifiers.  */
>
>or somesuch.
>
>> +/* Given type "T", return TRUE if the type is a vector type.  */
>> +extern gcc_jit_vector_type *
>> +gcc_jit_type_is_vector (gcc_jit_type *type);
>
>As above, this doesn't return TRUE, it's more of a dynamic cast.
>Likewise about responsibility for qualifiers.
>
>> +/* Given type "T", return the struct type or NULL.  */
>> +extern gcc_jit_struct *
>> +gcc_jit_type_is_struct (gcc_jit_type *type);
>
>Wording could be improved as noted above.
>
>> +/* Given a vector type, return the number of units it contains.  */
>> +extern ssize_t
>> +gcc_jit_vector_type_get_num_units (gcc_jit_vector_type *vector_type);
>> +
>> +/* Given a vector type, return the type of its elements.  */
>> +extern gcc_jit_type *
>> +gcc_jit_vector_type_get_element_type (gcc_jit_vector_type *vector_type);
>> +
>
>[...]
>
>> @@ -647,6 +708,15 @@ gcc_jit_struct_set_fields (gcc_jit_struct *struct_type,
>>  			   int num_fields,
>>  			   gcc_jit_field **fields);
>>
>> +/* Get a field by index.  */
>> +extern gcc_jit_field *
>> +gcc_jit_struct_get_field (gcc_jit_struct *struct_type,
>> +			   int index);
>> +
>> +/* Get the number of fields.  */
>> +extern ssize_t
>> +gcc_jit_struct_get_field_count (gcc_jit_struct *struct_type);
>
>Do we need these for unions?  Or skip them for now?

Let's skip them for now.

>> @@ -740,6 +810,14 @@ gcc_jit_function_as_object (gcc_jit_function *func);
>>  extern gcc_jit_param *
>>  gcc_jit_function_get_param (gcc_jit_function *func, int index);
>>
>> +/* Get the number of params of a function.  */
>> +extern ssize_t
>> +gcc_jit_function_get_param_count (gcc_jit_function *func);
>> +
>> +/* Get the return type of a function.  */
>> +extern gcc_jit_type *
>> +gcc_jit_function_get_return_type (gcc_jit_function *func);
>
>Are these redundant if there's a way to ask a gcc_jit_function for its
>type, and then to use the gcc_jit_function_type_ functions?
>(I don't mind if they are)

They're kind of redundant, but I'm not sure what would be the way to 
remove them. I would need a way to create a gcc_jit_function_type from a 
gcc_jit_function.

>> @@ -1518,6 +1596,47 @@ 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, 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_gcc_jit_function_reflection
>> +*/
>
>This has become a big mixture of things beyond just function
>reflection, so I think I'd prefer it if each entrypoint "funcname" had
>its own "LIBGCCJIT_HAVE_funcname" define.  Or just "REFLECTION" rather
>than "function_reflection", that's probably simpler.
>Note that the existing "HAVE_foo" defines are lowercase "foo" when it's
>an actual entrypoint named "foo", and are uppercase otherwise e.g.
>LIBGCCJIT_HAVE_SWITCH_STATEMENTS and LIBGCCJIT_HAVE_TIMING_API.
>
>> +extern gcc_jit_type *
>> +gcc_jit_function_get_return_type (gcc_jit_function *func);
>> +extern ssize_t
>> +gcc_jit_function_get_param_count (gcc_jit_function *func);
>> +extern gcc_jit_type *
>> +gcc_jit_type_is_array (gcc_jit_type *type);
>> +extern int
>> +gcc_jit_type_is_bool (gcc_jit_type *type);
>> +extern gcc_jit_function_type *
>> +gcc_jit_type_is_function_ptr_type (gcc_jit_type *type);
>> +extern gcc_jit_type *
>> +gcc_jit_function_type_get_return_type (gcc_jit_function_type *function_type);
>> +extern ssize_t
>> +gcc_jit_function_type_get_param_count (gcc_jit_function_type *function_type);
>> +extern gcc_jit_type *
>> +gcc_jit_function_type_get_param_type (gcc_jit_function_type *function_type,
>> +				int index);
>> +extern int
>> +gcc_jit_type_is_int (gcc_jit_type *type);
>> +extern gcc_jit_type *
>> +gcc_jit_type_is_pointer (gcc_jit_type *type);
>> +extern gcc_jit_vector_type *
>> +gcc_jit_type_is_vector (gcc_jit_type *type);
>> +extern gcc_jit_struct *
>> +gcc_jit_type_is_struct (gcc_jit_type *type);
>> +extern ssize_t
>> +gcc_jit_vector_type_get_num_units (gcc_jit_vector_type *vector_type);
>> +extern gcc_jit_type *
>> +gcc_jit_vector_type_get_element_type (gcc_jit_vector_type *vector_type);
>> +extern gcc_jit_type *
>> +gcc_jit_type_unqualified (gcc_jit_type *type);
>
>Is this part of the patch a duplicate copy of the various things added
>above?  Perhaps copy-and-paste error, or am I misreading this?
>
>[...]
>
>Hope this is constructive
>Dave
>