| Message ID | 20150327143810.GC25731@redhat.com |
|---|---|
| State | New |
| Headers | show |
OK. Jason
On Fri, Mar 27, 2015 at 7:38 AM, Marek Polacek <polacek@redhat.com> wrote: > In this testcase we were crashing while trying to gimplify a switch, because > the types of the switch condition and case constants didn't match. This ICE > started with my -Wswitch-with-enum-bit-fields fix where I used the unlowered > type so that we're able to get hold of the enum type. The problem with that > is with ordinary bit-fields: we'll get the underlying type (e.g. long int), > but subsequent perform_integral_promotions promotes that to int, see > cp_perform_integral_promotions. Fixed by using the type of the condition in > case we're not dealing with an enum bit-field, i.e. do what we've been doing > before the -Wswitch fix, which ought to make this fix very safe. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2015-03-27 Marek Polacek <polacek@redhat.com> > > PR c++/65556 > * semantics.c (finish_switch_cond): If the unlowered type is not an > enum, use the type of the condition. > > * c-c++-common/pr65556.c: New test. > The new test is failing for me on x86-32 and x32: https://gcc.gnu.org/ml/gcc-regression/2015-03/msg00392.html
On Fri, Mar 27, 2015 at 7:38 AM, Marek Polacek <polacek@redhat.com> wrote: > In this testcase we were crashing while trying to gimplify a switch, because > the types of the switch condition and case constants didn't match. This ICE > started with my -Wswitch-with-enum-bit-fields fix where I used the unlowered > type so that we're able to get hold of the enum type. The problem with that > is with ordinary bit-fields: we'll get the underlying type (e.g. long int), > but subsequent perform_integral_promotions promotes that to int, see > cp_perform_integral_promotions. Fixed by using the type of the condition in > case we're not dealing with an enum bit-field, i.e. do what we've been doing > before the -Wswitch fix, which ought to make this fix very safe. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2015-03-27 Marek Polacek <polacek@redhat.com> > > PR c++/65556 > * semantics.c (finish_switch_cond): If the unlowered type is not an > enum, use the type of the condition. > > * c-c++-common/pr65556.c: New test. > > diff --git gcc/cp/semantics.c gcc/cp/semantics.c > index f325e41..74af7e8 100644 > --- gcc/cp/semantics.c > +++ gcc/cp/semantics.c > @@ -1165,6 +1165,8 @@ finish_switch_cond (tree cond, tree switch_stmt) > } > /* We want unlowered type here to handle enum bit-fields. */ > orig_type = unlowered_expr_type (cond); > + if (TREE_CODE (orig_type) != ENUMERAL_TYPE) > + orig_type = TREE_TYPE (cond); > if (cond != error_mark_node) > { > /* Warn if the condition has boolean value. */ > diff --git gcc/testsuite/c-c++-common/pr65556.c gcc/testsuite/c-c++-common/pr65556.c > index e69de29..c6729a1 100644 > --- gcc/testsuite/c-c++-common/pr65556.c > +++ gcc/testsuite/c-c++-common/pr65556.c > @@ -0,0 +1,23 @@ > +/* PR c++/65556 */ > +/* { dg-do compile } */ > + > +struct S > +{ > + long l: 1; > + long l2: 41; > + unsigned long ul: 1; > + unsigned long ul2: 41; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This won't work with 32-bit long.
On Fri, Mar 27, 2015 at 11:14:31PM +0000, H.J. Lu wrote: > On Fri, Mar 27, 2015 at 7:38 AM, Marek Polacek <polacek@redhat.com> wrote: > > In this testcase we were crashing while trying to gimplify a switch, because > > the types of the switch condition and case constants didn't match. This ICE > > started with my -Wswitch-with-enum-bit-fields fix where I used the unlowered > > type so that we're able to get hold of the enum type. The problem with that > > is with ordinary bit-fields: we'll get the underlying type (e.g. long int), > > but subsequent perform_integral_promotions promotes that to int, see > > cp_perform_integral_promotions. Fixed by using the type of the condition in > > case we're not dealing with an enum bit-field, i.e. do what we've been doing > > before the -Wswitch fix, which ought to make this fix very safe. > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > > > 2015-03-27 Marek Polacek <polacek@redhat.com> > > > > PR c++/65556 > > * semantics.c (finish_switch_cond): If the unlowered type is not an > > enum, use the type of the condition. > > > > * c-c++-common/pr65556.c: New test. > > > > The new test is failing for me on x86-32 and x32: > > https://gcc.gnu.org/ml/gcc-regression/2015-03/msg00392.html Likewise on ARM. The testcase is fairly obviously not going to work for any target with 32-bit long: struct S { long l: 1; long l2: 41; unsigned long ul: 1; unsigned long ul2: 41; } s; .../c-c++-common/pr65556.c:9:3: error: width of 'ul2' exceeds its type Thanks, James
On Mon, Mar 30, 2015 at 03:00:54PM +0100, James Greenhalgh wrote: > Likewise on ARM. The testcase is fairly obviously not going to work > for any target with 32-bit long: > > struct S > { > long l: 1; > long l2: 41; > unsigned long ul: 1; > unsigned long ul2: 41; > } s; > > .../c-c++-common/pr65556.c:9:3: error: width of 'ul2' exceeds its type Patch to change s/long/long long/ preapproved, if it misbehaves the same with Marek's patch reverted. Jakub
On Mon, Mar 30, 2015 at 04:02:36PM +0200, Jakub Jelinek wrote: > On Mon, Mar 30, 2015 at 03:00:54PM +0100, James Greenhalgh wrote: > > Likewise on ARM. The testcase is fairly obviously not going to work > > for any target with 32-bit long: > > > > struct S > > { > > long l: 1; > > long l2: 41; > > unsigned long ul: 1; > > unsigned long ul2: 41; > > } s; > > > > .../c-c++-common/pr65556.c:9:3: error: width of 'ul2' exceeds its type > > Patch to change s/long/long long/ preapproved, if it misbehaves the same > with Marek's patch reverted. Or I could just remove the :41 bit-fields from the testcase altogether, they weren't needed to trigger the bug anyway. Marek
On Mon, Mar 30, 2015 at 04:06:56PM +0200, Marek Polacek wrote: > On Mon, Mar 30, 2015 at 04:02:36PM +0200, Jakub Jelinek wrote: > > On Mon, Mar 30, 2015 at 03:00:54PM +0100, James Greenhalgh wrote: > > > Likewise on ARM. The testcase is fairly obviously not going to work > > > for any target with 32-bit long: > > > > > > struct S > > > { > > > long l: 1; > > > long l2: 41; > > > unsigned long ul: 1; > > > unsigned long ul2: 41; > > > } s; > > > > > > .../c-c++-common/pr65556.c:9:3: error: width of 'ul2' exceeds its type > > > > Patch to change s/long/long long/ preapproved, if it misbehaves the same > > with Marek's patch reverted. > > Or I could just remove the :41 bit-fields from the testcase altogether, they > weren't needed to trigger the bug anyway. Works for me too. Or turn them into :21 or similar (long must be at least 32-bit). Jakub
diff --git gcc/cp/semantics.c gcc/cp/semantics.c index f325e41..74af7e8 100644 --- gcc/cp/semantics.c +++ gcc/cp/semantics.c @@ -1165,6 +1165,8 @@ finish_switch_cond (tree cond, tree switch_stmt) } /* We want unlowered type here to handle enum bit-fields. */ orig_type = unlowered_expr_type (cond); + if (TREE_CODE (orig_type) != ENUMERAL_TYPE) + orig_type = TREE_TYPE (cond); if (cond != error_mark_node) { /* Warn if the condition has boolean value. */ diff --git gcc/testsuite/c-c++-common/pr65556.c gcc/testsuite/c-c++-common/pr65556.c index e69de29..c6729a1 100644 --- gcc/testsuite/c-c++-common/pr65556.c +++ gcc/testsuite/c-c++-common/pr65556.c @@ -0,0 +1,23 @@ +/* PR c++/65556 */ +/* { dg-do compile } */ + +struct S +{ + long l: 1; + long l2: 41; + unsigned long ul: 1; + unsigned long ul2: 41; +} s; + +void +fn () +{ + switch (s.l) + case 0:; + switch (s.ul) + case 0:; + switch (s.l2) + case 0:; + switch (s.ul2) + case 0:; +}