From: Jason Merrill <jason@redhat.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: Fix constexpr evaluation of parameters passed by invisible reference [PR111284]
Date: Fri, 12 Apr 2024 15:05:26 -0400 [thread overview]
Message-ID: <a8965274-5086-40b4-95f3-4b32f8e73d0f@redhat.com> (raw)
In-Reply-To: <ZerSsh8z8fqSZDX6@tucnak>
On 3/8/24 03:56, Jakub Jelinek wrote:
> Hi!
>
> My r9-6136 changes to make a copy of constexpr function bodies before
> genericization modifies it broke the constant evaluation of non-POD
> arguments passed by value.
> In the callers such arguments are passed as reference to usually a
> TARGET_EXPR, but on the callee side until genericization they are just
> direct uses of a PARM_DECL with some class type.
> In cxx_bind_parameters_in_call I've used convert_from_reference to
> pretend it is passed by value and then cxx_eval_constant_expression
> is called there and evaluates that as an rvalue, followed by
> adjust_temp_type if the types don't match exactly (e.g. const Foo
> argument and passing to it reference to Foo TARGET_EXPR).
>
> The reason this doesn't work is that when the TARGET_EXPR in the caller
> is constant initialized, this for it is the address of the TARGET_EXPR_SLOT,
> but if the code later on pretends the PARM_DECL is just initialized to the
> rvalue of the constant evaluation of the TARGET_EXPR, it is as if there
> is a bitwise copy of the TARGET_EXPR to the callee, so this in the callee
> is then address of the PARM_DECL in the callee.
>
> The following patch attempts to fix that by constexpr evaluation of such
> arguments in the caller as an lvalue instead of rvalue, and on the callee
> side when seeing such a PARM_DECL, if we want an lvalue, lookup the value
> (lvalue) saved in ctx->globals (if any), and if wanting an rvalue,
> recursing with vc_prvalue on the looked up value (because it is there
> as an lvalue, nor rvalue).
>
> adjust_temp_type doesn't work for lvalues of non-scalarish types, for
> such types it relies on changing the type of a CONSTRUCTOR, but on the
> other side we know what we pass to the argument is addressable, so
> the patch on type mismatch takes address of the argument value, casts
> to reference to the desired type and dereferences it.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2024-03-08 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/111284
> * constexpr.cc (cxx_bind_parameters_in_call): For PARM_DECLs with
> TREE_ADDRESSABLE types use vc_glvalue rather than vc_prvalue for
> cxx_eval_constant_expression and if it doesn't have the same
> type as it should, cast the reference type to reference to type
> before convert_from_reference and instead of adjust_temp_type
> take address of the arg, cast to reference to type and then
> convert_from_reference.
> (cxx_eval_constant_expression) <case PARM_DECL>: For lval case
> on parameters with TREE_ADDRESSABLE types lookup result in
> ctx->globals if possible. Otherwise if lookup in ctx->globals
> was successful for parameter with TREE_ADDRESSABLE type,
> recurse with vc_prvalue on the returned value.
>
> * g++.dg/cpp1z/constexpr-111284.C: New test.
> * g++.dg/cpp1y/constexpr-lifetime7.C: Expect one error on a different
> line.
>
> --- gcc/cp/constexpr.cc.jj 2024-02-13 10:29:57.979155641 +0100
> +++ gcc/cp/constexpr.cc 2024-03-07 19:35:01.032412221 +0100
> @@ -1877,13 +1877,21 @@ cxx_bind_parameters_in_call (const const
> x = build_address (x);
> }
> if (TREE_ADDRESSABLE (type))
> - /* Undo convert_for_arg_passing work here. */
> - x = convert_from_reference (x);
> - /* Normally we would strip a TARGET_EXPR in an initialization context
> - such as this, but here we do the elision differently: we keep the
> - TARGET_EXPR, and use its CONSTRUCTOR as the value of the parm. */
> - arg = cxx_eval_constant_expression (ctx, x, vc_prvalue,
> - non_constant_p, overflow_p);
> + {
> + /* Undo convert_for_arg_passing work here. */
> + if (TYPE_REF_P (TREE_TYPE (x))
> + && !same_type_p (type, TREE_TYPE (TREE_TYPE (x))))
> + x = cp_fold_convert (build_reference_type (type), x);
> + x = convert_from_reference (x);
> + arg = cxx_eval_constant_expression (ctx, x, vc_glvalue,
> + non_constant_p, overflow_p);
> + }
> + else
> + /* Normally we would strip a TARGET_EXPR in an initialization context
> + such as this, but here we do the elision differently: we keep the
> + TARGET_EXPR, and use its CONSTRUCTOR as the value of the parm. */
> + arg = cxx_eval_constant_expression (ctx, x, vc_prvalue,
> + non_constant_p, overflow_p);
It seems simpler to move the convert_for_reference after the
cxx_eval_constant_expression rather than duplicate the call to
cxx_eval_constant_expression.
> /* Check we aren't dereferencing a null pointer when calling a non-static
> member function, which is undefined behaviour. */
> if (i == 0 && DECL_OBJECT_MEMBER_FUNCTION_P (fun)
> @@ -1909,7 +1917,16 @@ cxx_bind_parameters_in_call (const const
> {
> /* Make sure the binding has the same type as the parm. But
> only for constant args. */
> - if (!TYPE_REF_P (type))
> + if (TREE_ADDRESSABLE (type))
> + {
> + if (!same_type_p (type, TREE_TYPE (arg)))
> + {
> + arg = build_fold_addr_expr (arg);
> + arg = cp_fold_convert (build_reference_type (type), arg);
> + arg = convert_from_reference (arg);
> + }
> + }
It shouldn't be necessary to convert both here and above? In fact, the
testcase still passes with neither conversion, just moving the
convert_for_reference and the change below.
> + else if (!TYPE_REF_P (type))
> arg = adjust_temp_type (type, arg);
> if (!TREE_CONSTANT (arg))
> *non_constant_args = true;
> @@ -7499,9 +7516,19 @@ cxx_eval_constant_expression (const cons
>
> case PARM_DECL:
> if (lval && !TYPE_REF_P (TREE_TYPE (t)))
> - /* glvalue use. */;
> + {
> + /* glvalue use. */
> + if (TREE_ADDRESSABLE (TREE_TYPE (t)))
> + if (tree v = ctx->global->get_value (t))
> + r = v;
> + }
> else if (tree v = ctx->global->get_value (t))
> - r = v;
> + {
> + r = v;
> + if (TREE_ADDRESSABLE (TREE_TYPE (t)))
> + r = cxx_eval_constant_expression (ctx, r, vc_prvalue,
> + non_constant_p, overflow_p);
> + }
> else if (lval)
> /* Defer in case this is only used for its type. */;
> else if (ctx->global->is_outside_lifetime (t))
> --- gcc/testsuite/g++.dg/cpp1z/constexpr-111284.C.jj 2024-03-07 16:27:48.113651999 +0100
> +++ gcc/testsuite/g++.dg/cpp1z/constexpr-111284.C 2024-03-07 16:26:49.565466606 +0100
> @@ -0,0 +1,19 @@
> +// PR c++/111284
> +// { dg-do compile { target c++17 } }
> +
> +struct S {
> + S () = default;
> + constexpr S (const S &) noexcept : s{this} {}
> + constexpr S & operator= (const S &) noexcept { return *this; }
> + constexpr bool foo () const noexcept { return s == this; }
> + S *s = this;
> +};
> +
> +constexpr bool
> +bar (S x) noexcept
> +{
> + return x.foo ();
> +}
> +
> +static_assert (bar (S {}), "");
> +static_assert ([] (S x) { return x.foo (); } (S {}), "");
> --- gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime7.C.jj 2023-12-13 19:09:33.252657826 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime7.C 2024-03-07 19:49:49.342334090 +0100
> @@ -87,7 +87,7 @@ constexpr bool n1 = test_access<NonTrivi
> constexpr bool n2 = test_modification<NonTrivial>(); // { dg-message "in .constexpr." "" { target c++20 } }
> constexpr bool n3 = test_scope<NonTrivial>(); // { dg-message "in .constexpr." "" { target c++20 } }
> constexpr bool n4 = test_destroy_temp<NonTrivial>(); // { dg-message "in .constexpr." "" { target c++20 } }
> -constexpr bool n5 = test_parameter(NonTrivial{}); // { dg-error "destroying" "" { target c++20 } }
> +constexpr bool n5 = test_parameter(NonTrivial{}); // { dg-message "in .constexpr." "" { target c++20 } }
> constexpr bool n6 = test_bindings<NonTrivial>();
> #endif
>
>
> Jakub
>
next prev parent reply other threads:[~2024-04-12 19:05 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-08 8:56 Jakub Jelinek
2024-03-25 18:27 ` C++ Patch ping Jakub Jelinek
2024-04-12 19:05 ` Jason Merrill [this message]
2024-04-15 12:19 ` [PATCH] c++: Fix constexpr evaluation of parameters passed by invisible reference [PR111284] Jakub Jelinek
2024-04-23 15:52 ` [PATCH] c++, v2: " Jakub Jelinek
2024-04-25 18:38 ` 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=a8965274-5086-40b4-95f3-4b32f8e73d0f@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@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).