diff mbox

gengtype: Support explicit pointers in template arguments

Message ID 1398913642.8042.67.camel@surprise
State New
Headers show

Commit Message

David Malcolm May 1, 2014, 3:07 a.m. UTC 
Currently, gengtype does not support template arguments that are
explicitly pointers, such as:
  static GTY(()) vec<gimple_statement_base *> test_gimple; giving this
error:
  ../../src/gcc/gimple-expr.c:902: parse error: expected a string constant or ',', have '*' instead 
requiring them to be typedefs.

This patch removes this restriction, supporting up to a single pointer
in each template argument.

It also adds support for inheritance, allowing:
  static GTY(()) vec<gimple_statement_phi *, va_gc> *test_phis;
where the generated gt-FOO.c file contains:

  void
  gt_ggc_mx (struct gimple_statement_phi *& x)
  {
    if (x)
      gt_ggc_mx_gimple_statement_base ((void *) x);
  }

i.e. handling the subclass by calling the marking function for the base
class.

Doing so uncovered an issue within write_user_func_for_structure_ptr,
which would unconditionally write out a func.  This would lead to
gt-FOO.c containing duplicate functions e.g.:
gtype-desc.c:280: multiple definition of `gt_ggc_mx(gimple_statement_base*&)'
if more than one GTY template type referenced such a type; for example like this:

  static GTY(()) vec<gimple, va_gc> *test_old_style_gimple;
  static GTY(()) vec<gimple_statement_base *, va_gc> *test_new_style_gimple;

where "gimple" is just an alias for "gimple_statement_base *".  This
could be worked around by ensuring that the source tree consistently
uses vec<> of just one kind, but for robustness the patch also makes
write_user_func_for_structure_ptr idempotent, so it only writes out the
func once for each (struct, which-of-ggc/pch) combination.

Motivation:
In the "Compile-time gimple-checking" discussion, Richi asked me to look
at eliminating the const_gimple typedef in favor of having "gimple" be
the struct, thus making the pointer to a gimple be explicit in the type:
  http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01520.html
as in:
  http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01632.html
> [...] And I never liked the
> const_ typedef variants that were introduced.  The main reason for
> them was probably to avoid all the churn of replacing the tree pointer
> typedef with a tree (non-pointer) typedef.  The mistake was to
> repeat that for 'gimple' ...

I'm attempting a patch for that, but in the meantime, this patch adds
the needed support to gengtype.

Successfully bootstrapped&regtested on x86_64-unknown-linux-gnu (Fedora
20).

OK for trunk?

Dave

Comments

Richard Sandiford May 2, 2014, 4:43 p.m. UTC | #1 
David Malcolm <dmalcolm@redhat.com> writes:
> Currently, gengtype does not support template arguments that are
> explicitly pointers, such as:
>   static GTY(()) vec<gimple_statement_base *> test_gimple; giving this
> error:
>   ../../src/gcc/gimple-expr.c:902: parse error: expected a string constant or ',', have '*' instead 
> requiring them to be typedefs.
>
> This patch removes this restriction, supporting up to a single pointer
> in each template argument.

Sorry to ask, but would you mind holding on until after the wide-int merge
(hopefully the next few days, this time for real)?  It looks like it
might clash with the support for nested templates.

Thanks,
Richard
David Malcolm May 2, 2014, 5:04 p.m. UTC | #2 
On Fri, 2014-05-02 at 17:43 +0100, Richard Sandiford wrote:
> David Malcolm <dmalcolm@redhat.com> writes:
> > Currently, gengtype does not support template arguments that are
> > explicitly pointers, such as:
> >   static GTY(()) vec<gimple_statement_base *> test_gimple; giving this
> > error:
> >   ../../src/gcc/gimple-expr.c:902: parse error: expected a string constant or ',', have '*' instead 
> > requiring them to be typedefs.
> >
> > This patch removes this restriction, supporting up to a single pointer
> > in each template argument.
> 
> Sorry to ask, but would you mind holding on until after the wide-int merge
> (hopefully the next few days, this time for real)?  It looks like it
> might clash with the support for nested templates.

Ack.   [This is enabling work for something I've already been asked to
wait post-4.9.1 for, so sure.  I hope it won't be too hard to redo on
top of your changes.]

Good luck with the wide-int merge
Jeff Law May 5, 2014, 4:18 p.m. UTC | #3 
On 04/30/14 21:07, David Malcolm wrote:
> Currently, gengtype does not support template arguments that are
> explicitly pointers, such as:
>    static GTY(()) vec<gimple_statement_base *> test_gimple; giving this
> error:
>    ../../src/gcc/gimple-expr.c:902: parse error: expected a string constant or ',', have '*' instead
> requiring them to be typedefs.
>
> This patch removes this restriction, supporting up to a single pointer
> in each template argument.
>
> It also adds support for inheritance, allowing:
>    static GTY(()) vec<gimple_statement_phi *, va_gc> *test_phis;
> where the generated gt-FOO.c file contains:
>
>    void
>    gt_ggc_mx (struct gimple_statement_phi *& x)
>    {
>      if (x)
>        gt_ggc_mx_gimple_statement_base ((void *) x);
>    }
>
> i.e. handling the subclass by calling the marking function for the base
> class.
>
> Doing so uncovered an issue within write_user_func_for_structure_ptr,
> which would unconditionally write out a func.  This would lead to
> gt-FOO.c containing duplicate functions e.g.:
> gtype-desc.c:280: multiple definition of `gt_ggc_mx(gimple_statement_base*&)'
> if more than one GTY template type referenced such a type; for example like this:
>
>    static GTY(()) vec<gimple, va_gc> *test_old_style_gimple;
>    static GTY(()) vec<gimple_statement_base *, va_gc> *test_new_style_gimple;
>
> where "gimple" is just an alias for "gimple_statement_base *".  This
> could be worked around by ensuring that the source tree consistently
> uses vec<> of just one kind, but for robustness the patch also makes
> write_user_func_for_structure_ptr idempotent, so it only writes out the
> func once for each (struct, which-of-ggc/pch) combination.
>
> Motivation:
> In the "Compile-time gimple-checking" discussion, Richi asked me to look
> at eliminating the const_gimple typedef in favor of having "gimple" be
> the struct, thus making the pointer to a gimple be explicit in the type:
>    http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01520.html
> as in:
>    http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01632.html
>> [...] And I never liked the
>> const_ typedef variants that were introduced.  The main reason for
>> them was probably to avoid all the churn of replacing the tree pointer
>> typedef with a tree (non-pointer) typedef.  The mistake was to
>> repeat that for 'gimple' ...
>
> I'm attempting a patch for that, but in the meantime, this patch adds
> the needed support to gengtype.
>
> Successfully bootstrapped&regtested on x86_64-unknown-linux-gnu (Fedora
> 20).
>
> OK for trunk?
Basically OK.  Per the wide-int folks request, please hold off until the 
wide-int merge is done.

