Re: [PATCH][gcc] libgccjit: add bitfield support

[David Malcolm <dmalcolm@redhat.com>]

On Mon, 2019-06-03 at 09:51 +0000, Andrea Corallo wrote:
> Hi all,
> I would like to submit this patch that aims to introduce bitfields
> support into libgccjit.
> 
> A new entry point gcc_jit_context_new_bitfield is added plus relative
> testcase.
> 
> Checked with make check-jit does not introduce regressions.
> 
> Feedbacks are very welcome.
> 
> Bests
> 
> Andrea
> 
> 2019-06-01  Andrea Corallo andrea.corallo@arm.com
> 
> * docs/topics/compatibility.rst (LIBGCCJIT_ABI_12): New ABI tag.
> * docs/topics/types.rst: Add gcc_jit_context_new_bitfield.
> * jit-common.h (namespace recording): Add class bitfield.
> * jit-playback.c: Include "c-family/c-common.h"
> (playback::context::new_bitfield): New method.
> (playback::compound_type::set_fields): Add bitfield support.
> (playback::lvalue::jit_mark_addressable): Make this a method of
> lvalue
> plus return a bool to communicate success.
> (playback::lvalue::get_address): Check for jit_mark_addressable
> return
> value.
> * jit-playback.h (new_bitfield): New method.
> (class bitfield): New class.
> (class lvalue): Add jit_mark_addressable method.
> * jit-recording.c (recording::context::new_bitfield): New method.
> (recording::bitfield::replay_into): New method.
> (recording::bitfield::write_to_dump): Likewise.
> (recording::bitfield::make_debug_string): Likewise.
> (recording::bitfield::write_reproducer): Likewise.
> * jit-recording.h (class context): Add new_bitfield method.
> (class field): Make it derivable by class bitfield.
> (class bitfield): Add new class.
> * libgccjit++.h (class context): Add new_bitfield method.
> * libgccjit.c (struct gcc_jit_bitfield): New structure.
> (gcc_jit_context_new_bitfield): New function.
> * libgccjit.h
> (LIBGCCJIT_HAVE_gcc_jit_context_new_bitfield) New macro.
> (gcc_jit_context_new_bitfield): New function.
> * libgccjit.map (LIBGCCJIT_ABI_12) New ABI tag.
> 
> 
> 2019-06-01  Andrea Corallo andrea.corallo@arm.com
> 
> * jit.dg/all-non-failing-tests.h: Add test-accessing-bitfield.c.
> * jit.dg/test-accessing-bitfield.c: New testcase.

Thanks for working on this; sorry for the delay in reviewing it.

Overall, this looks close to being ready, but I have a few notes:

[...]

> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> index b74495c..7676e55 100644
> --- a/gcc/jit/jit-playback.c
> +++ b/gcc/jit/jit-playback.c
> @@ -31,6 +31,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "convert.h"
>  #include "stor-layout.h"
>  #include "print-tree.h"
> +#include "c-family/c-common.h"

Presumably this is for DECL_C_BIT_FIELD etc, and this would make
libgccjit piggy-back off of the C family's macros for using lang flag
4?

If so, I think it would be cleaner to instead take a copy of those
macros (at the top of jit-playback.c for now), renaming them to
  DECL_JIT_BIT_FIELD
etc, with a "compare with" comment referring to the C frontend macros.

That way libgccjit doesn't directly depend on implementation details of
the C-family of frontends, but it's easy to see where we took the code
from (I believe that libgccjit isn't currently making any use of lang
flags, so this would be the first).

