[PATCH v2] c++: wrong error with constexpr COMPOUND_EXPR [PR105321]

[Jason Merrill]

Ok.

On Thu, Apr 21, 2022, 9:20 AM Marek Polacek <polacek@redhat.com> wrote:

> On Thu, Apr 21, 2022 at 08:56:23AM -0400, Jason Merrill wrote:
> > On 4/20/22 18:40, Marek Polacek wrote:
> > > Here we issue a bogus error for the first assert in the test.  Therein
> > > we have
> > >
> > > <retval> = (void) (VIEW_CONVERT_EXPR<bool>(yes) || handle_error ());,
> VIEW_CONVERT_EXPR<int>(value);
> > >
> > > which has a COMPOUND_EXPR, so we get to cxx_eval_constant_expression
> > > <case COMPOUND_EXPR>.  The problem here is that we call
> > >
> > > 7044             /* Check that the LHS is constant and then discard
> it.  */
> > > 7045             cxx_eval_constant_expression (ctx, op0,
> > > 7046                                           true, non_constant_p,
> overflow_p,
> > > 7047                                           jump_target);
> > >
> > > where lval is always true, so the PARM_DECL 'yes' is not evaluated into
> > > its value.  r218832 changed the argument for 'lval' from false to true:
> > >
> > >     (cxx_eval_constant_expression) [COMPOUND_EXPR]: Pass true for lval.
> > >
> > > but I think we want to pass 'lval' instead.  Jakub tells me that's what
> > > we do for "(void) expr" as well.  [expr.comma] says that the left
> expression
> > > is a discarded-value expression, but [expr.context] doesn't suggest
> that
> > > we should always be passing false for lval as pre-r218832.
> >
> > In a discarded-value expression, we don't do the lvalue-rvalue
> conversion;
> > whether we want an lvalue for the RHS of the comma is irrelevant.
>
> Ah, that's what I misread -- [expr.context]/2.8 cares only about the right
> operand :(.
>
> > The bug here seems to be that we aren't doing the l->r conversion for the
> > LHS of the TRUTH_OR_EXPR; I'd think that cxx_eval_logical_expression
> should
> > pass false for lval to both recursive calls, there's no case where we
> > actually expect an lvalue from a TRUTH_*.
>
> Yeah, that makes sense.
>
> Bootstrap/regtest running on x86_64-pc-linux-gnu, ok for trunk/11.3 if it
> passes?
>
> -- >8 --
> Here we issue a bogus error for the first assert in the test.  Therein
> we have
>
> <retval> = (void) (VIEW_CONVERT_EXPR<bool>(yes) || handle_error ());,
> VIEW_CONVERT_EXPR<int>(value);
>
> which has a COMPOUND_EXPR, so we get to cxx_eval_constant_expression
> <case COMPOUND_EXPR>.  The problem here is that we call
>
> 7044             /* Check that the LHS is constant and then discard it.  */
> 7045             cxx_eval_constant_expression (ctx, op0,
> 7046                                           true, non_constant_p,
> overflow_p,
> 7047                                           jump_target);
>
> where lval is always true, so the PARM_DECL 'yes' is not evaluated into
> its value.
>
> Fixed by always passing false for 'lval' in cxx_eval_logical_expression;
> there's no case where we actually expect an lvalue from a TRUTH_*.
>
>         PR c++/105321
>
> gcc/cp/ChangeLog:
>
>         * constexpr.cc (cxx_eval_logical_expression): Always pass false
> for lval
>         to cxx_eval_constant_expression.
>
> gcc/testsuite/ChangeLog:
>
>         * g++.dg/cpp0x/constexpr-105321.C: New test.
> ---
>  gcc/cp/constexpr.cc                           |  9 ++++-----
>  gcc/testsuite/g++.dg/cpp0x/constexpr-105321.C | 18 ++++++++++++++++++
>  2 files changed, 22 insertions(+), 5 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-105321.C
>
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index e89440e770f..fa65290e938 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -4566,19 +4566,18 @@ cxx_eval_bit_cast (const constexpr_ctx *ctx, tree
> t, bool *non_constant_p,
>  static tree
>  cxx_eval_logical_expression (const constexpr_ctx *ctx, tree t,
>                               tree bailout_value, tree continue_value,
> -                            bool lval,
> -                            bool *non_constant_p, bool *overflow_p)
> +                            bool, bool *non_constant_p, bool *overflow_p)
>  {
>    tree r;
>    tree lhs = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0),
> -                                          lval,
> -                                          non_constant_p, overflow_p);
> +                                          /*lval*/false, non_constant_p,
> +                                          overflow_p);
>    VERIFY_CONSTANT (lhs);
>    if (tree_int_cst_equal (lhs, bailout_value))
>      return lhs;
>    gcc_assert (tree_int_cst_equal (lhs, continue_value));
>    r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 1),
> -                                   lval, non_constant_p,
> +                                   /*lval*/false, non_constant_p,
>                                     overflow_p);
>    VERIFY_CONSTANT (r);
>    return r;
> diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-105321.C
> b/gcc/testsuite/g++.dg/cpp0x/constexpr-105321.C
> new file mode 100644
> index 00000000000..adb6830ff22
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-105321.C
> @@ -0,0 +1,18 @@
> +// PR c++/105321
> +// { dg-do compile { target c++11 } }
> +
> +bool handle_error();
> +
> +constexpr int echo(int value, bool yes = true) noexcept
> +{
> +    return (yes || handle_error()), value;
> +}
> +
> +static_assert(echo(10) == 10, "");
> +
> +constexpr int echo2(int value, bool no = false) noexcept
> +{
> +    return (!no || handle_error()), value;
> +}
> +
> +static_assert(echo2(10) == 10, "");
>
> base-commit: 1e6c0e69af8da436e1d1d2d23d8c38410d78ecf2
> --
> 2.35.1
>
>