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

[Jason Merrill]

On 3/7/20 6:02 PM, Marek Polacek wrote:
> 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?

Yes, thanks.  So the problem is the interaction of finalize_type_size 
deciding that we don't need TYPE_USER_ALIGN if it's the same alignment 
we would get anyway, and layout_empty_base_or_field only adjusting the 
alignment if TYPE_USER_ALIGN is set, which happened to work before 
because CLASSTYPE_USER_ALIGN was accidentally(?) still set.

I think the right behavior is probably to update rli->*_align regardless 
of CLASSTYPE_USER_ALIGN (type); if an empty base doesn't have 
user-specified aligment, its alignment will be 1, so it shouldn't ever 
be harmful.  When I added that code I wasn't aware of the 
finalize_type_size behavior.

But I'm not confident that won't be an ABI break itself, so let's go 
ahead with your approach for GCC 10, except

> +      /* 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)

How about

&& CLASSTYPE_USER_ALIGN (t) == CLASSTYPE_USER_ALIGN (CLASSTYPE_AS_BASE (t))

?
Jason