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

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?

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.

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
	return false.

gcc/testsuite:
	* g++.dg/expr/pr113545.C: New text.
---
 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

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

> Date: Mon, 22 Jan 2024 14:33:59 -0500
> From: 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.

Honestly, it's more like posting a patch is more effective
than just opening a PR. ;)  And I was curious about that
constexpr thing that usually works, but blew up here.

> > 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.

The gcc_unreachable was indeed a clue in that direction.

>  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>.

B...b..but clang allows it... (and I have no clue about the
finer --or admittedly even coarser-- details of C++) ...and
not-my-code, just "porting" it.

Seriously though, thanks for the reference.  Also, maybe
something to consider for -fpermissive, if this changes to a
more graceful error path.

> 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.

Thanks for the review!

> > 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"

Oops!

> > 	return false.
> 
> More like with "break" but that's not important.

(Well, *effectively* return false.  I'd change to something
like "Replace call to gcc_unreachable with arrangement to
return false" if this were to go anywhere.)

> > gcc/testsuite:
> > 	* g++.dg/expr/pr113545.C: New text.
> 
> "test"

Gosh, horrible typos, thanks.

brgds, H-P

> Date: Mon, 22 Jan 2024 14:33:59 -0500
> From: Marek Polacek <polacek@redhat.com>

Oh, there was more...  Also, I think I misinterpreted your
reply as meaning that the test-case is ice-on-invalid and I
wrongly replied in agreement to that misinterpretation. :)

(For others at a comparable level of (lack of) C++ knowledge
to me: my reading of
https://en.cppreference.com/w/cpp/language/constexpr is that
a constexpr function can be validly called with an
expression that isn't "constexpr" (compile-time computable,
immediately computable, core constant expressions or
whatever the term), and the test-case is a valid example (of
two such invocations).  So, an expression calling such a
function is only truly "constexpr" with the "right"
parameters.  I know this isn't C++ 102, but many of us
hacking gcc aren't originally c++ hackers; that was just
happenstance.  I was about to write "aren't C++ hackers" but
then again, C++ happened to gcc, and c++11 at that.)

> On Mon, Jan 22, 2024 at 06:02:32PM +0100, Hans-Peter Nilsson wrote:

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

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.)

> > +++ b/gcc/testsuite/g++.dg/expr/pr113545.C
> > @@ -0,0 +1,49 @@
> 
> Please add
> 
> PR c++/113545

Certainly if the test is to change name and even if not
("git grep" wouldn't catch the file name).  Will do.

> > +  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).

But the expressions aren't declared constexpr, just const
(as in "here 'const' means run-time evaluation due to the
weirdness that is C++").

...oh, I see what you mean, these are valid, but you suggest
adding tests declared constexpr to check that they get a
matching error (not ICE :) !

Thanks again for the review, I think I'll at least re-work
the test-case into two separate ones.

brgds, H-P

> Date: Mon, 22 Jan 2024 14:33:59 -0500
> From: Marek Polacek <polacek@redhat.com>

> 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

> > +  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).

Like this?  Not sure about the value of that variant, but here goes.

I checked that these behave as expected (xfail as ICE properly) without the
previosly posted patch to cp/constexpr.cc and XPASS with it applied.

Ok to commit?

-- >8 --
Subject: [PATCH] c++: testcases for PR113545 (constexpr with switch and
 passing non-constexpr parameter)

gcc/testsuite:
	PR c++/113545
	* g++.dg/cpp0x/constexpr-reinterpret3.C,
	g++.dg/cpp0x/constexpr-reinterpret4.C: New tests.
---
 .../g++.dg/cpp0x/constexpr-reinterpret3.C     | 55 +++++++++++++++++++
 .../g++.dg/cpp0x/constexpr-reinterpret4.C     | 54 ++++++++++++++++++
 2 files changed, 109 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C

diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C
new file mode 100644
index 000000000000..319cc5e8bee9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C
@@ -0,0 +1,55 @@
+// PR c++/113545
+// { dg-do run { target c++11 } }
+// { dg-ice "PR112545 - constexpr function with switch called for reinterpret_cast" }
+
+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));
+  xyzzy(d);
+  unsigned const char e = swbar((__UINTPTR_TYPE__) &foo);
+  xyzzy(e);
+  unsigned const char f = ifbar((__UINTPTR_TYPE__) &foo);
+  xyzzy(f);
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C
new file mode 100644
index 000000000000..4d0fdf2c0a78
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C
@@ -0,0 +1,54 @@
+// PR c++/113545
+// { dg-do compile { 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 constexpr char c = swbar(reinterpret_cast<__UINTPTR_TYPE__>(&foo)); // { dg-error "conversion from pointer type" }
+  xyzzy(c);
+  unsigned constexpr char d = ifbar(reinterpret_cast<__UINTPTR_TYPE__>(&foo)); // { dg-error "conversion from pointer type" }
+  xyzzy(d);
+  unsigned constexpr char e = swbar((__UINTPTR_TYPE__) &foo); // { dg-error "conversion from pointer type" }
+  xyzzy(e);
+  unsigned constexpr char f = ifbar((__UINTPTR_TYPE__) &foo); // { dg-error "conversion from pointer type" }
+  xyzzy(f);
+}

Ping for the xfailed testsuite patch below the review
(actual constexpr.cc patch to be handled separately):

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Tue, 23 Jan 2024 05:55:00 +0100
> 
> > Date: Mon, 22 Jan 2024 14:33:59 -0500
> > From: Marek Polacek <polacek@redhat.com>
> 
> > 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
> 
> > > +  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).
> 
> Like this?  Not sure about the value of that variant, but here goes.
> 
> I checked that these behave as expected (xfail as ICE properly) without the
> previosly posted patch to cp/constexpr.cc and XPASS with it applied.
> 
> Ok to commit?
> 
> -- >8 --
> Subject: [PATCH] c++: testcases for PR113545 (constexpr with switch and
>  passing non-constexpr parameter)
> 
> gcc/testsuite:
> 	PR c++/113545
> 	* g++.dg/cpp0x/constexpr-reinterpret3.C,
> 	g++.dg/cpp0x/constexpr-reinterpret4.C: New tests.
> ---
>  .../g++.dg/cpp0x/constexpr-reinterpret3.C     | 55 +++++++++++++++++++
>  .../g++.dg/cpp0x/constexpr-reinterpret4.C     | 54 ++++++++++++++++++
>  2 files changed, 109 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C
> 
> diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C
> new file mode 100644
> index 000000000000..319cc5e8bee9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C
> @@ -0,0 +1,55 @@
> +// PR c++/113545
> +// { dg-do run { target c++11 } }
> +// { dg-ice "PR112545 - constexpr function with switch called for reinterpret_cast" }
> +
> +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));
> +  xyzzy(d);
> +  unsigned const char e = swbar((__UINTPTR_TYPE__) &foo);
> +  xyzzy(e);
> +  unsigned const char f = ifbar((__UINTPTR_TYPE__) &foo);
> +  xyzzy(f);
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C
> new file mode 100644
> index 000000000000..4d0fdf2c0a78
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C
> @@ -0,0 +1,54 @@
> +// PR c++/113545
> +// { dg-do compile { 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 constexpr char c = swbar(reinterpret_cast<__UINTPTR_TYPE__>(&foo)); // { dg-error "conversion from pointer type" }
> +  xyzzy(c);
> +  unsigned constexpr char d = ifbar(reinterpret_cast<__UINTPTR_TYPE__>(&foo)); // { dg-error "conversion from pointer type" }
> +  xyzzy(d);
> +  unsigned constexpr char e = swbar((__UINTPTR_TYPE__) &foo); // { dg-error "conversion from pointer type" }
> +  xyzzy(e);
> +  unsigned constexpr char f = ifbar((__UINTPTR_TYPE__) &foo); // { dg-error "conversion from pointer type" }
> +  xyzzy(f);
> +}
> -- 
> 2.30.2
>

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Tue, 30 Jan 2024 06:18:45 +0100

> Ping for the xfailed testsuite patch below the review
> (actual constexpr.cc patch to be handled separately):

Ping*2.  Again, this is for the xfailed test-case only.

