[cxx-conversion] Add Record Builder Class

Add class record_builder to ease construction of records and unions.  Use it
in some appropriate places.

Nathan please review the vxworks changes.

tree.h
	New class record_builder.

tree.c
	Implement record_builder member functions.

asan.c
	Change asan_global_struct to use record_builder.

coverage.c
	Change build_info_type() to use record_builder.  It now takes a
	record_builder as a parameter and returns the tree representing
	the type.

	Change build_fn_info_type() to use record_builder.  It now returns
	the tree representing the type.

	Modify coverage_obj_init() to call them appropriately.

tree-mudflap.c
	Change mf_make_mf_cache_struct_type() to use record_builder.

target.def
	Replace the emutls var_fields hook with object_type hook.  The
	essential difference is that the hook is now responsible for full
	construction of the type, not just adding fields.

targhooks.h
	Replace default_emutls_var_fields() with default_emutls_object_type().

tree-emutls.c
	Replace default_emutls_var_fields() with default_emutls_object_type().
	Use record_builder within default_emutls_object_type().

	Change get_emutls_object_type to use the new target hook.

doc/tm.texi.in
	Replace TARGET_EMUTLS_VAR_FIELDS with TARGET_EMUTLS_OBJECT_TYPE.

doc/tm.texi
	Replace TARGET_EMUTLS_VAR_FIELDS with TARGET_EMUTLS_OBJECT_TYPE.

config/vxworks.c
	Replace vxworks_emutls_var_fields() with vxworks_emutls_object_type().
	Use record_builder within vxworks_emutls_object_type().

Tested on x86_64.  Tested with config-list.mk on vxworks targets.

On Tue, Feb 12, 2013 at 2:47 PM, Lawrence Crowl <crowl@google.com> wrote:

