* C++ PATCH to fix ICE in strip_typedefs with -Wpadded (PR c++/79900)
@ 2017-03-08 23:01 Marek Polacek
2017-03-09 0:52 ` Jason Merrill
2017-03-09 21:06 ` Jakub Jelinek
0 siblings, 2 replies; 5+ messages in thread
From: Marek Polacek @ 2017-03-08 23:01 UTC (permalink / raw)
To: GCC Patches, Jason Merrill
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: C++ PATCH to fix ICE in strip_typedefs with -Wpadded (PR c++/79900)
2017-03-08 23:01 C++ PATCH to fix ICE in strip_typedefs with -Wpadded (PR c++/79900) Marek Polacek
@ 2017-03-09 0:52 ` Jason Merrill
2017-03-09 21:06 ` Jakub Jelinek
1 sibling, 0 replies; 5+ messages in thread
From: Jason Merrill @ 2017-03-09 0:52 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: C++ PATCH to fix ICE in strip_typedefs with -Wpadded (PR c++/79900)
2017-03-08 23:01 C++ PATCH to fix ICE in strip_typedefs with -Wpadded (PR c++/79900) Marek Polacek
2017-03-09 0:52 ` Jason Merrill
@ 2017-03-09 21:06 ` Jakub Jelinek
2017-03-09 22:32 ` Marek Polacek
1 sibling, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2017-03-09 21:06 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Jason Merrill
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: C++ PATCH to fix ICE in strip_typedefs with -Wpadded (PR c++/79900)
2017-03-09 21:06 ` Jakub Jelinek
@ 2017-03-09 22:32 ` Marek Polacek
2017-03-09 22:35 ` Jason Merrill
0 siblings, 1 reply; 5+ messages in thread
From: Marek Polacek @ 2017-03-09 22:32 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: GCC Patches, Jason Merrill
On Thu, Mar 09, 2017 at 10:06:53PM +0100, Jakub Jelinek wrote:
> 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.
Ah, of course...
> 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.
Like this? Passes with -m32.
2017-03-09 Marek Polacek <polacek@redhat.com>
* g++.dg/warn/Wpadded-1.C: Change types of struct B fields.
diff --git gcc/testsuite/g++.dg/warn/Wpadded-1.C gcc/testsuite/g++.dg/warn/Wpadded-1.C
index b3f0581..af375a4 100644
--- gcc/testsuite/g++.dg/warn/Wpadded-1.C
+++ gcc/testsuite/g++.dg/warn/Wpadded-1.C
@@ -4,8 +4,8 @@
template <class> struct A;
template <typename> struct B { // { dg-warning "padding struct size to alignment boundary" }
- long _M_off;
- int _M_state;
+ long long _M_off;
+ char _M_state;
};
template <> struct A<char> { typedef B<int> pos_type; };
enum _Ios_Openmode {};
Marek
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: C++ PATCH to fix ICE in strip_typedefs with -Wpadded (PR c++/79900)
2017-03-09 22:32 ` Marek Polacek
@ 2017-03-09 22:35 ` Jason Merrill
0 siblings, 0 replies; 5+ messages in thread
From: Jason Merrill @ 2017-03-09 22:35 UTC (permalink / raw)
To: Marek Polacek; +Cc: Jakub Jelinek, GCC Patches
Yep, I just checked in that change.
On Thu, Mar 9, 2017 at 5:32 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Thu, Mar 09, 2017 at 10:06:53PM +0100, Jakub Jelinek wrote:
>> 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.
>
> Ah, of course...
>
>> 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.
>
> Like this? Passes with -m32.
>
> 2017-03-09 Marek Polacek <polacek@redhat.com>
>
> * g++.dg/warn/Wpadded-1.C: Change types of struct B fields.
>
> diff --git gcc/testsuite/g++.dg/warn/Wpadded-1.C gcc/testsuite/g++.dg/warn/Wpadded-1.C
> index b3f0581..af375a4 100644
> --- gcc/testsuite/g++.dg/warn/Wpadded-1.C
> +++ gcc/testsuite/g++.dg/warn/Wpadded-1.C
> @@ -4,8 +4,8 @@
>
> template <class> struct A;
> template <typename> struct B { // { dg-warning "padding struct size to alignment boundary" }
> - long _M_off;
> - int _M_state;
> + long long _M_off;
> + char _M_state;
> };
> template <> struct A<char> { typedef B<int> pos_type; };
> enum _Ios_Openmode {};
>
> Marek
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-09 22:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-08 23:01 C++ PATCH to fix ICE in strip_typedefs with -Wpadded (PR c++/79900) Marek Polacek
2017-03-09 0:52 ` Jason Merrill
2017-03-09 21:06 ` Jakub Jelinek
2017-03-09 22:32 ` Marek Polacek
2017-03-09 22:35 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).