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