> 
> > From: Hans-Peter Nilsson <hp@axis.com>
> > Date: Tue, 23 Jan 2024 05:55:00 +0100
> > 
> > > Date: Mon, 22 Jan 2024 14:33:59 -0500
> > > From: Marek Polacek <polacek@redhat.com>
> > 
> > > 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
> > 
> > > > +  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).
> > 
> > Like this?  Not sure about the value of that variant, but here goes.
> > 
> > I checked that these behave as expected (xfail as ICE properly) without the
> > previosly posted patch to cp/constexpr.cc and XPASS with it applied.
> > 
> > Ok to commit?
> > 
> > -- >8 --
> > Subject: [PATCH] c++: testcases for PR113545 (constexpr with switch and
> >  passing non-constexpr parameter)
> > 
> > gcc/testsuite:
> > 	PR c++/113545
> > 	* g++.dg/cpp0x/constexpr-reinterpret3.C,
> > 	g++.dg/cpp0x/constexpr-reinterpret4.C: New tests.
> > ---
> >  .../g++.dg/cpp0x/constexpr-reinterpret3.C     | 55 +++++++++++++++++++
> >  .../g++.dg/cpp0x/constexpr-reinterpret4.C     | 54 ++++++++++++++++++
> >  2 files changed, 109 insertions(+)
> >  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C
> >  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C
> > 
> > diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C
> > new file mode 100644
> > index 000000000000..319cc5e8bee9
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret3.C
> > @@ -0,0 +1,55 @@
> > +// PR c++/113545
> > +// { dg-do run { target c++11 } }
> > +// { dg-ice "PR112545 - constexpr function with switch called for reinterpret_cast" }
> > +
> > +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));
> > +  xyzzy(d);
> > +  unsigned const char e = swbar((__UINTPTR_TYPE__) &foo);
> > +  xyzzy(e);
> > +  unsigned const char f = ifbar((__UINTPTR_TYPE__) &foo);
> > +  xyzzy(f);
> > +}
> > diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C
> > new file mode 100644
> > index 000000000000..4d0fdf2c0a78
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-reinterpret4.C
> > @@ -0,0 +1,54 @@
> > +// PR c++/113545
> > +// { dg-do compile { 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 constexpr char c = swbar(reinterpret_cast<__UINTPTR_TYPE__>(&foo)); // { dg-error "conversion from pointer type" }
> > +  xyzzy(c);
> > +  unsigned constexpr char d = ifbar(reinterpret_cast<__UINTPTR_TYPE__>(&foo)); // { dg-error "conversion from pointer type" }
> > +  xyzzy(d);
> > +  unsigned constexpr char e = swbar((__UINTPTR_TYPE__) &foo); // { dg-error "conversion from pointer type" }
> > +  xyzzy(e);
> > +  unsigned constexpr char f = ifbar((__UINTPTR_TYPE__) &foo); // { dg-error "conversion from pointer type" }
> > +  xyzzy(f);
> > +}
> > -- 
> > 2.30.2
> > 
>

> Date: Mon, 22 Jan 2024 14:33:59 -0500
> From: 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.

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.

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.)"


> > 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:

(Deleted -- see separate patch)

> > ---
> >  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

brgds, H-P

On 2/6/24 19:23, Hans-Peter Nilsson wrote:
>> Date: Mon, 22 Jan 2024 14:33:59 -0500
>> From: 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.
> 
> 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

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 <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.
> > 
> > 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

> Date: Wed, 7 Feb 2024 21:11:59 -0500
> From: Marek Polacek <polacek@redhat.com>

> 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 <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.
> > > 
> > > Thanks for the initial review.

> > 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,

No worries, feel very welcome to deal with handling the
actual fix.  Also, you're better prepared than me, when it
comes to dealing with any possible fallout. :)

I'll send an updated version of the test-cases, moving them
to the C++14 test directory (cpp1y, right?) and qualify them
as c++14 instead, as Jason pointed out.

brgds, H-P

On Thu, Feb 08, 2024 at 04:40:40PM +0100, Hans-Peter Nilsson wrote:
> > Date: Wed, 7 Feb 2024 21:11:59 -0500
> > From: Marek Polacek <polacek@redhat.com>
> 
> > 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 <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.
> > > > 
> > > > Thanks for the initial review.
> 
> > > 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,
> 
> No worries, feel very welcome to deal with handling the
> actual fix.  Also, you're better prepared than me, when it
> comes to dealing with any possible fallout. :)
> 
> I'll send an updated version of the test-cases, moving them
> to the C++14 test directory (cpp1y, right?) and qualify them
> as c++14 instead, as Jason pointed out.