> @@ -182,24 +163,9 @@ default_emutls_var_init (tree to, tree d
>  static tree
>  get_emutls_object_type (void)
>  {
> -  tree type, type_name, field;
> -
> -  type = emutls_object_type;
> -  if (type)
> -    return type;
> -
> -  emutls_object_type = type = lang_hooks.types.make_type (RECORD_TYPE);
> -  type_name = NULL;
> -  field = targetm.emutls.var_fields (type, &type_name);
> -  if (!type_name)
> -    type_name = get_identifier ("__emutls_object");
> -  type_name = build_decl (UNKNOWN_LOCATION,
> -                         TYPE_DECL, type_name, type);
> -  TYPE_NAME (type) = type_name;
> -  TYPE_FIELDS (type) = field;
> -  layout_type (type);
> -
> -  return type;
> +  if (!emutls_object_type)
> +    emutls_object_type = targetm.emutls.object_type ();
> +  return emutls_object_type;

Hm, this does not look like an idempotent change.   Where did I get lost?

===================================================================
> --- gcc/coverage.c      (revision 195904)
> +++ gcc/coverage.c      (working copy)
> @@ -121,8 +121,8 @@ static const char *const ctr_names[GCOV_
>  /* Forward declarations.  */
>  static void read_counts_file (void);
>  static tree build_var (tree, tree, int);
> -static void build_fn_info_type (tree, unsigned, tree);
> -static void build_info_type (tree, tree);
> +static tree build_fn_info_type (unsigned, tree);
> +static tree build_info_type (record_builder &, tree);

I don't really like unnecessary forward declarations.  But I guess
it's OK, since you are replacing existing ones.


> -static void
> -build_fn_info_type (tree type, unsigned counters, tree gcov_info_type)
> +static tree
> +build_fn_info_type (unsigned counters, tree gcov_info_type)

So, here you are changing the signature because the caller used to
create an empty record and now it doesn't need to?  We are going to be
creating it in the constructor, right?


Diego.

On 2/13/13, Diego Novillo <dnovillo@google.com> wrote:
> On Tue, Feb 12, 2013 at 2:47 PM, Lawrence Crowl <crowl@google.com> wrote:
>> @@ -182,24 +163,9 @@ default_emutls_var_init (tree to, tree d
>>  static tree
>>  get_emutls_object_type (void)
>>  {
>> -  tree type, type_name, field;
>> -
>> -  type = emutls_object_type;
>> -  if (type)
>> -    return type;
>> -
>> -  emutls_object_type = type = lang_hooks.types.make_type (RECORD_TYPE);
>> -  type_name = NULL;
>> -  field = targetm.emutls.var_fields (type, &type_name);
>> -  if (!type_name)
>> -    type_name = get_identifier ("__emutls_object");
>> -  type_name = build_decl (UNKNOWN_LOCATION,
>> -                         TYPE_DECL, type_name, type);
>> -  TYPE_NAME (type) = type_name;
>> -  TYPE_FIELDS (type) = field;
>> -  layout_type (type);
>> -
>> -  return type;
>> +  if (!emutls_object_type)
>> +    emutls_object_type = targetm.emutls.object_type ();
>> +  return emutls_object_type;
>
> Hm, this does not look like an idempotent change.  Where did I
> get lost?

The responsibility for creating the type has moved into the new hook.
The old hook only had responsibility for adding fields.  And changing
the name if it wasn't right.  The new structure has a much clearer
division of responsibility.

> ===================================================================
>> --- gcc/coverage.c      (revision 195904)
>> +++ gcc/coverage.c      (working copy)
>> @@ -121,8 +121,8 @@ static const char *const ctr_names[GCOV_
>>  /* Forward declarations.  */
>>  static void read_counts_file (void);
>>  static tree build_var (tree, tree, int);
>> -static void build_fn_info_type (tree, unsigned, tree);
>> -static void build_info_type (tree, tree);
>> +static tree build_fn_info_type (unsigned, tree);
>> +static tree build_info_type (record_builder &, tree);
>
> I don't really like unnecessary forward declarations.  But I guess
> it's OK, since you are replacing existing ones.

Yeah.  I figure applying style changes should be a separate patch.

>> -static void
>> -build_fn_info_type (tree type, unsigned counters, tree gcov_info_type)
>> +static tree
>> +build_fn_info_type (unsigned counters, tree gcov_info_type)
>
> So, here you are changing the signature because the caller used
> to create an empty record and now it doesn't need to?  We are
> going to be creating it in the constructor, right?

Correct.

Thanks.  The patch is OK for the branch.  You can address Nathan's
review after he's back and gets a chance to look at it.

Let me know when the patch is in.  I've got another merge in process.


Diego.

On 2/13/13, Diego Novillo <dnovillo@google.com> wrote:
> Thanks.  The patch is OK for the branch.  You can address Nathan's
> review after he's back and gets a chance to look at it.
>
> Let me know when the patch is in.  I've got another merge in process.

Committed.

On Tue, Feb 12, 2013 at 8:47 PM, Lawrence Crowl <crowl@google.com> wrote:
> Add class record_builder to ease construction of records and unions.  Use it
> in some appropriate places.
>
> Nathan please review the vxworks changes.
>
> tree.h
>         New class record_builder.
>
> tree.c
>         Implement record_builder member functions.
>
> asan.c
>         Change asan_global_struct to use record_builder.
>
> coverage.c
>         Change build_info_type() to use record_builder.  It now takes a
>         record_builder as a parameter and returns the tree representing
>         the type.
>
>         Change build_fn_info_type() to use record_builder.  It now returns
>         the tree representing the type.
>
>         Modify coverage_obj_init() to call them appropriately.
>
> tree-mudflap.c
>         Change mf_make_mf_cache_struct_type() to use record_builder.
>
> target.def
>         Replace the emutls var_fields hook with object_type hook.  The
>         essential difference is that the hook is now responsible for full
>         construction of the type, not just adding fields.
>
> targhooks.h
>         Replace default_emutls_var_fields() with default_emutls_object_type().
>
> tree-emutls.c
>         Replace default_emutls_var_fields() with default_emutls_object_type().
>         Use record_builder within default_emutls_object_type().
>
>         Change get_emutls_object_type to use the new target hook.
>
> doc/tm.texi.in
>         Replace TARGET_EMUTLS_VAR_FIELDS with TARGET_EMUTLS_OBJECT_TYPE.
>
> doc/tm.texi
>         Replace TARGET_EMUTLS_VAR_FIELDS with TARGET_EMUTLS_OBJECT_TYPE.
>
> config/vxworks.c
>         Replace vxworks_emutls_var_fields() with vxworks_emutls_object_type().
>         Use record_builder within vxworks_emutls_object_type().
>
> Tested on x86_64.  Tested with config-list.mk on vxworks targets.
>
>
> Index: gcc/tree-emutls.c
> ===================================================================
> --- gcc/tree-emutls.c   (revision 195904)
> +++ gcc/tree-emutls.c   (working copy)
> @@ -103,41 +103,22 @@ get_emutls_object_name (tree name)
>    return prefix_name (prefix, name);
>  }
>
> -/* Create the fields of the type for the control variables.  Ordinarily
> +/* Create the type for the control variables.  Ordinarily
>     this must match struct __emutls_object defined in emutls.c.  However
>     this is a target hook so that VxWorks can define its own layout.  */
>
>  tree
> -default_emutls_var_fields (tree type, tree *name ATTRIBUTE_UNUSED)
> +default_emutls_object_type (void)
>  {
> -  tree word_type_node, field, next_field;
> -
> -  field = build_decl (UNKNOWN_LOCATION,
> -                     FIELD_DECL, get_identifier ("__templ"), ptr_type_node);
> -  DECL_CONTEXT (field) = type;
> -  next_field = field;
> -
> -  field = build_decl (UNKNOWN_LOCATION,
> -                     FIELD_DECL, get_identifier ("__offset"),
> -                     ptr_type_node);
> -  DECL_CONTEXT (field) = type;
> -  DECL_CHAIN (field) = next_field;
> -  next_field = field;
> -
> -  word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
> -  field = build_decl (UNKNOWN_LOCATION,
> -                     FIELD_DECL, get_identifier ("__align"),
> -                     word_type_node);
> -  DECL_CONTEXT (field) = type;
> -  DECL_CHAIN (field) = next_field;
> -  next_field = field;
> -
> -  field = build_decl (UNKNOWN_LOCATION,
> -                     FIELD_DECL, get_identifier ("__size"), word_type_node);
> -  DECL_CONTEXT (field) = type;
> -  DECL_CHAIN (field) = next_field;
> -
> -  return field;
> +  tree word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
> +  record_builder rec;
> +  rec.add_field ("__size", word_type_node);
> +  rec.add_field ("__align", word_type_node);
> +  rec.add_field ("__offset", ptr_type_node);
> +  rec.add_field ("__templ", ptr_type_node);
> +  rec.layout ();

That's awkward - you want to hide the fact that layout has to happen
and that it has to happen "last".  Just make it a side-effect of
.as_tree ().  Note that add_field want's to return the FIELD_DECL
created, people may want to alter it.

Note that tag_name does not allow the way C++ uses this (it can be
a TYPE_DECL).

Overall I'm not sure this is a good abstraction unless you manage to
make the frontends use it.

Richard.


> +  rec.decl_name ("__emutls_object");
> +  return rec.as_tree ();
>  }
>
>  /* Initialize emulated tls object TO, which refers to TLS variable DECL and
> @@ -182,24 +163,9 @@ default_emutls_var_init (tree to, tree d
>  static tree
>  get_emutls_object_type (void)
>  {
> -  tree type, type_name, field;
> -
> -  type = emutls_object_type;
> -  if (type)
> -    return type;
> -
> -  emutls_object_type = type = lang_hooks.types.make_type (RECORD_TYPE);
> -  type_name = NULL;
> -  field = targetm.emutls.var_fields (type, &type_name);
> -  if (!type_name)
> -    type_name = get_identifier ("__emutls_object");
> -  type_name = build_decl (UNKNOWN_LOCATION,
> -                         TYPE_DECL, type_name, type);
> -  TYPE_NAME (type) = type_name;
> -  TYPE_FIELDS (type) = field;
> -  layout_type (type);
> -
> -  return type;
> +  if (!emutls_object_type)
> +    emutls_object_type = targetm.emutls.object_type ();
> +  return emutls_object_type;
>  }
>
>  /* Create a read-only variable like DECL, with the same DECL_INITIAL.
> Index: gcc/asan.c
> ===================================================================
> --- gcc/asan.c  (revision 195904)
> +++ gcc/asan.c  (working copy)
> @@ -1496,28 +1496,16 @@ transform_statements (void)
>  static tree
>  asan_global_struct (void)
>  {
> -  static const char *field_names[5]
> -    = { "__beg", "__size", "__size_with_redzone",
> -       "__name", "__has_dynamic_init" };
> -  tree fields[5], ret;
> -  int i;
> -
> -  ret = make_node (RECORD_TYPE);
> -  for (i = 0; i < 5; i++)
> -    {
> -      fields[i]
> -       = build_decl (UNKNOWN_LOCATION, FIELD_DECL,
> -                     get_identifier (field_names[i]),
> -                     (i == 0 || i == 3) ? const_ptr_type_node
> -                     : build_nonstandard_integer_type (POINTER_SIZE, 1));
> -      DECL_CONTEXT (fields[i]) = ret;
> -      if (i)
> -       DECL_CHAIN (fields[i - 1]) = fields[i];
> -    }
> -  TYPE_FIELDS (ret) = fields[0];
> -  TYPE_NAME (ret) = get_identifier ("__asan_global");
> -  layout_type (ret);
> -  return ret;
> +  tree ptrint_type = build_nonstandard_integer_type (POINTER_SIZE, 1);
> +  record_builder rec;
> +  rec.add_field ("__beg", const_ptr_type_node);
> +  rec.add_field ("__size", ptrint_type);
> +  rec.add_field ("__size_with_redzone", ptrint_type);
> +  rec.add_field ("__name", const_ptr_type_node);
> +  rec.add_field ("__has_dynamic_init", ptrint_type);
> +  rec.layout ();
> +  rec.tag_name ("__asan_global");
> +  return rec.as_tree ();
>  }
>
>  /* Append description of a single global DECL into vector V.
> Index: gcc/coverage.c
> ===================================================================
> --- gcc/coverage.c      (revision 195904)
> +++ gcc/coverage.c      (working copy)
> @@ -121,8 +121,8 @@ static const char *const ctr_names[GCOV_
>  /* Forward declarations.  */
>  static void read_counts_file (void);
>  static tree build_var (tree, tree, int);
> -static void build_fn_info_type (tree, unsigned, tree);
> -static void build_info_type (tree, tree);
> +static tree build_fn_info_type (unsigned, tree);
> +static tree build_info_type (record_builder &, tree);
>  static tree build_fn_info (const struct coverage_data *, tree, tree);
>  static tree build_info (tree, tree);
>  static bool coverage_obj_init (void);
> @@ -693,63 +693,47 @@ build_var (tree fn_decl, tree type, int
>    return var;
>  }
>
> -/* Creates the gcov_fn_info RECORD_TYPE.  */
> +/* Creates the gcov_fn_info RECORD_TYPE, given the number of COUNTERS and
> +   using the GCOV_INFO_TYPE.  */
>
> -static void
> -build_fn_info_type (tree type, unsigned counters, tree gcov_info_type)
> +static tree
> +build_fn_info_type (unsigned counters, tree gcov_info_type)
>  {
> -  tree ctr_info = lang_hooks.types.make_type (RECORD_TYPE);
> -  tree field, fields;
> -  tree array_type;
> -
>    gcc_assert (counters);
> -
> -  /* ctr_info::num */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     get_gcov_unsigned_t ());
> -  fields = field;
> -
> -  /* ctr_info::values */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     build_pointer_type (get_gcov_type ()));
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> -
> -  finish_builtin_struct (ctr_info, "__gcov_ctr_info", fields, NULL_TREE);
>
> +  record_builder bld_ctr_info;
> +  tree gcov_type = get_gcov_type ();
> +  tree gcov_ptr_type = build_pointer_type (gcov_type);
> +  bld_ctr_info.add_field (NULL_TREE, gcov_type, BUILTINS_LOCATION);
> +  bld_ctr_info.add_field (NULL_TREE, gcov_ptr_type, BUILTINS_LOCATION);
> +  bld_ctr_info.layout ();
> +  bld_ctr_info.decl_name ("__gcov_ctr_info", BUILTINS_LOCATION);
> +  tree ctr_info = bld_ctr_info.as_tree ();
> +
> +  record_builder bld_fn_info;
>    /* key */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     build_pointer_type (build_qualified_type
> -                                         (gcov_info_type, TYPE_QUAL_CONST)));
> -  fields = field;
> -
> +  tree pc_gcov_info
> +       = build_pointer_type (build_qualified_type (gcov_info_type,
> +                                                   TYPE_QUAL_CONST));
> +  bld_fn_info.add_field (NULL_TREE, pc_gcov_info, BUILTINS_LOCATION);
> +
>    /* ident */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     get_gcov_unsigned_t ());
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> -
> +  bld_fn_info.add_field (NULL_TREE, get_gcov_unsigned_t ());
> +
>    /* lineno_checksum */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     get_gcov_unsigned_t ());
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> +  bld_fn_info.add_field (NULL_TREE, get_gcov_unsigned_t ());
>
>    /* cfg checksum */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     get_gcov_unsigned_t ());
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> -
> -  array_type = build_index_type (size_int (counters - 1));
> -  array_type = build_array_type (ctr_info, array_type);
> +  bld_fn_info.add_field (NULL_TREE, get_gcov_unsigned_t ());
>
>    /* counters */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE, array_type);
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> +  tree index_type = build_index_type (size_int (counters - 1));
> +  tree array_type = build_array_type (ctr_info, index_type);
>
> -  finish_builtin_struct (type, "__gcov_fn_info", fields, NULL_TREE);
> +  bld_fn_info.add_field (NULL_TREE, array_type, BUILTINS_LOCATION);
> +  bld_fn_info.layout ();
> +  bld_fn_info.decl_name ("__gcov_fn_info", BUILTINS_LOCATION);
> +  return bld_fn_info.as_tree ();
>  }
>
>  /* Returns a CONSTRUCTOR for a gcov_fn_info.  DATA is
> @@ -819,69 +803,52 @@ build_fn_info (const struct coverage_dat
>    return build_constructor (type, v1);
>  }
>
> -/* Create gcov_info struct.  TYPE is the incomplete RECORD_TYPE to be
> +/* Return gcov_info struct.  BLD_INFO_TYPE is the record_builder to be
>     completed, and FN_INFO_PTR_TYPE is a pointer to the function info type.  */
>
> -static void
> -build_info_type (tree type, tree fn_info_ptr_type)
> +static tree
> +build_info_type (record_builder &bld_info_type, tree fn_info_ptr_type)
>  {
> -  tree field, fields = NULL_TREE;
> -  tree merge_fn_type;
> +  tree uns_type = get_gcov_unsigned_t ();
>
>    /* Version ident */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     get_gcov_unsigned_t ());
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> +  bld_info_type.add_field (NULL_TREE, uns_type, BUILTINS_LOCATION);
>
>    /* next pointer */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     build_pointer_type (build_qualified_type
> -                                         (type, TYPE_QUAL_CONST)));
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> +  tree self_type = bld_info_type.as_tree ();
> +  tree qual_info = build_qualified_type (self_type, TYPE_QUAL_CONST);
> +  tree ptr_info = build_pointer_type (qual_info);
> +  bld_info_type.add_field (NULL_TREE, ptr_info, BUILTINS_LOCATION);
>
>    /* stamp */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     get_gcov_unsigned_t ());
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> +  bld_info_type.add_field (NULL_TREE, uns_type, BUILTINS_LOCATION);
>
>    /* Filename */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     build_pointer_type (build_qualified_type
> -                                         (char_type_node, TYPE_QUAL_CONST)));
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> +  tree qual_char = build_qualified_type (char_type_node, TYPE_QUAL_CONST);
> +  tree ptr_char = build_pointer_type (qual_char);
> +  bld_info_type.add_field (NULL_TREE, ptr_char, BUILTINS_LOCATION);
>
>    /* merge fn array */
> -  merge_fn_type
> +  tree merge_fn_type
>      = build_function_type_list (void_type_node,
>                                 build_pointer_type (get_gcov_type ()),
> -                               get_gcov_unsigned_t (), NULL_TREE);
> -  merge_fn_type
> +                               uns_type, NULL_TREE);
> +  tree array_fn_type
>      = build_array_type (build_pointer_type (merge_fn_type),
>                         build_index_type (size_int (GCOV_COUNTERS - 1)));
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     merge_fn_type);
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> -
> +  bld_info_type.add_field (NULL_TREE, array_fn_type, BUILTINS_LOCATION);
> +
>    /* n_functions */
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     get_gcov_unsigned_t ());
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> -
> +  bld_info_type.add_field (NULL_TREE, uns_type, BUILTINS_LOCATION);
> +
>    /* function_info pointer pointer */
> -  fn_info_ptr_type = build_pointer_type
> +  tree fn_info_ptr_ptr_type = build_pointer_type
>      (build_qualified_type (fn_info_ptr_type, TYPE_QUAL_CONST));
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
> -                     fn_info_ptr_type);
> -  DECL_CHAIN (field) = fields;
> -  fields = field;
> +  bld_info_type.add_field (NULL_TREE, fn_info_ptr_ptr_type, BUILTINS_LOCATION);
>
> -  finish_builtin_struct (type, "__gcov_info", fields, NULL_TREE);
> +  bld_info_type.layout ();
> +  bld_info_type.decl_name ("__gcov_info");
> +  return bld_info_type.as_tree ();
>  }
>
>  /* Returns a CONSTRUCTOR for the gcov_info object.  INFO_TYPE is the
> @@ -974,7 +941,7 @@ build_info (tree info_type, tree fn_ary)
>  static bool
>  coverage_obj_init (void)
>  {
> -  tree gcov_info_type, ctor, stmt, init_fn;
> +  tree ctor, stmt, init_fn;
>    unsigned n_counters = 0;
>    unsigned ix;
>    struct coverage_data *fn;
> @@ -1005,12 +972,11 @@ coverage_obj_init (void)
>        n_counters++;
>
>    /* Build the info and fn_info types.  These are mutually recursive.  */
> -  gcov_info_type = lang_hooks.types.make_type (RECORD_TYPE);
> -  gcov_fn_info_type = lang_hooks.types.make_type (RECORD_TYPE);
> +  record_builder bld_info_type;
> +  gcov_fn_info_type = build_fn_info_type (n_counters,
> bld_info_type.as_tree ());
>    gcov_fn_info_ptr_type = build_pointer_type
>      (build_qualified_type (gcov_fn_info_type, TYPE_QUAL_CONST));
> -  build_fn_info_type (gcov_fn_info_type, n_counters, gcov_info_type);
> -  build_info_type (gcov_info_type, gcov_fn_info_ptr_type);
> +  tree gcov_info_type = build_info_type (bld_info_type, gcov_fn_info_ptr_type);
>
>    /* Build the gcov info var, this is referred to in its own
>       initializer.  */
> Index: gcc/tree-mudflap.c
> ===================================================================
> --- gcc/tree-mudflap.c  (revision 195904)
> +++ gcc/tree-mudflap.c  (working copy)
> @@ -317,23 +317,12 @@ mf_make_builtin (enum tree_code category
>  static inline tree
>  mf_make_mf_cache_struct_type (tree field_type)
>  {
> -  /* There is, abominably, no language-independent way to construct a
> -     RECORD_TYPE.  So we have to call the basic type construction
> -     primitives by hand.  */
> -  tree fieldlo = build_decl (UNKNOWN_LOCATION,
> -                            FIELD_DECL, get_identifier ("low"), field_type);
> -  tree fieldhi = build_decl (UNKNOWN_LOCATION,
> -                            FIELD_DECL, get_identifier ("high"), field_type);
> -
> -  tree struct_type = make_node (RECORD_TYPE);
> -  DECL_CONTEXT (fieldlo) = struct_type;
> -  DECL_CONTEXT (fieldhi) = struct_type;
> -  DECL_CHAIN (fieldlo) = fieldhi;
> -  TYPE_FIELDS (struct_type) = fieldlo;
> -  TYPE_NAME (struct_type) = get_identifier ("__mf_cache");
> -  layout_type (struct_type);
> -
> -  return struct_type;
> +  record_builder rec;
> +  rec.add_field ("low", field_type);
> +  rec.add_field ("high", field_type);
> +  rec.layout ();
> +  rec.tag_name ("__mf_cache");
> +  return rec.as_tree ();
>  }
>
>  /* Initialize the global tree nodes that correspond to mf-runtime.h
> Index: gcc/doc/tm.texi
> ===================================================================
> --- gcc/doc/tm.texi     (revision 195904)
> +++ gcc/doc/tm.texi     (working copy)
> @@ -9966,12 +9966,10 @@ Contains the prefix to be prepended to T
>  default of @code{NULL} uses a target-specific prefix.
>  @end deftypevr
>
> -@deftypefn {Target Hook} tree TARGET_EMUTLS_VAR_FIELDS (tree
> @var{type}, tree *@var{name})
> -Specifies a function that generates the FIELD_DECLs for a TLS control
> -object type.  @var{type} is the RECORD_TYPE the fields are for and
> -@var{name} should be filled with the structure tag, if the default of
> -@code{__emutls_object} is unsuitable.  The default creates a type suitable
> -for libgcc's emulated TLS function.
> +@deftypefn {Target Hook} tree TARGET_EMUTLS_OBJECT_TYPE ()
> +Specifies a function that generates the RECORD_TYPE for a TLS control
> +object type.  The default creates a type, with structure tag
> +@code{__emutls_object}, suitable for libgcc's emulated TLS function.
>  @end deftypefn
>
>  @deftypefn {Target Hook} tree TARGET_EMUTLS_VAR_INIT (tree @var{var},
> tree @var{decl}, tree @var{tmpl_addr})
> Index: gcc/doc/tm.texi.in
> ===================================================================
> --- gcc/doc/tm.texi.in  (revision 195904)
> +++ gcc/doc/tm.texi.in  (working copy)
> @@ -9827,12 +9827,10 @@ Contains the prefix to be prepended to T
>  default of @code{NULL} uses a target-specific prefix.
>  @end deftypevr
>
> -@hook TARGET_EMUTLS_VAR_FIELDS
> -Specifies a function that generates the FIELD_DECLs for a TLS control
> -object type.  @var{type} is the RECORD_TYPE the fields are for and
> -@var{name} should be filled with the structure tag, if the default of
> -@code{__emutls_object} is unsuitable.  The default creates a type suitable
> -for libgcc's emulated TLS function.
> +@hook TARGET_EMUTLS_OBJECT_TYPE
> +Specifies a function that generates the RECORD_TYPE for a TLS control
> +object type.  The default creates a type, with structure tag
> +@code{__emutls_object}, suitable for libgcc's emulated TLS function.
>  @end deftypefn
>
>  @hook TARGET_EMUTLS_VAR_INIT
> Index: gcc/targhooks.h
> ===================================================================
> --- gcc/targhooks.h     (revision 195904)
> +++ gcc/targhooks.h     (working copy)
> @@ -141,7 +141,7 @@ extern void default_target_option_overri
>  extern void hook_void_bitmap (bitmap);
>  extern int default_reloc_rw_mask (void);
>  extern tree default_mangle_decl_assembler_name (tree, tree);
> -extern tree default_emutls_var_fields (tree, tree *);
> +extern tree default_emutls_object_type ();
>  extern tree default_emutls_var_init (tree, tree, tree);
>  extern bool default_hard_regno_scratch_ok (unsigned int);
>  extern bool default_mode_dependent_address_p (const_rtx, addr_space_t);
> Index: gcc/config/vxworks.c
> ===================================================================
> --- gcc/config/vxworks.c        (revision 195904)
> +++ gcc/config/vxworks.c        (working copy)
> @@ -56,35 +56,18 @@ vxworks_asm_out_destructor (rtx symbol,
>    assemble_addr_to_section (symbol, sec);
>  }
>
> -/* Return the list of FIELD_DECLs that make up an emulated TLS
> -   variable's control object.  TYPE is the structure these are fields
> -   of and *NAME will be filled in with the structure tag that should
> -   be used.  */
> +/* Return the type of an emulated TLS variable's control object.  */
>
>  static tree
> -vxworks_emutls_var_fields (tree type, tree *name)
> +vxworks_emutls_object_type ()
>  {
> -  tree field, next_field;
> -
> -  *name = get_identifier ("__tls_var");
> -
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL,
> -                     get_identifier ("size"), unsigned_type_node);
> -  DECL_CONTEXT (field) = type;
> -  next_field = field;
> -
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL,
> -                     get_identifier ("module_id"), unsigned_type_node);
> -  DECL_CONTEXT (field) = type;
> -  DECL_CHAIN (field) = next_field;
> -  next_field = field;
> -
> -  field = build_decl (BUILTINS_LOCATION, FIELD_DECL,
> -                     get_identifier ("offset"), unsigned_type_node);
> -  DECL_CONTEXT (field) = type;
> -  DECL_CHAIN (field) = next_field;
> -
> -  return field;
> +  record_builder rec;
> +  rec.add_field ("offset", unsigned_type_node);
> +  rec.add_field ("module_id", unsigned_type_node);
> +  rec.add_field ("size", unsigned_type_node);
> +  rec.layout ();
> +  rec.decl_name ("__tls_var");
> +  return rec.as_tree ();
>  }
>
>  /* Return the CONSTRUCTOR to initialize an emulated TLS control
> @@ -131,7 +114,7 @@ vxworks_override_options (void)
>    targetm.emutls.tmpl_section = ".tls_data";
>    targetm.emutls.var_prefix = "__tls__";
>    targetm.emutls.tmpl_prefix = "";
> -  targetm.emutls.var_fields = vxworks_emutls_var_fields;
> +  targetm.emutls.object_type = vxworks_emutls_object_type;
>    targetm.emutls.var_init = vxworks_emutls_var_init;
>    targetm.emutls.var_align_fixed = true;
>    targetm.emutls.debug_form_tls_address = true;
> Index: gcc/target.def
> ===================================================================
> --- gcc/target.def      (revision 195904)
> +++ gcc/target.def      (working copy)
> @@ -2741,12 +2741,12 @@ DEFHOOKPOD
>   "",
>   const char *, NULL)
>
> -/* Function to generate field definitions of the proxy variable.  */
> +/* Function to generate type of the proxy variable.  */
>  DEFHOOK
> -(var_fields,
> +(object_type,
>   "",
> - tree, (tree type, tree *name),
> - default_emutls_var_fields)
> + tree, (),
> + default_emutls_object_type)
>
>  /* Function to initialize a proxy variable.  */
>  DEFHOOK
> Index: gcc/tree.c
> ===================================================================
> --- gcc/tree.c  (revision 195904)
> +++ gcc/tree.c  (working copy)
> @@ -11624,4 +11624,95 @@ warn_deprecated_use (tree node, tree att
>      }
>  }
>
> +
> +/* Construct a record builder with the identifier IDENT.
> +   It is a union if IS_UNION is true, otherwise it is a RECORD_TYPE.
> +   QUAL_RECORD_TYPE is not supported.  */
> +
> +record_builder::record_builder (bool is_union)
> +: building_ (lang_hooks.types.make_type (is_union ? UNION_TYPE : RECORD_TYPE)),
> +  last_field_ (NULL)
> +{
> +}
> +
> +
> +/* Add a field with an identifier IDENT and type TYPE to the record.  */
> +
> +void
> +record_builder::add_field (tree ident, tree type, source_location loc)
> +{
> +  tree this_field = build_decl (loc, FIELD_DECL, ident, type);
> +  DECL_CONTEXT (this_field) = building_;
> +  if (last_field_)
> +    DECL_CHAIN (last_field_) = this_field;
> +  else
> +    TYPE_FIELDS (building_) = this_field;
> +  last_field_ = this_field;
> +}
> +
> +void
> +record_builder::add_field (const char *ident, tree type, source_location loc)
> +{
> +  add_field (get_identifier (ident), type, loc);
> +}
> +
> +
> +/* Add a TYPE_NAME to the record.  This can be a tag name directly from IDENT,
> +   or a TYPE_DECL created with the IDENT.  */
> +
> +void
> +record_builder::tag_name (tree ident)
> +{
> +  gcc_assert (TREE_CODE (ident) == IDENTIFIER_NODE);
> +  TYPE_NAME (building_) = ident;
> +}
> +
> +void
> +record_builder::tag_name (const char *ident)
> +{
> +  tag_name (get_identifier (ident));
> +}
> +
> +void
> +record_builder::decl_name (tree ident, source_location loc)
> +{
> +  tree type_decl = build_decl (loc, TYPE_DECL, ident, building_);
> +  TYPE_NAME (building_) = type_decl;
> +}
> +
> +void
> +record_builder::decl_name (const char *ident, source_location loc)
> +{
> +  decl_name (get_identifier (ident), loc);
> +}
> +
> +
> +/* Layout the fields of the record, aligning with ALIGN_TYPE if given.
> +   Ensure that you call one of these functions after adding all fields.  */
> +
> +void
> +record_builder::layout ()
> +{
> +  layout_type (building_);
> +}
> +
> +void
> +record_builder::layout (tree align_type)
> +{
> +  TYPE_ALIGN (building_) = TYPE_ALIGN (align_type);
> +  TYPE_USER_ALIGN (building_) = TYPE_USER_ALIGN (align_type);
> +  layout ();
> +}
> +
> +
> +/* Return the record as a tree.  You may call this function any time after
> +   construction of the builder.  */
> +
> +tree
> +record_builder::as_tree ()
> +{
> +  return building_;
> +}
> +
> +
>  #include "gt-tree.h"
> Index: gcc/tree.h
> ===================================================================
> --- gcc/tree.h  (revision 195904)
> +++ gcc/tree.h  (working copy)
> @@ -6529,4 +6529,28 @@ builtin_decl_implicit_p (enum built_in_f
>           && builtin_info.implicit_p[uns_fncode]);
>  }
>
> +
> +/* A class for simplifying the construction of RECORD_TYPE and UNION_TYPE.  */
> +
> +class record_builder
> +{
> +public:
> +  record_builder (bool is_union = false);
> +  void add_field (tree ident, tree type,
> +                 source_location loc = UNKNOWN_LOCATION);
> +  void add_field (const char *ident, tree type,
> +                 source_location loc = UNKNOWN_LOCATION);
> +  void layout ();
> +  void layout (tree align_type);
> +  void tag_name (tree ident);
> +  void tag_name (const char *ident);
> +  void decl_name (tree ident, source_location loc = UNKNOWN_LOCATION);
> +  void decl_name (const char *ident, source_location loc = UNKNOWN_LOCATION);
> +  tree as_tree ();
> +private:
> +  tree building_;
> +  tree last_field_;
> +}; // class record_builder
> +
> +
>  #endif  /* GCC_TREE_H  */
>
> --
> Lawrence Crowl

