Re: [PATCH v2 4/4] Add documentation about gengtype and inheritance

[David Malcolm <dmalcolm at redhat dot com>]

On Tue, 2013-10-15 at 12:49 -0600, Jeff Law wrote:
> On 09/24/13 11:49, David Malcolm wrote:
> > gcc/
> > 	* doc/gty.texi (GTY Options): Add note about inheritance to
> > 	description of desc and tag.
> > 	(Inheritance and GTY): New.
> So what happens if I have a class hierarchy without a gty-user marker 
> which violates the assumptions made by your new code?
> 
> I'm not worried about existing code with gty-user markers as I can 
> easily see how those are avoided.  I'm thinking more of new code where 
> the author isn't as familiar with the gty machinery as they should be or 
> in the future if we're using this stuf extensively and someone mucks 
> around with the class hierarchy and breaks one of the assumptions.  I 
> just want to make sure those two cases fail-safe.

I went through the various requirements listed in the doc and
investigated what happens when they're violated.  Some lead to immediate
build failure in gengtype (which is relatively sane), but some lead to
silent lack of field-traversal.  The details:

(A) Abstract base class e.g.:

class GTY(()) foo
{
  virtual void some_method() = 0;

  tree p;
};

fails immediately in gengtype, since gengtype can't parse "virtual".


(B) Failure to set a discriminator "desc" like this:

class GTY(()) foo
{
  tree p;
};

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

static GTY(()) foo *dummy;

leads to silent failure, where all you get is a traversal of "p", not of
"q", even though "dummy" could point to an instance of bar.


(C) Multiple inheritance:
class GTY(()) baz : public foo, public bar
{
public:
  tree r;
};

fails immediately with a parse error in gengtype:
../../src/gcc/cgraph.c:3025: parse error: expected '{', have ','


(D) Templates:

template <typename T>
class GTY(()) foo
{
public:
  tree p;
};

class GTY(()) bar : public foo<int>
{
public:
  tree q;
};

fail immediately with a parse error in gengtype:
../../src/gcc/cgraph.c:3018: parse error: expected '{', have '<'


(E) Failure to set a tag on the base class:
/* No "tag" set on foo */
class GTY((desc("%h.kind"))) foo
{
public:
  int kind;
  tree p;
};


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

leads to a build-time failure, due to gengtype writing "case (null):"
into the switch statement, which is a syntax error.


(F) Failure to set a tag on one of the subclasses:
class GTY((desc("%h.kind"), tag("0"))) foo
{
public:
  int kind;
  tree p;
};


/* No "tag" set on bar: */
class GTY(()) bar : public foo
{
public:
  tree q;
};

gives traversal functions in which the switch() statement doesn't have a
way to mark bar; missing values for "tag" lead to no marking happening.
That said, omitting the tag could be what the user meant, and the other
patch only creates allocator functions for those subclasses that have a
tag.   I think it's probably safest to add a:
  default: gcc_unreachable ();
case to the autogenerated switches, to cover the case where a unexpected
tag value is obtained from a discriminator.


(G) Failure to add GTY marking at all to a subclass e.g.:
class GTY((desc("%h.kind"), tag("0"))) foo
{
public:
  int kind;
  tree p;
};

/* bar lacks GTY() */
class bar : public foo
{
public:
  tree q;
};

leads to bar not having a tag value for the discriminator, so this is
similar to (F).


Summarizing: 
(B), (F), (G): silent possibility of failing to visit fields during
traversal (bad, bad, bad)

(A),  (C), (D), (E): build-time failures (relatively benign by
comparison)

> As for this patch, it is OK once the prerequisites go in.

AIUI you've approved each patch in the whole series, and so far I've
committed patches 1 and 2 of the 4 (relatively safe).

Clearly I should add some robustness to better handle the more egregious
failure modes described above.  Should I post patches to do so before
committing further, or can I commit the rest of the approved patches in
the "v2" series (patches 3 and 4), doing the hardening patches as a
followup?

As an aside, has anyone tried writing a testsuite for gengtype and the
GTY machinery?

Thanks
Dave