make gengtype more robust against user error

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.
---
 gcc/doc/gty.texi     |   7 +++
 gcc/gengtype-state.c |   2 +
 gcc/gengtype.c       | 119 ++++++++++++++++++++++++++++++++++++++-------------
 gcc/gengtype.h       |  10 +++++
 4 files changed, 109 insertions(+), 29 deletions(-)

On Fri, 2013-10-25 at 17:37 -0400, 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?


I don't have the power to approve this patch, but I hope it will get
accepted.

Cheers.

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

On Mon, 2013-10-28 at 15:19 -0600, Jeff Law wrote:
> 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
> >
> [...]
> Thanks for diving into this stuff and getting it fixed.
> 
> OK for the trunk,

Thanks; it passed bootstrap&regtesting on x86_64-unknown-linux, so I've
committed it to trunk as r204148, completing (I hope) the addition of
support in gengtype for some forms of simple class hierarchies.

I believe this may also complete the prerequisites for my conversion of
the symtable types into a C++ class hierarchy (in that IIRC all of those
patches were approved, but were pending this gengtype feature)... but
I'll have to see to what extent they've bitrotted (and recheck the
bootstrap/regtest, naturally).

Dave