From: Marek Polacek <polacek@redhat.com>
To: Hans-Peter Nilsson <hp@axis.com>
Cc: jason@redhat.com, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: Don't ICE for unknown parameter to constexpr'd switch-statement, PR113545
Date: Thu, 8 Feb 2024 11:22:47 -0500 [thread overview]
Message-ID: <ZcT_10mppBpiHrEZ@redhat.com> (raw)
In-Reply-To: <20240208160721.F12F820415@pchp3.se.axis.com>
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
next prev parent reply other threads:[~2024-02-08 16:22 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-22 17:02 Hans-Peter Nilsson
2024-01-22 19:33 ` Marek Polacek
2024-01-23 0:55 ` Hans-Peter Nilsson
2024-01-23 2:23 ` Hans-Peter Nilsson
2024-01-23 4:55 ` Hans-Peter Nilsson
2024-01-30 5:18 ` Ping PATCH: testcase for "ICE for unknown parameter to constexpr'd switch-statement, PR113545" Hans-Peter Nilsson
2024-02-07 0:04 ` Ping*2 " Hans-Peter Nilsson
2024-02-07 0:23 ` [PATCH] c++: Don't ICE for unknown parameter to constexpr'd switch-statement, PR113545 Hans-Peter Nilsson
2024-02-07 21:32 ` Jason Merrill
2024-02-08 2:11 ` Marek Polacek
2024-02-08 15:40 ` Hans-Peter Nilsson
2024-02-08 15:44 ` Marek Polacek
2024-02-08 16:07 ` Hans-Peter Nilsson
2024-02-08 16:22 ` Marek Polacek [this message]
2024-02-08 16:42 ` Hans-Peter Nilsson
2024-02-09 4:02 ` [PATCH v2]: testcases for "ICE for unknown parameter to constexpr'd switch-statement, PR113545" Hans-Peter Nilsson
2024-02-09 15:30 ` [PATCH v3]: " Hans-Peter Nilsson
2024-02-09 16:02 ` [PATCH v4]: " Hans-Peter Nilsson
2024-02-09 18:22 ` Jason Merrill
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZcT_10mppBpiHrEZ@redhat.com \
--to=polacek@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hp@axis.com \
--cc=jason@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).