Fix representation of gcov_info_type

Fix representation of gcov_info_type

[Jan Hubicka]

Hi,
This is first bug noticed by the type consistency checks I added.

gcov_info_type is a structure that contains function pointer to itself.  While
building it we first build a structure w/o size and fields, then we build a
function type that produces a qualified variant of the structure (not sure why
that legwork is needed). Then we add fields via finish_builtin_struct.
It sets the fields to structure but not its variant and then does layout_type
that actually copies size to all variants. So we end up with TYPE_COMPLETE_P variant
that has size but no fields.  This is quite obviously wrong.

Fixed thus. Bootstrapped, lto-bootstrapped and regtested x86_64-linux, comitted.

	* stor-layout.c (finish_builtin_struct): Copy fields into
	the variants.

Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 212098)
+++ stor-layout.c	(working copy)
@@ -2065,7 +2065,7 @@ void
 finish_builtin_struct (tree type, const char *name, tree fields,
 		       tree align_type)
 {
-  tree tail, next;
+  tree tail, next, variant;
 
   for (tail = NULL_TREE; fields; tail = fields, fields = next)
     {
@@ -2074,6 +2074,10 @@ finish_builtin_struct (tree type, const
       DECL_CHAIN (fields) = tail;
     }
   TYPE_FIELDS (type) = tail;
+  for (variant = TYPE_MAIN_VARIANT (type);
+       variant != 0;
+       variant = TYPE_NEXT_VARIANT (variant))
+    TYPE_FIELDS (variant) = tail;
 
   if (align_type)
     {

Re: Fix representation of gcov_info_type

[Richard Biener]

On Sat, Jun 28, 2014 at 8:22 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> This is first bug noticed by the type consistency checks I added.
>
> gcov_info_type is a structure that contains function pointer to itself.  While
> building it we first build a structure w/o size and fields, then we build a
> function type that produces a qualified variant of the structure (not sure why
> that legwork is needed). Then we add fields via finish_builtin_struct.
> It sets the fields to structure but not its variant and then does layout_type
> that actually copies size to all variants. So we end up with TYPE_COMPLETE_P variant
> that has size but no fields.  This is quite obviously wrong.
>
> Fixed thus. Bootstrapped, lto-bootstrapped and regtested x86_64-linux, comitted.
>
>         * stor-layout.c (finish_builtin_struct): Copy fields into
>         the variants.
>
> Index: stor-layout.c
> ===================================================================
> --- stor-layout.c       (revision 212098)
> +++ stor-layout.c       (working copy)
> @@ -2065,7 +2065,7 @@ void
>  finish_builtin_struct (tree type, const char *name, tree fields,
>                        tree align_type)
>  {
> -  tree tail, next;
> +  tree tail, next, variant;
>
>    for (tail = NULL_TREE; fields; tail = fields, fields = next)
>      {
> @@ -2074,6 +2074,10 @@ finish_builtin_struct (tree type, const
>        DECL_CHAIN (fields) = tail;
>      }
>    TYPE_FIELDS (type) = tail;
> +  for (variant = TYPE_MAIN_VARIANT (type);
> +       variant != 0;
> +       variant = TYPE_NEXT_VARIANT (variant))
> +    TYPE_FIELDS (variant) = tail;

I think that's a bogus place to fix that.  Instead the caller should
use build_variant_type_copy.  Especially that the fixup above
depends on all variants being added before finish_builtin_struct
is called.

Please revert the above.

Thanks,
Richard.

>    if (align_type)
>      {

Re: Fix representation of gcov_info_type

[Jan Hubicka]

> > Index: stor-layout.c
> > ===================================================================
> > --- stor-layout.c       (revision 212098)
> > +++ stor-layout.c       (working copy)
> > @@ -2065,7 +2065,7 @@ void
> >  finish_builtin_struct (tree type, const char *name, tree fields,
> >                        tree align_type)
> >  {
> > -  tree tail, next;
> > +  tree tail, next, variant;
> >
> >    for (tail = NULL_TREE; fields; tail = fields, fields = next)
> >      {
> > @@ -2074,6 +2074,10 @@ finish_builtin_struct (tree type, const
> >        DECL_CHAIN (fields) = tail;
> >      }
> >    TYPE_FIELDS (type) = tail;
> > +  for (variant = TYPE_MAIN_VARIANT (type);
> > +       variant != 0;
> > +       variant = TYPE_NEXT_VARIANT (variant))
> > +    TYPE_FIELDS (variant) = tail;
> 
> I think that's a bogus place to fix that.  Instead the caller should
> use build_variant_type_copy.  Especially that the fixup above
> depends on all variants being added before finish_builtin_struct
> is called.
> 
> Please revert the above.

Sorry, I missed this email at the airport. Will test revert overnight.

I do not understand how you propose to fix it.

The variant is produced before the builtin structure is finished and it thus
have FIELDS and SIZE NULL (as the pointer to struct itself is field of the
struct):

  /* function_info pointer pointer */
  fn_info_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;

  finish_builtin_struct (type, "__gcov_info", fields, NULL_TREE);

which leads to build_variant_type_copy.

Once the struct is finished, we need to update the variants somewhere. Same
loop walking the variants appears in finalize_type_size that is transitively
called by finish_builtin_struct:

  /* Also layout any other variants of the type.  */
  if (TYPE_NEXT_VARIANT (type)
      || type != TYPE_MAIN_VARIANT (type))
    {
      tree variant;
      /* Record layout info of this variant.  */
      tree size = TYPE_SIZE (type);
      tree size_unit = TYPE_SIZE_UNIT (type);
      unsigned int align = TYPE_ALIGN (type);
      unsigned int user_align = TYPE_USER_ALIGN (type);
      enum machine_mode mode = TYPE_MODE (type);

      /* Copy it into all variants.  */
      for (variant = TYPE_MAIN_VARIANT (type);
           variant != 0;
           variant = TYPE_NEXT_VARIANT (variant))
        {
          TYPE_SIZE (variant) = size;
          TYPE_SIZE_UNIT (variant) = size_unit;
          TYPE_ALIGN (variant) = align;
          TYPE_USER_ALIGN (variant) = user_align;
          SET_TYPE_MODE (variant, mode);
        }
    }

Difference to my hunk is that the code works when called on non-main variant,
but I think it makes sense to always finish main variant of builtin structs.
(in fact I do not see why one would finalize size on non-main variants given
that the sizes must match either)

What would be correct fix then?
Honza


> 
> Thanks,
> Richard.
> 
> >    if (align_type)
> >      {

Re: Fix representation of gcov_info_type

[Richard Biener]

On Tue, Jul 8, 2014 at 2:25 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > Index: stor-layout.c
>> > ===================================================================
>> > --- stor-layout.c       (revision 212098)
>> > +++ stor-layout.c       (working copy)
>> > @@ -2065,7 +2065,7 @@ void
>> >  finish_builtin_struct (tree type, const char *name, tree fields,
>> >                        tree align_type)
>> >  {
>> > -  tree tail, next;
>> > +  tree tail, next, variant;
>> >
>> >    for (tail = NULL_TREE; fields; tail = fields, fields = next)
>> >      {
>> > @@ -2074,6 +2074,10 @@ finish_builtin_struct (tree type, const
>> >        DECL_CHAIN (fields) = tail;
>> >      }
>> >    TYPE_FIELDS (type) = tail;
>> > +  for (variant = TYPE_MAIN_VARIANT (type);
>> > +       variant != 0;
>> > +       variant = TYPE_NEXT_VARIANT (variant))
>> > +    TYPE_FIELDS (variant) = tail;
>>
>> I think that's a bogus place to fix that.  Instead the caller should
>> use build_variant_type_copy.  Especially that the fixup above
>> depends on all variants being added before finish_builtin_struct
>> is called.
>>
>> Please revert the above.
>
> Sorry, I missed this email at the airport. Will test revert overnight.
>
> I do not understand how you propose to fix it.
>
> The variant is produced before the builtin structure is finished and it thus
> have FIELDS and SIZE NULL (as the pointer to struct itself is field of the
> struct):
>
>   /* function_info pointer pointer */
>   fn_info_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;
>
>   finish_builtin_struct (type, "__gcov_info", fields, NULL_TREE);
>
> which leads to build_variant_type_copy.
>
> Once the struct is finished, we need to update the variants somewhere. Same
> loop walking the variants appears in finalize_type_size that is transitively
> called by finish_builtin_struct:
>
>   /* Also layout any other variants of the type.  */
>   if (TYPE_NEXT_VARIANT (type)
>       || type != TYPE_MAIN_VARIANT (type))
>     {
>       tree variant;
>       /* Record layout info of this variant.  */
>       tree size = TYPE_SIZE (type);
>       tree size_unit = TYPE_SIZE_UNIT (type);
>       unsigned int align = TYPE_ALIGN (type);
>       unsigned int user_align = TYPE_USER_ALIGN (type);
>       enum machine_mode mode = TYPE_MODE (type);
>
>       /* Copy it into all variants.  */
>       for (variant = TYPE_MAIN_VARIANT (type);
>            variant != 0;
>            variant = TYPE_NEXT_VARIANT (variant))
>         {
>           TYPE_SIZE (variant) = size;
>           TYPE_SIZE_UNIT (variant) = size_unit;
>           TYPE_ALIGN (variant) = align;
>           TYPE_USER_ALIGN (variant) = user_align;
>           SET_TYPE_MODE (variant, mode);
>         }
>     }
>
> Difference to my hunk is that the code works when called on non-main variant,
> but I think it makes sense to always finish main variant of builtin structs.
> (in fact I do not see why one would finalize size on non-main variants given
> that the sizes must match either)

So the issue seems to be:

 gcov_info_type = lang_hooks.types.make_type (RECORD_TYPE);
  gcov_fn_info_type = lang_hooks.types.make_type (RECORD_TYPE);
  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);

that __gcov_info has a member of type const __gcov_info * and that
rather than using the equivalent of

struct __gcov_info;
typedef const __gcov_info *gcov_fn_info_ptr_type;
struct __gcov_info {
...
   gcov_fn_info_ptr_type x;
};

we build the variant of the yet incomplete struct and complete
it later.

Sth like

Index: coverage.c
===================================================================
--- coverage.c  (revision 210965)
+++ coverage.c  (working copy)
@@ -1078,9 +1078,10 @@
   /* 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);
+  build_fn_info_type (gcov_fn_info_type, n_counters, gcov_info_type);
+  gcov_info_type = lang_hooks.types.make_type (RECORD_TYPE);
   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);

   /* Build the gcov info var, this is referred to in its own

should fix it by delaying the variant build for one case after the record
has been completed and retaining the incomplete declaration for
the pointer type.

Richard.

> What would be correct fix then?
> Honza
>
>
>>
>> Thanks,
>> Richard.
>>
>> >    if (align_type)
>> >      {

Re: Fix representation of gcov_info_type

[Jan Hubicka]

> 
> So the issue seems to be:
> 
>  gcov_info_type = lang_hooks.types.make_type (RECORD_TYPE);
>   gcov_fn_info_type = lang_hooks.types.make_type (RECORD_TYPE);
>   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);
> 
> that __gcov_info has a member of type const __gcov_info * and that
> rather than using the equivalent of
> 
> struct __gcov_info;
> typedef const __gcov_info *gcov_fn_info_ptr_type;
> struct __gcov_info {
> ...
>    gcov_fn_info_ptr_type x;
> };
> 
> we build the variant of the yet incomplete struct and complete
> it later.
> 
> Sth like
> 
> Index: coverage.c
> ===================================================================
> --- coverage.c  (revision 210965)
> +++ coverage.c  (working copy)
> @@ -1078,9 +1078,10 @@
>    /* 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);
> +  build_fn_info_type (gcov_fn_info_type, n_counters, gcov_info_type);
> +  gcov_info_type = lang_hooks.types.make_type (RECORD_TYPE);
>    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);
> 
>    /* Build the gcov info var, this is referred to in its own

Hmm, right. I somehow misread it that gcov_info_type variant is built, but I
hope it is not - will double check.  If not, then this should indeed work.
Still do not know how to use current finish_builtin_struct interface for case
where we want to have a structure that contains qualified pointer to itself.

Thanks,
Honza

Re: Fix representation of gcov_info_type

[Richard Biener]

On July 8, 2014 4:03:05 PM CEST, Jan Hubicka <hubicka@ucw.cz> wrote:
>> 
>> So the issue seems to be:
>> 
>>  gcov_info_type = lang_hooks.types.make_type (RECORD_TYPE);
>>   gcov_fn_info_type = lang_hooks.types.make_type (RECORD_TYPE);
>>   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);
>> 
>> that __gcov_info has a member of type const __gcov_info * and that
>> rather than using the equivalent of
>> 
>> struct __gcov_info;
>> typedef const __gcov_info *gcov_fn_info_ptr_type;
>> struct __gcov_info {
>> ...
>>    gcov_fn_info_ptr_type x;
>> };
>> 
>> we build the variant of the yet incomplete struct and complete
>> it later.
>> 
>> Sth like
>> 
>> Index: coverage.c
>> ===================================================================
>> --- coverage.c  (revision 210965)
>> +++ coverage.c  (working copy)
>> @@ -1078,9 +1078,10 @@
>>    /* 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);
>> +  build_fn_info_type (gcov_fn_info_type, n_counters,
>gcov_info_type);
>> +  gcov_info_type = lang_hooks.types.make_type (RECORD_TYPE);
>>    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);
>> 
>>    /* Build the gcov info var, this is referred to in its own
>
>Hmm, right. I somehow misread it that gcov_info_type variant is built,
>but I
>hope it is not - will double check.  If not, then this should indeed
>work.
>Still do not know how to use current finish_builtin_struct interface
>for case
>where we want to have a structure that contains qualified pointer to
>itself.

You probably can't.  But check what the C frontend ends up producing with

Struct x { const struct x *p; };

Doesn't it use an incomplete copy during the definition of x?

Richard.

>Thanks,
>Honza

Re: Fix representation of gcov_info_type

[Jan Hubicka]

> You probably can't.  But check what the C frontend ends up producing with
> 
> Struct x { const struct x *p; };
> 
> Doesn't it use an incomplete copy during the definition of x?

I was poking around this last week with the type identification. C frontend seems
always make the type complete by copying the fields in c-decl.c:finish_struct.
I tried to add check that types are complete when main variant is and learnt that
C++ FE seems in some cases leave the type incomplete even if main variant is complete
but not in such a simple cases.  I will look into it a bit more after arrival - but
that is my recollection. The finish_builtin_struct was only case where we set
DECL_SIZE but not DECL_FIELDS

Honza
> 
> Richard.
> 
> >Thanks,
> >Honza
>