On 10/20/20 3:33 AM, Jakub Jelinek wrote:
Hi!
For arguments passed by invisible reference, in the IL until genericization
we have the source types on the callee side and while on the caller side
we already pass references to the actual argument slot in the caller, we
undo that in cxx_bind_parameters_in_call's
if (TREE_ADDRESSABLE (type))
/* Undo convert_for_arg_passing work here. */
x = convert_from_reference (x);
This works fine most of the time, except when the type also has constexpr
destructor; in that case the destructor is invoked in the caller and thus
the unsharing we do to make sure that the callee doesn't modify caller's
values is in that case undesirable, it prevents the changes done in the
callee propagating to the caller which should see them for the constexpr
dtor evaluation.
The following patch fixes that. While it could be perhaps done for all
TREE_ADDRESSABLE types, I don't see the need to change the behavior
if there is no constexpr non-trivial dtor.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
I think this isn't enough; if bar calls foo twice, the second call will
find the value in the hash table and not change the temporary, so the
destructor will throw. I think we also need to set non_constant_args if
the argument type has a non-trivial destructor, so we don't try to
memoize the call.
Then setting arg back to orig_arg isn't needed, because we don't do the
first unsharing for the hash table.
And then I think that the second unsharing is unnecessary for an
argument in a non-memoized call, because we will have already unshared
it when making the copy in the caller.
<poke> <poke>
How does this look to you?
commit 70f568bf6a30bc57ae6bc04ed4b4ac6335f01bae
Author: Jakub Jelinek <ja...@redhat.com>
Date: Tue Oct 20 09:33:20 2020 +0200
c++: Fix constexpr dtors vs invisible ref [PR97388]
For arguments passed by invisible reference, in the IL until genericization
we have the source types on the callee side and while on the caller side
we already pass references to the actual argument slot in the caller, we
undo that in cxx_bind_parameters_in_call's
if (TREE_ADDRESSABLE (type))
/* Undo convert_for_arg_passing work here. */
x = convert_from_reference (x);
This works fine most of the time, except when the type also has constexpr
destructor; in that case the destructor is invoked in the caller and thus
the unsharing we do to make sure that the callee doesn't modify caller's
values is in that case undesirable, it prevents the changes done in the
callee propagating to the caller which should see them for the constexpr
dtor evaluation.
The following patch fixes that. While it could be perhaps done for all
TREE_ADDRESSABLE types, I don't see the need to change the behavior
if there is no constexpr non-trivial dtor.
Jason: And we need to avoid memoizing the call, because a later equivalent
call also needs to modify its argument. And we don't need to unshare
constructors when we aren't memoizing the call, because we already unshared
them when evaluating the TARGET_EXPR representing the copy-initialization of
the argument.
2020-10-20 Jakub Jelinek <ja...@redhat.com>
Jason Merrill <ja...@redhat.com>
PR c++/97388
* constexpr.c (cxx_bind_parameters_in_call): Set non_constant_args
if the parameter type has a non-trivial destructor.
(cxx_eval_call_expression): Only unshare arguments if we're
memoizing this evaluation.
* g++.dg/cpp2a/constexpr-dtor5.C: New test.
* g++.dg/cpp2a/constexpr-dtor6.C: New test.
* g++.dg/cpp2a/constexpr-dtor7.C: New test.
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 7ebdd308dcd..524ce9384cf 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -1602,6 +1602,11 @@ cxx_bind_parameters_in_call (const constexpr_ctx *ctx, tree t,
arg = adjust_temp_type (type, arg);
if (!TREE_CONSTANT (arg))
*non_constant_args = true;
+ else if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (type))
+ /* The destructor needs to see any modifications the callee makes
+ to the argument. */
+ *non_constant_args = true;
+
/* For virtual calls, adjust the this argument, so that it is
the object on which the method is called, rather than
one of its bases. */
@@ -2586,14 +2591,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
problems with verify_gimple. */
arg = unshare_expr_without_location (arg);
TREE_VEC_ELT (bound, i) = arg;
+
+ /* And then unshare again so the callee doesn't change the
+ argument values in the hash table. XXX Could we unshare
+ lazily in cxx_eval_store_expression? */
+ arg = unshare_constructor (arg);
+ if (TREE_CODE (arg) == CONSTRUCTOR)
+ vec_safe_push (ctors, arg);
}
- /* Don't share a CONSTRUCTOR that might be changed. This is not
- redundant with the unshare just above; we also don't want to
- change the argument values in the hash table. XXX Could we
- unshare lazily in cxx_eval_store_expression? */
- arg = unshare_constructor (arg);
- if (TREE_CODE (arg) == CONSTRUCTOR)
- vec_safe_push (ctors, arg);
ctx->global->values.put (remapped, arg);
remapped = DECL_CHAIN (remapped);
}
diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor5.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor5.C
new file mode 100644
index 00000000000..1739afbcc68
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor5.C
@@ -0,0 +1,35 @@
+// PR c++/97388
+// { dg-do compile { target c++20 } }
+
+struct S {
+ int m;
+ constexpr S () : m(1) {}
+ constexpr ~S () noexcept (false) { if (m == 1) { throw; } }
+};
+
+constexpr bool
+foo (S v)
+{
+ v.m = 2;
+ return true;
+}
+
+constexpr bool
+bar ()
+{
+ return foo (S ());
+}
+
+constexpr bool
+baz ()
+{
+ foo (S ());
+ return foo (S ());
+}
+
+static_assert (foo (S ()));
+static_assert (bar ());
+static_assert (baz ());
+constexpr bool x = foo (S ());
+constexpr bool y = bar ();
+constexpr bool z = baz ();
diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor6.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor6.C
new file mode 100644
index 00000000000..ce783a1df88
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor6.C
@@ -0,0 +1,36 @@
+// PR c++/97388
+// { dg-do compile { target c++20 } }
+
+struct S {
+ int *s;
+ constexpr S () : s(new int ()) {}
+ constexpr S (S &&x) noexcept : s(x.s) { x.s = nullptr; }
+ constexpr ~S () noexcept { delete s; }
+};
+
+constexpr bool
+foo (S v)
+{
+ auto x = static_cast<S &&> (v);
+ return true;
+}
+
+constexpr bool
+bar ()
+{
+ return foo (S ());
+}
+
+constexpr bool
+baz ()
+{
+ foo (S ());
+ return foo (S ());
+}
+
+static_assert (foo (S ()));
+static_assert (bar ());
+static_assert (baz ());
+constexpr bool x = foo (S ());
+constexpr bool y = bar ();
+constexpr bool z = baz ();
diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor7.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor7.C
new file mode 100644
index 00000000000..463eaca0539
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor7.C
@@ -0,0 +1,19 @@
+// PR c++/97388
+// { dg-do compile { target c++20 } }
+
+struct S {
+ int *s;
+ constexpr S () : s(new int) {} // { dg-error "is not a constant expression because allocated storage has not been deallocated" }
+ S (const S &) = delete;
+ S &operator= (const S &) = delete;
+ constexpr ~S () { delete s; }
+};
+
+constexpr bool
+foo (S v)
+{
+ v.s = nullptr;
+ return true;
+}
+
+static_assert (foo (S ())); // { dg-error "non-constant condition for static assertion" }
commit 849174d03e4dbdd88c945e7e33e2fddc78260706
Author: Jason Merrill <ja...@redhat.com>
Date: Wed Oct 28 17:30:05 2020 -0400
c++: Fix constexpr cleanup error handling.
In this testcase, the primary evaluation successfully produces 'true', and
then running one of the cleanups hits a double delete, making the whole
thing not a valid constant expression. So we were returning 'true' wrapped
in a NOP_EXPR to indicate its non-constancy, but evaluating that again is a
perfectly acceptable constant expression, so we weren't getting the verbose
diagnostic we were looking for.
So if non_constant_p gets set other than for overflow, go back to the
original expression.
With this change, we should never hit the manifestly_const_eval test, and
the is-constant-evaluated1.C test passes without it.
gcc/cp/ChangeLog:
PR c++/97388
* constexpr.c (cxx_eval_outermost_constant_expr): Revert to
original expression if evaluation sets non_constant_p.
gcc/testsuite/ChangeLog:
PR c++/97388
* g++.dg/cpp2a/constexpr-dtor8.C: New test.
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 524ce9384cf..cc225bbe156 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -6877,6 +6877,10 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant,
non_constant_p = true;
}
+ if (non_constant_p)
+ /* If we saw something bad, go back to our argument. The wrapping below is
+ only for the cases of TREE_CONSTANT argument or overflow. */
+ r = t;
if (!non_constant_p && overflow_p)
non_constant_p = true;
@@ -6893,12 +6897,6 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant,
return r;
else if (non_constant_p && TREE_CONSTANT (r))
{
- /* If __builtin_is_constant_evaluated () was evaluated to true
- and the result is not a valid constant expression, we need to
- punt. */
- if (manifestly_const_eval)
- return cxx_eval_outermost_constant_expr (t, true, strict,
- false, false, object);
/* This isn't actually constant, so unset TREE_CONSTANT.
Don't clear TREE_CONSTANT on ADDR_EXPR, as the middle-end requires
it to be set if it is invariant address, even when it is not
diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor8.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor8.C
new file mode 100644
index 00000000000..3048110ede3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-dtor8.C
@@ -0,0 +1,19 @@
+// PR c++/97388
+// { dg-do compile { target c++20 } }
+
+struct S {
+ int *s;
+ constexpr S () : s(new int) {}
+ S (const S &) = delete;
+ S &operator= (const S &) = delete;
+ constexpr ~S () { delete s; } // { dg-error "already deallocated" }
+};
+
+constexpr bool
+foo (S v)
+{
+ delete v.s;
+ return true;
+}
+
+static_assert (foo (S ())); // { dg-error "non-constant condition for static assertion" }
