[PATCH] Fix PR middle-end/81194, ICE during RTL pass: expand

[PATCH] Fix PR middle-end/81194, ICE during RTL pass: expand

[Peter Bergner]

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?

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; }

Re: [PATCH] Fix PR middle-end/81194, ICE during RTL pass: expand

[Richard Biener]

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; }
>

Re: [PATCH] Fix PR middle-end/81194, ICE during RTL pass: expand

[Richard Biener]

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; }
>>

Re: [PATCH] Fix PR middle-end/81194, ICE during RTL pass: expand

[Peter Bergner]

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