Re: TYPE_BINFO and canonical types at LTO

[Richard Biener <rguenther at suse dot de>]

On Fri, 14 Feb 2014, Jan Hubicka wrote:

> > > This smells bad, since it is given a canonical type that is after the
> > > structural equivalency merging that ignores BINFOs, so it may be completely
> > > different class with completely different bases than the original.  Bases are
> > > structuraly merged, too and may be exchanged for normal fields because
> > > DECL_ARTIFICIAL (that separate bases and fields) does not seem to be part of
> > > the canonical type definition in LTO.
> > 
> > Can you elaborate on that DECL_ARTIFICIAL thing?  That is, what is broken
> > by considering all fields during that merging?
> 
> To make the code work with LTO, one can not merge 
> struct B {struct A a}
> struct B: A {}
> 
> these IMO differ only by DECL_ARTIFICIAL flag on the fields.

"The code" == that BINFO walk?  Is that because we walk a completely
unrelated BINFO chain?  I'd say we should have merged its types
so that difference shouldn't matter.

Hopefully ;)

> > 
> > Note that the BINFO walk below only adds extra aliasing - it should
> > be harmless correctness-wise, no?
> 
> If it is needed for the second case, then it we will produce wrong code 
> with LTO when we merge first and second case toghetr.  If it is not 
> needed, then we are safe but we don't really need the loop then.
> 
> Having the testcase where the BINFO walk is needed for corectness, I can turn
> it into wrong code in LTO by interposing the class by structure.
> 
> I am rebuilding firefox with sanity checking that the second loop never adds anything
> useful. Lets see.
> > 
> > > I wonder if that code is needed after all:
> > >     case QUAL_UNION_TYPE:
> > >       /* Recursively record aliases for the base classes, if there are any.  */
> > >       if (TYPE_BINFO (type))
> > >         { 
> > >           int i;
> > >           tree binfo, base_binfo;
> > > 
> > >           for (binfo = TYPE_BINFO (type), i = 0;
> > >                BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
> > >             record_alias_subset (superset,
> > >                                  get_alias_set (BINFO_TYPE (base_binfo)));
> > >         }
> > >       for (field = TYPE_FIELDS (type); field != 0; field = DECL_CHAIN (field))
> > >         if (TREE_CODE (field) == FIELD_DECL && !DECL_NONADDRESSABLE_P (field))
> > >           record_alias_subset (superset, get_alias_set (TREE_TYPE (field)));
> > >       break;
> > > all bases are also fields of within the type, so the second loop should notice
> > > all the types seen by first loop if I am correct?
> > > So perhaps the loop can be dropped at first place. 
> > 
> > Yeah, I remember seeing that code and thinking the same multiple times.
> > Though I also vaguely remember that removing that loop regressed
> > something.  How is virtual inheritance represented in the fields?
> 
> struct A {int a;};
> struct B: virtual A {};
> struct C: virtual A {};
> struct D: B, C {};
> struct D *d;
> struct B *b;
> int t(void)
> {
>   d->a=1;
>   return b->a;
> }
> 
> I srepresented as:
> 
>  <record_type 0x7ffff6c58000 D addressable needs-constructing type_5 BLK
>     size <integer_cst 0x7ffff6ae98c0 type <integer_type 0x7ffff6ae7150 bitsizetype> constant 192>
>     unit size <integer_cst 0x7ffff6ae9880 type <integer_type 0x7ffff6ae70a8 sizetype> constant 24>
>     align 64 symtab 0 alias set 6 canonical type 0x7ffff6c58000
>     fields <field_decl 0x7ffff6c4ae40 D.2249
>         type <record_type 0x7ffff6c452a0 B addressable needs-constructing type_5 BLK
>             size <integer_cst 0x7ffff6ae9140 constant 128>
>             unit size <integer_cst 0x7ffff6ae9160 constant 16>
>             align 64 symtab 0 alias set 3 canonical type 0x7ffff6c452a0 fields <field_decl 0x7ffff6c4a2f8 _vptr.B> context <translation_unit_decl 0x7ffff6af1170 D.1>
>             full-name "struct B"
>             needs-constructor X() X(constX&) this=(X&) n_parents=1 use_template=0 interface-unknown
>             pointer_to_this <pointer_type 0x7ffff6c58b28> chain <type_decl 0x7ffff6c3ea10 B>>
>         ignored decl_6 BLK file t.C line 4 col 8
>         size <integer_cst 0x7ffff6ae90c0 constant 64>
>         unit size <integer_cst 0x7ffff6ae90e0 constant 8>
>         align 64 offset_align 128
>         offset <integer_cst 0x7ffff6ae9100 constant 0>
>         bit offset <integer_cst 0x7ffff6ae9180 constant 0> context <record_type 0x7ffff6c58000 D>
>         chain <field_decl 0x7ffff6c4aed8 D.2250 type <record_type 0x7ffff6c45b28 C>
>             ignored decl_6 BLK file t.C line 4 col 8 size <integer_cst 0x7ffff6ae90c0 64> unit size <integer_cst 0x7ffff6ae90e0 8>
>             align 64 offset_align 128 offset <integer_cst 0x7ffff6ae9100 0> bit offset <integer_cst 0x7ffff6ae90c0 64> context <record_type 0x7ffff6c58000 D> chain <type_decl 0x7ffff6c590b8 D>>> context <translation_unit_decl 0x7ffff6af1170 D.1>
>     full-name "struct D"
>     needs-constructor X() X(constX&) this=(X&) n_parents=2 use_template=0 interface-unknown
>     pointer_to_this <pointer_type 0x7ffff6c58a80> chain <type_decl 0x7ffff6c59000 D>>
> 
> 
> You my end up with A being a field, too, but that only bypas the recursion.
> 
> > 
> > But I'd be happy if this BINFO user would go away ;)
> 
> Me too, indeed.
> > 
> > (similar in general for the get_alias_set langhook - with LTO we
> > only preserve extra alias-set zero answers from that)
> 
> I see, I was surprised we construct alias sets at LTO time, I believed we
> always stream them in&out.

No, we stream out a flag basically, get_alias_set () == 0.  We don't have
a way to merge the alias set forests.

Richard.

> Honza