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
