diff mbox

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

Message ID 20160905171119.GU14857@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Sept. 5, 2016, 5:11 p.m. UTC 
Hi!

cxx_eval_switch_expr assumes that SWITCH_EXPR's body is always a
STATEMENT_LIST, but that doesn't have to be the case.
As the testcase shows, if there are any variable declarations in the
switch body, it can be also a BIND_EXPR, which cxx_eval_constant_expression
handles properly, and as bar in the testcase shows, it can be also just
a single statement (as try isn't allowed in constexpr functions, I think
we just want to do what cxx_eval_statement_list would do on such a statement
if it was wrapped into a STATEMENT_LIST - ignore it, as the case NNN: or default:
is not present.

Bootstrapped/regtested on x86_64-linux and i686-linux?  What about older
branches?

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

	PR c++/77467
	* constexpr.c (cxx_eval_switch_expr): Call cxx_eval_constant_expression
	instead of cxx_eval_statement_list, for body other than STATEMENT_LIST
	or BIND_EXPR don't evaluate the body at all.

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


	Jakub

Comments

Jason Merrill Sept. 16, 2016, 7:51 p.m. UTC | #1 
On Mon, Sep 5, 2016 at 1:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> +  /* If body is a statement other than STATEMENT_LIST or BIND_EXPR,
> +     it should be skipped.  E.g. switch (a) b = a;  */
> +  if (TREE_CODE (body) == STATEMENT_LIST
> +      || TREE_CODE (body) == BIND_EXPR)

I'm nervous about this optimization for useless code breaking other
things that might (one day) wrap a case label; I think I'd prefer to
drop the condition.

OK with that change, for trunk and 6.

Jason
Jakub Jelinek Sept. 16, 2016, 8:44 p.m. UTC | #2 
On Fri, Sep 16, 2016 at 03:51:11PM -0400, Jason Merrill wrote:
> On Mon, Sep 5, 2016 at 1:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > +  /* If body is a statement other than STATEMENT_LIST or BIND_EXPR,
> > +     it should be skipped.  E.g. switch (a) b = a;  */
> > +  if (TREE_CODE (body) == STATEMENT_LIST
> > +      || TREE_CODE (body) == BIND_EXPR)
> 
> I'm nervous about this optimization for useless code breaking other
> things that might (one day) wrap a case label; I think I'd prefer to
> drop the condition.

By droping the condition you mean unconditionally call
  cxx_eval_constant_expression (ctx, body, false,
			        non_constant_p, overflow_p, jump_target);
?  That is known not to work, that breaks the
+constexpr int
+bar (int x)
+{
+  int a = x;
+  switch (x)
+    a = x + 1;
+  return a;
+}
handling in the testcase, where body is the MODIFY_EXPR which doesn't have
the label and thus needs to be skipped.  The problem is that all the logic for
skipping statements until the label is found is in cxx_eval_statement_list
only.  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.
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?

	Jakub
Jason Merrill Sept. 19, 2016, 6:34 p.m. UTC | #3 
On Fri, Sep 16, 2016 at 4:44 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Sep 16, 2016 at 03:51:11PM -0400, Jason Merrill wrote:
>> On Mon, Sep 5, 2016 at 1:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > +  /* If body is a statement other than STATEMENT_LIST or BIND_EXPR,
>> > +     it should be skipped.  E.g. switch (a) b = a;  */
>> > +  if (TREE_CODE (body) == STATEMENT_LIST
>> > +      || TREE_CODE (body) == BIND_EXPR)
>>
>> I'm nervous about this optimization for useless code breaking other
>> things that might (one day) wrap a case label; I think I'd prefer to
>> drop the condition.
>
> By droping the condition you mean unconditionally call
>   cxx_eval_constant_expression (ctx, body, false,
>                                 non_constant_p, overflow_p, jump_target);
> ?  That is known not to work, that breaks the
> +constexpr int
> +bar (int x)
> +{
> +  int a = x;
> +  switch (x)
> +    a = x + 1;
> +  return a;
> +}
> handling in the testcase, where body is the MODIFY_EXPR which doesn't have
> the label and thus needs to be skipped.  The problem is that all the logic for
> skipping statements until the label is found is in cxx_eval_statement_list
> only.

Ah, right.

> 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 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?

Jason
diff mbox

Patch

--- gcc/cp/constexpr.c.jj	2016-08-30 08:42:06.000000000 +0200
+++ gcc/cp/constexpr.c	2016-09-05 11:34:30.185518395 +0200
@@ -3572,8 +3572,12 @@  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);
+  /* If body is a statement other than STATEMENT_LIST or BIND_EXPR,
+     it should be skipped.  E.g. switch (a) b = a;  */
+  if (TREE_CODE (body) == STATEMENT_LIST
+      || TREE_CODE (body) == BIND_EXPR)
+    cxx_eval_constant_expression (ctx, body, false,
+				  non_constant_p, overflow_p, jump_target);
   if (breaks (jump_target) || switches (jump_target))
     *jump_target = NULL_TREE;
   return NULL_TREE;
--- gcc/testsuite/g++.dg/cpp1y/constexpr-77467.C.jj	2016-09-05 11:19:30.593750642 +0200
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-77467.C	2016-09-05 11:37:11.929477518 +0200
@@ -0,0 +1,33 @@ 
+// 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, "");