[Bug middle-end/39954] [4.5 Regression] Revision 146817 caused unaligned access in gcc.dg/torture/pr26565.c

[rguenther at suse dot de]

------- Comment #22 from rguenther at suse dot de  2009-05-06 21:55 -------
Subject: Re:  [4.5 Regression] Revision 146817 caused
 unaligned access in gcc.dg/torture/pr26565.c

On Wed, 6 May 2009, matz at suse dot de wrote:

> ------- Comment #21 from matz at suse dot de  2009-05-06 21:39 -------
> Subject: Re:  [4.5 Regression] Revision 146817 caused
>  unaligned access in gcc.dg/torture/pr26565.c
> 
> On Wed, 6 May 2009, rguenther at suse dot de wrote:
> 
> > > +  tree ret;
> > > +  if (TYPE_PACKED (t))
> > > +    return t;
> > 
> > Walking the type variants and searching for what we now build would
> > fix the inefficiency.  And of course this function needs a comment ;)
> 
> I anyway am unsure about using variants of types for this.  E.g. some 
> other lookup functions when searching for a qualified type simply walk the 
> list and take the first one with matching qualifiers.  Now if there 
> suddenly are multiple variants with same qualifiers but different 
> alignment in there it might chose an overly aligned type accidentally.  
> Or maybe I'm confused, hmm...  (but yes, that's the obvious fix for the 
> inefficiency).  Can qualified types themself be a new base 
> (TYPE_MAIN_VARIANT) for a new chain?  In that case it would work just 
> fine.

Well, build_variant_type_copy already inserts the variant in the
list ... but yes, existing walkers would need to be changed to
also verify matching alignment to the main variant.  OTOH we
may end up creating regular variants for the different align
variants.  Ugh.

Maybe instead using build_distinct_type_copy on the main
variant and using type_hash_canon for efficiency and setting
TYPE_CANONICAL to the type with the natural alignment is better.
This may pose problems with C frontend internal compatibility
and diagnostics.

> 
> > > +  ret = build_variant_type_copy (t);
> > > +  TYPE_ALIGN (ret) = align * BITS_PER_UNIT;
> > > +  TYPE_USER_ALIGN (ret) = 1;
> > 
> > It seems that only ever place_field looks at this flag.
> 
> TYPE_USER_ALIGN?  By place_field and update_alignment_for_field, and it's 
> copied into DECL_USER_ALIGN (which is used in more places).
> 
> TYPE_USER_ALIGN only ever seems to guard calls to ADJUST_FIELD_ALIGN when 
> PCC_BITFIELD_TYPE_MATTERS or BITFIELD_NBYTES_LIMITED.  But I have no idea 
> why a user specified alignment should not also be affected by 
> ADJUST_FIELD_ALIGN.  It all seems to have to do with the general theme of 
> not overriding user-specified alignment in any way that the compiler 
> normally takes to derive alignments.
> 
> In any case it seems better to leave this alone, stor-layout.c is filled 
> with sometimes quite arcane conditions and special cases and probably 
> nobody in the world can test all combinations of strange ABIs and funny 
> requirements or backward compatibilities.
> 
> > How is the effect of setting it here?
> 
> Well, the user explicitely put "attribute((packed))" there so it seems 
> reasonable to deal with this as if he also specified an explicit 
> alignment.

Ok.

> > Overall I like this patch.
> 
> Much to my surprise it even seems to work up to now, bootstrap was okay 
> testsuite still running.

Probably making the type a variant at least makes the FE happy ;)

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39954