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

[Richard Biener <richard dot guenther at gmail dot com>]

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.

> 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?

My initial worry about C++-ifying gengtype was exactly because of such
arbitrary restrictions.  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

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
>