Bug 89785 - Incorrect "not a constant expression" error with switch statement that returns
Summary: Incorrect "not a constant expression" error with switch statement that returns
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 9.0
: P3 normal
Target Milestone: 9.0
Assignee: Jakub Jelinek
URL:
Keywords: rejects-valid
Depends on:
Blocks: constexpr
  Show dependency treegraph
 
Reported: 2019-03-21 07:22 UTC by Antony Polukhin
Modified: 2021-08-28 21:42 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-03-22 00:00:00


Attachments
gcc9-pr89785.patch (1.75 KB, patch)
2019-03-26 19:51 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antony Polukhin 2019-03-21 07:22:24 UTC
The following code fails to compile:

constexpr int Addrlen(int domain) {
    switch (domain) {
      case 0:
        return 0;
      case 2:
        return 42;
    }
    throw 42;
}

Error message is following:

<source>: In function 'constexpr int Addrlen(int)':
<source>:8:11: error: expression '<throw-expression>' is not a constant expression
    8 |     throw 42;
      |           ^~
Comment 1 Richard Biener 2019-03-21 08:39:07 UTC
So you say that Addrlen(0) and Addrlen(2) are proper constexprs?  Of course
Addrlen(1) is not.
Comment 2 Antony Polukhin 2019-03-21 09:00:37 UTC
> So you say that Addrlen(0) and Addrlen(2) are proper constexprs?  Of course
Addrlen(1) is not.

Yes. But GCC does not even allow to define the Addrlen function: https://godbolt.org/z/xqR2Lr
Comment 3 Jakub Jelinek 2019-03-21 09:36:56 UTC
I guess that is because potential_constant_expression_1 punts on THROW_EXPR.  There is some code to not recurse if e.g. IF_STMT or COND_EXPR condition evaluates to constant zero or nonzero, but that is not the case here.
I actually don't know what in the standard requires something like potential_constant_expression{,_1} rather than just punting during constexpr evaluation if some non-constexpr construct is encountered.  For the cases listed in dcl.constexpr which may not show up in constexpr functions I thought that is covered in the parser already.
Comment 4 Richard Biener 2019-03-21 13:05:47 UTC
Also consider the equivalent(?)

constexpr foo() { throw 42; } // with or without constexpr

constexpr int Addrlen(int domain) {
    switch (domain) {
      case 0:
        return 0;
      case 2:
        return 42;
    }
    foo();
}

