This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: TYPE_BINFO and canonical types at LTO


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

Honza
> 
> Richard.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]