c++: Don't ICE for unknown parameter to constexpr'd switch-statement, PR113545
Checks
Commit Message
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
Comments
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
@@ -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;
}
new file mode 100644
@@ -0,0 +1,49 @@
+// { 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));
+ xyzzy(d);
+}