On Thu, Feb 14, 2013 at 4:26 AM, Richard Biener
<richard.guenther@gmail.com> wrote:

> Note that tag_name does not allow the way C++ uses this (it can be
> a TYPE_DECL).
>
> Overall I'm not sure this is a good abstraction unless you manage to
> make the frontends use it.

I think that is a mistake.  This is a pure gimple facility.  The fact
that it now looks somewhat similar to what front ends do is a
consequence of the state of the gimple type system.

We very specifically do not want to cater to front ends here.  Why do
you think that should be a feature of this interface?


Diego.

On Thu, Feb 14, 2013 at 12:56 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Thu, Feb 14, 2013 at 4:26 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>
>> Note that tag_name does not allow the way C++ uses this (it can be
>> a TYPE_DECL).
>>
>> Overall I'm not sure this is a good abstraction unless you manage to
>> make the frontends use it.
>
> I think that is a mistake.  This is a pure gimple facility.  The fact
> that it now looks somewhat similar to what front ends do is a
> consequence of the state of the gimple type system.
>
> We very specifically do not want to cater to front ends here.  Why do
> you think that should be a feature of this interface?

Because it's otherwise almost unused.  No "usual" gimple pass builds
up record types.  What's the point in introducing the abstraction if
most of the users cannot use it?

