Re: [PATCH] make gengtype more robust against user error

[Jeff Law]

On 10/25/13 15:37, David Malcolm wrote:

This patch addresses various forms of failure described in
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg01974.html

It adds a "default: gcc_unreachable();" to the autogenerated switch()
statement in the routines for a base class, converting various kinds of
tag errors from leading to silent lack-of-field traversal into giving
run-time assertion failures (addressing (F) and (G))

It also issues an error within gengtype itself for a missing "desc"
(failure "B"), turning this into an error message from gengtype.

I found another potential failure mode:

(H) If you had:

class GTY((desc("%0.kind"), tag("0"))) foo
{
public:
   int kind;
   tree p;
};

class GTY((tag("1"))) bar : public foo
{
public:
   tree q;
};

static GTY(()) foo *dummy_foo;

and there are no explicit pointers to "bar" in the code, gengtype
treated it as unused, and silently omitted the case for it from
foo's marking routine.

I've updated set_gc_type_used so that it propagates usage down into
subclasses (which recurses), fixing this issue.

To do this efficiently we need to track subclasses in within gengtype,
so the patch also adds that, and uses it to eliminate an O(N^2).

Note that for error (G), if a class within the hierarchy omits a GTY
marker, gengtype doesn't parse it at all, and so doesn't parse the
inheritance information - so we can't issue a warning about this.
However, the lack of a tag will trigger a run-time assertion failure
due to hitting the "default:  gcc_unreachable();" in the switch.
The patch also adds a paragraph to the docs, spelling out the need
for evary class in such a hierarchy to have a GTY marker.

I believe this addresses all of the silent-lack-of-field-traversal
issues, converting them to gengtype errors or runtime assertions.
It also adds a handler for (E), turning this from a failure to
compile bogus C to a specific error in gengtype.

I'm bootstrapping/regtesting now.
OK for trunk if that passes?

gcc/
        * doc/gty.texi ("Inheritance and GTY"): Make it clear that
        to use autogenerated markers for a class-hierarchy, every class
        must have a GTY marker.
        * gengtype.h (struct type): Add linked list of subclasses to
        the "s" member of the union.
        (add_subclass): New decl.
        * gengtype-state.c (read_state_struct_type): Set up subclass
        linked list.
        * gengtype.c (get_ultimate_base_class): New.
        (add_subclass): New.
        (new_structure): Set up subclass linked list.
        (set_gc_used_type): Propagate usage information to subclasses.
        (output_mangled_typename): Use get_ultimate_base_class.
        (walk_subclasses): Use the subclass linked list, avoiding an
        O(N^2) when writing out all types.
        (walk_type): Issue an error if the base class is missing a tag,
        rather than generating bogus C code.  Add a gcc_unreachable
        default case, in case people omit tags from concrete subclasses,
        or get the values wrong.
        (write_func_for_structure): Issue an error for subclasses for
        which the base doesn't have a "desc", since otherwise the
        autogenerated routines for the base would silently fail to visit
        any subclass fields.
        (write_root): Use get_ultimate_base_class, tweaking constness of
        tp to match that function's signature.

Thanks for diving into this stuff and getting it fixed.

OK for the trunk,

Jeff