Fix representation of gcov_info_type

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


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)
>> >      {

> 
> 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

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

> 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
>