On Fri, Sep 20, 2013 at 4:05 PM, David Malcolm <dmalc...@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
>