If after wide-int, the only changes are trivial, then consider the patch 
pre-approved (post the final version here so that its archived).

If after wide-int merges the changes to this patch are non-trivial, then 
repost the RFA.

Obviously, you get to exercise some judgement as to what constitutes 
trivial vs non-trivial.

Jeff
diff mbox

Patch

From 8d7538cb6676a2f158ea9be56820a1d57eb34b82 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Wed, 30 Apr 2014 15:46:16 -0400
Subject: [PATCH] gengtype: Support explicit pointers in template arguments

gcc/
	* gengtype-parse.c (require3): New.
	(require_template_declaration): Update to support optional single *
	on a type.

	* gengtype.c (get_ultimate_base_class): Add a non-const overload.
	(create_user_defined_type): Handle a single level of explicit
	pointerness within template arguments.
	(struct write_types_data): Add field "kind".
	(filter_type_name): Handle "*" character.
	(write_user_func_for_structure_ptr): Require a write_types_data
	rather than just a prefix string, so that we can look up the kind
	of the wtd and use it as an index into wrote_user_func_for_ptr,
	ensuring that such functions are written at most once.  Support
	subclasses by invoking the marking function of the ultimate base
	class.
	(write_user_func_for_structure_body): Require a write_types_data
	rather than just a prefix string, so that we can pass this to
	write_user_func_for_structure_ptr.
	(write_func_for_structure): Likewise.
	(ggc_wtd): Add initializer of new "kind" field.
	(pch_wtd): Likewise.

	* gengtype.h (enum write_types_kinds): New.
	(struct type): Add field wrote_user_func_for_ptr to the "s"
	union member.
---
 gcc/gengtype-parse.c | 38 ++++++++++++++++++++++++++---
 gcc/gengtype.c       | 69 +++++++++++++++++++++++++++++++++++++++++-----------
 gcc/gengtype.h       | 12 +++++++++
 3 files changed, 102 insertions(+), 17 deletions(-)

diff --git a/gcc/gengtype-parse.c b/gcc/gengtype-parse.c
index bb7bcf7..a374ef4 100644
--- a/gcc/gengtype-parse.c
+++ b/gcc/gengtype-parse.c
@@ -197,6 +197,23 @@  require2 (int t1, int t2)
   return v;
 }
 
