Re: [PATCH] c++: Fix ABI issue with alignas on armv7hl [PR94050]

[Jason Merrill <jason at redhat dot com>]

On 3/6/20 8:12 PM, Marek Polacek wrote:
> On Fri, Mar 06, 2020 at 05:49:07PM -0500, Jason Merrill wrote:
> > On 3/5/20 2:40 PM, Marek Polacek wrote:
> > > The static_assert in the following test was failing on armv7hl because
> > > we were disregarding the alignas specifier on Cell.  BaseShape's data
> > > takes up 20B on 32-bit architectures, but we failed to round up its
> > > TYPE_SIZE.  This happens since the
> > > <https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01189.html>
> > > patch: here, in layout_class_type for TenuredCell, we see that the size
> > > of TenuredCell and its CLASSTYPE_AS_BASE match, so we set
> > >
> > >     CLASSTYPE_AS_BASE (t) = t;
> > >
> > > But while TYPE_USER_ALIGN of TenuredCell was 0, TYPE_USER_ALIGN of its
> > > CLASSTYPE_AS_BASE was 1.
> >
> > Surely the bug is that TYPE_USER_ALIGN isn't set for TenuredCell?
>
> That's my understanding.

So why is that?  Where is setting TYPE_USER_ALIGN on as-base 
TenuredCell, and why isn't it setting it on the main type as well?  Or 
is it getting cleared on the main type for some reason?

> I think it's clear that we're losing the alignas
> specifier and so the TYPE_SIZE of BaseShape is wrong.  Before the patch from
> June patch we'd set it...
>
> >    After we replace it, it's no longer 1.  Then
> > > we perform layout_empty_base_or_field for TenuredCell and since
> > > TYPE_USER_ALIGN of its CLASSTYPE_AS_BASE is now 0, we don't do this
> > > adjustment:
> > >
> > >     if (CLASSTYPE_USER_ALIGN (type))
> > >       {
> > >         rli->record_align = MAX (rli->record_align, CLASSTYPE_ALIGN (type));
> > >         if (warn_packed)
> > >           rli->unpacked_align = MAX (rli->unpacked_align, CLASSTYPE_ALIGN (type));
> > >         TYPE_USER_ALIGN (rli->t) = 1;
> > >       }

I would expect it to be set here on TenuredCell prime, and then copied 
to the as-base type with

>       TYPE_USER_ALIGN (base_t) = TYPE_USER_ALIGN (t);

> ...here, but we no longer do.  Is there any other direction I could pursue?
> Well, perhaps...
>
> > > --- a/gcc/cp/class.c
> > > +++ b/gcc/cp/class.c
> > > @@ -6705,6 +6705,10 @@ layout_class_type (tree t, tree *virtuals_p)
> > >      /* If we didn't end up needing an as-base type, don't use it.  */
> > >      if (CLASSTYPE_AS_BASE (t) != t
> > > +      /* If T's CLASSTYPE_AS_BASE is TYPE_USER_ALIGN, but T is not,
> > > +	 replacing the as-base type would change CLASSTYPE_USER_ALIGN,
> > > +	 causing us to lose the user-specified alignment as in PR94050.  */
> > > +      && !CLASSTYPE_USER_ALIGN (t)
> > >          && tree_int_cst_equal (TYPE_SIZE (t),
> > >    			     TYPE_SIZE (CLASSTYPE_AS_BASE (t))))
> > >        CLASSTYPE_AS_BASE (t) = t;
>
> ...we could copy the TYPE_USER_ALIGN flag to t if its old CLASSTYPE_AS_BASE
> had it.  But my fix seems to be safer.

The flag should be set properly regardless of whether we do this 
optimization.

Jason