Re: [PATCH 0/3] Support some cases of inheritance in gengtype; use it for symtab

[David Malcolm <dmalcolm at redhat dot com>]

On Mon, 2013-09-23 at 13:19 +0200, Richard Biener wrote:
> On Fri, Sep 20, 2013 at 4:05 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> > There have been various discussions about how to handle inheritance
> > within ggc and PCH: whether to extend gengtype to support C++ syntax
> > such as templates, or to require people to use GTY((user)) and manually
> > write field-traversal.
> >
> > I've attempted to introduce class hierarchies in a few places recently,
> > for example for the symtab types:
> >   http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00668.html
> >
> > and the gimple types:
> >   http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01788.html
> >
> > In each case the GTY((user)) is always the most painful part: it
> > requires ugly hand-written code which must be kept up-to-date with
> > changes to the underlying types, lest we run into
> > difficult-to-track-down crashes inside the GC and PCH.
> >
> > I got sick of writing and debugging these GTY((user)) traversal
> > functions, for IMHO quite modest use of C++ (no templates etc), so I had
> > a go at implementing support for inheritance in gengtype, as seen in the
> > following patch series.
> >
> > The key compromise I'm introducing, as I see it, is to require the user
> > to "opt in" to the support for the inheritance, on a class-by-class
> > basis, and not attempt to support all of C++, but only single
> > inheritance, without templates.
> 
> Please make sure to update doc/gty.texi with this feature and its
> restrictions.

Thanks; I've added documentation in the latest version of the patches:
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01803.html

> > The idea is that if you add a GTY marker to a subclass, you are
> > "opting in" to gengtype, and on any restrictions on the base class that
> > gengtype needs to impose to avoid having to deal with all of C++ syntax:
> > specifically, no multiple inheritance, and the base class can't be a
> > template.
> >
> > If those restrictions are too much, e.g. you have something like:
> >
> >    struct alloc_pool_hasher : typed_noop_remove <alloc_pool_descriptor>
> >
> > then don't GTY-mark the class, and gengtype won't attempt to parse the
> > base class (as per the current parser).
> 
> Such restrictions are bad - does gengtype at least diagnose the case where
> a "bad" base class is used?

I'm sorry if my initial email was unclear.  The point I was trying to
make is that we already have restrictions: we currently can't use any
kind of inheritance with GTY, unless we use GTY((user)) and do things
"by hand".  This patch makes things less restrictive, in that some forms
of inheritance can now be used without needing to use GTY((user)).

The example I picked is one that is already in the source tree, and
which doesn't use GTY.  I chose that particular example as an example of
inheritance within the source tree, to illustrate that adding this
parser support to gengtype doesn't break this existing code: it doesn't
introduce a restriction to the use of C++ in the code as a whole.  The
restriction merely applies to the feature itself, and can easily be
avoided (e.g. by not using GTY, or by using GTY((user))).

To answer your question, in the example above, if one adds a GTY marker
to alloc_pool_hasher:

struct GTY(()) alloc_pool_hasher : typed_noop_remove <alloc_pool_descriptor>

and adds alloc-pool.c to GTFILES so that it's present in gtyp-input.list
and thus gets parsed by gengtype, then one gets this error:

../../src/gcc/alloc-pool.c:86: parse error: expected '{', have '<'

So yes: gengtype will fail immediately with an error message.

Note that you can still either omit GTY, or use GTY((user)) for such
cases, fixing the error message. (however, fwiw, it's meaningless to try
to GTY-mark this particular class, given that it's a traits type, not
holding any data itself).

> My initial worry about C++-ifying gengtype was exactly because of such
> arbitrary restrictions. 

Note that gengtype already has an arbitrary restriction: no inheritance,
unless you use GTY((user)).

This patch simply loosens the restriction somewhat, by allowing some
inheritance.   If code still falls within the looser restrictions, that
code can continue to use GTY((user)), as before.

> Isn't it possible to remove the restruction by
> requiring to GTY annotate the subclassing itself?  Like
> 
> struct alloc_pool_hasher : typed_noop_remove <alloc_pool_descriptor> GTY((...))
> 
> with whatever information required to "parse" the class given explicitely as
> arguments of the GTY?  Like for the single inheritance case with a descriptor
> you'd give out a tag to identify all types participating in the
> inheritance group

