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 <pola...@redhat.com> > > > > > 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