Fix PR middle-end/81564: ICE in group_case_labels_stmt()

The test case for PR81564 exposes an issue where the case labels for a
switch statement point to blocks that have already been removed by an
earlier call to cleanup_tree_cfg().  In that case, the code in
group_case_labels_stmt() that does:

  base_bb = label_to_block (CASE_LABEL (base_case));

...returns a NULL base_bb and we SEGV later on when we dereference it:

  if (EDGE_COUNT (base_bb->succs) == 0
      ...

The fix here is to just treat case labels that point to blocks that have
already been deleted similarly to case labels that point to the default
case statement, by removing them.

This passed bootstrap and regtesting on powerpc64le-linux with no regressions.
Ok for trunk?

Peter

gcc/
	PR middle-end/81564
	* tree-cfg.c (group_case_labels_stmt): Handle already deleted blocks.

gcc/testsuite/
	PR middle-end/81564
	* gcc.dg/pr81564.c: New test.

On Wed, Jul 26, 2017 at 9:35 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> The test case for PR81564 exposes an issue where the case labels for a
> switch statement point to blocks that have already been removed by an
> earlier call to cleanup_tree_cfg().  In that case, the code in
> group_case_labels_stmt() that does:
>
>   base_bb = label_to_block (CASE_LABEL (base_case));
>
> ...returns a NULL base_bb and we SEGV later on when we dereference it:
>
>   if (EDGE_COUNT (base_bb->succs) == 0
>       ...
>
> The fix here is to just treat case labels that point to blocks that have
> already been deleted similarly to case labels that point to the default
> case statement, by removing them.
>
> This passed bootstrap and regtesting on powerpc64le-linux with no regressions.
> Ok for trunk?

Ok.

Thanks,
Richard.

> Peter
>
> gcc/
>         PR middle-end/81564
>         * tree-cfg.c (group_case_labels_stmt): Handle already deleted blocks.
>
> gcc/testsuite/
>         PR middle-end/81564
>         * gcc.dg/pr81564.c: New test.
>
> Index: gcc/tree-cfg.c
> ===================================================================
> --- gcc/tree-cfg.c      (revision 250581)
> +++ gcc/tree-cfg.c      (working copy)
> @@ -1701,8 +1701,9 @@ group_case_labels_stmt (gswitch *stmt)
>        gcc_assert (base_case);
>        base_bb = label_to_block (CASE_LABEL (base_case));
>
> -      /* Discard cases that have the same destination as the default case.  */
> -      if (base_bb == default_bb)
> +      /* Discard cases that have the same destination as the default case or
> +        whose destination blocks have already been removed as unreachable.  */
> +      if (base_bb == NULL || base_bb == default_bb)
>         {
>           i++;
>           continue;
> Index: gcc/testsuite/gcc.dg/pr81564.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/pr81564.c      (nonexistent)
> +++ gcc/testsuite/gcc.dg/pr81564.c      (working copy)
> @@ -0,0 +1,21 @@
> +/* PR middle-end/81564 ICE in group_case_labels_stmt().  */
> +/* { dg-do compile }  */
> +/* { dg-options "-O2" }  */
> +
> +struct a {
> +    int b;
> +    int c;
> +};
> +
> +void
> +foo (void)
> +{
> +  struct a *e;
> +  switch (e->c)
> +  {
> +    case 7:
> +    case 3:
> +      if (__builtin_expect(!0, 0))
> +       __builtin_unreachable();
> +  }
> +}
>

On 7/27/17 2:48 AM, Richard Biener wrote:
> On Wed, Jul 26, 2017 at 9:35 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
>> The fix here is to just treat case labels that point to blocks that have
>> already been deleted similarly to case labels that point to the default
>> case statement, by removing them.
>>
>> This passed bootstrap and regtesting on powerpc64le-linux with no regressions.
>> Ok for trunk?
> 
> Ok.

Thanks, committed as revision 250628.

Peter

On Wed, Jul 26, 2017 at 9:35 PM, Peter Bergner wrote:
> The test case for PR81564 exposes an issue where the case labels for a
> switch statement point to blocks that have already been removed by an
> earlier call to cleanup_tree_cfg().  In that case, the code in
> group_case_labels_stmt() that does:

How can a basic block be removed (apparently as unreachable) if there
are still case labels leading to it?

Apparently there is enough information to make CASE_LABEL be set to
NULL. Why is the case label not just removed (or redirected to the
default, or ...)?

The patch feels like it's papering over another issue.
group_case_labels is an optional thing to do, basically just a
simplification. The compiler should run even if you never group the
case labels...

Ciao!
Steven

On 7/27/17 12:21 PM, Steven Bosscher wrote:
> On Wed, Jul 26, 2017 at 9:35 PM, Peter Bergner wrote:
>> The test case for PR81564 exposes an issue where the case labels for a
>> switch statement point to blocks that have already been removed by an
>> earlier call to cleanup_tree_cfg().  In that case, the code in
>> group_case_labels_stmt() that does:
> 
> How can a basic block be removed (apparently as unreachable) if there
> are still case labels leading to it?
> 
> Apparently there is enough information to make CASE_LABEL be set to
> NULL. Why is the case label not just removed (or redirected to the
> default, or ...)?

My bad above.  The block is actually deleted during the process of grouping
the labels.  The switch statement we have entering group_case_labels_stmt() is:

  switch (...)
  {
    case 3:
    case 7:
      __builtin_unreachable();
  }

We first handle case "3" and we notice that it leads to an "unreachable" block,
so we delete the edge to that block and then the block itself:

      /* Discard cases that have an unreachable destination block.  */
      if (EDGE_COUNT (base_bb->succs) == 0
          && gimple_seq_unreachable_p (bb_seq (base_bb)))
        {
          edge base_edge = find_edge (gimple_bb (stmt), base_bb);
          if (base_edge != NULL)
            remove_edge_and_dominated_blocks (base_edge);
          i = next_index;
          continue;
        }

Next time through the loop, we handle case "7" which pointed to the same
block as case "3", but since we just deleted the block it points to, that
is why 'base_bb = label_to_block (CASE_LABEL (base_case));' now returns a
NULL basic block.  So the incongruous issue you state is just a temporary
artifact of the process of cleaning up the unreachable blocks.  The NULL
basic block is just a sign that we deleted that case's block in an earlier
loop iteration, so we're correct in just removing it like the patch does.

Sorry for the poor initial description!

Peter