On Thu, Jan 28, 2021 at 03:18:55PM -0500, Jason Merrill via Gcc-patches wrote:
> On 1/28/21 10:34 AM, Marek Polacek wrote:
> > A year ago I submitted this patch:
> >
> > ~~
> > Here we trip on the TYPE_USER_ALIGN (t) assert in strip_typedefs: it
> > gets "const d[0]" with TYPE_USER_ALIGN=0 but the result built by
> > build_cplus_array_type is "const char[0]" with TYPE_USER_ALIGN=1.
> >
> > When we strip_typedefs the element of the array "const d", we see it's
> > a typedef_variant_p, so we look at its DECL_ORIGINAL_TYPE, which is
> > char, but we need to add the const qualifier, so we call
> > cp_build_qualified_type -> build_qualified_type
> > where get_qualified_type checks to see if we already have such a type
> > by walking the variants list, which in this case is:
> >
> > char -> c -> const char -> const char -> d -> const d
> >
> > Because check_base_type only checks TYPE_ALIGN and not TYPE_USER_ALIGN,
> > we choose the first const char, which has TYPE_USER_ALIGN set. If the
> > element type of an array has TYPE_USER_ALIGN, the array type gets it too.
> >
> > So we can make check_base_type stricter. I was afraid that it might make
> > us reuse types less often, but measuring showed that we build the same
> > amount of types with and without the patch, while bootstrapping.
> > ~~
> >
> > However, the patch broke a few tests on STRICT_ALIGNMENT platforms and
> > had to be reverted. This is another try. The original patch is kept
> > unchanged, but I added the finalize_type_size hunk that ought to fix the
> > STRICT_ALIGNMENT issues.
> >
> > The problem is that finalize_type_size can clear TYPE_USER_ALIGN on the
> > main variant of a type, but doesn't clear it on any of the variants.
> > Then we end up with types which share the same TYPE_MAIN_VARIANT, but
> > their TYPE_CANONICAL differs and then the usual "canonical types differ
> > for identical types" follows.
> >
> > I've created alignas19.C to exercise this scenario. What happens is:
> > - when parsing the class S we create a type S in xref_tag,
> > - we see alignas(8) so common_handle_aligned_attribute sets T_U_A in S,
> > - we parse the member function fn and build_memfn_type creates a copy
> > of S to add const; this variant has T_U_A set,
> > - we finish_struct S which calls layout_class_type -> finish_record_type
> > -> finalize_size_type where we reset T_U_A in S (but const S keeps it),
> > - finish_non_static_data_member for arr calls maybe_dummy_object with
> > type = S,
> > - maybe_dummy_object calls same_type_ignoring_top_level_qualifiers_p
> > to check if S and TREE_TYPE (current_class_ref), which is const S,
> > are the same,
> > - same_type_ignoring_top_level_qualifiers_p creates cv-unqualified
> > versions of the passed types. Previously we'd use our main variant
> > S when stripping "const S" of const, but since the T_U_A flags don't
> > match (check_base_type), we create a new variant S'. Then we crash in
> > comptypes because S and S' have the same TYPE_MAIN_VARIANT but
> > different TYPE_CANONICALs.
> >
> > With my patch we'll clear T_U_A for S's variants too, and then instead
> > of S' we'll just use S. Does this seem sane?
> >
> > Bootstrapped/regtested on
> > * x86_64-pc-linux-gnu
> > * powerpc64le-unknown-linux-gnu
> > * aarch64-linux-gnu
> > ok for trunk?
> >
> > gcc/ChangeLog:
> >
> > PR c++/94775
> > * stor-layout.c (finalize_type_size): If we reset TYPE_USER_ALIGN in
> > the main variant, maybe reset it in its variants too.
> > * tree.c (check_base_type): Return true only if TYPE_USER_ALIGN match.
> > (check_aligned_type): Check if TYPE_USER_ALIGN match.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c++/94775
> > * g++.dg/cpp0x/alignas19.C: New test.
> > * g++.dg/warn/Warray-bounds15.C: New test.
> > ---
> > gcc/stor-layout.c | 16 +++++++--
> > gcc/testsuite/g++.dg/cpp0x/alignas19.C | 8 +++++
> > gcc/testsuite/g++.dg/warn/Warray-bounds15.C | 40 +++++++++++++++++++++
> > gcc/tree.c | 4 ++-
> > 4 files changed, 64 insertions(+), 4 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/cpp0x/alignas19.C
> > create mode 100644 gcc/testsuite/g++.dg/warn/Warray-bounds15.C
> >
> > diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c
> > index 7d6b1b5ce52..784f131ebb8 100644
> > --- a/gcc/stor-layout.c
> > +++ b/gcc/stor-layout.c
> > @@ -1926,6 +1926,7 @@ finalize_type_size (tree type)
> > However, where strict alignment is not required, avoid
> > over-aligning structures, since most compilers do not do this
> > alignment. */
> > + bool tua_cleared_p = false;
> > if (TYPE_MODE (type) != BLKmode
> > && TYPE_MODE (type) != VOIDmode
> > && (STRICT_ALIGNMENT || !AGGREGATE_TYPE_P (type)))
> > @@ -1937,7 +1938,9 @@ finalize_type_size (tree type)
> > if (mode_align >= TYPE_ALIGN (type))
> > {
> > SET_TYPE_ALIGN (type, mode_align);
> > - TYPE_USER_ALIGN (type) = 0;
> > + /* Remember that we're about to reset this flag. */
> > + tua_cleared_p = TYPE_USER_ALIGN (type);
> > + TYPE_USER_ALIGN (type) = false;
> > }
> > }
> > @@ -1991,14 +1994,21 @@ finalize_type_size (tree type)
> > /* Copy it into all variants. */
> > for (variant = TYPE_MAIN_VARIANT (type);
> > - variant != 0;
> > + variant != NULL_TREE;
> > variant = TYPE_NEXT_VARIANT (variant))
> > {
> > TYPE_SIZE (variant) = size;
> > TYPE_SIZE_UNIT (variant) = size_unit;
> > unsigned valign = align;
> > if (TYPE_USER_ALIGN (variant))
> > - valign = MAX (valign, TYPE_ALIGN (variant));
> > + {
> > + valign = MAX (valign, TYPE_ALIGN (variant));
> > + /* If we reset TYPE_USER_ALIGN on the main variant, we might
> > + need to reset it on the variants too. TYPE_MODE will be set
> > + to MODE in this variant, so we can use that. */
> > + if (tua_cleared_p && GET_MODE_ALIGNMENT (mode) >= valign)
Thanks for taking a look.
> I'm not sure why you want to check both of these conditions. Maybe drop the
> local variable?
I did it so that I don't have to repeat the STRICT_ALIGNMENT / TYPE_MODE checks
above.
> Also this existing code seems more complex than necessary to handle variants
> with different alignment than the primary class, which I think is now
> ill-formed because you can't add attributes to a class after the definition.
Aha, if all the variants' TYPE_MODEs and TYPE_ALIGNs must match, then my new
GET_MODE_ALIGNMENT check is redundant. But I admit I feel too nervous about
dropping it, especially in stage4. And given the !AGGREGATE_TYPE_P check
above, we might reset T_U_A even for non-classes.
> > + TYPE_USER_ALIGN (variant) = false;
>
> > + }
> > else
> > TYPE_USER_ALIGN (variant) = user_align;
> > SET_TYPE_ALIGN (variant, valign);
> > diff --git a/gcc/testsuite/g++.dg/cpp0x/alignas19.C
> > b/gcc/testsuite/g++.dg/cpp0x/alignas19.C
> > new file mode 100644
> > index 00000000000..892125b54df
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp0x/alignas19.C
> > @@ -0,0 +1,8 @@
> > +// PR c++/94775
> > +// { dg-do compile { target c++11 } }
> > +// { dg-additional-options "-mstrict-align" { target { aarch64*-*-*
> > powerpc*-*-linux* powerpc*-*-elf* } } }
> > +
> > +struct alignas(8) S {
> > + S *arr[1];
> > + void fn () const { (void) arr[0]; }
> > +};
> > diff --git a/gcc/testsuite/g++.dg/warn/Warray-bounds15.C
> > b/gcc/testsuite/g++.dg/warn/Warray-bounds15.C
> > new file mode 100644
> > index 00000000000..0a18f637e0e
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/warn/Warray-bounds15.C
> > @@ -0,0 +1,40 @@
> > +// PR c++/94775
> > +// { dg-do compile { target c++14 } }
> > +// { dg-options "-O2 -Warray-bounds" }
> > +
> > +template <typename> using a = int;
> > +template <bool, typename, typename> using b = int;
> > +typedef char d;
> > +template <long> using e = int;
> > +template <int f, int q> struct h { using i = b<q, a<e<f>>, e<f>>; };
> > +template <long f, bool g> using j = typename h<f, g>::i;
> > +long ab, k, aj;
> > +const d l[]{};
> > +class m {
> > +public:
> > + m(int);
> > +};
> > +class n {
> > + void ad() const;
> > + template <class ae> void o(long) const {
> > + using c __attribute__((aligned(1))) = const ae;
> > + }
> > + long p;
> > + template <class, class>
> > + auto s(unsigned long, unsigned long, unsigned long, unsigned long) const;
> > + template <bool = false> auto q(unsigned long, unsigned long) const;
> > +};
> > +template <class, class>
> > +auto n::s(unsigned long, unsigned long, unsigned long, unsigned long t)
> > const {
> > + o<d>(p);
> > + return t;
> > +}
> > +template <bool g> auto n::q(unsigned long p1, unsigned long p2) const {
> > + using r = j<4, false>;
> > + using ai = j<4, g>;
> > + return s<ai, r>(ab, k, p1, p2);
> > +}
> > +void n::ad() const {
> > + long f(l[aj]); // { dg-warning "outside array bounds" }
> > + m(q(8, f));
> > +}
> > diff --git a/gcc/tree.c b/gcc/tree.c
> > index f9d57e6d409..430b76168b2 100644
> > --- a/gcc/tree.c
> > +++ b/gcc/tree.c
> > @@ -6573,7 +6573,8 @@ check_base_type (const_tree cand, const_tree base)
> > TYPE_ATTRIBUTES (base)))
> > return false;
> > /* Check alignment. */
> > - if (TYPE_ALIGN (cand) == TYPE_ALIGN (base))
> > + if (TYPE_ALIGN (cand) == TYPE_ALIGN (base)
> > + && TYPE_USER_ALIGN (cand) == TYPE_USER_ALIGN (base))
> > return true;
> > /* Atomic types increase minimal alignment. We must to do so as well
> > or we get duplicated canonical types. See PR88686. */
> > @@ -6608,6 +6609,7 @@ check_aligned_type (const_tree cand, const_tree base,
> > unsigned int align)
> > && TYPE_CONTEXT (cand) == TYPE_CONTEXT (base)
> > /* Check alignment. */
> > && TYPE_ALIGN (cand) == align
> > + && TYPE_USER_ALIGN (cand) == TYPE_USER_ALIGN (base)
> > && attribute_list_equal (TYPE_ATTRIBUTES (cand),
> > TYPE_ATTRIBUTES (base))
> > && check_lang_type (cand, base));
> >
> > base-commit: 1cdca4261e88f4dc9c3293c6b3c2fff3071ca32b
> >
>
Marek
