diff mbox

[C++] Fix constexpr switch handling (PR c++/77467, take 2)

Message ID 20160920161226.GW7282@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Sept. 20, 2016, 4:12 p.m. UTC 
On Mon, Sep 19, 2016 at 02:34:17PM -0400, Jason Merrill wrote:
> > For STATEMENT_LIST that is called by cxx_eval_constant_expression,
> > for BIND_EXPR if we are lucky enough that BIND_EXPR_BODY is a STATEMENT_LIST
> > too (otherwise I assume even my patch doesn't fix it, it would need to
> > verify that).  If body is some other statement, then it really should be
> > skipped, but it isn't, because cxx_eval_constant_expression ignores it.
> 
> > I wonder if we e.g. cxx_eval_constant_expression couldn't early in the
> > function for if (*jump_target) return immediately unless code is something
> > like STATEMENT_LIST or BIND_EXPR with BIND_EXPR_BODY being STATEMENT_LIST,
> > or perhaps in the future other construct containing other stmts.
> 
> We might assert !jump_target before the call to
> cxx_eval_store_expression, to make sure we don't accidentally evaluate
> one when we're trying to jump.

I've done that.

> > I've beeing thinking about TRY block, but at least on the testcases I've
> > tried it has been rejected in constexpr functions, I think one can't branch
> > into statement expressions, so that should be fine, OpenMP/OpenACC
> > constructs are hopefully also rejected in constexpr, what else?
> 
> LOOP_EXPR, COND_EXPR?

Looking at those and adding further testcases revealed lots of other issues,
which the following patch attempts to deal with.

One is that handling the CASE_LABEL_EXPR or LABEL_EXPR only inside of
cxx_eval_statement_list doesn't really work well, because as the testcase
shows, for labeled null statements inside say if then block or else block
there is no STATEMENT_LIST wrapping it up.

