Re: [PATCH] Improve -Wimplicit-fallthrough for nested switches (PR c/79153)

[Richard Biener <rguenther at suse dot de>]

On December 1, 2017 12:16:55 AM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>A nested switch that can fallthrough (either because it has break stmts
>or because it doesn't have default: and doesn't cover all cases) isn't
>unfortunately reported with -Wimplicit-fallthrough, because we first
>gimplify the nested switch and then we just see a label added
>implicitly
>for break; or for the default: case if it was missing and the cases
>don't
>cover all possible values and punt on that.
>
>The following patch fixes it by telling the -Wimplicit-fallthrough
>code about the special labels added for break; or missing default:
>through a new SWITCH_BREAK_LABEL_P flag.  Additionally it makes sure
>that GIMPLE_LABEL is emitted still inside of the switch's body sequence
>and if it is a nested switch, it wraps the GIMPLE_SWITCH with the body
>that ends in a GIMPLE_LABEL with SWITCH_BREAK_LABEL_P in a GIMPLE_BIND
>(without vars or blocks).  With this, the -Wimplicit-fallthrough code
>can recognize this and treat the whole switch that can fall through as
>a single statement for the purpose of the warning.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK. 

Richard. 

>2017-11-30  Jakub Jelinek  <jakub@redhat.com>
>
>	PR c/79153
>	* tree.h (SWITCH_BREAK_LABEL_P): Define.
>	* gimplify.c (collect_fallthrough_labels): Handle GIMPLE_BIND
>	starting with a GIMPLE_SWITCH and ending with GIMPLE_LABEL with
>	SWITCH_BREAK_LABEL_P set on the label.
>	(gimplify_switch_expr): Set SWITCH_BREAK_LABEL_P on the label
>	added for default case if it was missing and not all cases covered.
>	Wrap GIMPLE_SWITCH and the switch_body_seq into a GIMPLE_BIND if
>	switch_body_seq ends with a GIMPLE_LABEL with SWITCH_BREAK_LABEL_P
>	set on the label.
>	* tree-chrec.c (evolution_function_is_univariate_p): Add return true;
>	to avoid -Wimplicit-fallthrough warning.
>	* config/i386/i386.c (ix86_expand_special_args_builtin): Add
>	FALLTHRU comment to avoid -Wimplicit-fallthrough warning.
>c/
>	* c-parser.c: Include tree-iterator.h.
>	(c_parser_switch_statement): Emit LABEL_EXPR for the break label
>	into SWITCH_BODY instead of after it and set SWITCH_BREAK_LABEL_P
>	on it.
>cp/
>	* cp-gimplify.c (genericize_switch_stmt): Emit LABEL_EXPR for the
>	break label into SWITCH_BODY instead of after it and set
>	SWITCH_BREAK_LABEL_P on it.
>	* parser.c (cp_parser_objc_expression): Add FALLTHRU comment to avoid
>	-Wimplicit-fallthrough warning.
>fortran/
>	* match.c (gfc_match): Add FALLTHRU comment to avoid
>	-Wimplicit-fallthrough warning.
>testsuite/
>	* c-c++-common/Wimplicit-fallthrough-7.c: Adjust expected warning
>	line.
>	* c-c++-common/Wimplicit-fallthrough-36.c: New test.
>
>--- gcc/tree.h.jj	2017-11-28 22:21:17.000000000 +0100
>+++ gcc/tree.h	2017-11-30 17:55:35.299197618 +0100
>@@ -766,6 +766,11 @@ extern void omp_clause_range_check_faile
> #define FALLTHROUGH_LABEL_P(NODE) \
>   (LABEL_DECL_CHECK (NODE)->base.private_flag)
> 
>+/* Set on the artificial label created for break; stmt from a switch.
>+   This is used to implement -Wimplicit-fallthrough.  */
>+#define SWITCH_BREAK_LABEL_P(NODE) \
>+  (LABEL_DECL_CHECK (NODE)->base.protected_flag)
>+
> /* Nonzero means this expression is volatile in the C sense:
>    its address should be of type `volatile WHATEVER *'.
>    In other words, the declared item is volatile qualified.
>--- gcc/gimplify.c.jj	2017-11-28 12:11:40.000000000 +0100
>+++ gcc/gimplify.c	2017-11-30 19:04:32.694173903 +0100
>@@ -1897,6 +1897,27 @@ collect_fallthrough_labels (gimple_stmt_
> 
>   do
>     {
>+      if (gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_BIND)
>+	{
>+	  /* Recognize the special GIMPLE_BIND added by gimplify_switch_expr,
>+	     which starts on a GIMPLE_SWITCH and ends with a break label.
>+	     Handle that as a single statement that can fall through.  */
>+	  gbind *bind = as_a <gbind *> (gsi_stmt (*gsi_p));
>+	  gimple *first = gimple_seq_first_stmt (gimple_bind_body (bind));
>+	  gimple *last = gimple_seq_last_stmt (gimple_bind_body (bind));
>+	  if (last
>+	      && gimple_code (first) == GIMPLE_SWITCH
>+	      && gimple_code (last) == GIMPLE_LABEL)
>+	    {
>+	      tree label = gimple_label_label (as_a <glabel *> (last));
>+	      if (SWITCH_BREAK_LABEL_P (label))
>+		{
>+		  prev = bind;
>+		  gsi_next (gsi_p);
>+		  continue;
>+		}
>+	    }
>+	}
>       if (gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_BIND
> 	  || gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_TRY)
> 	{
>@@ -2315,6 +2336,7 @@ gimplify_switch_expr (tree *expr_p, gimp
>       preprocess_case_label_vec_for_gimple (labels, index_type,
> 					    &default_case);
> 
>+      bool add_bind = false;
>       if (!default_case)
> 	{
> 	  glabel *new_default;
>@@ -2322,14 +2344,46 @@ gimplify_switch_expr (tree *expr_p, gimp
> 	  default_case
> 	    = build_case_label (NULL_TREE, NULL_TREE,
> 				create_artificial_label (UNKNOWN_LOCATION));
>+	  if (old_in_switch_expr)
>+	    {
>+	      SWITCH_BREAK_LABEL_P (CASE_LABEL (default_case)) = 1;
>+	      add_bind = true;
>+	    }
> 	  new_default = gimple_build_label (CASE_LABEL (default_case));
> 	  gimplify_seq_add_stmt (&switch_body_seq, new_default);
> 	}
>+      else if (old_in_switch_expr)
>+	{
>+	  gimple *last = gimple_seq_last_stmt (switch_body_seq);
>+	  if (last && gimple_code (last) == GIMPLE_LABEL)
>+	    {
>+	      tree label = gimple_label_label (as_a <glabel *> (last));
>+	      if (SWITCH_BREAK_LABEL_P (label))
>+		add_bind = true;
>+	    }
>+	}
> 
>       switch_stmt = gimple_build_switch (SWITCH_COND (switch_expr),
>-					   default_case, labels);
>-      gimplify_seq_add_stmt (pre_p, switch_stmt);
>-      gimplify_seq_add_seq (pre_p, switch_body_seq);
>+					 default_case, labels);
>+      /* For the benefit of -Wimplicit-fallthrough, if switch_body_seq
>+	 ends with a GIMPLE_LABEL holding SWITCH_BREAK_LABEL_P LABEL_DECL,
>+	 wrap the GIMPLE_SWITCH up to that GIMPLE_LABEL into a GIMPLE_BIND,
>+	 so that we can easily find the start and end of the switch
>+	 statement.  */
>+      if (add_bind)
>+	{
>+	  gimple_seq bind_body = NULL;
>+	  gimplify_seq_add_stmt (&bind_body, switch_stmt);
>+	  gimple_seq_add_seq (&bind_body, switch_body_seq);
>+	  gbind *bind = gimple_build_bind (NULL_TREE, bind_body, NULL_TREE);
>+	  gimple_set_location (bind, EXPR_LOCATION (switch_expr));
>+	  gimplify_seq_add_stmt (pre_p, bind);
>+	}
>+      else
>+	{
>+	  gimplify_seq_add_stmt (pre_p, switch_stmt);
>+	  gimplify_seq_add_seq (pre_p, switch_body_seq);
>+	}
>       labels.release ();
>     }
>   else
>--- gcc/tree-chrec.c.jj	2017-10-11 22:37:51.000000000 +0200
>+++ gcc/tree-chrec.c	2017-11-30 20:39:01.613902149 +0100
>@@ -1161,6 +1161,7 @@ evolution_function_is_univariate_p (cons
> 	    return false;
> 	  break;
> 	}
>+      return true;
> 
>     default:
>       return true;
>--- gcc/config/i386/i386.c.jj	2017-11-30 09:42:46.000000000 +0100
>+++ gcc/config/i386/i386.c	2017-11-30 20:40:02.484133842 +0100
>@@ -34985,6 +34985,7 @@ ix86_expand_special_args_builtin (const
> 	default:
> 	  break;
> 	}
>+      /* FALLTHRU */
>     case V64QI_FTYPE_PCCHAR_V64QI_UDI:
>     case V32QI_FTYPE_PCCHAR_V32QI_USI:
>     case V16QI_FTYPE_PCCHAR_V16QI_UHI:
>--- gcc/c/c-parser.c.jj	2017-11-30 12:13:49.000000000 +0100
>+++ gcc/c/c-parser.c	2017-11-30 18:35:03.735146924 +0100
>@@ -67,6 +67,7 @@ along with GCC; see the file COPYING3.
> #include "run-rtl-passes.h"
> #include "intl.h"
> #include "c-family/name-hint.h"
>+#include "tree-iterator.h"
> 
> /* We need to walk over decls with incomplete struct/union/enum types
>    after parsing the whole translation unit.
>@@ -5846,14 +5847,15 @@ c_parser_switch_statement (c_parser *par
>if (!open_brace_p && c_parser_peek_token (parser)->type !=
>CPP_SEMICOLON)
>warn_for_multistatement_macros (loc_after_labels, next_loc, switch_loc,
> 				    RID_SWITCH);
>-  c_finish_case (body, ce.original_type);
>   if (c_break_label)
>     {
>       location_t here = c_parser_peek_token (parser)->location;
>       tree t = build1 (LABEL_EXPR, void_type_node, c_break_label);
>       SET_EXPR_LOCATION (t, here);
>-      add_stmt (t);
>+      SWITCH_BREAK_LABEL_P (c_break_label) = 1;
>+      append_to_statement_list_force (t, &body);
>     }
>+  c_finish_case (body, ce.original_type);
>   c_break_label = save_break;
>   add_stmt (c_end_compound_stmt (switch_loc, block, flag_isoc99));
>   c_parser_maybe_reclassify_token (parser);
>--- gcc/cp/cp-gimplify.c.jj	2017-11-28 22:23:34.000000000 +0100
>+++ gcc/cp/cp-gimplify.c	2017-11-30 19:26:36.432776337 +0100
>@@ -330,11 +330,13 @@ genericize_switch_stmt (tree *stmt_p, in
>   cp_walk_tree (&type, cp_genericize_r, data, NULL);
>   *walk_subtrees = 0;
> 
>+  if (TREE_USED (break_block))
>+    SWITCH_BREAK_LABEL_P (break_block) = 1;
>+  finish_bc_block (&body, bc_break, break_block);
>   *stmt_p = build2_loc (stmt_locus, SWITCH_EXPR, type, cond, body);
>   SWITCH_ALL_CASES_P (*stmt_p) = SWITCH_STMT_ALL_CASES_P (stmt);
>   gcc_checking_assert (!SWITCH_STMT_NO_BREAK_P (stmt)
> 		       || !TREE_USED (break_block));
>-  finish_bc_block (stmt_p, bc_break, break_block);
> }
> 
> /* Genericize a CONTINUE_STMT node *STMT_P.  */
>--- gcc/cp/parser.c.jj	2017-11-30 09:42:44.000000000 +0100
>+++ gcc/cp/parser.c	2017-11-30 20:40:37.868687216 +0100
>@@ -29003,6 +29003,7 @@ cp_parser_objc_expression (cp_parser* pa
> 	default:
> 	  break;
> 	}
>+      /* FALLTHRU */
>     default:
>       error_at (kwd->location,
> 		"misplaced %<@%D%> Objective-C++ construct",
>--- gcc/fortran/match.c.jj	2017-10-28 09:00:47.000000000 +0200
>+++ gcc/fortran/match.c	2017-11-30 20:41:45.464834013 +0100
>@@ -1240,6 +1240,7 @@ loop:
> 	default:
> 	  gfc_internal_error ("gfc_match(): Bad match code %c", c);
> 	}
>+      /* FALLTHRU */
> 
>     default:
> 
>--- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-7.c.jj	2017-04-28
>22:17:03.000000000 +0200
>+++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-7.c	2017-11-30
>19:14:47.535557117 +0100
>@@ -51,9 +51,9 @@ f (int i)
>     {
>     case 1:
>       {
>-	switch (i + 2)
>+	switch (i + 2) /* { dg-warning "statement may fall through" } */
> 	  case 4:
>-	    bar (1); /* { dg-warning "statement may fall through" } */
>+	    bar (1);
> 	  case 5:
> 	    bar (5);
> 	    return;
>--- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-36.c.jj	2017-11-30
>19:11:32.029979082 +0100
>+++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-36.c	2017-11-30
>19:11:07.000000000 +0100
>@@ -0,0 +1,72 @@
>+/* PR c/79153 */
>+/* { dg-do compile } */
>+/* { dg-options "-Wimplicit-fallthrough" } */
>+
>+int
>+test (int v1, int v2)
>+{
>+  switch (v1)
>+    {
>+    case 3:
>+      switch (v2)	/* { dg-warning "this statement may fall through" }
>*/
>+	{
>+	case 1:
>+	  return 28;
>+	case 2:
>+	  return 38;
>+	case 3:
>+	  return 88;
>+	default:
>+	  break;
>+	}
>+    case 4:		/* { dg-message "here" } */
>+      return 168;
>+    case 5:
>+      switch (v2)	/* { dg-warning "this statement may fall through" }
>*/
>+	{
>+	case 4:
>+	  break;
>+	case 5:
>+	  return 38;
>+	case 6:
>+	  return 88;
>+	}
>+    case 6:		/* { dg-message "here" } */
>+      return 169;
>+    case 7:
>+      switch (v2)	/* { dg-warning "this statement may fall through" }
>*/
>+	{
>+	case 7:
>+	  return 38;
>+	case 8:
>+	  return 88;
>+	}
>+    case 8:		/* { dg-message "here" } */
>+      return 170;
>+    case 9:
>+      switch (v2)	/* { dg-bogus "this statement may fall through" } */
>+	{
>+	case 9:
>+	  return 38;
>+	case 10:
>+	  return 88;
>+	default:
>+	  return 89;
>+	}
>+    case 10:
>+      return 171;
>+    case 11:
>+      switch (v2)	/* { dg-bogus "this statement may fall through" } */
>+	{
>+	case -__INT_MAX__ - 1 ... 31:
>+	  return 38;
>+	case 32:
>+	  return 88;
>+	case 33 ... __INT_MAX__:
>+	  return 89;
>+	}
>+    case 12:
>+      return 172;
>+    }
>+  return -1;
>+}
>
>	Jakub