FWIW it's meaningless for this particular class to be GTY-marked: as
mentioned above, it's a bundle of traits for use elsewhere, and doesn't
actually hold any data.

But speaking to the general point about templates: I don't have a need
to support automated generation of traversal functions for templates, we
can use GTY((user)) for that if we need to.

In contrast, I'm regularly running into a need for doing it for
non-template base classes, and I'm sick of writing GTY((user)) functions
by hand for this.  I think my patches provide a simpler way of doing it,
avoiding the need to use GTY((user)) and thus the use of hand-written
traversal functions (which I want to avoid, due to the risk of bugs).

Hope this clarifies things.

> struct bar GTY((tag("fancy1"),desc("code")))
> {
>   int code;
> }
> 
> struct foo : bar GTY((tag("fancy1")))
> {
> ...
> }
> 
> struct foobar : bar GTY((tag("fancy1")))
> {
> ...
> }
> 
> etc.?  That would support "fancy" base classes(?), where you'd of course
> need to emit possibly templated GC walkers.  So maybe it this way wouldn't
> easily defeat the no-template case (at least if the derived classes are
> templated as well).  It would support a fully specified template base.
> 
> Richard.
> 
> > I can tweak things so that GTY((user)) classes are also excluded.
> >
> > This compromise allows gengtype to autogenerate traversal functions for
> > simple uses of inheritance, avoiding the need for hand-maintaining them,
> > whilst allowing all of C++ for places that need it, without having to
> > support parsing all of that C++ syntax in gengtype - IMHO the best of
> > both approaches.
> >
> > You put a "desc" GTY option on the base class to signify how to
> > discriminate between subclasses in the marker function.  Every concrete
> > subclass should have a "tag" GTY option, and gengtype builds a single
> > set of traversal functions, for the base class of the hierarchy, with
> > a big switch statement on the desc, using the tag values as the cases.
> > This is of course very similar to how unions are handled, and is similar
> > to the first part of this proposal from Lawrence Crowl:
> >   http://gcc.gnu.org/ml/gcc/2012-08/msg00267.html
> > Within such a hierarchy, traversal functions are only generated for the
> > base class.
> >
> > I was able to use this to reimplement my port of the symtab types to a
> > C++ class hierarchy.  The old, ugly version of the patch can be seen at:
> >
> >   http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00672.html
> >
> > I've attached an updated version of the above patch to the following
> > patch series, which uses the new gengtype inheritance support to
> > eliminate all of the horrible hand-written traversal functions, and the
> > ugly dummy GTY roots that are needed to workaround bugs in GTY((user)).
> >
> > Successfully bootstrapped and regtested this (plus the followup
> > autogenerated symtab refactoring from:
> >   http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00669.html )
> >
> > OK for trunk?  IMHO this is a much simpler approach; it avoids
> > having to deal with GTY((user)) and manually-written markers.
> >
> > Presumably I would also need to update the docs.
> >
> > For reference, here's what one of the generated functions looks like (I
> > believe this is not significantly uglier that what gtype-desc.c already
> > contains).  Note how the fields appear in reverse order of *types*
> > (i.e. from deepest subclass up to parent), within a field they appear in
> > natural order.  So, for example, in "case SYMTAB_FUNCTION:" the cgraph
> > fields appear, then the base symtab fields.  The two pch functions are
> > similar.
> >
> > void
> > gt_ggc_mx_symtab_node_base (void *x_p)
> > {
> >   struct symtab_node_base * const x = (struct symtab_node_base *)x_p;
> >   if (ggc_test_and_set_mark (x))
> >     {
> >       switch (((*x)).type)
> >         {
> >         case SYMTAB_SYMBOL:
> >           gt_ggc_m_9tree_node ((*x).decl);
> >           gt_ggc_m_16symtab_node_base ((*x).next);
> >           gt_ggc_m_16symtab_node_base ((*x).previous);
> >           gt_ggc_m_16symtab_node_base ((*x).next_sharing_asm_name);
> >           gt_ggc_m_16symtab_node_base ((*x).previous_sharing_asm_name);
> >           gt_ggc_m_16symtab_node_base ((*x).same_comdat_group);
> >           gt_ggc_m_20vec_ipa_ref_t_va_gc_ ((*x).ref_list.references);
> >           gt_ggc_m_9tree_node ((*x).alias_target);
> >           gt_ggc_m_18lto_file_decl_data ((*x).lto_file_data);
> >           break;
> >         case SYMTAB_VARIABLE:
> >           {
> >             varpool_node *sub = static_cast <varpool_node *> (x);
> >             gt_ggc_m_9tree_node ((*sub).decl);
> >             gt_ggc_m_16symtab_node_base ((*sub).next);
> >             gt_ggc_m_16symtab_node_base ((*sub).previous);
> >             gt_ggc_m_16symtab_node_base ((*sub).next_sharing_asm_name);
> >             gt_ggc_m_16symtab_node_base ((*sub).previous_sharing_asm_name);
> >             gt_ggc_m_16symtab_node_base ((*sub).same_comdat_group);
> >             gt_ggc_m_20vec_ipa_ref_t_va_gc_ ((*sub).ref_list.references);
> >             gt_ggc_m_9tree_node ((*sub).alias_target);
> >             gt_ggc_m_18lto_file_decl_data ((*sub).lto_file_data);
> >           }
> >           break;
> >         case SYMTAB_FUNCTION:
> >           {
> >             cgraph_node *sub = static_cast <cgraph_node *> (x);
> >             gt_ggc_m_11cgraph_edge ((*sub).callers);
> >             gt_ggc_m_11cgraph_edge ((*sub).indirect_calls);
> >             gt_ggc_m_16symtab_node_base ((*sub).origin);
> >             gt_ggc_m_16symtab_node_base ((*sub).nested);
> >             gt_ggc_m_16symtab_node_base ((*sub).next_nested);
> >             gt_ggc_m_16symtab_node_base ((*sub).next_sibling_clone);
> >             gt_ggc_m_16symtab_node_base ((*sub).prev_sibling_clone);
> >             gt_ggc_m_16symtab_node_base ((*sub).clones);
> >             gt_ggc_m_16symtab_node_base ((*sub).clone_of);
> >             gt_ggc_m_P11cgraph_edge4htab ((*sub).call_site_hash);
> >             gt_ggc_m_9tree_node ((*sub).former_clone_of);
> >             gt_ggc_m_16symtab_node_base ((*sub).global.inlined_to);
> >             gt_ggc_m_28vec_ipa_replace_map_p_va_gc_ ((*sub).clone.tree_map);
> >             gt_ggc_m_15bitmap_head_def ((*sub).clone.args_to_skip);
> >             gt_ggc_m_15bitmap_head_def ((*sub).clone.combined_args_to_skip);
> >             gt_ggc_m_9tree_node ((*sub).thunk.alias);
> >             gt_ggc_m_9tree_node ((*sub).decl);
> >             gt_ggc_m_16symtab_node_base ((*sub).next);
> >             gt_ggc_m_16symtab_node_base ((*sub).previous);
> >             gt_ggc_m_16symtab_node_base ((*sub).next_sharing_asm_name);
> >             gt_ggc_m_16symtab_node_base ((*sub).previous_sharing_asm_name);
> >             gt_ggc_m_16symtab_node_base ((*sub).same_comdat_group);
> >             gt_ggc_m_20vec_ipa_ref_t_va_gc_ ((*sub).ref_list.references);
> >             gt_ggc_m_9tree_node ((*sub).alias_target);
> >             gt_ggc_m_18lto_file_decl_data ((*sub).lto_file_data);
> >           }
> >           break;
> >         }
> >     }
> > }
> >
> > David Malcolm (3):
> >   Parse base classes for GTY-marked types
> >   Handle simple inheritance in gengtype.
> >   Convert symtab, cgraph and varpool nodes into a real class hierarchy
> >
> >  gcc/cgraph.h         |  38 +++++------------
> >  gcc/gengtype-parse.c |  47 +++++++++++++++++++--
> >  gcc/gengtype-state.c |   2 +
> >  gcc/gengtype.c       | 115 +++++++++++++++++++++++++++++++++++++++++++++------
> >  gcc/gengtype.h       |   9 +++-
> >  gcc/ipa-ref.h        |   6 +--
> >  gcc/is-a.h           |   6 +--
> >  gcc/symtab.c         |   4 +-
> >  8 files changed, 175 insertions(+), 52 deletions(-)
> >
> > --
> > 1.7.11.7
> >