if Addrlen is allowed to be constexpr when foo() is not called
[and is not declared constexpr].  And whether foo may be declared
constexpr or not.
Comment 5 Jonathan Wakely 2019-03-21 13:18:47 UTC
(In reply to Richard Biener from comment #4)
> Also consider the equivalent(?)
> 
> constexpr foo() { throw 42; } // with or without constexpr
> 
> constexpr int Addrlen(int domain) {
>     switch (domain) {
>       case 0:
>         return 0;
>       case 2:
>         return 42;
>     }
>     foo();
> }
> 
> if Addrlen is allowed to be constexpr when foo() is not called
> [and is not declared constexpr].  And whether foo may be declared
> constexpr or not.

foo cannot be constexpr, but no diagnostic is required:

> For a constexpr function or constexpr constructor that is neither defaulted nor a template, if no argument values exist such that an invocation of the function or constructor could be an evaluated subexpression of a core constant expression (7.7), or, for a constructor, a constant initializer for some object (6.8.3.2), the program is ill-formed, no diagnostic required.

Addrlen *can* be constexpr, because there are argument values that allow it to be evaluated at compile time, without ever reaching the throw (or reaching the call to the non-constexpr foo).
Comment 6 Jakub Jelinek 2019-03-22 13:52:18 UTC
The bug is then in potential_constant_expression_1:
    case SWITCH_STMT:
      if (!RECUR (SWITCH_STMT_COND (t), rval))
        return false;
      /* FIXME we don't check SWITCH_STMT_BODY currently, because even
         unreachable labels would be checked.  */
      return true;

This is not conservatively correct as the testcases show.
The question is how much effort we do want to spend on that.

The simplest might be to add *jump_target = integer_zero_node; before the return true; above, that will just assume any switch can return and not check both the switch body (which isn't checked even now) nor anything after the switch.

Another option, slightly more involved, is say cp_walk_tree on SWITCH_STMT_BODY, looking for any RETURN_EXPRs or CONTINUE_STMTs (the latter only when not nested inside of a FOR_STMT, DO_STMT, WHILE_STMT and RANGE_FOR_STMT) and if we find a RETURN_EXPR, set jump_target to that, if we don't, but find a CONTINUE_STMT not nested in further looping constructs, set *jump_target to that CONTINUE_STMT, otherwise keep it unchanged.  To me this looks like the best approach for now.

Another option is try to do something for the case when SWITCH_STMT_COND is constant, but already for that we'd need to greatly extend the simplified:
  if (*jump_target)
    /* If we are jumping, ignore everything.  This is simpler than the
       cxx_eval_constant_expression handling because we only need to be
       conservatively correct, and we don't necessarily have a constant value
       available, so we don't bother with switch tracking.  */
    return true;
so that we do walk statements that can embed other statements like cxx_eval_constant_expression does and all of that stuff, with label_matches etc.

And if the above is done, we could have some also some mode in which for non-constant SWITCH_STMT_COND we could say set *jump_target to the SWITCH_STMT itself and treat it as a walk which walks the switch body, if it finds some CASE_LABEL_EXPR, it will change it to some other mode which will start processing all the expressions; if we detect something that is not a potential constant expression, we would at the level of whole stmts switch back to that *jump_target = SWITCH_STMT mode and keep looking for another CASE_LABEL_EXPR
and that way, if for any of the switch condition values there would be a potential constant expression, we'd succeed, otherwise fail, and even set jump_target properly based on that.  But this would be quite a lot of work.

Jason, any preferences here?
Comment 7 Marek Polacek 2019-03-22 14:46:18 UTC
(I'd be interested in implementing one of the above, but would also like to hear from Jason first.)
Comment 8 Jason Merrill 2019-03-26 17:43:44 UTC
(In reply to Jakub Jelinek from comment #6)
> Another option, slightly more involved, is say cp_walk_tree on
> SWITCH_STMT_BODY, looking for any RETURN_EXPRs or CONTINUE_STMTs (the latter
> only when not nested inside of a FOR_STMT, DO_STMT, WHILE_STMT and
> RANGE_FOR_STMT) and if we find a RETURN_EXPR, set jump_target to that, if we
> don't, but find a CONTINUE_STMT not nested in further looping constructs,
> set *jump_target to that CONTINUE_STMT, otherwise keep it unchanged.  To me
> this looks like the best approach for now.

I agree.
Comment 9 Jakub Jelinek 2019-03-26 19:51:11 UTC
Created attachment 46027 [details]
gcc9-pr89785.patch

Untested fix.
Comment 10 Jakub Jelinek 2019-03-28 14:48:44 UTC
Author: jakub
Date: Thu Mar 28 14:47:47 2019
New Revision: 269995

URL: https://gcc.gnu.org/viewcvs?rev=269995&root=gcc&view=rev
Log:
	PR c++/89785
	* constexpr.c (struct check_for_return_continue_data): New type.
	(check_for_return_continue): New function.
	(potential_constant_expression_1) <case SWITCH_STMT>: Walk
	SWITCH_STMT_BODY to find RETURN_EXPRs or CONTINUE_STMTs not nested
	in loop bodies and set *jump_target to that if found.

	* g++.dg/cpp1y/constexpr-89785-1.C: New test.
	* g++.dg/cpp1y/constexpr-89785-2.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/cpp1y/constexpr-89785-1.C
    trunk/gcc/testsuite/g++.dg/cpp1y/constexpr-89785-2.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/constexpr.c
    trunk/gcc/testsuite/ChangeLog
Comment 11 Jakub Jelinek 2019-03-28 15:34:59 UTC
Fixed for 9.1+.