Another is that the default: label can be arbitrarily nested (in inner
STATEMENT_LIST, or LOOP_EXPR body, or COND_EXPR then or else block, so
trying to handle it in cxx_eval_statement_list also works only in the
simplest switch case, we really need to process (with initial skipping mode)
the whole SWITCH_EXPR body and if we get through everything and (as
optimization) determine there has been a default: label, restart processing
it again with a flag to satisfy switches (jump_target) with the default:
label of the current switch.

Another thing is that the skipping of statements in cxx_eval_statement_list
didn't really work correctly for COND_EXPR/LOOP_EXPR, so the patch pretty
much removes all the skip/label/etc. handling from cxx_eval_statement_list.

And, lastly the handling of LOOP_EXPR and COND_EXPR when initially skipping
had to be changed.  For COND_EXPR, we don't want to evaluate the condition
in that case (one can't goto into the condition expression), and need to
process then block with skipping and, if it isn't skipped at its end, not
process else block, otherwise process it.  And for LOOP_EXPR, if still
skipping we'd need not to iterate again on the body, as the whole loop has
been bypassed in that case.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-09-20  Jakub Jelinek  <jakub@redhat.com>

	PR c++/77467
	* constexpr.c (enum constexpr_switch_state): New.
	(struct constexpr_ctx): Add css_state field.
	(label_matches): Add CTX and STMT arguments, remove I and
	DEFAULT_LABEL.  For CASE_LABEL_EXPR assert ctx->css_state != NULL,
	handle default labels according to css_state.
	(cxx_eval_statement_list): Remove statement skipping, label_matches
	and default_label handling code.
	(cxx_eval_loop_expr): Exit after first iteration even if
	switches (jump_target).
	(cxx_eval_switch_expr): Set up css_state field in ctx, if default
	label has been seen in the body, but no cases matched, evaluate
	the body second time.
	(cxx_eval_constant_expression): Handle stmt skipping and label_matches
	here.  Handle PREDICT_EXPR.  For MODIFY_EXPR or INIT_EXPR, assert
	statement is not skipped.  For COND_EXPR during skipping, don't
	evaluate condition, just the then block and if still skipping at the
	end also the else block.
	(cxx_eval_outermost_constant_expr): Adjust constexpr_ctx initializer.
	(is_sub_constant_expr): Likewise.

	* g++.dg/cpp1y/constexpr-77467.C: New test.



	Jakub

Comments

Jakub Jelinek Sept. 27, 2016, 9:24 p.m. UTC | #1 
Hi!

On Tue, Sep 20, 2016 at 06:12:26PM +0200, Jakub Jelinek wrote:
> 2016-09-20  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/77467
> 	* constexpr.c (enum constexpr_switch_state): New.
> 	(struct constexpr_ctx): Add css_state field.
> 	(label_matches): Add CTX and STMT arguments, remove I and
> 	DEFAULT_LABEL.  For CASE_LABEL_EXPR assert ctx->css_state != NULL,
> 	handle default labels according to css_state.
> 	(cxx_eval_statement_list): Remove statement skipping, label_matches
> 	and default_label handling code.
> 	(cxx_eval_loop_expr): Exit after first iteration even if
> 	switches (jump_target).
> 	(cxx_eval_switch_expr): Set up css_state field in ctx, if default
> 	label has been seen in the body, but no cases matched, evaluate
> 	the body second time.
> 	(cxx_eval_constant_expression): Handle stmt skipping and label_matches
> 	here.  Handle PREDICT_EXPR.  For MODIFY_EXPR or INIT_EXPR, assert
> 	statement is not skipped.  For COND_EXPR during skipping, don't
> 	evaluate condition, just the then block and if still skipping at the
> 	end also the else block.
> 	(cxx_eval_outermost_constant_expr): Adjust constexpr_ctx initializer.
> 	(is_sub_constant_expr): Likewise.
> 
> 	* g++.dg/cpp1y/constexpr-77467.C: New test.

I'd like to ping this patch.  Ok for trunk?

	Jakub
Jason Merrill Sept. 28, 2016, 2:38 p.m. UTC | #2 
OK, thanks.

Jason
diff mbox

Patch

--- gcc/cp/constexpr.c.jj	2016-09-19 16:35:32.835574406 +0200
+++ gcc/cp/constexpr.c	2016-09-20 12:51:48.270305879 +0200
@@ -900,6 +900,18 @@  struct constexpr_call_hasher : ggc_ptr_h
   static bool equal (constexpr_call *, constexpr_call *);
 };
 
+enum constexpr_switch_state {
+  /* Used when processing a switch for the first time by cxx_eval_switch_expr
+     and default: label for that switch has not been seen yet.  */
+  css_default_not_seen,
+  /* Used when processing a switch for the first time by cxx_eval_switch_expr
+     and default: label for that switch has been seen already.  */
+  css_default_seen,
+  /* Used when processing a switch for the second time by
+     cxx_eval_switch_expr, where default: label should match.  */
+  css_default_processing
+};
+
 /* The constexpr expansion context.  CALL is the current function
    expansion, CTOR is the current aggregate initializer, OBJECT is the
    object being initialized by CTOR, either a VAR_DECL or a _REF.  VALUES
@@ -919,6 +931,8 @@  struct constexpr_ctx {
   tree ctor;
   /* The object we're building the CONSTRUCTOR for.  */
   tree object;
+  /* If inside SWITCH_EXPR.  */
+  constexpr_switch_state *css_state;
   /* Whether we should error on a non-constant expression or fail quietly.  */
   bool quiet;
   /* Whether we are strictly conforming to constant expression rules or
@@ -3487,14 +3501,12 @@  switches (tree *jump_target)
 }
 
 /* Subroutine of cxx_eval_statement_list.  Determine whether the statement
-   at I matches *jump_target.  If we're looking for a case label and we see
-   the default label, copy I into DEFAULT_LABEL.  */
+   STMT matches *jump_target.  If we're looking for a case label and we see
+   the default label, note it in ctx->css_state.  */
 
 static bool
-label_matches (tree *jump_target, tree_stmt_iterator i,
-	       tree_stmt_iterator& default_label)
+label_matches (const constexpr_ctx *ctx, tree *jump_target, tree stmt)
 {
-  tree stmt = tsi_stmt (i);
   switch (TREE_CODE (*jump_target))
     {
     case LABEL_DECL:
@@ -3506,8 +3518,18 @@  label_matches (tree *jump_target, tree_s
     case INTEGER_CST:
       if (TREE_CODE (stmt) == CASE_LABEL_EXPR)
 	{
+	  gcc_assert (ctx->css_state != NULL);
 	  if (!CASE_LOW (stmt))
-	    default_label = i;
+	    {
+	      /* default: should appear just once in a SWITCH_EXPR
+		 body (excluding nested SWITCH_EXPR).  */
+	      gcc_assert (*ctx->css_state != css_default_seen);
+	      /* When evaluating SWITCH_EXPR body for the second time,
+		 return true for the default: label.  */
+	      if (*ctx->css_state == css_default_processing)
+		return true;
+	      *ctx->css_state = css_default_seen;
+	    }
 	  else if (CASE_HIGH (stmt))
 	    {
 	      if (tree_int_cst_le (CASE_LOW (stmt), *jump_target)
@@ -3534,7 +3556,6 @@  cxx_eval_statement_list (const constexpr
 			 tree *jump_target)
 {
   tree_stmt_iterator i;
-  tree_stmt_iterator default_label = tree_stmt_iterator();
   tree local_target;
   /* In a statement-expression we want to return the last value.  */
   tree r = NULL_TREE;
@@ -3545,18 +3566,7 @@  cxx_eval_statement_list (const constexpr
     }
   for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i))
     {
-    reenter:
       tree stmt = tsi_stmt (i);
-      if (*jump_target)
-	{
-	  if (TREE_CODE (stmt) == STATEMENT_LIST)
-	    /* The label we want might be inside.  */;
-	  else if (label_matches (jump_target, i, default_label))
-	    /* Found it.  */
-	    *jump_target = NULL_TREE;
-	  else
-	    continue;
-	}
       r = cxx_eval_constant_expression (ctx, stmt, false,
 					non_constant_p, overflow_p,
 					jump_target);
@@ -3565,12 +3575,6 @@  cxx_eval_statement_list (const constexpr
       if (returns (jump_target) || breaks (jump_target))
 	break;
     }
-  if (switches (jump_target) && !tsi_end_p (default_label))
-    {
-      i = default_label;
-      *jump_target = NULL_TREE;
-      goto reenter;
-    }
   return r;
 }
 
@@ -3609,7 +3613,10 @@  cxx_eval_loop_expr (const constexpr_ctx
 	  break;
 	}
     }
-  while (!returns (jump_target) && !breaks (jump_target) && !*non_constant_p);
+  while (!returns (jump_target)
+	 && !breaks (jump_target)
+	 && !switches (jump_target)
+	 && !*non_constant_p);
 
   if (breaks (jump_target))
     *jump_target = NULL_TREE;
@@ -3632,8 +3639,20 @@  cxx_eval_switch_expr (const constexpr_ct
   *jump_target = cond;
 
   tree body = TREE_OPERAND (t, 1);
-  cxx_eval_statement_list (ctx, body,
-			   non_constant_p, overflow_p, jump_target);
+  constexpr_ctx new_ctx = *ctx;
+  constexpr_switch_state css = css_default_not_seen;
+  new_ctx.css_state = &css;
+  cxx_eval_constant_expression (&new_ctx, body, false,
+				non_constant_p, overflow_p, jump_target);
+  if (switches (jump_target) && css == css_default_seen)
+    {
+      /* If the SWITCH_EXPR body has default: label, process it once again,
+	 this time instructing label_matches to return true for default:
+	 label on switches (jump_target).  */
+      css = css_default_processing;
+      cxx_eval_constant_expression (&new_ctx, body, false,
+				    non_constant_p, overflow_p, jump_target);
+    }
   if (breaks (jump_target) || switches (jump_target))
     *jump_target = NULL_TREE;
   return NULL_TREE;
@@ -3653,6 +3672,27 @@  cxx_eval_constant_expression (const cons
   constexpr_ctx new_ctx;
   tree r = t;
 
+  if (jump_target && *jump_target)
+    {
+      /* If we are jumping, ignore all statements/expressions except those
+	 that could have LABEL_EXPR or CASE_LABEL_EXPR in their bodies.  */
+      switch (TREE_CODE (t))
+	{
+	case BIND_EXPR:
+	case STATEMENT_LIST:
+	case LOOP_EXPR:
+	case COND_EXPR:
+	  break;
+	case LABEL_EXPR:
+	case CASE_LABEL_EXPR:
+	  if (label_matches (ctx, jump_target, t))
+	    /* Found it.  */
+	    *jump_target = NULL_TREE;
+	  return NULL_TREE;
+	default:
+	  return NULL_TREE;
+	}
+    }
   if (t == error_mark_node)
     {
       *non_constant_p = true;
@@ -3733,6 +3773,7 @@  cxx_eval_constant_expression (const cons
     case LABEL_DECL:
     case LABEL_EXPR:
     case CASE_LABEL_EXPR:
+    case PREDICT_EXPR:
       return t;
 
     case PARM_DECL:
@@ -3838,6 +3879,7 @@  cxx_eval_constant_expression (const cons
 
     case INIT_EXPR:
     case MODIFY_EXPR:
+      gcc_assert (jump_target == NULL || *jump_target == NULL_TREE);
       r = cxx_eval_store_expression (ctx, t, lval,
 				     non_constant_p, overflow_p);
       break;
@@ -4068,6 +4110,22 @@  cxx_eval_constant_expression (const cons
       break;
 
     case COND_EXPR:
+      if (jump_target && *jump_target)
+	{
+	  /* When jumping to a label, the label might be either in the
+	     then or else blocks, so process then block first in skipping
+	     mode first, and if we are still in the skipping mode at its end,
+	     process the else block too.  */
+	  r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 1),
+					    lval, non_constant_p, overflow_p,
+					    jump_target);
+	  if (*jump_target)
+	    r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 2),
+					      lval, non_constant_p, overflow_p,
+					      jump_target);
+	  break;
+	}
+      /* FALLTHRU */
     case VEC_COND_EXPR:
       r = cxx_eval_conditional_expression (ctx, t, lval,
 					   non_constant_p, overflow_p,
@@ -4343,7 +4401,7 @@  cxx_eval_outermost_constant_expr (tree t
   bool overflow_p = false;
   hash_map<tree,tree> map;
 
-  constexpr_ctx ctx = { NULL, &map, NULL, NULL, NULL,
+  constexpr_ctx ctx = { NULL, &map, NULL, NULL, NULL, NULL,
 			allow_non_constant, strict };
 
   tree type = initialized_type (t);
@@ -4463,7 +4521,7 @@  is_sub_constant_expr (tree t)
   bool overflow_p = false;
   hash_map <tree, tree> map;
 
-  constexpr_ctx ctx = { NULL, &map, NULL, NULL, NULL, true, true };
+  constexpr_ctx ctx = { NULL, &map, NULL, NULL, NULL, NULL, true, true };
 
   cxx_eval_constant_expression (&ctx, t, false, &non_constant_p,
 				&overflow_p);
--- gcc/testsuite/g++.dg/cpp1y/constexpr-77467.C.jj	2016-09-20 10:26:51.226148386 +0200
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-77467.C	2016-09-20 12:35:44.507603391 +0200
@@ -0,0 +1,128 @@ 
+// PR c++/77467
+// { dg-do compile { target c++14 } }
+
+constexpr int
+foo (const int x, const unsigned n) noexcept
+{
+  switch (n)
+    {
+    case 0:
+      return 1;
+    case 1:
+      return x;
+    default:
+      const auto m = (n >> 1);
+      const auto y = foo (x, m);
+      return ((m << 1) == n) ? y * y : x * y * y;
+    }
+}
+
+static_assert (foo (3, 2) == 9, "");
+static_assert (foo (2, 3) == 8, "");
+
+constexpr int
+bar (int x)
+{
+  int a = x;
+  switch (x)
+    a = x + 1;
+  return a;
+}
+
+static_assert (bar (0) == 0, "");
+static_assert (bar (1) == 1, "");
+
+constexpr int
+baz (const int x, int y) noexcept
+{
+  switch (x)
+    {
+    case 0:
+      return 1;
+    case 1:
+      return x;
+    case 2:
+      if ((y += 2) == 0)
+	{
+	case 3:
+	  y += 4;
+	  break;
+	}
+      else
+	{
+	case 4:
+	  y += 8;
+	  break;
+	}
+      break;
+    case 5:
+      for (y = 0; y < 3; y++)
+	{
+	case 7:
+	  if (y == -4)
+	    y += 3;
+	  if (y == -3)
+	    continue;
+	  if (y == -2)
+	    {
+	      y += 18;
+	      break;
+	    }
+	  if (y == 2)
+	    {
+	    case 6:
+	      y += 12;
+	    default:
+	      y++;
+	      break;
+	    }
+	}
+      break;
+    case -1:
+    case -2:
+      switch (y)
+	{
+	case 19:
+	  y += 2;
+	  break;
+	case 20:
+	  y += 3;
+	  if (x == 2)
+	    case 21:;
+	  y += 2;
+	  if (x == 3)
+	    default:;
+	  y += 4;
+	  break;
+	}
+      return x + y + 1;
+    }
+  return x + y;
+}
+
+static_assert (baz (0, 7) == 1, "");
+static_assert (baz (1, 7) == 1, "");
+static_assert (baz (2, -2) == 6, "");
+static_assert (baz (2, 0) == 12, "");
+static_assert (baz (3, 1) == 8, "");
+static_assert (baz (4, 2) == 14, "");
+static_assert (baz (5, -20) == 20, "");
+static_assert (baz (6, 5) == 24, "");
+static_assert (baz (7, -5) == 22, "");
+static_assert (baz (7, -4) == 22, "");
+static_assert (baz (7, -3) == 23, "");
+static_assert (baz (7, -2) == 23, "");
+static_assert (baz (7, -1) == 22, "");
+static_assert (baz (7, 0) == 22, "");
+static_assert (baz (7, 2) == 22, "");
+static_assert (baz (7, 6) == 14, "");
+static_assert (baz (8, 9) == 18, "");
+static_assert (baz (8, -2) == 7, "");
+static_assert (baz (-1, 19) == 21, "");
+static_assert (baz (-1, 20) == 29, "");
+static_assert (baz (-1, 21) == 27, "");
+static_assert (baz (-1, 5) == 9, "");
+static_assert (baz (-2, 19) == 20, "");
+static_assert (baz (-2, 20) == 28, "");
+static_assert (baz (-2, 21) == 26, "");
+static_assert (baz (-2, 5) == 8, "");