>  #include "gimplify.h"
>  #include "gcc-driver-name.h"
>  #include "attribs.h"
> @@ -263,6 +264,48 @@ new_field (location *loc,
>    return new field (decl);
>  }
>  
> +/* Construct a playback::bitfield instance (wrapping a tree).  */
> +
> +playback::field *
> +playback::context::
> +new_bitfield (location *loc,
> +	      type *type,
> +	      int width,
> +	      const char *name)
> +{
> +  gcc_assert (type);
> +  gcc_assert (name);
> +  gcc_assert (width);
> +
> +  /* compare with c/c-decl.c:grokfield,  grokdeclarator and
> +     check_bitfield_type_and_width.  */
> +
> +  tree tree_type = type->as_tree ();
> +  if (TREE_CODE (tree_type) != INTEGER_TYPE
> +      && TREE_CODE (tree_type) != BOOLEAN_TYPE)
> +    {
> +      add_error (loc, "bit-field %s has invalid type", name);

Ideally this error message would identify the type, and be more precise
about what the problem with it is.

I initially thought something like:

  add_error (loc,
             "bit-field %s has invalid type %s (must be integer or boolean)",
             name, type->get_debug_string ());

would work, but type is a playback::type, rather than a
recording::type.

Is there a way to catch this problem earlier, before we reach playback?
Alternatively:

  add_error (loc,
             "bit-field %s has invalid type (must be integer or boolean)");
             name);

would at least be more precise about the problem.

It would be good to have test coverage for this; see e.g.
  gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_field-opaque-struct.c
and similar, where testcases have the form:

test-error-API_ENTRYPOINT-WHAT-WENT-WRONG.c


> +      return NULL;
> +    }
> +  tree tree_width = build_int_cst (integer_type_node, width);
> +  if (compare_tree_int (tree_width, TYPE_PRECISION (tree_type)) > 0)
> +    {
> +      add_error (loc, "bit-field %s exceeds its type", name);

Similarly, could this message be reworded to:

   add_error (loc,
              "width of bit-field %s (width: %i) is wider than its type (width: %i)",
              name, width, something (TYPE_PRECISION (tree_type)));

(using "something" as I can't remember the incantation to get from tree
int cst with known small integer back to int)

or could the problem be caught earlier?

Again, would be good to have a testcase.
(any non-trivial error-handling ideally ought to have a testcase).

[...]

> @@ -1197,20 +1247,30 @@ dereference (location *loc)
>    return new lvalue (get_context (), datum);
>  }
>  
> -/* Mark EXP saying that we need to be able to take the
> +/* Mark the lvalue saying that we need to be able to take the
>     address of it; it should not be allocated in a register.
> -   Compare with e.g. c/c-typeck.c: c_mark_addressable.  */
> +   Compare with e.g. c/c-typeck.c: c_mark_addressable
really_atomic_lvalue.
> +   Returns true if successful.  */

"Returns false if a failure occurred (an error will already have been
added to the active context for this case)."

(or somesuch).