Richard.

>
> Diego.

On Thu, Feb 14, 2013 at 7:52 AM, Richard Biener
<richard.guenther@gmail.com> wrote:

> Because it's otherwise almost unused.  No "usual" gimple pass builds
> up record types.  What's the point in introducing the abstraction if
> most of the users cannot use it?

There may be few users on the gimple side, but you are mixing two
orthogonal issues.  Having a similar facility for FEs may be
desirable, but not *this* one.

Perhaps we could have a parent class provide a more generalized set of
services.  Each front end could use it or derive from it for its own
use.  The gimple version could do the same.  Could that work?


Diego.

On Thu, Feb 14, 2013 at 2:01 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Thu, Feb 14, 2013 at 7:52 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>
>> Because it's otherwise almost unused.  No "usual" gimple pass builds
>> up record types.  What's the point in introducing the abstraction if
>> most of the users cannot use it?
>
> There may be few users on the gimple side, but you are mixing two
> orthogonal issues.  Having a similar facility for FEs may be
> desirable, but not *this* one.
>
> Perhaps we could have a parent class provide a more generalized set of
> services.  Each front end could use it or derive from it for its own
> use.  The gimple version could do the same.  Could that work?

They all share layout_type so they should be able to share the record
builder.

Richard.

>
> Diego.