+/* If the next token does not have one of the codes T1, T2 or T3, report a
+   parse error; otherwise return the token's value.  */
+static const char *
+require3 (int t1, int t2, int t3)
+{
+  int u = token ();
+  const char *v = advance ();
+  if (u != t1 && u != t2 && u != t3)
+    {
+      parse_error ("expected %s, %s or %s, have %s",
+		   print_token (t1, 0), print_token (t2, 0),
+		   print_token (t3, 0), print_token (u, v));
+      return 0;
+    }
+  return v;
+}
+
 /* Near-terminals.  */
 
 /* C-style string constant concatenation: STRING+
@@ -228,7 +245,9 @@  string_seq (void)
 
 /* The caller has detected a template declaration that starts
    with TMPL_NAME.  Parse up to the closing '>'.  This recognizes
-   simple template declarations of the form ID<ID1,ID2,...,IDn>.
+   simple template declarations of the form ID<ID1,ID2,...,IDn>,
+   potentially with a single level of indirection e.g.
+     ID<ID1 *, ID2, ID3 *, ..., IDn>.
    It does not try to parse anything more sophisticated than that.
 
    Returns the template declaration string "ID<ID1,ID2,...,IDn>".  */
@@ -237,6 +256,7 @@  static const char *
 require_template_declaration (const char *tmpl_name)
 {
   char *str;
+  int num_indirections = 0;
 
   /* Recognize the opening '<'.  */
   require ('<');
@@ -245,9 +265,21 @@  require_template_declaration (const char *tmpl_name)
   /* Read the comma-separated list of identifiers.  */
   while (token () != '>')
     {
-      const char *id = require2 (ID, ',');
+      const char *id = require3 (ID, '*', ',');
       if (id == NULL)
-	id = ",";
+	{
+	  if (T.code == '*')
+	    {
+	      id = "*";
+	      if (num_indirections++)
+		parse_error ("only one level of indirection is supported"
+			     " in template arguments");
+	    }
+	  else
+	    id = ",";
+	}
+      else
+	num_indirections = 0;
       str = concat (str, id, (char *) 0);
     }
 
diff --git a/gcc/gengtype.c b/gcc/gengtype.c
index 031004a..ddfaeaf 100644
--- a/gcc/gengtype.c
+++ b/gcc/gengtype.c
@@ -146,6 +146,14 @@  get_ultimate_base_class (const_type_p s)
     s = s->u.s.base_class;
   return s;
 }
+
+static type_p
+get_ultimate_base_class (type_p s)
+{
+  while (s->u.s.base_class)
+    s = s->u.s.base_class;
+  return s;
+}
 
 /* Input file handling. */
 
@@ -590,7 +598,7 @@  create_user_defined_type (const char *type_name, struct fileloc *pos)
       /* We only accept simple template declarations (see
 	 require_template_declaration), so we only need to parse a
 	 comma-separated list of strings, implicitly assumed to
-	 be type names.  */
+	 be type names, potentially with "*" characters.  */
       char *arg = open_bracket + 1;
       char *type_id = strtok (arg, ",>");
       pair_p fields = 0;
@@ -598,8 +606,28 @@  create_user_defined_type (const char *type_name, struct fileloc *pos)
 	{
 	  /* Create a new field for every type found inside the template
 	     parameter list.  */
-	  const char *field_name = xstrdup (type_id);
-	  type_p arg_type = resolve_typedef (field_name, pos);
+
+	  /* Support a single trailing "*" character.  */
+	  const char *star = strchr (type_id, '*');
+	  int is_ptr = (star != NULL);
+	  size_t offset_to_star = star - type_id;
+	  if (is_ptr)
+	    offset_to_star = star - type_id;
+
+	  char *field_name = xstrdup (type_id);
+
+	  type_p arg_type;
+	  if (is_ptr)
+	    {
+	      /* Strip off the first '*' character (and any subsequent text). */
+	      *(field_name + offset_to_star) = '\0';
+
+	      arg_type = find_structure (field_name, TYPE_STRUCT);
+	      arg_type = create_pointer (arg_type);
+	    }
+	  else
+	    arg_type = resolve_typedef (field_name, pos);
+
 	  fields = create_field_at (fields, arg_type, field_name, 0, pos);
 	  type_id = strtok (0, ",>");
 	}
@@ -2468,6 +2496,7 @@  struct write_types_data
   const char *reorder_note_routine;
   const char *comment;
   int skip_hooks;		/* skip hook generation if non zero */
+  enum write_types_kinds kind;
 };
 
 static void output_escaped_param (struct walk_type_data *d,
@@ -2544,7 +2573,8 @@  filter_type_name (const char *type_name)
       size_t i;
       char *s = xstrdup (type_name);
       for (i = 0; i < strlen (s); i++)
-	if (s[i] == '<' || s[i] == '>' || s[i] == ':' || s[i] == ',')
+	if (s[i] == '<' || s[i] == '>' || s[i] == ':' || s[i] == ','
+	    || s[i] == '*')
 	  s[i] = '_';
       return s;
     }
