C++ PATCH to fix ICE in strip_typedefs with -Wpadded (PR c++/79900)

We crash on an assert in strip_typedefs, because we find ourselves in a
scenario where RESULT, the main variant of a struct, was modified in
finalize_record_size (its TYPE_ALIGN changed), but its variants (T in
strip_typedefs) weren't fixed-up yet; that will happen slightly later in
finalize_type_size.  So there's a discrepancy that confuses the code.

This patch implements what Jason suggested to me on IRC, i.e., skip the
attribute handling if RESULT is complete and T isn't.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-03-08  Marek Polacek  <polacek@redhat.com>

	PR c++/79900 - ICE in strip_typedefs
	* tree.c (strip_typedefs): Skip the attribute handling if T is
	a variant type which hasn't been updated yet.

	* g++.dg/warn/Wpadded-1.C: New test.


	Marek

OK.

On Wed, Mar 8, 2017 at 6:00 PM, Marek Polacek <polacek@redhat.com> wrote:
> We crash on an assert in strip_typedefs, because we find ourselves in a
> scenario where RESULT, the main variant of a struct, was modified in
> finalize_record_size (its TYPE_ALIGN changed), but its variants (T in
> strip_typedefs) weren't fixed-up yet; that will happen slightly later in
> finalize_type_size.  So there's a discrepancy that confuses the code.
>
> This patch implements what Jason suggested to me on IRC, i.e., skip the
> attribute handling if RESULT is complete and T isn't.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2017-03-08  Marek Polacek  <polacek@redhat.com>
>
>         PR c++/79900 - ICE in strip_typedefs
>         * tree.c (strip_typedefs): Skip the attribute handling if T is
>         a variant type which hasn't been updated yet.
>
>         * g++.dg/warn/Wpadded-1.C: New test.
>
> diff --git gcc/cp/tree.c gcc/cp/tree.c
> index d3c63b8..b3a4a10 100644
> --- gcc/cp/tree.c
> +++ gcc/cp/tree.c
> @@ -1548,29 +1548,40 @@ strip_typedefs (tree t, bool *remove_attributes)
>         result = TYPE_MAIN_VARIANT (t);
>      }
>    gcc_assert (!typedef_variant_p (result));
> -  if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
> -      || TYPE_ALIGN (t) != TYPE_ALIGN (result))
> +
> +  if (COMPLETE_TYPE_P (result) && !COMPLETE_TYPE_P (t))
> +  /* If RESULT is complete and T isn't, it's likely the case that T
> +     is a variant of RESULT which hasn't been updated yet.  Skip the
> +     attribute handling.  */;
> +  else
>      {
> -      gcc_assert (TYPE_USER_ALIGN (t));
> -      if (remove_attributes)
> -       *remove_attributes = true;
> -      else
> +      if (TYPE_USER_ALIGN (t) != TYPE_USER_ALIGN (result)
> +         || TYPE_ALIGN (t) != TYPE_ALIGN (result))
>         {
> -         if (TYPE_ALIGN (t) == TYPE_ALIGN (result))
> -           result = build_variant_type_copy (result);
> +         gcc_assert (TYPE_USER_ALIGN (t));
> +         if (remove_attributes)
> +           *remove_attributes = true;
>           else
> -           result = build_aligned_type (result, TYPE_ALIGN (t));
> -         TYPE_USER_ALIGN (result) = true;
> +           {
> +             if (TYPE_ALIGN (t) == TYPE_ALIGN (result))
> +               result = build_variant_type_copy (result);
> +             else
> +               result = build_aligned_type (result, TYPE_ALIGN (t));
> +             TYPE_USER_ALIGN (result) = true;
> +           }
> +       }
> +
> +      if (TYPE_ATTRIBUTES (t))
> +       {
> +         if (remove_attributes)
> +           result = apply_identity_attributes (result, TYPE_ATTRIBUTES (t),
> +                                               remove_attributes);
> +         else
> +           result = cp_build_type_attribute_variant (result,
> +                                                     TYPE_ATTRIBUTES (t));
>         }
>      }
> -  if (TYPE_ATTRIBUTES (t))
> -    {
> -      if (remove_attributes)
> -       result = apply_identity_attributes (result, TYPE_ATTRIBUTES (t),
> -                                           remove_attributes);
> -      else
> -       result = cp_build_type_attribute_variant (result, TYPE_ATTRIBUTES (t));
> -    }
> +
>    return cp_build_qualified_type (result, cp_type_quals (t));
>  }
>
> diff --git gcc/testsuite/g++.dg/warn/Wpadded-1.C gcc/testsuite/g++.dg/warn/Wpadded-1.C
> index e69de29..b3f0581 100644
> --- gcc/testsuite/g++.dg/warn/Wpadded-1.C
> +++ gcc/testsuite/g++.dg/warn/Wpadded-1.C
> @@ -0,0 +1,22 @@
> +// PR c++/79900 - ICE in strip_typedefs
> +// { dg-do compile }
> +// { dg-options "-Wpadded" }
> +
> +template <class> struct A;
> +template <typename> struct B { // { dg-warning "padding struct size to alignment boundary" }
> +  long _M_off;
> +  int _M_state;
> +};
> +template <> struct A<char> { typedef B<int> pos_type; };
> +enum _Ios_Openmode {};
> +struct C {
> +  typedef _Ios_Openmode openmode;
> +};
> +template <typename, typename _Traits> struct D {
> +  typedef typename _Traits::pos_type pos_type;
> +  pos_type m_fn1(pos_type, C::openmode);
> +};
> +template class D<char, A<char> >;
> +template <typename _CharT, typename _Traits>
> +typename D<_CharT, _Traits>::pos_type D<_CharT, _Traits>::m_fn1(pos_type x,
> +                                                                C::openmode) { return x; }
>
>         Marek

