On 3/8/20 5:33 PM, Marek Polacek wrote:
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?
OK, thanks.
-- >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