Right, cpp1y is c++14.  Note that we wouldn't push the tests separately
to the patch itself, unless it's something that already works.  Thanks,

Marek

> Date: Thu, 8 Feb 2024 10:44:31 -0500
> From: Marek Polacek <polacek@redhat.com>
> Cc: jason@redhat.com, gcc-patches@gcc.gnu.org
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> On Thu, Feb 08, 2024 at 04:40:40PM +0100, Hans-Peter Nilsson wrote:
> > > Date: Wed, 7 Feb 2024 21:11:59 -0500
> > > From: Marek Polacek <polacek@redhat.com>
> > 
> > > 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 <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.
> > > > > 
> > > > > Thanks for the initial review.
> > 
> > > > 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,
> > 
> > No worries, feel very welcome to deal with handling the
> > actual fix.  Also, you're better prepared than me, when it
> > comes to dealing with any possible fallout. :)
> > 
> > I'll send an updated version of the test-cases, moving them
> > to the C++14 test directory (cpp1y, right?) and qualify them
> > as c++14 instead, as Jason pointed out.
> 
> Right, cpp1y is c++14.  Note that we wouldn't push the tests separately
> to the patch itself, unless it's something that already works.  Thanks,
> 
> Marek

But, the tests work.  They passes as-is, as they track the
ICE, but will XPASS (that part) once a fix is committed (at
which time: "I checked that these behave as expected (xfail
as ICE properly) without the previosly posted patch to
cp/constexpr.cc and XPASS with it applied."

Once the fix works, the xfail for the ICE should be removed.
(Hm, better comment on the patches in a reply to that message. :)

The point is that for this type of bug it's useful to have a
test-case tracking it, before a fix is committed.

brgds, H-P

On Thu, Feb 08, 2024 at 05:07:21PM +0100, Hans-Peter Nilsson wrote:
> > Date: Thu, 8 Feb 2024 10:44:31 -0500
> > From: Marek Polacek <polacek@redhat.com>
> > Cc: jason@redhat.com, gcc-patches@gcc.gnu.org
> > Content-Type: text/plain; charset=us-ascii
> > Content-Disposition: inline
> > 
> > On Thu, Feb 08, 2024 at 04:40:40PM +0100, Hans-Peter Nilsson wrote:
> > > > Date: Wed, 7 Feb 2024 21:11:59 -0500
> > > > From: Marek Polacek <polacek@redhat.com>
> > > 
> > > > 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 <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.
> > > > > > 
> > > > > > Thanks for the initial review.
> > > 
> > > > > 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,
> > > 
> > > No worries, feel very welcome to deal with handling the
> > > actual fix.  Also, you're better prepared than me, when it
> > > comes to dealing with any possible fallout. :)
> > > 
> > > I'll send an updated version of the test-cases, moving them
> > > to the C++14 test directory (cpp1y, right?) and qualify them
> > > as c++14 instead, as Jason pointed out.
> > 
> > Right, cpp1y is c++14.  Note that we wouldn't push the tests separately
> > to the patch itself, unless it's something that already works.  Thanks,
> > 
> > Marek
> 
> But, the tests work.  They passes as-is, as they track the
> ICE, but will XPASS (that part) once a fix is committed (at
> which time: "I checked that these behave as expected (xfail
> as ICE properly) without the previosly posted patch to
> cp/constexpr.cc and XPASS with it applied."

I'm confused; are you planning to use the dg-ice directive I invented
some years ago?  I wanted to encourage people to add tests for
old unfixed PRs so that if a fix fixes an (un)related older problem, we
know it before pushing the patch.
(I don't think an XFAIL will work for an ICE -- that prompted dg-ice.)
 
> Once the fix works, the xfail for the ICE should be removed.
> (Hm, better comment on the patches in a reply to that message. :)
> 
> The point is that for this type of bug it's useful to have a
> test-case tracking it, before a fix is committed.

I'd tend to agree but here we already have a fix, so one commit seems
better than multiple commits.  But if that's what you want to do, I
guess I'm not going to stand in your way.

Marek

> Date: Thu, 8 Feb 2024 11:22:47 -0500
> From: Marek Polacek <polacek@redhat.com>

> I'm confused; are you planning to use the dg-ice directive I invented
> some years ago?

Please, let's keep the discussion about the test-cases in
that thread.

brgds, H-P