On Sun, Mar 08, 2020 at 01:25:17AM -0500, Jason Merrill wrote: > 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 wish we had a comment to that effect -- then we'd know for sure that it wasn't just an accident. > 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. And given it only triggers on STRICT_ALIGNMENT, no wonder we haven't seen this before. > 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 Yeah, it's tricky stuff :/ > > + /* 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)) Presumably you mean TYPE_USER_ALIGN, but sure. Thanks, Bootstrapped/regtested on x86_64-linux, ok for trunk? -- >8 -- 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; While TYPE_USER_ALIGN of TenuredCell was 0, because finalize_type_size called from finish_record_layout reset it, TYPE_USER_ALIGN of its CLASSTYPE_AS_BASE still remained 1. 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; } where rli->t is BaseShape. Then finalize_record_size won't use the correct rli->record_align and therefore /* Round the size up to be a multiple of the required alignment. */ TYPE_SIZE (rli->t) = round_up (unpadded_size, TYPE_ALIGN (rli->t)); after this we end up with the wrong size. Since the original fix was to avoid creating extra copies for LTO purposes, I think the following fix should be acceptable. PR c++/94050 - ABI issue with alignas on armv7hl. * class.c (layout_class_type): Don't replace a class's CLASSTYPE_AS_BASE if their TYPE_USER_ALIGN don't match. * g++.dg/abi/align3.C: New test. --- gcc/cp/class.c | 4 ++++ gcc/testsuite/g++.dg/abi/align3.C | 12 ++++++++++++ 2 files changed, 16 insertions(+) create mode 100644 gcc/testsuite/g++.dg/abi/align3.C diff --git a/gcc/cp/class.c b/gcc/cp/class.c index b3787f75d7b..5340799fdd3 100644 --- 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. */ + && TYPE_USER_ALIGN (t) == TYPE_USER_ALIGN (CLASSTYPE_AS_BASE (t)) && tree_int_cst_equal (TYPE_SIZE (t), TYPE_SIZE (CLASSTYPE_AS_BASE (t)))) CLASSTYPE_AS_BASE (t) = t; diff --git a/gcc/testsuite/g++.dg/abi/align3.C b/gcc/testsuite/g++.dg/abi/align3.C new file mode 100644 index 00000000000..a56693a34b8 --- /dev/null +++ b/gcc/testsuite/g++.dg/abi/align3.C @@ -0,0 +1,12 @@ +// PR c++/94050 - ABI issue with alignas on armv7hl. +// { dg-do compile { target c++11 } } + +struct alignas(8) Cell {}; +struct TenuredCell : public Cell {}; +struct BaseShape : public TenuredCell { + void *p; + unsigned q, r; + void *s; + __UINTPTR_TYPE__ t; +}; +static_assert (sizeof (BaseShape) % 8 == 0, ""); base-commit: 5e1b4e60c1823115ba7ff0e8c4b35f6058ad9969 -- Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