On Thu, Mar 09, 2017 at 12:00:48AM +0100, Marek Polacek wrote:
> We crash on an assert in strip_typedefs, because we find ourselves in a
> scenario where RESULT, the main variant of a struct, was modified in
> finalize_record_size (its TYPE_ALIGN changed), but its variants (T in
> strip_typedefs) weren't fixed-up yet; that will happen slightly later in
> finalize_type_size.  So there's a discrepancy that confuses the code.
> 
> This patch implements what Jason suggested to me on IRC, i.e., skip the
> attribute handling if RESULT is complete and T isn't.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2017-03-08  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/79900 - ICE in strip_typedefs
> 	* tree.c (strip_typedefs): Skip the attribute handling if T is
> 	a variant type which hasn't been updated yet.
> 
> 	* g++.dg/warn/Wpadded-1.C: New test.
> 
> diff --git gcc/testsuite/g++.dg/warn/Wpadded-1.C gcc/testsuite/g++.dg/warn/Wpadded-1.C
> index e69de29..b3f0581 100644
> --- gcc/testsuite/g++.dg/warn/Wpadded-1.C
> +++ gcc/testsuite/g++.dg/warn/Wpadded-1.C
> @@ -0,0 +1,22 @@
> +// PR c++/79900 - ICE in strip_typedefs
> +// { dg-do compile }
> +// { dg-options "-Wpadded" }
> +
> +template <class> struct A;
> +template <typename> struct B { // { dg-warning "padding struct size to alignment boundary" }
> +  long _M_off;
> +  int _M_state;
> +};

This fails on i686-linux, there is obviously no padding of struct size to
alignment boundary in that case.
Can't _M_state be e.g. char, then it would work at least on all targets
where long has alignment of at least 2 (that can be all of them)?
Perhaps also make _M_off long long or add a double or long double field.

> +template <> struct A<char> { typedef B<int> pos_type; };
> +enum _Ios_Openmode {};
> +struct C {
> +  typedef _Ios_Openmode openmode;
> +};
> +template <typename, typename _Traits> struct D {
> +  typedef typename _Traits::pos_type pos_type;
> +  pos_type m_fn1(pos_type, C::openmode);
> +};
> +template class D<char, A<char> >;
> +template <typename _CharT, typename _Traits>
> +typename D<_CharT, _Traits>::pos_type D<_CharT, _Traits>::m_fn1(pos_type x,
> +                                                                C::openmode) { return x; }

	Jakub