On Sat, Mar 07, 2020 at 01:21:41AM -0500, Jason Merrill wrote:
> 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?
Yeah, it's getting cleared in finalize_type_size, called from
6703 /* Let the back end lay out the type. */
6704 finish_record_layout (rli, /*free_p=*/true);
because finalize_type_size has
1930 /* Don't override a larger alignment requirement coming from a user
1931 alignment of one of the fields. */
1932 if (mode_align >= TYPE_ALIGN (type))
1933 {
1934 SET_TYPE_ALIGN (type, mode_align);
1935 TYPE_USER_ALIGN (type) = 0;
1936 }
(for aggregates it is only done on STRICT_ALIGNMENT platforms which is why we
won't see this problem on e.g. i686).
Here's the story of TYPE_USER_ALIGN:
- first we set TYPE_USER_ALIGN on Cell in common_handle_aligned_attribute,
as expected
- in layout_class_type for Cell we copy T_U_A to its as-base type:
TYPE_USER_ALIGN (base_t) = TYPE_USER_ALIGN (t);
- then we call finish_record_layout and T_U_A is cleared on Cell, but not its
as-base type. In finalize_type_size mode_align == TYPE_ALIGN (type) == 64.
- so in layout_empty_base_or_field we do this:
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;
}
type is Cell and rli->t is TenuredCell
- then in layout_class_type we copy T_U_A from TenuredCell to its as-base type,
then finish_record_layout clears it from the main type
- then we perform the new optimization by replacing the as-base type, making
T_U_A set to 0
- and so BaseShape's T_U_A is never set.
Does this explain things better?
(Of course we can't simply move the optimization before finish_record_layout
because that would disable it altogether.)
I wonder about changing the >= to > in finalize_type_size... Thoughts?
> > 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);
Correct, but see above.
> > ...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.
That makes sense, yeah.
Marek
