This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[C/C++ PATCH] Fix promoted switch condition out of range diagnostics (PR c/89888, take 2)


On Tue, Apr 09, 2019 at 09:06:33AM +0200, Jakub Jelinek wrote:
> Alternatively, I believe we could remove from the patch the in-place
> replacement of CASE_LABEL_EXPRs with LABEL_EXPRs if we want to remove,
> just splay_tree_remove those, because by my reading of
> preprocess_case_label_vec_for_gimple we also ignore the fully out of range
> CASE_LABEL_EXPRs there, shall I tweak the patch and rebootstrap?

Here is a version of the patch that doesn't overwrite those CASE_LABEL_EXPRs
as gimplifier handles those, just removes them from the splay tree.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-04-09  Jakub Jelinek  <jakub@redhat.com>

	PR c/89888
	* c-common.h (c_add_case_label): Remove orig_type and outside_range_p
	arguments.
	(c_do_switch_warnings): Remove outside_range_p argument.
	* c-common.c (check_case_bounds): Removed.
	(c_add_case_label): Remove orig_type and outside_range_p arguments.
	Don't call check_case_bounds.  Fold low_value as well as high_value.
	* c-warn.c (c_do_switch_warnings): Remove outside_range_p argument.
	Check for case labels outside of range of original type here and
	adjust them.
c/
	* c-typeck.c (struct c_switch): Remove outside_range_p member.
	(c_start_case): Don't clear it.
	(do_case): Adjust c_add_case_label caller.
	(c_finish_case): Adjust c_do_switch_warnings caller.
cp/
	* decl.c (struct cp_switch): Remove outside_range_p member.
	(push_switch): Don't clear it.
	(pop_switch): Adjust c_do_switch_warnings caller.
	(finish_case_label): Adjust c_add_case_label caller.
testsuite/
	* c-c++-common/pr89888.c: New test.
	* g++.dg/torture/pr40335.C: Change dg-bogus into dg-warning.
	Don't expect -Wswitch-unreachable warning.

--- gcc/c-family/c-common.h.jj	2019-03-20 12:24:57.320308115 +0100
+++ gcc/c-family/c-common.h	2019-04-08 19:02:28.522104090 +0200
@@ -988,8 +988,7 @@ extern tree boolean_increment (enum tree
 
 extern int case_compare (splay_tree_key, splay_tree_key);
 
-extern tree c_add_case_label (location_t, splay_tree, tree, tree, tree, tree,
-			      bool *);
+extern tree c_add_case_label (location_t, splay_tree, tree, tree, tree);
 extern bool c_switch_covers_all_cases_p (splay_tree, tree);
 
 extern tree build_function_call (location_t, tree, tree);
@@ -1291,8 +1290,7 @@ extern void sizeof_pointer_memaccess_war
 					      bool (*) (tree, tree));
 extern void check_main_parameter_types (tree decl);
 extern void warnings_for_convert_and_check (location_t, tree, tree, tree);
-extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool,
-				  bool);
+extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool);
 extern void warn_for_omitted_condop (location_t, tree);
 extern bool warn_for_restrict (unsigned, tree *, unsigned);
 extern void warn_for_address_or_pointer_of_packed_member (tree, tree);
--- gcc/c-family/c-common.c.jj	2019-03-26 08:52:54.938604825 +0100
+++ gcc/c-family/c-common.c	2019-04-09 01:28:01.199754481 +0200
@@ -314,8 +314,6 @@ const struct fname_var_t fname_vars[] =
 struct visibility_flags visibility_options;
 
 static tree check_case_value (location_t, tree);
-static bool check_case_bounds (location_t, tree, tree, tree *, tree *,
-			       bool *);
 
 
 static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
@@ -2103,86 +2101,6 @@ check_case_value (location_t loc, tree v
   return value;
 }
 