@@ -3508,10 +3538,10 @@  write_marker_function_name (outf_p of, type_p s, const char *prefix)
 
 /* Write on OF a user-callable routine to act as an entry point for
    the marking routine for S, generated by write_func_for_structure.
-   PREFIX is the prefix to use to distinguish ggc and pch markers.  */
+   WTD distinguishes between ggc and pch markers.  */
 
 static void
-write_user_func_for_structure_ptr (outf_p of, type_p s, const char *prefix)
+write_user_func_for_structure_ptr (outf_p of, type_p s, const write_types_data *wtd)
 {
   /* Parameterized structures are not supported in user markers. There
      is no way for the marker function to know which specific type
@@ -3541,13 +3571,23 @@  write_user_func_for_structure_ptr (outf_p of, type_p s, const char *prefix)
 	break;
       }
 
+  DBGPRINTF ("write_user_func_for_structure_ptr: %s %s", s->u.s.tag,
+	     wtd->prefix);
+
+  /* Only write the function once. */
+  if (s->u.s.wrote_user_func_for_ptr[wtd->kind])
+    return;
+  s->u.s.wrote_user_func_for_ptr[wtd->kind] = true;
+
   oprintf (of, "\nvoid\n");
-  oprintf (of, "gt_%sx (", prefix);
+  oprintf (of, "gt_%sx (", wtd->prefix);
   write_type_decl (of, s);
   oprintf (of, " *& x)\n");
   oprintf (of, "{\n");
   oprintf (of, "  if (x)\n    ");
-  write_marker_function_name (of, alias_of ? alias_of : s, prefix);
+  write_marker_function_name (of,
+			      alias_of ? alias_of : get_ultimate_base_class (s),
+			      wtd->prefix);
   oprintf (of, " ((void *) x);\n");
   oprintf (of, "}\n");
 }
@@ -3585,7 +3625,8 @@  write_user_func_for_structure_body (type_p s, const char *prefix,
    which just marks the fields of T.  */
 
 static void
-write_user_marking_functions (type_p s, const char *prefix,
+write_user_marking_functions (type_p s,
+			      const write_types_data *w,
 			      struct walk_type_data *d)
 {
   gcc_assert (s->kind == TYPE_USER_STRUCT);
@@ -3597,10 +3638,10 @@  write_user_marking_functions (type_p s, const char *prefix,
 	{
 	  type_p pointed_to_type = fld_type->u.p;
 	  if (union_or_struct_p (pointed_to_type))
-	    write_user_func_for_structure_ptr (d->of, pointed_to_type, prefix);
+	    write_user_func_for_structure_ptr (d->of, pointed_to_type, w);
 	}
       else if (union_or_struct_p (fld_type))
-	write_user_func_for_structure_body (fld_type, prefix, d);
+	write_user_func_for_structure_body (fld_type, w->prefix, d);
     }
 }
 
@@ -3798,7 +3839,7 @@  write_func_for_structure (type_p orig_s, type_p s, type_p *param,
   oprintf (d.of, "}\n");
 
   if (orig_s->kind == TYPE_USER_STRUCT)
-    write_user_marking_functions (orig_s, wtd->prefix, &d);
+    write_user_marking_functions (orig_s, wtd, &d);
 }
 
 
@@ -3976,14 +4017,14 @@  write_types (outf_p output_header, type_p structures, type_p param_structs,
 static const struct write_types_data ggc_wtd = {
   "ggc_m", NULL, "ggc_mark", "ggc_test_and_set_mark", NULL,
   "GC marker procedures.  ",
-  FALSE
+  FALSE, WTK_GGC
 };
 
 static const struct write_types_data pch_wtd = {
   "pch_n", "pch_p", "gt_pch_note_object", "gt_pch_note_object",
   "gt_pch_note_reorder",
   "PCH type-walking procedures.  ",
-  TRUE
+  TRUE, WTK_PCH
 };
 
 /* Write out the local pointer-walking routines.  */
diff --git a/gcc/gengtype.h b/gcc/gengtype.h
index 345a545..6369001 100644
--- a/gcc/gengtype.h
+++ b/gcc/gengtype.h
@@ -127,7 +127,15 @@  extern type_p structures;
 extern type_p param_structs;
 extern pair_p variables;
 
+/* An enum for distinguishing GGC vs PCH.  */
 
+enum write_types_kinds
+{
+  WTK_GGC,
+  WTK_PCH,
+
+  NUM_WTK
+};
 
 /* Discrimating kind of types we can understand.  */
 
@@ -302,6 +310,10 @@  struct type {
       type_p first_subclass;
       /* The next in that list.  */
       type_p next_sibling_class;
+
+      /* Have we already written ggc/pch user func for ptr to this?
+	 (in write_user_func_for_structure_ptr).  */
+      bool wrote_user_func_for_ptr[NUM_WTK];
     } s;
 
     /* when TYPE_SCALAR: */
-- 
1.8.5.3