On Wed, Feb 07, 2024 at 04:32:57PM -0500, Jason Merrill wrote:
> On 2/6/24 19:23, Hans-Peter Nilsson wrote:
> > > Date: Mon, 22 Jan 2024 14:33:59 -0500
> > > From: Marek Polacek <[email protected]>
> >
> > > On Mon, Jan 22, 2024 at 06:02:32PM +0100, Hans-Peter Nilsson wrote:
> > > > I don't really know whether this is the right way to treat
> > > > CONVERT_EXPR as below, but... Regtested native
> > > > x86_64-linux-gnu. Ok to commit?
> > >
> > > Thanks for taking a look at this problem.
> >
> > Thanks for the initial review.
> >
> > > > brgds, H-P
> > > >
> > > > -- >8 --
> > > > That gcc_unreachable at the default-label seems to be over
> > > > the top. It seems more correct to just say "that's not
> > > > constant" to whatever's not known (to be constant), when
> > > > looking for matches in switch-statements.
> > >
> > > Unfortunately this doesn't seem correct to me; I don't think we
> > > should have gotten that far. It appears that we lose track of
> > > the reinterpret_cast, which is not allowed in a constant expression:
> > > <http://eel.is/c++draft/expr.const#5.15>.
> > >
> > > cp_convert -> ... -> convert_to_integer_1 gives us a CONVERT_EXPR
> > > but we only set REINTERPRET_CAST_P on NOP_EXPRs:
> > >
> > > expr = cp_convert (type, expr, complain);
> > > if (TREE_CODE (expr) == NOP_EXPR)
> > > /* Mark any nop_expr that created as a reintepret_cast. */
> > > REINTERPRET_CAST_P (expr) = true;
> > >
> > > so when evaluating baz we get (long unsigned int) &foo, which
> > > passes verify_constant.
> > > I don't have a good suggestion yet, sorry.
> >
> > But, with this patch, we're letting the non-constant case
> > take the same path and failing for the same reason, albeit
> > much later than desired, for the switch code as for the
> > if-chain code. Isn't that better than the current ICE?
> >
> > I mean, if there's a risk of accepts-invalid (like, some
> > non-constant case incorrectly "constexpr'd"), then that risk
> > is as already there, for the if-chain case.
> >
> > Anyway, this is a bit too late in the release season and
> > isn't a regression, thus I can't argue for it being a
> > suitable stop-gap measure...
> >
> > I'm unassigning myself from the PR as I have no clue how to
> > fix the actual non-constexpr-operand-seen-too-late bug.
>
> I think it would be better to check in cxx_eval_switch_expr that the
> condition is an INTEGER_CST, since VERIFY_CONSTANT isn't specific enough in
> this case, like the attached:
>
> > Though, I'm asking again; any clue regarding:
> >
> > "I briefly considered one of the cpp[0-9a-z]* subdirectories
> > but found no rule.
> >
> > Isn't constexpr c++11 and therefor cpp0x isn't a good match
> > (contrary to the many constexpr tests therein)?
> >
> > What *is* the actual rule for putting a test in
> > g++.dg/cpp0x, cpp1x and cpp1y (et al)?
> > (I STFW but found nothing.)"
>
> C++11 was called C++0x until it was actually done, a couple of years later
> than expected. Since that experience the C++ committee has moved to
> time-based rather than feature-based releases...
>
> Incidentally, these testcases seem to require C++14; you can't have a switch
> in a constexpr function in C++11.
>
> Jason
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index 2ebb1470dd5..fa346fe01c9 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -7106,6 +7106,16 @@ cxx_eval_switch_expr (const constexpr_ctx *ctx, tree t,
> cond = cxx_eval_constant_expression (ctx, cond, vc_prvalue,
> non_constant_p, overflow_p);
> VERIFY_CONSTANT (cond);
> + if (TREE_CODE (cond) != INTEGER_CST)
> + {
> + /* If the condition doesn't reduce to an INTEGER_CST it isn't a usable
> + switch condition even if it's constant enough for other things
> + (c++/113545). */
> + gcc_checking_assert (ctx->quiet);
> + *non_constant_p = true;
> + return t;
> + }
> +
> *jump_target = cond;
>
> tree body
The patch makes sense to me, although I'm afraid that losing the
REINTERPRET_CAST_P flag will cause other issues.
HP, sorry that I never got back to you. I would be more than happy to
take the patch above, add some tests and test/bootstrap it, unless you
want to do that yourself.
Thanks & sorry again,
Marek
