Message ID | 9b47585a-9f43-4568-2880-d077e92c121a@vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Thu, Jun 29, 2017 at 5:01 AM, Peter Bergner <bergner@vnet.ibm.com> wrote: > With the fix to PR51513 and follow on fixes for PR80707, PR80775 and PR80823, > we can now end up with switch statements that contain nothing but a default > case statement. The expand_case() function contains code that assumes we > have at least one non-default case statement, leading to the ICE reported > in the PR81194. > > This patch fixes the bug by expanding switch statements that contain only > a default case statement, as a GOTO to the default case's label. > > This passed bootstrap and regtesting on x86_64-linux with no regressions. > Ok for trunk? Ok. Btw, I'm curious -- which pass ends up with just the default case? IMHO it would be a cfgcleanup task but eventually we fail to run it? That is cleanup_control_expr_graph calls find_taken_edge () and maybe that doesn't handle the case where there's just the default label and thus the value of the switch var doesn't matter ... Thanks, Richard. > Peter > > gcc/ > PR middle-end/81194 > * cfgexpand.c (expand_gimple_stmt_1): Handle switch statements > with only one label. > * stmt.c (expand_case): Assert NCASES is greater than one. > > gcc/testsuite/ > PR middle-end/81194 > * g++.dg/pr81194.C: New test. > > Index: gcc/cfgexpand.c > =================================================================== > --- gcc/cfgexpand.c (revision 249747) > +++ gcc/cfgexpand.c (working copy) > @@ -3566,7 +3566,13 @@ > case GIMPLE_PREDICT: > break; > case GIMPLE_SWITCH: > - expand_case (as_a <gswitch *> (stmt)); > + { > + gswitch *swtch = as_a <gswitch *> (stmt); > + if (gimple_switch_num_labels (swtch) == 1) > + expand_goto (CASE_LABEL (gimple_switch_default_label (swtch))); > + else > + expand_case (swtch); > + } > break; > case GIMPLE_ASM: > expand_asm_stmt (as_a <gasm *> (stmt)); > Index: gcc/stmt.c > =================================================================== > --- gcc/stmt.c (revision 249747) > +++ gcc/stmt.c (working copy) > @@ -1142,8 +1142,11 @@ > /* cleanup_tree_cfg removes all SWITCH_EXPR with their index > expressions being INTEGER_CST. */ > gcc_assert (TREE_CODE (index_expr) != INTEGER_CST); > - > > + /* Optimization of switch statements with only one label has already > + occurred, so we should never see them at this point. */ > + gcc_assert (ncases > 1); > + > do_pending_stack_adjust (); > > /* Find the default case target label. */ > Index: gcc/testsuite/g++.dg/pr81194.C > =================================================================== > --- gcc/testsuite/g++.dg/pr81194.C (nonexistent) > +++ gcc/testsuite/g++.dg/pr81194.C (working copy) > @@ -0,0 +1,60 @@ > +// { dg-do compile } > +// { dg-options "-O2 -std=c++17 -fno-exceptions" } > + > +template <class a> struct b { typedef a *c; }; > +class e {}; > +template <typename a> class d { > +public: > + typedef typename b<a>::c c; > + c begin(); > + c end(); > +}; > +struct f { > + enum { g } h; > +}; > +struct i { > + d<f *> j(); > +}; > +struct l { > + d<i *> k(); > +}; > +class ac; > +class o { > +public: > + o(int *, int *, int *, ac *); > +}; > +class ac { > +public: > + ac(e); > + virtual o *ae(int *, int *, int *, int *); > +}; > +class p { > + void af(f *m) { > + switch (m->h) > + case f::g: > + ag(); > + } > + > +public: > + void n() { > + l ah; > + for (i *ai : ah.k()) > + for (f *m : ai->j()) > + af(m); > + } > + virtual void ag() { __builtin_unreachable(); } > +}; > +template <typename = int> class an : o { > +public: > + an(int *, int *, int *, int *, ac *); > +}; > +class q : ac { > +public: > + q() : ac([]() -> e {}()) {} > + o *ae(int *ap, int *aq, int *ar, int *as) { an(ap, aq, ar, as, this); } > +}; > +template <typename at> > +an<at>::an(int *, int *aq, int *ar, int *as, ac *au) : o(aq, ar, as, au) { > + p().n(); > +} > +void av() { new q; } >
On Thu, Jun 29, 2017 at 10:34 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Jun 29, 2017 at 5:01 AM, Peter Bergner <bergner@vnet.ibm.com> wrote: >> With the fix to PR51513 and follow on fixes for PR80707, PR80775 and PR80823, >> we can now end up with switch statements that contain nothing but a default >> case statement. The expand_case() function contains code that assumes we >> have at least one non-default case statement, leading to the ICE reported >> in the PR81194. >> >> This patch fixes the bug by expanding switch statements that contain only >> a default case statement, as a GOTO to the default case's label. >> >> This passed bootstrap and regtesting on x86_64-linux with no regressions. >> Ok for trunk? > > Ok. > > Btw, I'm curious -- which pass ends up with just the default case? IMHO > it would be a cfgcleanup task but eventually we fail to run it? That is > cleanup_control_expr_graph calls find_taken_edge () and maybe that > doesn't handle the case where there's just the default label and thus > the value of the switch var doesn't matter ... To answer myself the unreachable case vanishes at execute_cleanup_cfg_post_optimizing via group_case_labels. find_taken_edge wouldn't handle this case either. I am testing a patch fixing both - your patch should still go in. Thanks, Richard. > Thanks, > Richard. > >> Peter >> >> gcc/ >> PR middle-end/81194 >> * cfgexpand.c (expand_gimple_stmt_1): Handle switch statements >> with only one label. >> * stmt.c (expand_case): Assert NCASES is greater than one. >> >> gcc/testsuite/ >> PR middle-end/81194 >> * g++.dg/pr81194.C: New test. >> >> Index: gcc/cfgexpand.c >> =================================================================== >> --- gcc/cfgexpand.c (revision 249747) >> +++ gcc/cfgexpand.c (working copy) >> @@ -3566,7 +3566,13 @@ >> case GIMPLE_PREDICT: >> break; >> case GIMPLE_SWITCH: >> - expand_case (as_a <gswitch *> (stmt)); >> + { >> + gswitch *swtch = as_a <gswitch *> (stmt); >> + if (gimple_switch_num_labels (swtch) == 1) >> + expand_goto (CASE_LABEL (gimple_switch_default_label (swtch))); >> + else >> + expand_case (swtch); >> + } >> break; >> case GIMPLE_ASM: >> expand_asm_stmt (as_a <gasm *> (stmt)); >> Index: gcc/stmt.c >> =================================================================== >> --- gcc/stmt.c (revision 249747) >> +++ gcc/stmt.c (working copy) >> @@ -1142,8 +1142,11 @@ >> /* cleanup_tree_cfg removes all SWITCH_EXPR with their index >> expressions being INTEGER_CST. */ >> gcc_assert (TREE_CODE (index_expr) != INTEGER_CST); >> - >> >> + /* Optimization of switch statements with only one label has already >> + occurred, so we should never see them at this point. */ >> + gcc_assert (ncases > 1); >> + >> do_pending_stack_adjust (); >> >> /* Find the default case target label. */ >> Index: gcc/testsuite/g++.dg/pr81194.C >> =================================================================== >> --- gcc/testsuite/g++.dg/pr81194.C (nonexistent) >> +++ gcc/testsuite/g++.dg/pr81194.C (working copy) >> @@ -0,0 +1,60 @@ >> +// { dg-do compile } >> +// { dg-options "-O2 -std=c++17 -fno-exceptions" } >> + >> +template <class a> struct b { typedef a *c; }; >> +class e {}; >> +template <typename a> class d { >> +public: >> + typedef typename b<a>::c c; >> + c begin(); >> + c end(); >> +}; >> +struct f { >> + enum { g } h; >> +}; >> +struct i { >> + d<f *> j(); >> +}; >> +struct l { >> + d<i *> k(); >> +}; >> +class ac; >> +class o { >> +public: >> + o(int *, int *, int *, ac *); >> +}; >> +class ac { >> +public: >> + ac(e); >> + virtual o *ae(int *, int *, int *, int *); >> +}; >> +class p { >> + void af(f *m) { >> + switch (m->h) >> + case f::g: >> + ag(); >> + } >> + >> +public: >> + void n() { >> + l ah; >> + for (i *ai : ah.k()) >> + for (f *m : ai->j()) >> + af(m); >> + } >> + virtual void ag() { __builtin_unreachable(); } >> +}; >> +template <typename = int> class an : o { >> +public: >> + an(int *, int *, int *, int *, ac *); >> +}; >> +class q : ac { >> +public: >> + q() : ac([]() -> e {}()) {} >> + o *ae(int *ap, int *aq, int *ar, int *as) { an(ap, aq, ar, as, this); } >> +}; >> +template <typename at> >> +an<at>::an(int *, int *aq, int *ar, int *as, ac *au) : o(aq, ar, as, au) { >> + p().n(); >> +} >> +void av() { new q; } >>
On 6/29/17 3:51 AM, Richard Biener wrote: > On Thu, Jun 29, 2017 at 10:34 AM, Richard Biener > To answer myself the unreachable case vanishes at > execute_cleanup_cfg_post_optimizing > via group_case_labels. find_taken_edge wouldn't handle this case either. > > I am testing a patch fixing both - your patch should still go in. Ok, committed as revision 249783. Thanks. Peter
Index: gcc/cfgexpand.c =================================================================== --- gcc/cfgexpand.c (revision 249747) +++ gcc/cfgexpand.c (working copy) @@ -3566,7 +3566,13 @@ case GIMPLE_PREDICT: break; case GIMPLE_SWITCH: - expand_case (as_a <gswitch *> (stmt)); + { + gswitch *swtch = as_a <gswitch *> (stmt); + if (gimple_switch_num_labels (swtch) == 1) + expand_goto (CASE_LABEL (gimple_switch_default_label (swtch))); + else + expand_case (swtch); + } break; case GIMPLE_ASM: expand_asm_stmt (as_a <gasm *> (stmt)); Index: gcc/stmt.c =================================================================== --- gcc/stmt.c (revision 249747) +++ gcc/stmt.c (working copy) @@ -1142,8 +1142,11 @@ /* cleanup_tree_cfg removes all SWITCH_EXPR with their index expressions being INTEGER_CST. */ gcc_assert (TREE_CODE (index_expr) != INTEGER_CST); - + /* Optimization of switch statements with only one label has already + occurred, so we should never see them at this point. */ + gcc_assert (ncases > 1); + do_pending_stack_adjust (); /* Find the default case target label. */ Index: gcc/testsuite/g++.dg/pr81194.C =================================================================== --- gcc/testsuite/g++.dg/pr81194.C (nonexistent) +++ gcc/testsuite/g++.dg/pr81194.C (working copy) @@ -0,0 +1,60 @@ +// { dg-do compile } +// { dg-options "-O2 -std=c++17 -fno-exceptions" } + +template <class a> struct b { typedef a *c; }; +class e {}; +template <typename a> class d { +public: + typedef typename b<a>::c c; + c begin(); + c end(); +}; +struct f { + enum { g } h; +}; +struct i { + d<f *> j(); +}; +struct l { + d<i *> k(); +}; +class ac; +class o { +public: + o(int *, int *, int *, ac *); +}; +class ac { +public: + ac(e); + virtual o *ae(int *, int *, int *, int *); +}; +class p { + void af(f *m) { + switch (m->h) + case f::g: + ag(); + } + +public: + void n() { + l ah; + for (i *ai : ah.k()) + for (f *m : ai->j()) + af(m); + } + virtual void ag() { __builtin_unreachable(); } +}; +template <typename = int> class an : o { +public: + an(int *, int *, int *, int *, ac *); +}; +class q : ac { +public: + q() : ac([]() -> e {}()) {} + o *ae(int *ap, int *aq, int *ar, int *as) { an(ap, aq, ar, as, this); } +}; +template <typename at> +an<at>::an(int *, int *aq, int *ar, int *as, ac *au) : o(aq, ar, as, au) { + p().n(); +} +void av() { new q; }