-/* See if the case values LOW and HIGH are in the range of the original
-   type (i.e. before the default conversion to int) of the switch testing
-   expression.
-   TYPE is the promoted type of the testing expression, and ORIG_TYPE is
-   the type before promoting it.  CASE_LOW_P is a pointer to the lower
-   bound of the case label, and CASE_HIGH_P is the upper bound or NULL
-   if the case is not a case range.
-   The caller has to make sure that we are not called with NULL for
-   CASE_LOW_P (i.e. the default case).  OUTSIDE_RANGE_P says whether there
-   was a case value that doesn't fit into the range of the ORIG_TYPE.
-   Returns true if the case label is in range of ORIG_TYPE (saturated or
-   untouched) or false if the label is out of range.  */
-
-static bool
-check_case_bounds (location_t loc, tree type, tree orig_type,
-		   tree *case_low_p, tree *case_high_p,
-		   bool *outside_range_p)
-{
-  tree min_value, max_value;
-  tree case_low = *case_low_p;
-  tree case_high = case_high_p ? *case_high_p : case_low;
-
-  /* If there was a problem with the original type, do nothing.  */
-  if (orig_type == error_mark_node)
-    return true;
-
-  min_value = TYPE_MIN_VALUE (orig_type);
-  max_value = TYPE_MAX_VALUE (orig_type);
-
-  /* We'll really need integer constants here.  */
-  case_low = fold (case_low);
-  case_high = fold (case_high);
-
-  /* Case label is less than minimum for type.  */
-  if (tree_int_cst_compare (case_low, min_value) < 0
-      && tree_int_cst_compare (case_high, min_value) < 0)
-    {
-      warning_at (loc, 0, "case label value is less than minimum value "
-		  "for type");
-      *outside_range_p = true;
-      return false;
-    }
-
-  /* Case value is greater than maximum for type.  */
-  if (tree_int_cst_compare (case_low, max_value) > 0
-      && tree_int_cst_compare (case_high, max_value) > 0)
-    {
-      warning_at (loc, 0, "case label value exceeds maximum value for type");
-      *outside_range_p = true;
-      return false;
-    }
-
-  /* Saturate lower case label value to minimum.  */
-  if (tree_int_cst_compare (case_high, min_value) >= 0
-      && tree_int_cst_compare (case_low, min_value) < 0)
-    {
-      warning_at (loc, 0, "lower value in case label range"
-		  " less than minimum value for type");
-      *outside_range_p = true;
-      case_low = min_value;
-    }
-
-  /* Saturate upper case label value to maximum.  */
-  if (tree_int_cst_compare (case_low, max_value) <= 0
-      && tree_int_cst_compare (case_high, max_value) > 0)
-    {
-      warning_at (loc, 0, "upper value in case label range"
-		  " exceeds maximum value for type");
-      *outside_range_p = true;
-      case_high = max_value;
-    }
-
-  if (*case_low_p != case_low)
-    *case_low_p = convert (type, case_low);
-  if (case_high_p && *case_high_p != case_high)
-    *case_high_p = convert (type, case_high);
-
-  return true;
-}
-
 /* Return an integer type with BITS bits of precision,
    that is unsigned if UNSIGNEDP is nonzero, otherwise signed.  */
 
