* [C++ PATCH] Fix constexpr switch handling (PR c++/77467, take 2)
2016-09-19 18:49 ` Jason Merrill
@ 2016-09-20 16:29 ` Jakub Jelinek
2016-09-27 21:33 ` Patch ping Jakub Jelinek
2016-09-28 14:42 ` [C++ PATCH] Fix constexpr switch handling (PR c++/77467, take 2) Jason Merrill
0 siblings, 2 replies; 7+ messages in thread
From: Jakub Jelinek @ 2016-09-20 16:29 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches List
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.
--- 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, "");
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread