From: Patrick Palka <ppalka@redhat.com>
To: Patrick Palka <ppalka@redhat.com>
Cc: gcc-patches@gcc.gnu.org, jason@redhat.com
Subject: Re: [PATCH] c++: constantness of local var in constexpr fn [PR111703, PR112269]
Date: Fri, 10 Nov 2023 09:49:26 -0500 (EST) [thread overview]
Message-ID: <9bf57f95-1810-b16c-5ec9-d56552ea4232@idea> (raw)
In-Reply-To: <805a8e2e-7944-c8f3-164f-a47dfe4de9bc@idea>
On Wed, 1 Nov 2023, Patrick Palka wrote:
> On Tue, 31 Oct 2023, Patrick Palka wrote:
>
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk? Does it look OK for release branches as well for sake of PR111703?
Ping.
> >
> > -- >8 --
> >
> > potential_constant_expression was incorrectly treating most local
> > variables from a constexpr function as (potentially) constant because it
> > wasn't considering the 'now' parameter. This patch fixes this by
> > relaxing some var_in_maybe_constexpr_fn checks accordingly, which turns
> > out to partially fix two recently reported regressions:
> >
> > PR111703 is a regression caused by r11-550-gf65a3299a521a4 for
> > restricting constexpr evaluation during warning-dependent folding.
> > The mechanism is intended to restrict only constant evaluation of the
> > instantiated non-dependent expression, but it also ends up restricting
> > constant evaluation (as part of satisfaction) during instantiation of
> > the expression, in particular when resolving the ck_rvalue conversion of
> > the 'x' argument into a copy constructor call.
>
> Oops, this analysis is inaccurate for this specific testcase (although
> the general idea is the same)... We don't call fold_for_warn on 'f(x)'
> but rather on its 'x' argument that has been processed by
> convert_arguments into an IMPLICIT_CONV_EXPR. And it's the
> instantiation of this IMPLICIT_CONV_EXPR that turns it into a copy
> constructor call. There is no ck_rvalue conversion at all here since
> 'f' is a function pointer, not an actual function, and so ICSes don't
> get computed (IIUC). If 'f' is changed to be an actual function then
> there's no issue since build_over_call doesn't perform argument
> conversions when in a template context and therefore doesn't call
> check_function_arguments on the converted arguments (from which the
> problematic fold_for_warn call occurs).
>
> > This seems like a bug in
> > the mechanism[1], though I don't know if we want to refine the mechanism
> > or get rid of it completely since the original testcases which motivated
> > the mechanism are fixed more simply by r13-1225-gb00b95198e6720. In any
> > case, this patch partially fixes this by making us correctly treat 'x'
> > and therefore 'f(x)' in the below testcase as non-constant, which
> > prevents the problematic warning-dependent folding from occurring at
> > all. If this bug crops up again then I figure we could decide what to
> > do with the mechanism then.
> >
> > PR112269 is caused by r14-4796-g3e3d73ed5e85e7 for merging tsubst_copy
> > into tsubst_copy_and_build. tsubst_copy used to exit early when 'args'
> > was empty, behavior which that commit deliberately didn't preserve.
> > This early exit masked the fact that COMPLEX_EXPR wasn't handled by
> > tsubst at all, and is a tree code that apparently we could see during
> > warning-dependent folding on some targets. A complete fix is to add
> > handling for this tree code in tsubst_expr, but this patch should fix
> > the reported testsuite failures since the situations where COMPLEX_EXPR
> > crops up in <complex> turn out to not be constant expressions in the
> > first place after this patch.
N.B. adding COMPLEX_EXPR handling to tsubst_expr is complicated by the
fact that these COMPLEX_EXRRs are created by convert_to_complex (a
middle-end routine) which occasionally creates SAVE_EXPR sub trees which
we don't expect to see inside templated trees...
> >
> > [1]: The mechanism incorrectly assumes that instantiation of the
> > non-dependent expression shouldn't induce any template instantiation
> > since ahead of time checking of the expression should've already induced
> > whatever template instantiation was needed, but in this case although
> > overload resolution was performed ahead of time, a ck_rvalue conversion
> > gets resolved to a copy constructor call only at instantiation time.
> >
> > PR c++/111703
> >
> > gcc/cp/ChangeLog:
> >
> > * constexpr.cc (potential_constant_expression_1) <case VAR_DECL>:
> > Only consider var_in_maybe_constexpr_fn if 'now' is false.
> > <case INDIRECT_REF>: Likewise.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/cpp2a/concepts-fn8.C: New test.
> > ---
> > gcc/cp/constexpr.cc | 4 ++--
> > gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C | 24 +++++++++++++++++++++++
> > 2 files changed, 26 insertions(+), 2 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
> >
> > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> > index c05760e6789..8a6b210144a 100644
> > --- a/gcc/cp/constexpr.cc
> > +++ b/gcc/cp/constexpr.cc
> > @@ -9623,7 +9623,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
> > return RECUR (DECL_VALUE_EXPR (t), rval);
> > }
> > if (want_rval
> > - && !var_in_maybe_constexpr_fn (t)
> > + && (now || !var_in_maybe_constexpr_fn (t))
> > && !type_dependent_expression_p (t)
> > && !decl_maybe_constant_var_p (t)
> > && (strict
> > @@ -9737,7 +9737,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
> > STRIP_NOPS (x);
> > if (is_this_parameter (x) && !is_capture_proxy (x))
> > {
> > - if (!var_in_maybe_constexpr_fn (x))
> > + if (now || !var_in_maybe_constexpr_fn (x))
> > {
> > if (flags & tf_error)
> > constexpr_error (loc, fundef_p, "use of %<this%> in a "
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C b/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
> > new file mode 100644
> > index 00000000000..3f63a5b28d7
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
> > @@ -0,0 +1,24 @@
> > +// PR c++/111703
> > +// { dg-do compile { target c++20 } }
> > +
> > +template<class T>
> > +constexpr bool always_true() { return true; }
> > +
> > +struct P {
> > + P() = default;
> > +
> > + template<class T>
> > + requires (always_true<T>()) // { dg-bogus "used before its definition" }
> > + constexpr P(const T&) { }
> > +
> > + int n, m;
> > +};
> > +
> > +void (*f)(P);
> > +
> > +template<class T>
> > +constexpr bool g() {
> > + P x;
> > + f(x); // { dg-bogus "from here" }
> > + return true;
> > +}
> > --
> > 2.42.0.526.g3130c155df
> >
> >
>
next prev parent reply other threads:[~2023-11-10 14:49 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-31 18:17 Patrick Palka
2023-11-01 15:07 ` Patrick Palka
2023-11-10 14:49 ` Patrick Palka [this message]
2023-11-14 22:57 ` 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=9bf57f95-1810-b16c-5ec9-d56552ea4232@idea \
--to=ppalka@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--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).