Re: [PATCH] c++: Don't ICE for unknown parameter to constexpr'd switch-statement, PR113545

[Marek Polacek <polacek@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.
 
> 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.

> With this patch, the code generated for the (inlined) call to
> ifbar equals that to swbar, except for the comparisons being
> in another order.
> 
> gcc/cp:
> 	PR c++/113545
> 	* constexpr.cc (label_matches): Replace call to_unreachable with

"to gcc_unreachable"

> 	return false.

More like with "break" but that's not important.
 
> gcc/testsuite:
> 	* g++.dg/expr/pr113545.C: New text.

"test"

> ---
>  gcc/cp/constexpr.cc                 |  3 +-
>  gcc/testsuite/g++.dg/expr/pr113545.C | 49 +++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/expr/pr113545.C
> 
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index 6350fe154085..30caf3322fff 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -6922,7 +6922,8 @@ label_matches (const constexpr_ctx *ctx, tree *jump_target, tree stmt)
>        break;
>  
>      default:
> -      gcc_unreachable ();
> +      /* Something else, like CONVERT_EXPR.  Unknown whether it matches.  */
> +      break;
>      }
>    return false;
>  }
> diff --git a/gcc/testsuite/g++.dg/expr/pr113545.C b/gcc/testsuite/g++.dg/expr/pr113545.C
> new file mode 100644
> index 000000000000..914ffdeb8e16
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/expr/pr113545.C

The problem seems to be more about conversion so g++.dg/conversion/reinterpret5.C
or g++.dg/cpp0x/constexpr-reinterpret3.C seems more appropriate.

> @@ -0,0 +1,49 @@

Please add

PR c++/113545

> +// { dg-do run { target c++11 } }
> +
> +char foo;
> +
> +// This one caught a call to gcc_unreachable in
> +// cp/constexpr.cc:label_matches, when passed a convert_expr from the
> +// cast in the call.
> +constexpr unsigned char swbar(__UINTPTR_TYPE__ baz)
> +{
> +  switch (baz)
> +    {
> +    case 13:
> +      return 11;
> +    case 14:
> +      return 78;
> +    case 2048:
> +      return 13;
> +    default:
> +      return 42;
> +    }
> +}
> +
> +// For reference, the equivalent* if-statements.
> +constexpr unsigned char ifbar(__UINTPTR_TYPE__ baz)
> +{
> +  if (baz == 13)
> +    return 11;
> +  else if (baz == 14)
> +    return 78;
> +  else if (baz == 2048)
> +    return 13;
> +  else
> +    return 42;
> +}
> +
> +__attribute__ ((__noipa__))
> +void xyzzy(int x)
> +{
> +  if (x != 42)
> +    __builtin_abort ();
> +}
> +
> +int main()
> +{
> +  unsigned const char c = swbar(reinterpret_cast<__UINTPTR_TYPE__>(&foo));
> +  xyzzy(c);
> +  unsigned const char d = ifbar(reinterpret_cast<__UINTPTR_TYPE__>(&foo));

I suppose we should also test a C-style cast (which leads to a reinterpret_cast
in this case).

Maybe check we get an error when c/d are constexpr (that used to ICE).

> +  xyzzy(d);
> +}
> -- 
> 2.30.2
> 

Marek