@@ -4873,13 +4791,12 @@ case_compare (splay_tree_key k1, splay_t
    usual C/C++ syntax, rather than the GNU case range extension.
    CASES is a tree containing all the case ranges processed so far;
    COND is the condition for the switch-statement itself.
-   OUTSIDE_RANGE_P says whether there was a case value that doesn't
-   fit into the range of the ORIG_TYPE.  Returns the CASE_LABEL_EXPR
-   created, or ERROR_MARK_NODE if no CASE_LABEL_EXPR is created.  */
+   Returns the CASE_LABEL_EXPR created, or ERROR_MARK_NODE if no
+   CASE_LABEL_EXPR is created.  */
 
 tree
-c_add_case_label (location_t loc, splay_tree cases, tree cond, tree orig_type,
-		  tree low_value, tree high_value, bool *outside_range_p)
+c_add_case_label (location_t loc, splay_tree cases, tree cond,
+		  tree low_value, tree high_value)
 {
   tree type;
   tree label;
@@ -4913,6 +4830,7 @@ c_add_case_label (location_t loc, splay_
     {
       low_value = check_case_value (loc, low_value);
       low_value = convert_and_check (loc, type, low_value);
+      low_value = fold (low_value);
       if (low_value == error_mark_node)
 	goto error_out;
     }
@@ -4920,6 +4838,7 @@ c_add_case_label (location_t loc, splay_
     {
       high_value = check_case_value (loc, high_value);
       high_value = convert_and_check (loc, type, high_value);
+      high_value = fold (high_value);
       if (high_value == error_mark_node)
 	goto error_out;
     }
@@ -4935,15 +4854,6 @@ c_add_case_label (location_t loc, splay_
 	warning_at (loc, 0, "empty range specified");
     }
 
-  /* See if the case is in range of the type of the original testing
-     expression.  If both low_value and high_value are out of range,
-     don't insert the case label and return NULL_TREE.  */
-  if (low_value
-      && !check_case_bounds (loc, type, orig_type,
-			     &low_value, high_value ? &high_value : NULL,
-			     outside_range_p))
-    return NULL_TREE;
-
   /* Look up the LOW_VALUE in the table of case labels we already
      have.  */
   node = splay_tree_lookup (cases, (splay_tree_key) low_value);
--- gcc/c-family/c-warn.c.jj	2019-04-08 10:11:26.648251537 +0200
+++ gcc/c-family/c-warn.c	2019-04-09 01:30:35.157348989 +0200
@@ -1428,12 +1428,87 @@ match_case_to_enum (splay_tree_node node
 
 void
 c_do_switch_warnings (splay_tree cases, location_t switch_location,
-		      tree type, tree cond, bool bool_cond_p,
-		      bool outside_range_p)
+		      tree type, tree cond, bool bool_cond_p)
 {
   splay_tree_node default_node;
   splay_tree_node node;
   tree chain;
+  bool outside_range_p = false;
+
+  if (type != error_mark_node
+      && type != TREE_TYPE (cond)
+      && INTEGRAL_TYPE_P (type)
+      && INTEGRAL_TYPE_P (TREE_TYPE (cond))
+      && (!tree_int_cst_equal (TYPE_MIN_VALUE (type),
+			       TYPE_MIN_VALUE (TREE_TYPE (cond)))
+	  || !tree_int_cst_equal (TYPE_MAX_VALUE (type),
+				  TYPE_MAX_VALUE (TREE_TYPE (cond)))))
+    {
+      tree min_value = TYPE_MIN_VALUE (type);
+      tree max_value = TYPE_MAX_VALUE (type);
+
+      node = splay_tree_predecessor (cases, (splay_tree_key) min_value);
+      if (node && node->key)
+	{
+	  outside_range_p = true;
+	  /* There is at least one case smaller than TYPE's minimum value.
+	     NODE itself could be still a range overlapping the valid values,
+	     but any predecessors thereof except the default case will be
+	     completely outside of range.  */
+	  if (CASE_HIGH ((tree) node->value)
+	      && tree_int_cst_compare (CASE_HIGH ((tree) node->value),
+				       min_value) >= 0)
+	    {
+	      location_t loc = EXPR_LOCATION ((tree) node->value);
+	      warning_at (loc, 0, "lower value in case label range"
+				  " less than minimum value for type");
+	      CASE_LOW ((tree) node->value) = convert (TREE_TYPE (cond),
+						       min_value);
+	      node->key = (splay_tree_key) CASE_LOW ((tree) node->value);
+	    }
+	  /* All the following ones are completely outside of range.  */
+	  do
+	    {
+	      node = splay_tree_predecessor (cases,
+					     (splay_tree_key) min_value);
+	      if (node == NULL || !node->key)
+		break;
+	      location_t loc = EXPR_LOCATION ((tree) node->value);
+	      warning_at (loc, 0, "case label value is less than minimum "
+				  "value for type");
+	      splay_tree_remove (cases, node->key);
+	    }
+	  while (1);
+	}
+      node = splay_tree_lookup (cases, (splay_tree_key) max_value);
+      if (node == NULL)
+	node = splay_tree_predecessor (cases, (splay_tree_key) max_value);
+      /* Handle a single node that might partially overlap the range.  */
+      if (node
+	  && node->key
+	  && CASE_HIGH ((tree) node->value)
+	  && tree_int_cst_compare (CASE_HIGH ((tree) node->value),
+				   max_value) > 0)
+	{
+	  location_t loc = EXPR_LOCATION ((tree) node->value);
+	  warning_at (loc, 0, "upper value in case label range"
+			      " exceeds maximum value for type");
+	  CASE_HIGH ((tree) node->value)
+	    = convert (TREE_TYPE (cond), max_value);
+	  outside_range_p = true;
+	}
+      /* And any nodes that are completely outside of the range.  */
+      while ((node = splay_tree_successor (cases,
+					   (splay_tree_key) max_value))
+	     != NULL)
+	{
+	  location_t loc = EXPR_LOCATION ((tree) node->value);
+	  warning_at (loc, 0,
+		      "case label value exceeds maximum value for type");
+	  splay_tree_remove (cases, node->key);
+	  outside_range_p = true;
+	}
+    }
 
   if (!warn_switch && !warn_switch_enum && !warn_switch_default
       && !warn_switch_bool)
--- gcc/c/c-typeck.c.jj	2019-02-28 08:17:03.044512874 +0100
+++ gcc/c/c-typeck.c	2019-04-08 19:02:47.371799493 +0200
@@ -10686,10 +10686,6 @@ struct c_switch {
   /* Remember whether the controlling expression had boolean type
      before integer promotions for the sake of -Wswitch-bool.  */
   bool bool_cond_p;
-
-  /* Remember whether there was a case value that is outside the
-     range of the ORIG_TYPE.  */
-  bool outside_range_p;
 };
 
 /* A stack of the currently active switch statements.  The innermost
@@ -10766,7 +10762,6 @@ c_start_case (location_t switch_loc,
   cs->cases = splay_tree_new (case_compare, NULL, NULL);
   cs->bindings = c_get_switch_bindings ();
   cs->bool_cond_p = bool_cond_p;
-  cs->outside_range_p = false;
   cs->next = c_switch_stack;
   c_switch_stack = cs;
 
@@ -10812,9 +10807,7 @@ do_case (location_t loc, tree low_value,
 
   label = c_add_case_label (loc, c_switch_stack->cases,
 			    SWITCH_COND (c_switch_stack->switch_expr),
-			    c_switch_stack->orig_type,
-			    low_value, high_value,
-			    &c_switch_stack->outside_range_p);
+			    low_value, high_value);
   if (label == error_mark_node)
     label = NULL_TREE;
   return label;
@@ -10835,8 +10828,7 @@ c_finish_case (tree body, tree type)
   switch_location = EXPR_LOCATION (cs->switch_expr);
   c_do_switch_warnings (cs->cases, switch_location,
 			type ? type : TREE_TYPE (cs->switch_expr),
-			SWITCH_COND (cs->switch_expr),
-			cs->bool_cond_p, cs->outside_range_p);
+			SWITCH_COND (cs->switch_expr), cs->bool_cond_p);
   if (c_switch_covers_all_cases_p (cs->cases, TREE_TYPE (cs->switch_expr)))
     SWITCH_ALL_CASES_P (cs->switch_expr) = 1;
 
--- gcc/cp/decl.c.jj	2019-04-08 10:11:28.185226285 +0200
+++ gcc/cp/decl.c	2019-04-08 19:03:26.186172295 +0200
@@ -3475,9 +3475,6 @@ struct cp_switch
      label.  We need a tree, rather than simply a hash table, because
      of the GNU case range extension.  */
   splay_tree cases;
-  /* Remember whether there was a case value that is outside the
-     range of the original type of the controlling expression.  */
-  bool outside_range_p;
   /* Remember whether a default: case label has been seen.  */
   bool has_default_p;
   /* Remember whether a BREAK_STMT has been seen in this SWITCH_STMT.  */
@@ -3506,7 +3503,6 @@ push_switch (tree switch_stmt)
   p->next = switch_stack;
   p->switch_stmt = switch_stmt;
   p->cases = splay_tree_new (case_compare, NULL, NULL);
-  p->outside_range_p = false;
   p->has_default_p = false;
   p->break_stmt_seen_p = false;
   p->in_loop_body_p = false;
@@ -3527,8 +3523,7 @@ pop_switch (void)
   if (!processing_template_decl)
     c_do_switch_warnings (cs->cases, switch_location,
 			  SWITCH_STMT_TYPE (cs->switch_stmt),
-			  SWITCH_STMT_COND (cs->switch_stmt),
-			  bool_cond_p, cs->outside_range_p);
+			  SWITCH_STMT_COND (cs->switch_stmt), bool_cond_p);
 
   /* For the benefit of block_may_fallthru remember if the switch body
      case labels cover all possible values and if there are break; stmts.  */
@@ -3643,9 +3638,7 @@ finish_case_label (location_t loc, tree
   low_value = case_conversion (type, low_value);
   high_value = case_conversion (type, high_value);
 
-  r = c_add_case_label (loc, switch_stack->cases, cond, type,
-			low_value, high_value,
-			&switch_stack->outside_range_p);
+  r = c_add_case_label (loc, switch_stack->cases, cond, low_value, high_value);
 
   /* After labels, make any new cleanups in the function go into their
      own new (temporary) binding contour.  */
--- gcc/testsuite/c-c++-common/pr89888.c.jj	2019-04-08 19:54:37.619554189 +0200
+++ gcc/testsuite/c-c++-common/pr89888.c	2019-04-08 20:28:41.390495161 +0200
@@ -0,0 +1,67 @@
+/* PR c/89888 */
+/* { dg-do compile { target { int32 } } } */
+/* { dg-options "" } */
+
+long long y;
+
+void
+foo (unsigned char x)
+{
+  switch (x)
+    {
+    case -1: y = -1; break;			/* { dg-message "previously used here" } */
+						/* { dg-warning "case label value is less than minimum value for type" "" { target *-*-* } .-1 } */
+    case 0xffffffff: y = 0xffffffff; break;	/* { dg-error "duplicate case value" } */
+    case ~0U: y = ~0U; break;			/* { dg-error "duplicate case value" } */
+    }
+}
+
+void
+bar (unsigned char x)
+{
+  switch (x)
+    {
+    case -1: y = -1; break;			/* { dg-message "previously used here" } */
+						/* { dg-warning "case label value is less than minimum value for type" "" { target *-*-* } .-1  } */
+    case -1: y = -1; break;			/* { dg-error "duplicate case value" } */
+    case -1: y = -1; break;			/* { dg-error "duplicate case value" } */
+    }
+}
+
+void
+baz (unsigned char x)
+{
+  switch (x)
+    {
+    case -7: y = -7; break;			/* { dg-warning "case label value is less than minimum value for type" } */
+    case -5 ... 2: y = -5; break;		/* { dg-warning "lower value in case label range less than minimum value for type" } */
+    case 18: y = 18; break;
+    case (unsigned char) -2 ... 4 + (unsigned char) -2: y = 2; break;	/* { dg-warning "upper value in case label range exceeds maximum value for type" } */
+    case 24 + (unsigned char) -2: y = 3; break;	/* { dg-warning "case label value exceeds maximum value for type" } */
+    }
+}
+
+void
+qux (unsigned char x)
+{
+  switch (x)
+    {
+    case (unsigned char) -1 ... 1 + (unsigned char) -1: y = 2; break;	/* { dg-warning "upper value in case label range exceeds maximum value for type" } */
+    case -12: y = -7; break;			/* { dg-warning "case label value is less than minimum value for type" } */
+    case 18: y = 18; break;
+    case 27 + (unsigned char) -1: y = 3; break;	/* { dg-warning "case label value exceeds maximum value for type" } */
+    case -1 ... 0: y = -5; break;		/* { dg-warning "lower value in case label range less than minimum value for type" } */
+    }
+}
+
+void
+quux (unsigned char x)
+{
+  switch (x)
+    {
+    case (unsigned char) -2 ... (unsigned char) -1: y = 2; break;
+    case 18: y = 18; break;
+    case 1 + (unsigned char) -1: y = 3; break;	/* { dg-warning "case label value exceeds maximum value for type" } */
+    case -2 ... -1: y = -5; break;		/* { dg-warning "case label value is less than minimum value for type" } */
+    }
+}
--- gcc/testsuite/g++.dg/torture/pr40335.C.jj	2016-05-24 10:55:59.788180310 +0200
+++ gcc/testsuite/g++.dg/torture/pr40335.C	2019-04-09 01:35:39.138513488 +0200
@@ -7,8 +7,8 @@ main (void)
   int i = -1; 
   switch ((signed char) i)
     {
-      case 255: /* { dg-bogus "exceeds maximum value" "" { xfail *-*-* } } */
-	abort (); /* { dg-warning "statement will never be executed" } */
+      case 255: /* { dg-warning "exceeds maximum value" } */
+	abort ();
       default:
 	break;
     }


	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]