On Thu, Feb 14, 2013 at 8:06 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Feb 14, 2013 at 2:01 PM, Diego Novillo <dnovillo@google.com> wrote:
>> On Thu, Feb 14, 2013 at 7:52 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>
>>> Because it's otherwise almost unused.  No "usual" gimple pass builds
>>> up record types.  What's the point in introducing the abstraction if
>>> most of the users cannot use it?
>>
>> There may be few users on the gimple side, but you are mixing two
>> orthogonal issues.  Having a similar facility for FEs may be
>> desirable, but not *this* one.
>>
>> Perhaps we could have a parent class provide a more generalized set of
>> services.  Each front end could use it or derive from it for its own
>> use.  The gimple version could do the same.  Could that work?
>
> They all share layout_type so they should be able to share the record
> builder.

That's why I was proposing a hierarchy.  It's true that there is
shared behaviour we want, but I'm sure that there will be services
needed by FEs that are not required in gimple.


Diego.

On 2/14/13, Richard Biener <richard.guenther@gmail.com> wrote:
> On Tue, Feb 12, 2013 at 8:47 PM, Lawrence Crowl <crowl@google.com> wrote:
> > Add class record_builder to ease construction of records and unions.  Use
> > it
> > in some appropriate places.
....
> >  tree
> > -default_emutls_var_fields (tree type, tree *name ATTRIBUTE_UNUSED)
> > +default_emutls_object_type (void)
> >  {
> > -  tree word_type_node, field, next_field;
> > -
> > -  field = build_decl (UNKNOWN_LOCATION,
> > -                     FIELD_DECL, get_identifier ("__templ"), ptr_type_node);
> > -  DECL_CONTEXT (field) = type;
> > -  next_field = field;
> > -
> > -  field = build_decl (UNKNOWN_LOCATION,
> > -                     FIELD_DECL, get_identifier ("__offset"),
> > -                     ptr_type_node);
> > -  DECL_CONTEXT (field) = type;
> > -  DECL_CHAIN (field) = next_field;
> > -  next_field = field;
> > -
> > -  word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
> > -  field = build_decl (UNKNOWN_LOCATION,
> > -                     FIELD_DECL, get_identifier ("__align"),
> > -                     word_type_node);
> > -  DECL_CONTEXT (field) = type;
> > -  DECL_CHAIN (field) = next_field;
> > -  next_field = field;
> > -
> > -  field = build_decl (UNKNOWN_LOCATION,
> > -                     FIELD_DECL, get_identifier ("__size"), word_type_node);
> > -  DECL_CONTEXT (field) = type;
> > -  DECL_CHAIN (field) = next_field;
> > -
> > -  return field;
> > +  tree word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
> > +  record_builder rec;
> > +  rec.add_field ("__size", word_type_node);
> > +  rec.add_field ("__align", word_type_node);
> > +  rec.add_field ("__offset", ptr_type_node);
> > +  rec.add_field ("__templ", ptr_type_node);
> > +  rec.layout ();
>
> That's awkward - you want to hide the fact that layout has
> to happen and that it has to happen "last".  Just make it a
> side-effect of .as_tree ().

Sometimes you want to construct recursive types, and for that you
need .as_tree to execute before layout.  This feature is used in
the patch.

> Note that add_field want's to return the FIELD_DECL created,
> people may want to alter it.

Do you have a use case?  Until we have one, I'm not convinced that
we should widen the interface.

> Note that tag_name does not allow the way C++ uses this (it can
> be a TYPE_DECL).

That is what the .decl_name member function does.

> Overall I'm not sure this is a good abstraction unless you manage
> to make the frontends use it.

The intent is for use by the middle/back ends constructing code.
As such, it should be using middle/back end types, not front-end
types.

On Thu, Feb 14, 2013 at 8:44 PM, Lawrence Crowl <crowl@google.com> wrote:
> On 2/14/13, Richard Biener <richard.guenther@gmail.com> wrote:
>> On Tue, Feb 12, 2013 at 8:47 PM, Lawrence Crowl <crowl@google.com> wrote:
>> > Add class record_builder to ease construction of records and unions.  Use
>> > it
>> > in some appropriate places.
> ....
>> >  tree
>> > -default_emutls_var_fields (tree type, tree *name ATTRIBUTE_UNUSED)
>> > +default_emutls_object_type (void)
>> >  {
>> > -  tree word_type_node, field, next_field;
>> > -
>> > -  field = build_decl (UNKNOWN_LOCATION,
>> > -                     FIELD_DECL, get_identifier ("__templ"), ptr_type_node);
>> > -  DECL_CONTEXT (field) = type;
>> > -  next_field = field;
>> > -
>> > -  field = build_decl (UNKNOWN_LOCATION,
>> > -                     FIELD_DECL, get_identifier ("__offset"),
>> > -                     ptr_type_node);
>> > -  DECL_CONTEXT (field) = type;
>> > -  DECL_CHAIN (field) = next_field;
>> > -  next_field = field;
>> > -
>> > -  word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
>> > -  field = build_decl (UNKNOWN_LOCATION,
>> > -                     FIELD_DECL, get_identifier ("__align"),
>> > -                     word_type_node);
>> > -  DECL_CONTEXT (field) = type;
>> > -  DECL_CHAIN (field) = next_field;
>> > -  next_field = field;
>> > -
>> > -  field = build_decl (UNKNOWN_LOCATION,
>> > -                     FIELD_DECL, get_identifier ("__size"), word_type_node);
>> > -  DECL_CONTEXT (field) = type;
>> > -  DECL_CHAIN (field) = next_field;
>> > -
>> > -  return field;
>> > +  tree word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
>> > +  record_builder rec;
>> > +  rec.add_field ("__size", word_type_node);
>> > +  rec.add_field ("__align", word_type_node);
>> > +  rec.add_field ("__offset", ptr_type_node);
>> > +  rec.add_field ("__templ", ptr_type_node);
>> > +  rec.layout ();
>>
>> That's awkward - you want to hide the fact that layout has
>> to happen and that it has to happen "last".  Just make it a
>> side-effect of .as_tree ().
>
> Sometimes you want to construct recursive types, and for that you
> need .as_tree to execute before layout.  This feature is used in
> the patch.
>
>> Note that add_field want's to return the FIELD_DECL created,
>> people may want to alter it.
>
> Do you have a use case?  Until we have one, I'm not convinced that
> we should widen the interface.
>
>> Note that tag_name does not allow the way C++ uses this (it can
>> be a TYPE_DECL).
>
> That is what the .decl_name member function does.
>
>> Overall I'm not sure this is a good abstraction unless you manage
>> to make the frontends use it.
>
> The intent is for use by the middle/back ends constructing code.
> As such, it should be using middle/back end types, not front-end
> types.

Note that there is no such thing as a "middle-end" or "back-end" type.

Richard.

> --
> Lawrence Crowl

On Fri, Feb 15, 2013 at 3:04 AM, Richard Biener
<richard.guenther@gmail.com> wrote:

> Note that there is no such thing as a "middle-end" or "back-end" type.

but we do conceptually have them.

-- Gaby

On 02/12/13 19:47, Lawrence Crowl wrote:
> Add class record_builder to ease construction of records and unions.  Use it
> in some appropriate places.
>
> Nathan please review the vxworks changes.

> config/vxworks.c
> 	Replace vxworks_emutls_var_fields() with vxworks_emutls_object_type().
> 	Use record_builder within vxworks_emutls_object_type().
>
> Tested on x86_64.  Tested with config-list.mk on vxworks targets.

The vxworks bits look fine, thanks.