>  
> -static void
> -jit_mark_addressable (tree exp)
> +bool
> +playback::lvalue::
> +jit_mark_addressable (location *loc)
>  {

Given that this is becoming a member function, we can lose the "jit_"
prefix, I think.

> -  tree x = exp;
> +  tree x = as_tree ();;
>  
>    while (1)
>      switch (TREE_CODE (x))
>        {
>        case COMPONENT_REF:
> -	/* (we don't yet support bitfields)  */
> +	if (DECL_C_BIT_FIELD (TREE_OPERAND (x, 1)))
> +	  {
> +	    gcc_assert (gcc::jit::active_playback_ctxt);
> +	    gcc::jit::
> +	      active_playback_ctxt->add_error (loc,
> +					       "cannot take address
of "
> +					       "bit-field");

Can you add a testcase for this error-handling please?

[...]

> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> index a332fe8..0678d07 100644
> --- a/gcc/jit/jit-recording.c
> +++ b/gcc/jit/jit-recording.c

[...]

> +/* Override the default implementation of
> +   recording::memento::write_to_dump.  Dump each bit field
> +   by dumping a line of the form:
> +      TYPE NAME:WIDTH;
> +   so that we can build up a struct/union field-byfield.  */
Nit: "field by field", I suppose.

[...]

> +/* Implementation of recording::memento::write_reproducer for
bitfields.  */
> +
> +void
> +recording::bitfield::write_reproducer (reproducer &r)
> +{
> +  const char *id = r.make_identifier (this, "bitfield");
> +  r.write ("  gcc_jit_field *%s =\n"
> +	   "    gcc_jit_context_new_bitfield (%s,\n"
> +	   "                               %s, /* gcc_jit_location
*loc */\n"
> +	   "                               %s, /* gcc_jit_type
*type, */\n"
> +	   "                               %d, /* int *width, */\n"

This writes a stray "*" in the auto-generated comment here - isn't it
an "int", rather than an "int *"?
                                                 
> +	   "                               %s); /* const char *name
*/\n",
> +	   id,
> +	   r.get_identifier (get_context ()),
> +	   r.get_identifier (m_loc),
> +	   r.get_identifier_as_type (m_type),
> +	   m_width,
> +	   m_name->get_debug_string ());
> +}
> +
>  /* The implementation of class gcc::jit::recording::compound_type */
>  
>  /* The constructor for gcc::jit::recording::compound_type.  */
> diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> index b9f2250..5e2d64e 100644
> --- a/gcc/jit/jit-recording.h
> +++ b/gcc/jit/jit-recording.h

[...]

> @@ -822,9 +828,9 @@ public:
>    compound_type * get_container () const { return m_container; }
>    void set_container (compound_type *c) { m_container = c; }
>  
> -  void replay_into (replayer *) FINAL OVERRIDE;
> +  void replay_into (replayer *);
>  
> -  void write_to_dump (dump &d) FINAL OVERRIDE;
> +  void write_to_dump (dump &d);

Although these are no longer FINAL, aren't they still "OVERRIDE"?

> 
>    playback::field *
>    playback_field () const
> @@ -833,16 +839,40 @@ public:
>    }
>  
>  private:
> -  string * make_debug_string () FINAL OVERRIDE;
> -  void write_reproducer (reproducer &r) FINAL OVERRIDE;
> +  string * make_debug_string ();
> +  void write_reproducer (reproducer &r);

Likewise.


> -private:
> +protected:
>    location *m_loc;
>    type *m_type;
>    string *m_name;
>    compound_type *m_container;
>  };
>  
> +
> +class bitfield : public field
> +{
> +public:
> +  bitfield (context *ctxt,
> +            location *loc,
> +            type *type,
> +            int width,
> +            string *name)
> +    : field (ctxt, loc, type, name)
> +  { m_width = width; }

Please use:
  m_width (width)
member init syntax, rather than doing it in the body of the ctor.

[...]

> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> index e4f17f8..f1265a5 100644
> --- a/gcc/jit/libgccjit.c
> +++ b/gcc/jit/libgccjit.c

[...]

> @@ -556,6 +560,35 @@ gcc_jit_context_new_field (gcc_jit_context
*ctxt,
>  
>  /* Public entrypoint.  See description in libgccjit.h.
>  
> +   After error-checking, the real work is done by the
> +   gcc::jit::recording::context::new_bitfield method, in
> +   jit-recording.c.  */
> +
> +gcc_jit_field *
> +gcc_jit_context_new_bitfield (gcc_jit_context *ctxt,
> +			      gcc_jit_location *loc,
> +			      gcc_jit_type *type,
> +			      int width,
> +			      const char *name)
> +{
> +  RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context");
> +  JIT_LOG_FUNC (ctxt->get_logger ());
> +  /* LOC can be NULL.  */
> +  RETURN_NULL_IF_FAIL (type, ctxt, loc, "NULL type");
> +  RETURN_NULL_IF_FAIL (width > 0, ctxt, NULL, "invalid width");

Would be better to be more precise, something like:

  RETURN_NULL_IF_FAIL_PRINTF2 (
    width > 0, ctxt, loc,
    "invalid width %d for bitfield \"%s\" (must be > 0)",
    name, width);

or somesuch, moving the check for NULL name before this.

In fact, it would be ideal to move the check for NULL name to before
that for the type, and to specify the name in that message too (the
more pertinent information we can give the user, the better).

> +  RETURN_NULL_IF_FAIL (name, ctxt, loc, "NULL name");
> +  RETURN_NULL_IF_FAIL_PRINTF2 (
> +    type->has_known_size (),
> +    ctxt, loc,
> +    "unknown size for field \"%s\" (type: %s)",
> +    name,
> +    type->get_debug_string ());
> +
> +  return (gcc_jit_field *)ctxt->new_bitfield (loc, type, width,
name);
> +}
> +
> +/* Public entrypoint.  See description in libgccjit.h.
> +
>     After error-checking, this calls the trivial
>     gcc::jit::recording::memento::as_object method (a field is a
>     memento), in jit-recording.h.  */

[...]

Thanks
Dave