| Message ID | 20201020073320.GT2176@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Fix up constexpr evaluation of arguments passed by invisible reference [PR97388] | expand |
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?
On Thu, Oct 29, 2020 at 11:09:05AM -0400, Jason Merrill wrote: > 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. For the testcases with constexpr new in there it wouldn't be memoized already, but you are right that for other cases making it non_constant_args is desirable. > Then setting arg back to orig_arg isn't needed, because we don't do the > first unsharing for the hash table. Yes. > 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. I'm not sure about this one, but if it works, I'm not against that, the less unsharing the better for compile time memory unless it breaks stuff. I'll bootstrap/regtest your patchset (or do you want to do that)? Jakub
On 10/29/20 11:40 AM, Jakub Jelinek wrote: > On Thu, Oct 29, 2020 at 11:09:05AM -0400, Jason Merrill wrote: >> 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. > > For the testcases with constexpr new in there it wouldn't be memoized > already, but you are right that for other cases making it non_constant_args > is desirable. > >> Then setting arg back to orig_arg isn't needed, because we don't do the >> first unsharing for the hash table. > > Yes. > >> 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. > > I'm not sure about this one, but if it works, I'm not against that, the less > unsharing the better for compile time memory unless it breaks stuff. > > I'll bootstrap/regtest your patchset (or do you want to do that)? I already did, thanks. Jason
--- gcc/cp/constexpr.c.jj 2020-10-19 09:32:28.000000000 +0200 +++ gcc/cp/constexpr.c 2020-10-19 15:34:17.707431525 +0200 @@ -2579,6 +2579,7 @@ cxx_eval_call_expression (const constexp for (int i = 0; i < TREE_VEC_LENGTH (bound); ++i) { tree arg = TREE_VEC_ELT (bound, i); + tree orig_arg = arg; if (entry) { /* Unshare args going into the hash table to separate them @@ -2587,13 +2588,24 @@ cxx_eval_call_expression (const constexp arg = unshare_expr_without_location (arg); TREE_VEC_ELT (bound, i) = 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); + /* For arguments passed by invisible reference, if they have + non-trivial dtors, use the original argument without any + unsharing, because changes in the function call should be + reflected in the caller. */ + if (TREE_ADDRESSABLE (TREE_TYPE (arg)) + && TYPE_HAS_NONTRIVIAL_DESTRUCTOR (TREE_TYPE (arg))) + arg = orig_arg; + else + { + /* 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); } --- gcc/testsuite/g++.dg/cpp2a/constexpr-dtor5.C.jj 2020-10-19 15:48:19.460200816 +0200 +++ gcc/testsuite/g++.dg/cpp2a/constexpr-dtor5.C 2020-10-19 15:47:47.715662068 +0200 @@ -0,0 +1,26 @@ +// 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 ()); +} + +static_assert (foo (S ())); +static_assert (bar ()); +constexpr bool x = foo (S ()); +constexpr bool y = bar (); --- gcc/testsuite/g++.dg/cpp2a/constexpr-dtor6.C.jj 2020-10-19 15:48:22.475157012 +0200 +++ gcc/testsuite/g++.dg/cpp2a/constexpr-dtor6.C 2020-10-19 15:47:56.204538722 +0200 @@ -0,0 +1,27 @@ +// 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 ()); +} + +static_assert (foo (S ())); +static_assert (bar ()); +constexpr bool x = foo (S ()); +constexpr bool y = bar (); --- gcc/testsuite/g++.dg/cpp2a/constexpr-dtor7.C.jj 2020-10-19 15:48:25.316115731 +0200 +++ gcc/testsuite/g++.dg/cpp2a/constexpr-dtor7.C 2020-10-19 15:48:00.907470388 +0200 @@ -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" }
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? 2020-10-20 Jakub Jelinek <jakub@redhat.com> PR c++/97388 * constexpr.c (cxx_eval_call_expression): Don't unshare arguments if they have TREE_ADDRESSABLE type with non-trivial dtor. * g++.dg/cpp2a/constexpr-dtor5.C: New test. * g++.dg/cpp2a/constexpr-dtor6.C: New test. * g++.dg/cpp2a/constexpr-dtor7.C: New test. Jakub