| Message ID | 20200408234918.3196090-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Stray RESULT_DECLs in result of constexpr function call [PR94034] | expand |
On 4/8/20 7:49 PM, Patrick Palka wrote: > When evaluating the initializer of 'a' in the following example > > struct A { A *p = this; }; > constexpr A foo() { return {}; } > constexpr A a = foo(); > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo > gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the 'this' > should really be resolved to '&a'. > It seems to me that the right approach would be to immediately resolve the > PLACEHOLDER_EXPR to the correct target object during evaluation of 'foo()', so > that we could use 'this' to access objects adjacent to the current object in the > ultimate storage location. (I think #c2 of PR c++/94537 is an example of such > usage of 'this', which currently doesn't work. But as #c1 shows we don't seem > to handle this case correctly in non-constexpr initialization either.) As I commented in the PR, the standard doesn't require this to work because A is trivially copyable, and our ABI makes it impossible. But there's still a constexpr bug when we add A() = default; A(const A&); clang doesn't require the constructors to make this work for constant initialization, but similarly can't make it work for non-constant initialization. > I haven't yet been able to make a solution using the above approach work -- > making sure we use the ultimate object instead of the RESULT_DECL whenever we > access ctx->global->values is proving to be tricky and subtle. Do we need to go through ctx->global->values? Would it work for the RESULT_DECL case in cxx_eval_constant_expression to go to straight to ctx->object or ctx->ctor instead? Jason
On Thu, 9 Apr 2020, Jason Merrill wrote: > On 4/8/20 7:49 PM, Patrick Palka wrote: > > When evaluating the initializer of 'a' in the following example > > > > struct A { A *p = this; }; > > constexpr A foo() { return {}; } > > constexpr A a = foo(); > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo > > gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the > > 'this' > > should really be resolved to '&a'. > > > It seems to me that the right approach would be to immediately resolve the > > PLACEHOLDER_EXPR to the correct target object during evaluation of 'foo()', > > so > > that we could use 'this' to access objects adjacent to the current object in > > the > > ultimate storage location. (I think #c2 of PR c++/94537 is an example of > > such > > usage of 'this', which currently doesn't work. But as #c1 shows we don't > > seem > > to handle this case correctly in non-constexpr initialization either.) > > As I commented in the PR, the standard doesn't require this to work because A > is trivially copyable, and our ABI makes it impossible. But there's still a > constexpr bug when we add > > A() = default; A(const A&); > > clang doesn't require the constructors to make this work for constant > initialization, but similarly can't make it work for non-constant > initialization. That makes a lot of sense, thanks for the detailed explanation. > > > I haven't yet been able to make a solution using the above approach work -- > > making sure we use the ultimate object instead of the RESULT_DECL whenever > > we > > access ctx->global->values is proving to be tricky and subtle. > > Do we need to go through ctx->global->values? Would it work for the > RESULT_DECL case in cxx_eval_constant_expression to go to straight to > ctx->object or ctx->ctor instead? I attempted that at some point, but IIRC we still end up not resolving some RESULT_DECLs because not all of them get processed through cxx_eval_constant_expression before we manipulate ctx->global->values with them. I'll try this approach more carefully and report back with more specifics.
On Thu, 9 Apr 2020, Patrick Palka wrote: > On Thu, 9 Apr 2020, Jason Merrill wrote: > > > On 4/8/20 7:49 PM, Patrick Palka wrote: > > > When evaluating the initializer of 'a' in the following example > > > > > > struct A { A *p = this; }; > > > constexpr A foo() { return {}; } > > > constexpr A a = foo(); > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo > > > gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the > > > 'this' > > > should really be resolved to '&a'. > > > > > It seems to me that the right approach would be to immediately resolve the > > > PLACEHOLDER_EXPR to the correct target object during evaluation of 'foo()', > > > so > > > that we could use 'this' to access objects adjacent to the current object in > > > the > > > ultimate storage location. (I think #c2 of PR c++/94537 is an example of > > > such > > > usage of 'this', which currently doesn't work. But as #c1 shows we don't > > > seem > > > to handle this case correctly in non-constexpr initialization either.) > > > > As I commented in the PR, the standard doesn't require this to work because A > > is trivially copyable, and our ABI makes it impossible. But there's still a > > constexpr bug when we add > > > > A() = default; A(const A&); > > > > clang doesn't require the constructors to make this work for constant > > initialization, but similarly can't make it work for non-constant > > initialization. > > That makes a lot of sense, thanks for the detailed explanation. > > > > > > I haven't yet been able to make a solution using the above approach work -- > > > making sure we use the ultimate object instead of the RESULT_DECL whenever > > > we > > > access ctx->global->values is proving to be tricky and subtle. > > > > Do we need to go through ctx->global->values? Would it work for the > > RESULT_DECL case in cxx_eval_constant_expression to go to straight to > > ctx->object or ctx->ctor instead? > > I attempted that at some point, but IIRC we still end up not resolving > some RESULT_DECLs because not all of them get processed through > cxx_eval_constant_expression before we manipulate ctx->global->values > with them. I'll try this approach more carefully and report back with > more specifics. It turns out that immediately resolving RESULT_DECLs/'this' to the ultimate ctx->object would not interact well with constexpr_call caching: struct A { A() = default; A(const A&); A *ap = this; }; constexpr A foo() { return {}; } constexpr A a = foo(); constexpr A b = foo(); static_assert(b.ap == &b); // fails Evaluation of the first call to foo() returns {&a}, since we resolve 'this' to &a due to guaranteed RVO, and we cache this result. Evaluation of the second call to foo() just returns the cached result from the constexpr_call cache, and so we get {&a} again. So it seems we would have to mark the result of a constexpr call as not cacheable whenever this RVO applies during its evaluation, even when doing the RVO has no observable difference in the final result (i.e. the constructor does not try to save the 'this' pointer). Would the performance impact of disabling caching whenever RVO applies during constexpr call evaluation be worth it, or should we go with something like my first patch which "almost works," and which marks a constexpr call as not cacheable only when we replace a RESULT_DECL in the result of the call?
On 4/10/20 1:04 PM, Patrick Palka wrote: > On Thu, 9 Apr 2020, Patrick Palka wrote: >> On Thu, 9 Apr 2020, Jason Merrill wrote: >> >>> On 4/8/20 7:49 PM, Patrick Palka wrote: >>>> When evaluating the initializer of 'a' in the following example >>>> >>>> struct A { A *p = this; }; >>>> constexpr A foo() { return {}; } >>>> constexpr A a = foo(); >>>> >>>> the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo >>>> gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the >>>> 'this' >>>> should really be resolved to '&a'. >>> >>>> It seems to me that the right approach would be to immediately resolve the >>>> PLACEHOLDER_EXPR to the correct target object during evaluation of 'foo()', >>>> so >>>> that we could use 'this' to access objects adjacent to the current object in >>>> the >>>> ultimate storage location. (I think #c2 of PR c++/94537 is an example of >>>> such >>>> usage of 'this', which currently doesn't work. But as #c1 shows we don't >>>> seem >>>> to handle this case correctly in non-constexpr initialization either.) >>> >>> As I commented in the PR, the standard doesn't require this to work because A >>> is trivially copyable, and our ABI makes it impossible. But there's still a >>> constexpr bug when we add >>> >>> A() = default; A(const A&); >>> >>> clang doesn't require the constructors to make this work for constant >>> initialization, but similarly can't make it work for non-constant >>> initialization. >> >> That makes a lot of sense, thanks for the detailed explanation. >> >>> >>>> I haven't yet been able to make a solution using the above approach work -- >>>> making sure we use the ultimate object instead of the RESULT_DECL whenever >>>> we >>>> access ctx->global->values is proving to be tricky and subtle. >>> >>> Do we need to go through ctx->global->values? Would it work for the >>> RESULT_DECL case in cxx_eval_constant_expression to go to straight to >>> ctx->object or ctx->ctor instead? >> >> I attempted that at some point, but IIRC we still end up not resolving >> some RESULT_DECLs because not all of them get processed through >> cxx_eval_constant_expression before we manipulate ctx->global->values >> with them. I'll try this approach more carefully and report back with >> more specifics. > > It turns out that immediately resolving RESULT_DECLs/'this' to the > ultimate ctx->object would not interact well with constexpr_call > caching: > > struct A { A() = default; A(const A&); A *ap = this; }; > constexpr A foo() { return {}; } > constexpr A a = foo(); > constexpr A b = foo(); > static_assert(b.ap == &b); // fails > > Evaluation of the first call to foo() returns {&a}, since we resolve > 'this' to &a due to guaranteed RVO, and we cache this result. > Evaluation of the second call to foo() just returns the cached result > from the constexpr_call cache, and so we get {&a} again. > > So it seems we would have to mark the result of a constexpr call as not > cacheable whenever this RVO applies during its evaluation, even when > doing the RVO has no observable difference in the final result (i.e. the > constructor does not try to save the 'this' pointer). > > Would the performance impact of disabling caching whenever RVO applies > during constexpr call evaluation be worth it, or should we go with > something like my first patch which "almost works," and which marks a > constexpr call as not cacheable only when we replace a RESULT_DECL in > the result of the call? Could we search through the result of the call for ctx->object and cache if we don't find it? Jason
On Fri, 10 Apr 2020, Jason Merrill wrote: > On 4/10/20 1:04 PM, Patrick Palka wrote: > > On Thu, 9 Apr 2020, Patrick Palka wrote: > > > On Thu, 9 Apr 2020, Jason Merrill wrote: > > > > > > > On 4/8/20 7:49 PM, Patrick Palka wrote: > > > > > When evaluating the initializer of 'a' in the following example > > > > > > > > > > struct A { A *p = this; }; > > > > > constexpr A foo() { return {}; } > > > > > constexpr A a = foo(); > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned > > > > > by foo > > > > > gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, > > > > > the > > > > > 'this' > > > > > should really be resolved to '&a'. > > > > > > > > > It seems to me that the right approach would be to immediately resolve > > > > > the > > > > > PLACEHOLDER_EXPR to the correct target object during evaluation of > > > > > 'foo()', > > > > > so > > > > > that we could use 'this' to access objects adjacent to the current > > > > > object in > > > > > the > > > > > ultimate storage location. (I think #c2 of PR c++/94537 is an example > > > > > of > > > > > such > > > > > usage of 'this', which currently doesn't work. But as #c1 shows we > > > > > don't > > > > > seem > > > > > to handle this case correctly in non-constexpr initialization either.) > > > > > > > > As I commented in the PR, the standard doesn't require this to work > > > > because A > > > > is trivially copyable, and our ABI makes it impossible. But there's > > > > still a > > > > constexpr bug when we add > > > > > > > > A() = default; A(const A&); > > > > > > > > clang doesn't require the constructors to make this work for constant > > > > initialization, but similarly can't make it work for non-constant > > > > initialization. > > > > > > That makes a lot of sense, thanks for the detailed explanation. > > > > > > > > > > > > I haven't yet been able to make a solution using the above approach > > > > > work -- > > > > > making sure we use the ultimate object instead of the RESULT_DECL > > > > > whenever > > > > > we > > > > > access ctx->global->values is proving to be tricky and subtle. > > > > > > > > Do we need to go through ctx->global->values? Would it work for the > > > > RESULT_DECL case in cxx_eval_constant_expression to go to straight to > > > > ctx->object or ctx->ctor instead? > > > > > > I attempted that at some point, but IIRC we still end up not resolving > > > some RESULT_DECLs because not all of them get processed through > > > cxx_eval_constant_expression before we manipulate ctx->global->values > > > with them. I'll try this approach more carefully and report back with > > > more specifics. > > > > It turns out that immediately resolving RESULT_DECLs/'this' to the > > ultimate ctx->object would not interact well with constexpr_call > > caching: > > > > struct A { A() = default; A(const A&); A *ap = this; }; > > constexpr A foo() { return {}; } > > constexpr A a = foo(); > > constexpr A b = foo(); > > static_assert(b.ap == &b); // fails > > > > Evaluation of the first call to foo() returns {&a}, since we resolve > > 'this' to &a due to guaranteed RVO, and we cache this result. > > Evaluation of the second call to foo() just returns the cached result > > from the constexpr_call cache, and so we get {&a} again. > > > > So it seems we would have to mark the result of a constexpr call as not > > cacheable whenever this RVO applies during its evaluation, even when > > doing the RVO has no observable difference in the final result (i.e. the > > constructor does not try to save the 'this' pointer). > > > > Would the performance impact of disabling caching whenever RVO applies > > during constexpr call evaluation be worth it, or should we go with > > something like my first patch which "almost works," and which marks a > > constexpr call as not cacheable only when we replace a RESULT_DECL in > > the result of the call? > > Could we search through the result of the call for ctx->object and cache if we > don't find it? Hmm, I think the result of the call could still depend on ctx->object without ctx->object explicitly appearing in the result. Consider the following testcase: struct A { A() = default; A(const A&); constexpr A(const A *q) : d{this - p} { } long d = 0; }; constexpr A baz(const A *q) { return A(p); }; constexpr A a = baz(&a); constexpr A b = baz(&a); // no error The initialization of 'b' should be ill-formed, but the result of the first call to baz(&a) would be {0}, so we would cache it and then reuse the result when initializing 'b'.
On Fri, 10 Apr 2020, Patrick Palka wrote: > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > On 4/10/20 1:04 PM, Patrick Palka wrote: > > > On Thu, 9 Apr 2020, Patrick Palka wrote: > > > > On Thu, 9 Apr 2020, Jason Merrill wrote: > > > > > > > > > On 4/8/20 7:49 PM, Patrick Palka wrote: > > > > > > When evaluating the initializer of 'a' in the following example > > > > > > > > > > > > struct A { A *p = this; }; > > > > > > constexpr A foo() { return {}; } > > > > > > constexpr A a = foo(); > > > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned > > > > > > by foo > > > > > > gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, > > > > > > the > > > > > > 'this' > > > > > > should really be resolved to '&a'. > > > > > > > > > > > It seems to me that the right approach would be to immediately resolve > > > > > > the > > > > > > PLACEHOLDER_EXPR to the correct target object during evaluation of > > > > > > 'foo()', > > > > > > so > > > > > > that we could use 'this' to access objects adjacent to the current > > > > > > object in > > > > > > the > > > > > > ultimate storage location. (I think #c2 of PR c++/94537 is an example > > > > > > of > > > > > > such > > > > > > usage of 'this', which currently doesn't work. But as #c1 shows we > > > > > > don't > > > > > > seem > > > > > > to handle this case correctly in non-constexpr initialization either.) > > > > > > > > > > As I commented in the PR, the standard doesn't require this to work > > > > > because A > > > > > is trivially copyable, and our ABI makes it impossible. But there's > > > > > still a > > > > > constexpr bug when we add > > > > > > > > > > A() = default; A(const A&); > > > > > > > > > > clang doesn't require the constructors to make this work for constant > > > > > initialization, but similarly can't make it work for non-constant > > > > > initialization. > > > > > > > > That makes a lot of sense, thanks for the detailed explanation. > > > > > > > > > > > > > > > I haven't yet been able to make a solution using the above approach > > > > > > work -- > > > > > > making sure we use the ultimate object instead of the RESULT_DECL > > > > > > whenever > > > > > > we > > > > > > access ctx->global->values is proving to be tricky and subtle. > > > > > > > > > > Do we need to go through ctx->global->values? Would it work for the > > > > > RESULT_DECL case in cxx_eval_constant_expression to go to straight to > > > > > ctx->object or ctx->ctor instead? > > > > > > > > I attempted that at some point, but IIRC we still end up not resolving > > > > some RESULT_DECLs because not all of them get processed through > > > > cxx_eval_constant_expression before we manipulate ctx->global->values > > > > with them. I'll try this approach more carefully and report back with > > > > more specifics. > > > > > > It turns out that immediately resolving RESULT_DECLs/'this' to the > > > ultimate ctx->object would not interact well with constexpr_call > > > caching: > > > > > > struct A { A() = default; A(const A&); A *ap = this; }; > > > constexpr A foo() { return {}; } > > > constexpr A a = foo(); > > > constexpr A b = foo(); > > > static_assert(b.ap == &b); // fails > > > > > > Evaluation of the first call to foo() returns {&a}, since we resolve > > > 'this' to &a due to guaranteed RVO, and we cache this result. > > > Evaluation of the second call to foo() just returns the cached result > > > from the constexpr_call cache, and so we get {&a} again. > > > > > > So it seems we would have to mark the result of a constexpr call as not > > > cacheable whenever this RVO applies during its evaluation, even when > > > doing the RVO has no observable difference in the final result (i.e. the > > > constructor does not try to save the 'this' pointer). > > > > > > Would the performance impact of disabling caching whenever RVO applies > > > during constexpr call evaluation be worth it, or should we go with > > > something like my first patch which "almost works," and which marks a > > > constexpr call as not cacheable only when we replace a RESULT_DECL in > > > the result of the call? > > > > Could we search through the result of the call for ctx->object and cache if we > > don't find it? > > Hmm, I think the result of the call could still depend on ctx->object > without ctx->object explicitly appearing in the result. Consider the > following testcase: > > struct A { > A() = default; A(const A&); > constexpr A(const A *q) : d{this - p} { } Oops sorry, that 'q' should be a 'p'. > long d = 0; > }; > > constexpr A baz(const A *q) { return A(p); }; And same here. > constexpr A a = baz(&a); > constexpr A b = baz(&a); // no error > > The initialization of 'b' should be ill-formed, but the result of the > first call to baz(&a) would be {0}, so we would cache it and then reuse > the result when initializing 'b'. >
On 4/10/20 2:15 PM, Patrick Palka wrote: > On Fri, 10 Apr 2020, Patrick Palka wrote: > >> On Fri, 10 Apr 2020, Jason Merrill wrote: >> >>> On 4/10/20 1:04 PM, Patrick Palka wrote: >>>> On Thu, 9 Apr 2020, Patrick Palka wrote: >>>>> On Thu, 9 Apr 2020, Jason Merrill wrote: >>>>> >>>>>> On 4/8/20 7:49 PM, Patrick Palka wrote: >>>>>>> When evaluating the initializer of 'a' in the following example >>>>>>> >>>>>>> struct A { A *p = this; }; >>>>>>> constexpr A foo() { return {}; } >>>>>>> constexpr A a = foo(); >>>>>>> >>>>>>> the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned >>>>>>> by foo >>>>>>> gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, >>>>>>> the >>>>>>> 'this' >>>>>>> should really be resolved to '&a'. >>>>>> >>>>>>> It seems to me that the right approach would be to immediately resolve >>>>>>> the >>>>>>> PLACEHOLDER_EXPR to the correct target object during evaluation of >>>>>>> 'foo()', >>>>>>> so >>>>>>> that we could use 'this' to access objects adjacent to the current >>>>>>> object in >>>>>>> the >>>>>>> ultimate storage location. (I think #c2 of PR c++/94537 is an example >>>>>>> of >>>>>>> such >>>>>>> usage of 'this', which currently doesn't work. But as #c1 shows we >>>>>>> don't >>>>>>> seem >>>>>>> to handle this case correctly in non-constexpr initialization either.) >>>>>> >>>>>> As I commented in the PR, the standard doesn't require this to work >>>>>> because A >>>>>> is trivially copyable, and our ABI makes it impossible. But there's >>>>>> still a >>>>>> constexpr bug when we add >>>>>> >>>>>> A() = default; A(const A&); >>>>>> >>>>>> clang doesn't require the constructors to make this work for constant >>>>>> initialization, but similarly can't make it work for non-constant >>>>>> initialization. >>>>> >>>>> That makes a lot of sense, thanks for the detailed explanation. >>>>> >>>>>> >>>>>>> I haven't yet been able to make a solution using the above approach >>>>>>> work -- >>>>>>> making sure we use the ultimate object instead of the RESULT_DECL >>>>>>> whenever >>>>>>> we >>>>>>> access ctx->global->values is proving to be tricky and subtle. >>>>>> >>>>>> Do we need to go through ctx->global->values? Would it work for the >>>>>> RESULT_DECL case in cxx_eval_constant_expression to go to straight to >>>>>> ctx->object or ctx->ctor instead? >>>>> >>>>> I attempted that at some point, but IIRC we still end up not resolving >>>>> some RESULT_DECLs because not all of them get processed through >>>>> cxx_eval_constant_expression before we manipulate ctx->global->values >>>>> with them. I'll try this approach more carefully and report back with >>>>> more specifics. >>>> >>>> It turns out that immediately resolving RESULT_DECLs/'this' to the >>>> ultimate ctx->object would not interact well with constexpr_call >>>> caching: >>>> >>>> struct A { A() = default; A(const A&); A *ap = this; }; >>>> constexpr A foo() { return {}; } >>>> constexpr A a = foo(); >>>> constexpr A b = foo(); >>>> static_assert(b.ap == &b); // fails >>>> >>>> Evaluation of the first call to foo() returns {&a}, since we resolve >>>> 'this' to &a due to guaranteed RVO, and we cache this result. >>>> Evaluation of the second call to foo() just returns the cached result >>>> from the constexpr_call cache, and so we get {&a} again. >>>> >>>> So it seems we would have to mark the result of a constexpr call as not >>>> cacheable whenever this RVO applies during its evaluation, even when >>>> doing the RVO has no observable difference in the final result (i.e. the >>>> constructor does not try to save the 'this' pointer). >>>> >>>> Would the performance impact of disabling caching whenever RVO applies >>>> during constexpr call evaluation be worth it, or should we go with >>>> something like my first patch which "almost works," and which marks a >>>> constexpr call as not cacheable only when we replace a RESULT_DECL in >>>> the result of the call? >>> >>> Could we search through the result of the call for ctx->object and cache if we >>> don't find it? >> >> Hmm, I think the result of the call could still depend on ctx->object >> without ctx->object explicitly appearing in the result. Consider the >> following testcase: >> >> struct A { >> A() = default; A(const A&); >> constexpr A(const A *q) : d{this - p} { } > > Oops sorry, that 'q' should be a 'p'. > >> long d = 0; >> }; >> >> constexpr A baz(const A *q) { return A(p); }; > > And same here. > >> constexpr A a = baz(&a); >> constexpr A b = baz(&a); // no error >> >> The initialization of 'b' should be ill-formed, but the result of the >> first call to baz(&a) would be {0}, so we would cache it and then reuse >> the result when initializing 'b'. Ah, true. Can we still cache if we're initializing something that isn't TREE_STATIC? Jason
On Fri, 10 Apr 2020, Jason Merrill wrote: > On 4/10/20 2:15 PM, Patrick Palka wrote: > > On Fri, 10 Apr 2020, Patrick Palka wrote: > > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > > > On 4/10/20 1:04 PM, Patrick Palka wrote: > > > > > On Thu, 9 Apr 2020, Patrick Palka wrote: > > > > > > On Thu, 9 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > On 4/8/20 7:49 PM, Patrick Palka wrote: > > > > > > > > When evaluating the initializer of 'a' in the following example > > > > > > > > > > > > > > > > struct A { A *p = this; }; > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > constexpr A a = foo(); > > > > > > > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer > > > > > > > > returned > > > > > > > > by foo > > > > > > > > gets resolved to the RESULT_DECL of foo. But due to guaranteed > > > > > > > > RVO, > > > > > > > > the > > > > > > > > 'this' > > > > > > > > should really be resolved to '&a'. > > > > > > > > > > > > > > > It seems to me that the right approach would be to immediately > > > > > > > > resolve > > > > > > > > the > > > > > > > > PLACEHOLDER_EXPR to the correct target object during evaluation > > > > > > > > of > > > > > > > > 'foo()', > > > > > > > > so > > > > > > > > that we could use 'this' to access objects adjacent to the > > > > > > > > current > > > > > > > > object in > > > > > > > > the > > > > > > > > ultimate storage location. (I think #c2 of PR c++/94537 is an > > > > > > > > example > > > > > > > > of > > > > > > > > such > > > > > > > > usage of 'this', which currently doesn't work. But as #c1 shows > > > > > > > > we > > > > > > > > don't > > > > > > > > seem > > > > > > > > to handle this case correctly in non-constexpr initialization > > > > > > > > either.) > > > > > > > > > > > > > > As I commented in the PR, the standard doesn't require this to > > > > > > > work > > > > > > > because A > > > > > > > is trivially copyable, and our ABI makes it impossible. But > > > > > > > there's > > > > > > > still a > > > > > > > constexpr bug when we add > > > > > > > > > > > > > > A() = default; A(const A&); > > > > > > > > > > > > > > clang doesn't require the constructors to make this work for > > > > > > > constant > > > > > > > initialization, but similarly can't make it work for non-constant > > > > > > > initialization. > > > > > > > > > > > > That makes a lot of sense, thanks for the detailed explanation. > > > > > > > > > > > > > > > > > > > > > I haven't yet been able to make a solution using the above > > > > > > > > approach > > > > > > > > work -- > > > > > > > > making sure we use the ultimate object instead of the > > > > > > > > RESULT_DECL > > > > > > > > whenever > > > > > > > > we > > > > > > > > access ctx->global->values is proving to be tricky and subtle. > > > > > > > > > > > > > > Do we need to go through ctx->global->values? Would it work for > > > > > > > the > > > > > > > RESULT_DECL case in cxx_eval_constant_expression to go to straight > > > > > > > to > > > > > > > ctx->object or ctx->ctor instead? > > > > > > > > > > > > I attempted that at some point, but IIRC we still end up not > > > > > > resolving > > > > > > some RESULT_DECLs because not all of them get processed through > > > > > > cxx_eval_constant_expression before we manipulate > > > > > > ctx->global->values > > > > > > with them. I'll try this approach more carefully and report back > > > > > > with > > > > > > more specifics. > > > > > > > > > > It turns out that immediately resolving RESULT_DECLs/'this' to the > > > > > ultimate ctx->object would not interact well with constexpr_call > > > > > caching: > > > > > > > > > > struct A { A() = default; A(const A&); A *ap = this; }; > > > > > constexpr A foo() { return {}; } > > > > > constexpr A a = foo(); > > > > > constexpr A b = foo(); > > > > > static_assert(b.ap == &b); // fails > > > > > > > > > > Evaluation of the first call to foo() returns {&a}, since we resolve > > > > > 'this' to &a due to guaranteed RVO, and we cache this result. > > > > > Evaluation of the second call to foo() just returns the cached result > > > > > from the constexpr_call cache, and so we get {&a} again. > > > > > > > > > > So it seems we would have to mark the result of a constexpr call as > > > > > not > > > > > cacheable whenever this RVO applies during its evaluation, even when > > > > > doing the RVO has no observable difference in the final result (i.e. > > > > > the > > > > > constructor does not try to save the 'this' pointer). > > > > > > > > > > Would the performance impact of disabling caching whenever RVO applies > > > > > during constexpr call evaluation be worth it, or should we go with > > > > > something like my first patch which "almost works," and which marks a > > > > > constexpr call as not cacheable only when we replace a RESULT_DECL in > > > > > the result of the call? > > > > > > > > Could we search through the result of the call for ctx->object and cache > > > > if we > > > > don't find it? > > > > > > Hmm, I think the result of the call could still depend on ctx->object > > > without ctx->object explicitly appearing in the result. Consider the > > > following testcase: > > > > > > struct A { > > > A() = default; A(const A&); > > > constexpr A(const A *q) : d{this - p} { } > > > > Oops sorry, that 'q' should be a 'p'. > > > > > long d = 0; > > > }; > > > > > > constexpr A baz(const A *q) { return A(p); }; > > > > And same here. > > > > > constexpr A a = baz(&a); > > > constexpr A b = baz(&a); // no error > > > > > > The initialization of 'b' should be ill-formed, but the result of the > > > first call to baz(&a) would be {0}, so we would cache it and then reuse > > > the result when initializing 'b'. > > Ah, true. Can we still cache if we're initializing something that isn't > TREE_STATIC? Hmm, we correctly compile the analogous non-TREE_STATIC testcase struct A { A() = default; A(const A&); constexpr A(const A *p) : d{this == p} { } bool d; }; constexpr A baz(const A *p) { return A(p); } void foo() { constexpr A x = baz(&x); constexpr A y = baz(&x); static_assert(!y.d); } because the calls 'baz(&x)' are considered to have non-constant arguments. Unfortunately, we can come up with another trick to fool the constexpr_call cache in the presence of RVO even in the !TREE_STATIC case: struct B { B() = default; B(const B&); constexpr B(int) : q{!this[-1].q} { } bool q = false; }; constexpr B baz() { return B(0); } void foo() { constexpr B d[2] = { {}, baz() }; constexpr B e = baz(); } The initialization of the local variable 'e' should be invalid, but if we cached the first call to 'baz' then we would wrongly accept it.
On 4/10/20 5:47 PM, Patrick Palka wrote: > On Fri, 10 Apr 2020, Jason Merrill wrote: >> On 4/10/20 2:15 PM, Patrick Palka wrote: >>> On Fri, 10 Apr 2020, Patrick Palka wrote: >>> >>>> On Fri, 10 Apr 2020, Jason Merrill wrote: >>>> >>>>> On 4/10/20 1:04 PM, Patrick Palka wrote: >>>>>> On Thu, 9 Apr 2020, Patrick Palka wrote: >>>>>>> On Thu, 9 Apr 2020, Jason Merrill wrote: >>>>>>> >>>>>>>> On 4/8/20 7:49 PM, Patrick Palka wrote: >>>>>>>>> When evaluating the initializer of 'a' in the following example >>>>>>>>> >>>>>>>>> struct A { A *p = this; }; >>>>>>>>> constexpr A foo() { return {}; } >>>>>>>>> constexpr A a = foo(); >>>>>>>>> >>>>>>>>> the PLACEHOLDER_EXPR for 'this' in the aggregate initializer >>>>>>>>> returned >>>>>>>>> by foo >>>>>>>>> gets resolved to the RESULT_DECL of foo. But due to guaranteed >>>>>>>>> RVO, >>>>>>>>> the >>>>>>>>> 'this' >>>>>>>>> should really be resolved to '&a'. >>>>>>>> >>>>>>>>> It seems to me that the right approach would be to immediately >>>>>>>>> resolve >>>>>>>>> the >>>>>>>>> PLACEHOLDER_EXPR to the correct target object during evaluation >>>>>>>>> of >>>>>>>>> 'foo()', >>>>>>>>> so >>>>>>>>> that we could use 'this' to access objects adjacent to the >>>>>>>>> current >>>>>>>>> object in >>>>>>>>> the >>>>>>>>> ultimate storage location. (I think #c2 of PR c++/94537 is an >>>>>>>>> example >>>>>>>>> of >>>>>>>>> such >>>>>>>>> usage of 'this', which currently doesn't work. But as #c1 shows >>>>>>>>> we >>>>>>>>> don't >>>>>>>>> seem >>>>>>>>> to handle this case correctly in non-constexpr initialization >>>>>>>>> either.) >>>>>>>> >>>>>>>> As I commented in the PR, the standard doesn't require this to >>>>>>>> work >>>>>>>> because A >>>>>>>> is trivially copyable, and our ABI makes it impossible. But >>>>>>>> there's >>>>>>>> still a >>>>>>>> constexpr bug when we add >>>>>>>> >>>>>>>> A() = default; A(const A&); >>>>>>>> >>>>>>>> clang doesn't require the constructors to make this work for >>>>>>>> constant >>>>>>>> initialization, but similarly can't make it work for non-constant >>>>>>>> initialization. >>>>>>> >>>>>>> That makes a lot of sense, thanks for the detailed explanation. >>>>>>> >>>>>>>> >>>>>>>>> I haven't yet been able to make a solution using the above >>>>>>>>> approach >>>>>>>>> work -- >>>>>>>>> making sure we use the ultimate object instead of the >>>>>>>>> RESULT_DECL >>>>>>>>> whenever >>>>>>>>> we >>>>>>>>> access ctx->global->values is proving to be tricky and subtle. >>>>>>>> >>>>>>>> Do we need to go through ctx->global->values? Would it work for >>>>>>>> the >>>>>>>> RESULT_DECL case in cxx_eval_constant_expression to go to straight >>>>>>>> to >>>>>>>> ctx->object or ctx->ctor instead? >>>>>>> >>>>>>> I attempted that at some point, but IIRC we still end up not >>>>>>> resolving >>>>>>> some RESULT_DECLs because not all of them get processed through >>>>>>> cxx_eval_constant_expression before we manipulate >>>>>>> ctx->global->values >>>>>>> with them. I'll try this approach more carefully and report back >>>>>>> with >>>>>>> more specifics. >>>>>> >>>>>> It turns out that immediately resolving RESULT_DECLs/'this' to the >>>>>> ultimate ctx->object would not interact well with constexpr_call >>>>>> caching: >>>>>> >>>>>> struct A { A() = default; A(const A&); A *ap = this; }; >>>>>> constexpr A foo() { return {}; } >>>>>> constexpr A a = foo(); >>>>>> constexpr A b = foo(); >>>>>> static_assert(b.ap == &b); // fails >>>>>> >>>>>> Evaluation of the first call to foo() returns {&a}, since we resolve >>>>>> 'this' to &a due to guaranteed RVO, and we cache this result. >>>>>> Evaluation of the second call to foo() just returns the cached result >>>>>> from the constexpr_call cache, and so we get {&a} again. >>>>>> >>>>>> So it seems we would have to mark the result of a constexpr call as >>>>>> not >>>>>> cacheable whenever this RVO applies during its evaluation, even when >>>>>> doing the RVO has no observable difference in the final result (i.e. >>>>>> the >>>>>> constructor does not try to save the 'this' pointer). >>>>>> >>>>>> Would the performance impact of disabling caching whenever RVO applies >>>>>> during constexpr call evaluation be worth it, or should we go with >>>>>> something like my first patch which "almost works," and which marks a >>>>>> constexpr call as not cacheable only when we replace a RESULT_DECL in >>>>>> the result of the call? >>>>> >>>>> Could we search through the result of the call for ctx->object and cache >>>>> if we >>>>> don't find it? >>>> >>>> Hmm, I think the result of the call could still depend on ctx->object >>>> without ctx->object explicitly appearing in the result. Consider the >>>> following testcase: >>>> >>>> struct A { >>>> A() = default; A(const A&); >>>> constexpr A(const A *q) : d{this - p} { } >>> >>> Oops sorry, that 'q' should be a 'p'. >>> >>>> long d = 0; >>>> }; >>>> >>>> constexpr A baz(const A *q) { return A(p); }; >>> >>> And same here. >>> >>>> constexpr A a = baz(&a); >>>> constexpr A b = baz(&a); // no error >>>> >>>> The initialization of 'b' should be ill-formed, but the result of the >>>> first call to baz(&a) would be {0}, so we would cache it and then reuse >>>> the result when initializing 'b'. >> >> Ah, true. Can we still cache if we're initializing something that isn't >> TREE_STATIC? > > Hmm, we correctly compile the analogous non-TREE_STATIC testcase > > struct A { > A() = default; A(const A&); > constexpr A(const A *p) : d{this == p} { } > bool d; > }; > > constexpr A baz(const A *p) { return A(p); } > > void foo() { > constexpr A x = baz(&x); > constexpr A y = baz(&x); > static_assert(!y.d); > } > > because the calls 'baz(&x)' are considered to have non-constant arguments. > > > Unfortunately, we can come up with another trick to fool the constexpr_call > cache in the presence of RVO even in the !TREE_STATIC case: > > struct B { > B() = default; B(const B&); > constexpr B(int) : q{!this[-1].q} { } > bool q = false; > }; > > constexpr B baz() { return B(0); } > > void foo() > { > constexpr B d[2] = { {}, baz() }; > constexpr B e = baz(); > } > > The initialization of the local variable 'e' should be invalid, but if we > cached the first call to 'baz' then we would wrongly accept it. Right. We ought to be able to distinguish between uses of the RESULT_DECL only for storing to it (cacheable) and any other use, either reading from it or messing with its address. But I think that's a future direction, and we should stick with your patch that almost works for GCC 10. Jason
On Sat, 11 Apr 2020, Jason Merrill wrote: > On 4/10/20 5:47 PM, Patrick Palka wrote: > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > On 4/10/20 2:15 PM, Patrick Palka wrote: > > > > On Fri, 10 Apr 2020, Patrick Palka wrote: > > > > > > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > On 4/10/20 1:04 PM, Patrick Palka wrote: > > > > > > > On Thu, 9 Apr 2020, Patrick Palka wrote: > > > > > > > > On Thu, 9 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > > > On 4/8/20 7:49 PM, Patrick Palka wrote: > > > > > > > > > > When evaluating the initializer of 'a' in the following > > > > > > > > > > example > > > > > > > > > > > > > > > > > > > > struct A { A *p = this; }; > > > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > > > constexpr A a = foo(); > > > > > > > > > > > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer > > > > > > > > > > returned > > > > > > > > > > by foo > > > > > > > > > > gets resolved to the RESULT_DECL of foo. But due to > > > > > > > > > > guaranteed > > > > > > > > > > RVO, > > > > > > > > > > the > > > > > > > > > > 'this' > > > > > > > > > > should really be resolved to '&a'. > > > > > > > > > > > > > > > > > > > It seems to me that the right approach would be to > > > > > > > > > > immediately > > > > > > > > > > resolve > > > > > > > > > > the > > > > > > > > > > PLACEHOLDER_EXPR to the correct target object during > > > > > > > > > > evaluation > > > > > > > > > > of > > > > > > > > > > 'foo()', > > > > > > > > > > so > > > > > > > > > > that we could use 'this' to access objects adjacent to the > > > > > > > > > > current > > > > > > > > > > object in > > > > > > > > > > the > > > > > > > > > > ultimate storage location. (I think #c2 of PR c++/94537 is > > > > > > > > > > an > > > > > > > > > > example > > > > > > > > > > of > > > > > > > > > > such > > > > > > > > > > usage of 'this', which currently doesn't work. But as #c1 > > > > > > > > > > shows > > > > > > > > > > we > > > > > > > > > > don't > > > > > > > > > > seem > > > > > > > > > > to handle this case correctly in non-constexpr > > > > > > > > > > initialization > > > > > > > > > > either.) > > > > > > > > > > > > > > > > > > As I commented in the PR, the standard doesn't require this to > > > > > > > > > work > > > > > > > > > because A > > > > > > > > > is trivially copyable, and our ABI makes it impossible. But > > > > > > > > > there's > > > > > > > > > still a > > > > > > > > > constexpr bug when we add > > > > > > > > > > > > > > > > > > A() = default; A(const A&); > > > > > > > > > > > > > > > > > > clang doesn't require the constructors to make this work for > > > > > > > > > constant > > > > > > > > > initialization, but similarly can't make it work for > > > > > > > > > non-constant > > > > > > > > > initialization. > > > > > > > > > > > > > > > > That makes a lot of sense, thanks for the detailed explanation. > > > > > > > > > > > > > > > > > > > > > > > > > > > I haven't yet been able to make a solution using the above > > > > > > > > > > approach > > > > > > > > > > work -- > > > > > > > > > > making sure we use the ultimate object instead of the > > > > > > > > > > RESULT_DECL > > > > > > > > > > whenever > > > > > > > > > > we > > > > > > > > > > access ctx->global->values is proving to be tricky and > > > > > > > > > > subtle. > > > > > > > > > > > > > > > > > > Do we need to go through ctx->global->values? Would it work > > > > > > > > > for > > > > > > > > > the > > > > > > > > > RESULT_DECL case in cxx_eval_constant_expression to go to > > > > > > > > > straight > > > > > > > > > to > > > > > > > > > ctx->object or ctx->ctor instead? > > > > > > > > > > > > > > > > I attempted that at some point, but IIRC we still end up not > > > > > > > > resolving > > > > > > > > some RESULT_DECLs because not all of them get processed through > > > > > > > > cxx_eval_constant_expression before we manipulate > > > > > > > > ctx->global->values > > > > > > > > with them. I'll try this approach more carefully and report > > > > > > > > back > > > > > > > > with > > > > > > > > more specifics. > > > > > > > > > > > > > > It turns out that immediately resolving RESULT_DECLs/'this' to the > > > > > > > ultimate ctx->object would not interact well with constexpr_call > > > > > > > caching: > > > > > > > > > > > > > > struct A { A() = default; A(const A&); A *ap = this; }; > > > > > > > constexpr A foo() { return {}; } > > > > > > > constexpr A a = foo(); > > > > > > > constexpr A b = foo(); > > > > > > > static_assert(b.ap == &b); // fails > > > > > > > > > > > > > > Evaluation of the first call to foo() returns {&a}, since we > > > > > > > resolve > > > > > > > 'this' to &a due to guaranteed RVO, and we cache this result. > > > > > > > Evaluation of the second call to foo() just returns the cached > > > > > > > result > > > > > > > from the constexpr_call cache, and so we get {&a} again. > > > > > > > > > > > > > > So it seems we would have to mark the result of a constexpr call > > > > > > > as > > > > > > > not > > > > > > > cacheable whenever this RVO applies during its evaluation, even > > > > > > > when > > > > > > > doing the RVO has no observable difference in the final result > > > > > > > (i.e. > > > > > > > the > > > > > > > constructor does not try to save the 'this' pointer). > > > > > > > > > > > > > > Would the performance impact of disabling caching whenever RVO > > > > > > > applies > > > > > > > during constexpr call evaluation be worth it, or should we go with > > > > > > > something like my first patch which "almost works," and which > > > > > > > marks a > > > > > > > constexpr call as not cacheable only when we replace a RESULT_DECL > > > > > > > in > > > > > > > the result of the call? > > > > > > > > > > > > Could we search through the result of the call for ctx->object and > > > > > > cache > > > > > > if we > > > > > > don't find it? > > > > > > > > > > Hmm, I think the result of the call could still depend on ctx->object > > > > > without ctx->object explicitly appearing in the result. Consider the > > > > > following testcase: > > > > > > > > > > struct A { > > > > > A() = default; A(const A&); > > > > > constexpr A(const A *q) : d{this - p} { } > > > > > > > > Oops sorry, that 'q' should be a 'p'. > > > > > > > > > long d = 0; > > > > > }; > > > > > > > > > > constexpr A baz(const A *q) { return A(p); }; > > > > > > > > And same here. > > > > > > > > > constexpr A a = baz(&a); > > > > > constexpr A b = baz(&a); // no error > > > > > > > > > > The initialization of 'b' should be ill-formed, but the result of the > > > > > first call to baz(&a) would be {0}, so we would cache it and then > > > > > reuse > > > > > the result when initializing 'b'. > > > > > > Ah, true. Can we still cache if we're initializing something that isn't > > > TREE_STATIC? > > > > Hmm, we correctly compile the analogous non-TREE_STATIC testcase > > > > struct A { > > A() = default; A(const A&); > > constexpr A(const A *p) : d{this == p} { } > > bool d; > > }; > > > > constexpr A baz(const A *p) { return A(p); } > > > > void foo() { > > constexpr A x = baz(&x); > > constexpr A y = baz(&x); > > static_assert(!y.d); > > } > > > > because the calls 'baz(&x)' are considered to have non-constant arguments. > > > > > > Unfortunately, we can come up with another trick to fool the constexpr_call > > cache in the presence of RVO even in the !TREE_STATIC case: > > > > struct B { > > B() = default; B(const B&); > > constexpr B(int) : q{!this[-1].q} { } > > bool q = false; > > }; > > > > constexpr B baz() { return B(0); } > > > > void foo() > > { > > constexpr B d[2] = { {}, baz() }; > > constexpr B e = baz(); > > } > > > > The initialization of the local variable 'e' should be invalid, but if we > > cached the first call to 'baz' then we would wrongly accept it. > > Right. > > We ought to be able to distinguish between uses of the RESULT_DECL only for > storing to it (cacheable) and any other use, either reading from it or messing > with its address. But I think that's a future direction, and we should stick > with your patch that almost works for GCC 10. Sounds good. Does the following then look OK to commit? -- >8 -- Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call [PR94034] When evaluating the initializer of 'a' in the following example struct A { A() = default; A(const A&); A *p = this; }; constexpr A foo() { return {}; } constexpr A a = foo(); the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the 'this' should really be resolved to '&a'. Fixing this properly by immediately resolving 'this' and PLACEHOLDER_EXPRs to the ultimate object under construction would in general mean that we would no longer be able to cache constexpr calls for which RVO possibly applies, because the result of the call may now depend on the ultimate object under construction. So as a mostly correct stopgap solution that retains cachability of RVO'd constexpr calls, this patch fixes this issue by rewriting all occurrences of the RESULT_DECL in the result of a constexpr function call with the current object under construction, after the call returns. This means the 'this' pointer during construction of the temporary will still point to the temporary object instead of the ultimate object, but besides that this approach seems functionally equivalent to the proper approach. Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK to commit? gcc/cp/ChangeLog: PR c++/94034 * constexpr.c (replace_result_decl_data): New struct. (replace_result_decl_data_r): New function. (replace_result_decl): New function. (cxx_eval_call_expression): Use it. * tree.c (build_aggr_init_expr): Propagate the location of the initializer to the AGGR_INIT_EXPR. gcc/testsuite/ChangeLog: PR c++/94034 * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. --- gcc/cp/constexpr.c | 51 +++++++++++++++++++ gcc/cp/tree.c | 3 ++ .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++ .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 ++++++++++++++++++ .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ 6 files changed, 204 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 5793430c88d..4cf5812e8a5 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call, return cp_build_addr_expr (obj, complain); } +/* Data structure used by replace_result_decl and replace_result_decl_r. */ + +struct replace_result_decl_data +{ + /* The RESULT_DECL we want to replace. */ + tree decl; + /* The replacement for DECL. */ + tree replacement; + /* Whether we've performed any replacements. */ + bool changed; +}; + +/* Helper function for replace_result_decl, called through cp_walk_tree. */ + +static tree +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) +{ + replace_result_decl_data *d = (replace_result_decl_data *) data; + + if (*tp == d->decl) + { + *tp = unshare_expr (d->replacement); + d->changed = true; + *walk_subtrees = 0; + } + else if (TYPE_P (*tp)) + *walk_subtrees = 0; + + return NULL_TREE; +} + +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy of) + REPLACEMENT within *TP. Returns true iff a replacement was performed. */ + +static bool +replace_result_decl (tree *tp, tree decl, tree replacement) +{ + gcc_assert (TREE_CODE (decl) == RESULT_DECL); + replace_result_decl_data data = { decl, replacement, false }; + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); + return data.changed; +} + /* Subroutine of cxx_eval_constant_expression. Evaluate the call expression tree T in the context of OLD_CALL expression evaluation. */ @@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, break; } } + + /* Rewrite all occurrences of the function's RESULT_DECL with the + current object under construction. */ + if (ctx->object + && (same_type_ignoring_top_level_qualifiers_p + (TREE_TYPE (res), TREE_TYPE (ctx->object)))) + if (replace_result_decl (&result, res, ctx->object)) + cacheable = false; } else /* Couldn't get a function copy to evaluate. */ diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index d1192b7e094..1d49d43c229 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -669,6 +669,9 @@ build_aggr_init_expr (tree type, tree init) else rval = init; + if (location_t loc = EXPR_LOCATION (init)) + SET_EXPR_LOCATION (rval, loc); + return rval; } diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C new file mode 100644 index 00000000000..bb844b952e2 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C @@ -0,0 +1,26 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A { + A *ap = this; +}; + +constexpr A foo() +{ + return {}; +} + +constexpr A bar() +{ + return foo(); +} + +void +baz() +{ + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } +} + +constexpr A a = foo(); +constexpr A b = bar(); diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C new file mode 100644 index 00000000000..f847fe9809f --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C @@ -0,0 +1,27 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A { + A() = default; A(const A&); + A *ap = this; +}; + +constexpr A foo() +{ + return {}; +} + +constexpr A bar() +{ + return foo(); +} + +void +baz() +{ + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } +} + +constexpr A a = foo(); +constexpr A b = bar(); diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C new file mode 100644 index 00000000000..5a40cd0b845 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C @@ -0,0 +1,49 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A +{ + A *p = this; + int n = 2; + int m = p->n++; +}; + +constexpr A +foo() +{ + return {}; +} + +constexpr A +bar() +{ + A a = foo(); + a.p->n = 5; + return a; +} + +static_assert(bar().n == 5, ""); + +constexpr int +baz() +{ + A b = foo(); + b.p->n = 10; + A c = foo(); + if (c.p->n != 3 || c.p->m != 2) + __builtin_abort(); + bar(); + return 0; +} + +static_assert(baz() == 0, ""); + +constexpr int +quux() +{ + const A d = foo(); + d.p->n++; // { dg-error "const object" } + return 0; +} + +static_assert(quux() == 0, ""); // { dg-error "non-constant" } diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C new file mode 100644 index 00000000000..86d8ab4e759 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C @@ -0,0 +1,48 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A +{ + A() = default; A(const A&); + A *p = this; + int n = 2; + int m = p->n++; +}; + +constexpr A +foo() +{ + return {}; +} + +constexpr A +bar() +{ + A a = foo(); + a.p->n = 5; + return a; // { dg-error "non-.constexpr." } +} + +constexpr int +baz() +{ + A b = foo(); + b.p->n = 10; + A c = foo(); + if (c.p->n != 3 || c.p->m != 2) + __builtin_abort(); + foo(); + return 0; +} + +static_assert(baz() == 0, ""); + +constexpr int +quux() +{ + const A d = foo(); + d.p->n++; // { dg-error "const object" } + return 0; +} + +static_assert(quux() == 0, ""); // { dg-error "non-constant" }
On 4/12/20 9:43 AM, Patrick Palka wrote: > On Sat, 11 Apr 2020, Jason Merrill wrote: > >> On 4/10/20 5:47 PM, Patrick Palka wrote: >>> On Fri, 10 Apr 2020, Jason Merrill wrote: >>>> On 4/10/20 2:15 PM, Patrick Palka wrote: >>>>> On Fri, 10 Apr 2020, Patrick Palka wrote: >>>>> >>>>>> On Fri, 10 Apr 2020, Jason Merrill wrote: >>>>>> >>>>>>> On 4/10/20 1:04 PM, Patrick Palka wrote: >>>>>>>> On Thu, 9 Apr 2020, Patrick Palka wrote: >>>>>>>>> On Thu, 9 Apr 2020, Jason Merrill wrote: >>>>>>>>> >>>>>>>>>> On 4/8/20 7:49 PM, Patrick Palka wrote: >>>>>>>>>>> When evaluating the initializer of 'a' in the following >>>>>>>>>>> example >>>>>>>>>>> >>>>>>>>>>> struct A { A *p = this; }; >>>>>>>>>>> constexpr A foo() { return {}; } >>>>>>>>>>> constexpr A a = foo(); >>>>>>>>>>> >>>>>>>>>>> the PLACEHOLDER_EXPR for 'this' in the aggregate initializer >>>>>>>>>>> returned >>>>>>>>>>> by foo >>>>>>>>>>> gets resolved to the RESULT_DECL of foo. But due to >>>>>>>>>>> guaranteed >>>>>>>>>>> RVO, >>>>>>>>>>> the >>>>>>>>>>> 'this' >>>>>>>>>>> should really be resolved to '&a'. >>>>>>>>>> >>>>>>>>>>> It seems to me that the right approach would be to >>>>>>>>>>> immediately >>>>>>>>>>> resolve >>>>>>>>>>> the >>>>>>>>>>> PLACEHOLDER_EXPR to the correct target object during >>>>>>>>>>> evaluation >>>>>>>>>>> of >>>>>>>>>>> 'foo()', >>>>>>>>>>> so >>>>>>>>>>> that we could use 'this' to access objects adjacent to the >>>>>>>>>>> current >>>>>>>>>>> object in >>>>>>>>>>> the >>>>>>>>>>> ultimate storage location. (I think #c2 of PR c++/94537 is >>>>>>>>>>> an >>>>>>>>>>> example >>>>>>>>>>> of >>>>>>>>>>> such >>>>>>>>>>> usage of 'this', which currently doesn't work. But as #c1 >>>>>>>>>>> shows >>>>>>>>>>> we >>>>>>>>>>> don't >>>>>>>>>>> seem >>>>>>>>>>> to handle this case correctly in non-constexpr >>>>>>>>>>> initialization >>>>>>>>>>> either.) >>>>>>>>>> >>>>>>>>>> As I commented in the PR, the standard doesn't require this to >>>>>>>>>> work >>>>>>>>>> because A >>>>>>>>>> is trivially copyable, and our ABI makes it impossible. But >>>>>>>>>> there's >>>>>>>>>> still a >>>>>>>>>> constexpr bug when we add >>>>>>>>>> >>>>>>>>>> A() = default; A(const A&); >>>>>>>>>> >>>>>>>>>> clang doesn't require the constructors to make this work for >>>>>>>>>> constant >>>>>>>>>> initialization, but similarly can't make it work for >>>>>>>>>> non-constant >>>>>>>>>> initialization. >>>>>>>>> >>>>>>>>> That makes a lot of sense, thanks for the detailed explanation. >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> I haven't yet been able to make a solution using the above >>>>>>>>>>> approach >>>>>>>>>>> work -- >>>>>>>>>>> making sure we use the ultimate object instead of the >>>>>>>>>>> RESULT_DECL >>>>>>>>>>> whenever >>>>>>>>>>> we >>>>>>>>>>> access ctx->global->values is proving to be tricky and >>>>>>>>>>> subtle. >>>>>>>>>> >>>>>>>>>> Do we need to go through ctx->global->values? Would it work >>>>>>>>>> for >>>>>>>>>> the >>>>>>>>>> RESULT_DECL case in cxx_eval_constant_expression to go to >>>>>>>>>> straight >>>>>>>>>> to >>>>>>>>>> ctx->object or ctx->ctor instead? >>>>>>>>> >>>>>>>>> I attempted that at some point, but IIRC we still end up not >>>>>>>>> resolving >>>>>>>>> some RESULT_DECLs because not all of them get processed through >>>>>>>>> cxx_eval_constant_expression before we manipulate >>>>>>>>> ctx->global->values >>>>>>>>> with them. I'll try this approach more carefully and report >>>>>>>>> back >>>>>>>>> with >>>>>>>>> more specifics. >>>>>>>> >>>>>>>> It turns out that immediately resolving RESULT_DECLs/'this' to the >>>>>>>> ultimate ctx->object would not interact well with constexpr_call >>>>>>>> caching: >>>>>>>> >>>>>>>> struct A { A() = default; A(const A&); A *ap = this; }; >>>>>>>> constexpr A foo() { return {}; } >>>>>>>> constexpr A a = foo(); >>>>>>>> constexpr A b = foo(); >>>>>>>> static_assert(b.ap == &b); // fails >>>>>>>> >>>>>>>> Evaluation of the first call to foo() returns {&a}, since we >>>>>>>> resolve >>>>>>>> 'this' to &a due to guaranteed RVO, and we cache this result. >>>>>>>> Evaluation of the second call to foo() just returns the cached >>>>>>>> result >>>>>>>> from the constexpr_call cache, and so we get {&a} again. >>>>>>>> >>>>>>>> So it seems we would have to mark the result of a constexpr call >>>>>>>> as >>>>>>>> not >>>>>>>> cacheable whenever this RVO applies during its evaluation, even >>>>>>>> when >>>>>>>> doing the RVO has no observable difference in the final result >>>>>>>> (i.e. >>>>>>>> the >>>>>>>> constructor does not try to save the 'this' pointer). >>>>>>>> >>>>>>>> Would the performance impact of disabling caching whenever RVO >>>>>>>> applies >>>>>>>> during constexpr call evaluation be worth it, or should we go with >>>>>>>> something like my first patch which "almost works," and which >>>>>>>> marks a >>>>>>>> constexpr call as not cacheable only when we replace a RESULT_DECL >>>>>>>> in >>>>>>>> the result of the call? >>>>>>> >>>>>>> Could we search through the result of the call for ctx->object and >>>>>>> cache >>>>>>> if we >>>>>>> don't find it? >>>>>> >>>>>> Hmm, I think the result of the call could still depend on ctx->object >>>>>> without ctx->object explicitly appearing in the result. Consider the >>>>>> following testcase: >>>>>> >>>>>> struct A { >>>>>> A() = default; A(const A&); >>>>>> constexpr A(const A *q) : d{this - p} { } >>>>> >>>>> Oops sorry, that 'q' should be a 'p'. >>>>> >>>>>> long d = 0; >>>>>> }; >>>>>> >>>>>> constexpr A baz(const A *q) { return A(p); }; >>>>> >>>>> And same here. >>>>> >>>>>> constexpr A a = baz(&a); >>>>>> constexpr A b = baz(&a); // no error >>>>>> >>>>>> The initialization of 'b' should be ill-formed, but the result of the >>>>>> first call to baz(&a) would be {0}, so we would cache it and then >>>>>> reuse >>>>>> the result when initializing 'b'. >>>> >>>> Ah, true. Can we still cache if we're initializing something that isn't >>>> TREE_STATIC? >>> >>> Hmm, we correctly compile the analogous non-TREE_STATIC testcase >>> >>> struct A { >>> A() = default; A(const A&); >>> constexpr A(const A *p) : d{this == p} { } >>> bool d; >>> }; >>> >>> constexpr A baz(const A *p) { return A(p); } >>> >>> void foo() { >>> constexpr A x = baz(&x); >>> constexpr A y = baz(&x); >>> static_assert(!y.d); >>> } >>> >>> because the calls 'baz(&x)' are considered to have non-constant arguments. >>> >>> >>> Unfortunately, we can come up with another trick to fool the constexpr_call >>> cache in the presence of RVO even in the !TREE_STATIC case: >>> >>> struct B { >>> B() = default; B(const B&); >>> constexpr B(int) : q{!this[-1].q} { } >>> bool q = false; >>> }; >>> >>> constexpr B baz() { return B(0); } >>> >>> void foo() >>> { >>> constexpr B d[2] = { {}, baz() }; >>> constexpr B e = baz(); >>> } >>> >>> The initialization of the local variable 'e' should be invalid, but if we >>> cached the first call to 'baz' then we would wrongly accept it. >> >> Right. >> >> We ought to be able to distinguish between uses of the RESULT_DECL only for >> storing to it (cacheable) and any other use, either reading from it or messing >> with its address. But I think that's a future direction, and we should stick >> with your patch that almost works for GCC 10. > > Sounds good. Does the following then look OK to commit? > > -- >8 -- > > Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call [PR94034] > > When evaluating the initializer of 'a' in the following example > > struct A { > A() = default; A(const A&); > A *p = this; > }; > constexpr A foo() { return {}; } > constexpr A a = foo(); > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo > gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the 'this' > should really be resolved to '&a'. > > Fixing this properly by immediately resolving 'this' and PLACEHOLDER_EXPRs to > the ultimate object under construction would in general mean that we would no > longer be able to cache constexpr calls for which RVO possibly applies, because > the result of the call may now depend on the ultimate object under construction. > > So as a mostly correct stopgap solution that retains cachability of RVO'd > constexpr calls, this patch fixes this issue by rewriting all occurrences of the > RESULT_DECL in the result of a constexpr function call with the current object > under construction, after the call returns. This means the 'this' pointer > during construction of the temporary will still point to the temporary object > instead of the ultimate object, but besides that this approach seems > functionally equivalent to the proper approach. > > Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > OK to commit? > > gcc/cp/ChangeLog: > > PR c++/94034 > * constexpr.c (replace_result_decl_data): New struct. > (replace_result_decl_data_r): New function. > (replace_result_decl): New function. > (cxx_eval_call_expression): Use it. > * tree.c (build_aggr_init_expr): Propagate the location of the > initializer to the AGGR_INIT_EXPR. > > gcc/testsuite/ChangeLog: > > PR c++/94034 > * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. > --- > gcc/cp/constexpr.c | 51 +++++++++++++++++++ > gcc/cp/tree.c | 3 ++ > .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 ++++++++++++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ > 6 files changed, 204 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > index 5793430c88d..4cf5812e8a5 100644 > --- a/gcc/cp/constexpr.c > +++ b/gcc/cp/constexpr.c > @@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call, > return cp_build_addr_expr (obj, complain); > } > > +/* Data structure used by replace_result_decl and replace_result_decl_r. */ > + > +struct replace_result_decl_data > +{ > + /* The RESULT_DECL we want to replace. */ > + tree decl; > + /* The replacement for DECL. */ > + tree replacement; > + /* Whether we've performed any replacements. */ > + bool changed; > +}; > + > +/* Helper function for replace_result_decl, called through cp_walk_tree. */ > + > +static tree > +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) > +{ > + replace_result_decl_data *d = (replace_result_decl_data *) data; > + > + if (*tp == d->decl) > + { > + *tp = unshare_expr (d->replacement); > + d->changed = true; > + *walk_subtrees = 0; > + } > + else if (TYPE_P (*tp)) > + *walk_subtrees = 0; > + > + return NULL_TREE; > +} > + > +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy of) > + REPLACEMENT within *TP. Returns true iff a replacement was performed. */ > + > +static bool > +replace_result_decl (tree *tp, tree decl, tree replacement) > +{ > + gcc_assert (TREE_CODE (decl) == RESULT_DECL); > + replace_result_decl_data data = { decl, replacement, false }; > + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); > + return data.changed; > +} > + > /* Subroutine of cxx_eval_constant_expression. > Evaluate the call expression tree T in the context of OLD_CALL expression > evaluation. */ > @@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > break; > } > } > + > + /* Rewrite all occurrences of the function's RESULT_DECL with the > + current object under construction. */ > + if (ctx->object > + && (same_type_ignoring_top_level_qualifiers_p > + (TREE_TYPE (res), TREE_TYPE (ctx->object)))) When can they have different types? > + if (replace_result_decl (&result, res, ctx->object)) > + cacheable = false; > } > else > /* Couldn't get a function copy to evaluate. */ > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > index d1192b7e094..1d49d43c229 100644 > --- a/gcc/cp/tree.c > +++ b/gcc/cp/tree.c > @@ -669,6 +669,9 @@ build_aggr_init_expr (tree type, tree init) > else > rval = init; > > + if (location_t loc = EXPR_LOCATION (init)) > + SET_EXPR_LOCATION (rval, loc); > + > return rval; > } > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > new file mode 100644 > index 00000000000..bb844b952e2 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > @@ -0,0 +1,26 @@ > +// PR c++/94034 > +// { dg-do compile { target c++14 } } > + > +struct A { > + A *ap = this; > +}; > + > +constexpr A foo() > +{ > + return {}; > +} > + > +constexpr A bar() > +{ > + return foo(); > +} > + > +void > +baz() > +{ > + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } > + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } > +} > + > +constexpr A a = foo(); > +constexpr A b = bar(); > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > new file mode 100644 > index 00000000000..f847fe9809f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > @@ -0,0 +1,27 @@ > +// PR c++/94034 > +// { dg-do compile { target c++14 } } > + > +struct A { > + A() = default; A(const A&); > + A *ap = this; > +}; > + > +constexpr A foo() > +{ > + return {}; > +} > + > +constexpr A bar() > +{ > + return foo(); > +} > + > +void > +baz() > +{ > + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } > + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } > +} > + > +constexpr A a = foo(); > +constexpr A b = bar(); > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > new file mode 100644 > index 00000000000..5a40cd0b845 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > @@ -0,0 +1,49 @@ > +// PR c++/94034 > +// { dg-do compile { target c++14 } } > + > +struct A > +{ > + A *p = this; > + int n = 2; > + int m = p->n++; > +}; > + > +constexpr A > +foo() > +{ > + return {}; > +} > + > +constexpr A > +bar() > +{ > + A a = foo(); > + a.p->n = 5; > + return a; > +} > + > +static_assert(bar().n == 5, ""); > + > +constexpr int > +baz() > +{ > + A b = foo(); > + b.p->n = 10; > + A c = foo(); > + if (c.p->n != 3 || c.p->m != 2) > + __builtin_abort(); > + bar(); > + return 0; > +} > + > +static_assert(baz() == 0, ""); > + > +constexpr int > +quux() > +{ > + const A d = foo(); > + d.p->n++; // { dg-error "const object" } > + return 0; > +} > + > +static_assert(quux() == 0, ""); // { dg-error "non-constant" } > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > new file mode 100644 > index 00000000000..86d8ab4e759 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > @@ -0,0 +1,48 @@ > +// PR c++/94034 > +// { dg-do compile { target c++14 } } > + > +struct A > +{ > + A() = default; A(const A&); > + A *p = this; > + int n = 2; > + int m = p->n++; > +}; > + > +constexpr A > +foo() > +{ > + return {}; > +} > + > +constexpr A > +bar() > +{ > + A a = foo(); > + a.p->n = 5; > + return a; // { dg-error "non-.constexpr." } > +} > + > +constexpr int > +baz() > +{ > + A b = foo(); > + b.p->n = 10; > + A c = foo(); > + if (c.p->n != 3 || c.p->m != 2) > + __builtin_abort(); > + foo(); > + return 0; > +} > + > +static_assert(baz() == 0, ""); > + > +constexpr int > +quux() > +{ > + const A d = foo(); > + d.p->n++; // { dg-error "const object" } > + return 0; > +} > + > +static_assert(quux() == 0, ""); // { dg-error "non-constant" } >
On Mon, 13 Apr 2020, Jason Merrill wrote: > On 4/12/20 9:43 AM, Patrick Palka wrote: > > On Sat, 11 Apr 2020, Jason Merrill wrote: > > > > > On 4/10/20 5:47 PM, Patrick Palka wrote: > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > On 4/10/20 2:15 PM, Patrick Palka wrote: > > > > > > On Fri, 10 Apr 2020, Patrick Palka wrote: > > > > > > > > > > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > On 4/10/20 1:04 PM, Patrick Palka wrote: > > > > > > > > > On Thu, 9 Apr 2020, Patrick Palka wrote: > > > > > > > > > > On Thu, 9 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > > > > > > > On 4/8/20 7:49 PM, Patrick Palka wrote: > > > > > > > > > > > > When evaluating the initializer of 'a' in the following > > > > > > > > > > > > example > > > > > > > > > > > > > > > > > > > > > > > > struct A { A *p = this; }; > > > > > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > > > > > constexpr A a = foo(); > > > > > > > > > > > > > > > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate > > > > > > > > > > > > initializer > > > > > > > > > > > > returned > > > > > > > > > > > > by foo > > > > > > > > > > > > gets resolved to the RESULT_DECL of foo. But due to > > > > > > > > > > > > guaranteed > > > > > > > > > > > > RVO, > > > > > > > > > > > > the > > > > > > > > > > > > 'this' > > > > > > > > > > > > should really be resolved to '&a'. > > > > > > > > > > > > > > > > > > > > > > > It seems to me that the right approach would be to > > > > > > > > > > > > immediately > > > > > > > > > > > > resolve > > > > > > > > > > > > the > > > > > > > > > > > > PLACEHOLDER_EXPR to the correct target object during > > > > > > > > > > > > evaluation > > > > > > > > > > > > of > > > > > > > > > > > > 'foo()', > > > > > > > > > > > > so > > > > > > > > > > > > that we could use 'this' to access objects adjacent to > > > > > > > > > > > > the > > > > > > > > > > > > current > > > > > > > > > > > > object in > > > > > > > > > > > > the > > > > > > > > > > > > ultimate storage location. (I think #c2 of PR c++/94537 > > > > > > > > > > > > is > > > > > > > > > > > > an > > > > > > > > > > > > example > > > > > > > > > > > > of > > > > > > > > > > > > such > > > > > > > > > > > > usage of 'this', which currently doesn't work. But as > > > > > > > > > > > > #c1 > > > > > > > > > > > > shows > > > > > > > > > > > > we > > > > > > > > > > > > don't > > > > > > > > > > > > seem > > > > > > > > > > > > to handle this case correctly in non-constexpr > > > > > > > > > > > > initialization > > > > > > > > > > > > either.) > > > > > > > > > > > > > > > > > > > > > > As I commented in the PR, the standard doesn't require > > > > > > > > > > > this to > > > > > > > > > > > work > > > > > > > > > > > because A > > > > > > > > > > > is trivially copyable, and our ABI makes it impossible. > > > > > > > > > > > But > > > > > > > > > > > there's > > > > > > > > > > > still a > > > > > > > > > > > constexpr bug when we add > > > > > > > > > > > > > > > > > > > > > > A() = default; A(const A&); > > > > > > > > > > > > > > > > > > > > > > clang doesn't require the constructors to make this work > > > > > > > > > > > for > > > > > > > > > > > constant > > > > > > > > > > > initialization, but similarly can't make it work for > > > > > > > > > > > non-constant > > > > > > > > > > > initialization. > > > > > > > > > > > > > > > > > > > > That makes a lot of sense, thanks for the detailed > > > > > > > > > > explanation. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I haven't yet been able to make a solution using the > > > > > > > > > > > > above > > > > > > > > > > > > approach > > > > > > > > > > > > work -- > > > > > > > > > > > > making sure we use the ultimate object instead of the > > > > > > > > > > > > RESULT_DECL > > > > > > > > > > > > whenever > > > > > > > > > > > > we > > > > > > > > > > > > access ctx->global->values is proving to be tricky and > > > > > > > > > > > > subtle. > > > > > > > > > > > > > > > > > > > > > > Do we need to go through ctx->global->values? Would it > > > > > > > > > > > work > > > > > > > > > > > for > > > > > > > > > > > the > > > > > > > > > > > RESULT_DECL case in cxx_eval_constant_expression to go to > > > > > > > > > > > straight > > > > > > > > > > > to > > > > > > > > > > > ctx->object or ctx->ctor instead? > > > > > > > > > > > > > > > > > > > > I attempted that at some point, but IIRC we still end up not > > > > > > > > > > resolving > > > > > > > > > > some RESULT_DECLs because not all of them get processed > > > > > > > > > > through > > > > > > > > > > cxx_eval_constant_expression before we manipulate > > > > > > > > > > ctx->global->values > > > > > > > > > > with them. I'll try this approach more carefully and report > > > > > > > > > > back > > > > > > > > > > with > > > > > > > > > > more specifics. > > > > > > > > > > > > > > > > > > It turns out that immediately resolving RESULT_DECLs/'this' to > > > > > > > > > the > > > > > > > > > ultimate ctx->object would not interact well with > > > > > > > > > constexpr_call > > > > > > > > > caching: > > > > > > > > > > > > > > > > > > struct A { A() = default; A(const A&); A *ap = this; }; > > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > > constexpr A a = foo(); > > > > > > > > > constexpr A b = foo(); > > > > > > > > > static_assert(b.ap == &b); // fails > > > > > > > > > > > > > > > > > > Evaluation of the first call to foo() returns {&a}, since we > > > > > > > > > resolve > > > > > > > > > 'this' to &a due to guaranteed RVO, and we cache this result. > > > > > > > > > Evaluation of the second call to foo() just returns the cached > > > > > > > > > result > > > > > > > > > from the constexpr_call cache, and so we get {&a} again. > > > > > > > > > > > > > > > > > > So it seems we would have to mark the result of a constexpr > > > > > > > > > call > > > > > > > > > as > > > > > > > > > not > > > > > > > > > cacheable whenever this RVO applies during its evaluation, > > > > > > > > > even > > > > > > > > > when > > > > > > > > > doing the RVO has no observable difference in the final result > > > > > > > > > (i.e. > > > > > > > > > the > > > > > > > > > constructor does not try to save the 'this' pointer). > > > > > > > > > > > > > > > > > > Would the performance impact of disabling caching whenever RVO > > > > > > > > > applies > > > > > > > > > during constexpr call evaluation be worth it, or should we go > > > > > > > > > with > > > > > > > > > something like my first patch which "almost works," and which > > > > > > > > > marks a > > > > > > > > > constexpr call as not cacheable only when we replace a > > > > > > > > > RESULT_DECL > > > > > > > > > in > > > > > > > > > the result of the call? > > > > > > > > > > > > > > > > Could we search through the result of the call for ctx->object > > > > > > > > and > > > > > > > > cache > > > > > > > > if we > > > > > > > > don't find it? > > > > > > > > > > > > > > Hmm, I think the result of the call could still depend on > > > > > > > ctx->object > > > > > > > without ctx->object explicitly appearing in the result. Consider > > > > > > > the > > > > > > > following testcase: > > > > > > > > > > > > > > struct A { > > > > > > > A() = default; A(const A&); > > > > > > > constexpr A(const A *q) : d{this - p} { } > > > > > > > > > > > > Oops sorry, that 'q' should be a 'p'. > > > > > > > > > > > > > long d = 0; > > > > > > > }; > > > > > > > > > > > > > > constexpr A baz(const A *q) { return A(p); }; > > > > > > > > > > > > And same here. > > > > > > > > > > > > > constexpr A a = baz(&a); > > > > > > > constexpr A b = baz(&a); // no error > > > > > > > > > > > > > > The initialization of 'b' should be ill-formed, but the result of > > > > > > > the > > > > > > > first call to baz(&a) would be {0}, so we would cache it and then > > > > > > > reuse > > > > > > > the result when initializing 'b'. > > > > > > > > > > Ah, true. Can we still cache if we're initializing something that > > > > > isn't > > > > > TREE_STATIC? > > > > > > > > Hmm, we correctly compile the analogous non-TREE_STATIC testcase > > > > > > > > struct A { > > > > A() = default; A(const A&); > > > > constexpr A(const A *p) : d{this == p} { } > > > > bool d; > > > > }; > > > > > > > > constexpr A baz(const A *p) { return A(p); } > > > > > > > > void foo() { > > > > constexpr A x = baz(&x); > > > > constexpr A y = baz(&x); > > > > static_assert(!y.d); > > > > } > > > > > > > > because the calls 'baz(&x)' are considered to have non-constant > > > > arguments. > > > > > > > > > > > > Unfortunately, we can come up with another trick to fool the > > > > constexpr_call > > > > cache in the presence of RVO even in the !TREE_STATIC case: > > > > > > > > struct B { > > > > B() = default; B(const B&); > > > > constexpr B(int) : q{!this[-1].q} { } > > > > bool q = false; > > > > }; > > > > > > > > constexpr B baz() { return B(0); } > > > > > > > > void foo() > > > > { > > > > constexpr B d[2] = { {}, baz() }; > > > > constexpr B e = baz(); > > > > } > > > > > > > > The initialization of the local variable 'e' should be invalid, but if > > > > we > > > > cached the first call to 'baz' then we would wrongly accept it. > > > > > > Right. > > > > > > We ought to be able to distinguish between uses of the RESULT_DECL only > > > for > > > storing to it (cacheable) and any other use, either reading from it or > > > messing > > > with its address. But I think that's a future direction, and we should > > > stick > > > with your patch that almost works for GCC 10. > > > > Sounds good. Does the following then look OK to commit? > > > > -- >8 -- > > > > Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call > > [PR94034] > > > > When evaluating the initializer of 'a' in the following example > > > > struct A { > > A() = default; A(const A&); > > A *p = this; > > }; > > constexpr A foo() { return {}; } > > constexpr A a = foo(); > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo > > gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the > > 'this' > > should really be resolved to '&a'. > > > > Fixing this properly by immediately resolving 'this' and PLACEHOLDER_EXPRs > > to > > the ultimate object under construction would in general mean that we would > > no > > longer be able to cache constexpr calls for which RVO possibly applies, > > because > > the result of the call may now depend on the ultimate object under > > construction. > > > > So as a mostly correct stopgap solution that retains cachability of RVO'd > > constexpr calls, this patch fixes this issue by rewriting all occurrences of > > the > > RESULT_DECL in the result of a constexpr function call with the current > > object > > under construction, after the call returns. This means the 'this' pointer > > during construction of the temporary will still point to the temporary > > object > > instead of the ultimate object, but besides that this approach seems > > functionally equivalent to the proper approach. > > > > Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, does this > > look > > OK to commit? > > > > gcc/cp/ChangeLog: > > > > PR c++/94034 > > * constexpr.c (replace_result_decl_data): New struct. > > (replace_result_decl_data_r): New function. > > (replace_result_decl): New function. > > (cxx_eval_call_expression): Use it. > > * tree.c (build_aggr_init_expr): Propagate the location of the > > initializer to the AGGR_INIT_EXPR. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/94034 > > * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. > > * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. > > * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. > > * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. > > --- > > gcc/cp/constexpr.c | 51 +++++++++++++++++++ > > gcc/cp/tree.c | 3 ++ > > .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++ > > .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ > > .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 ++++++++++++++++++ > > .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ > > 6 files changed, 204 insertions(+) > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > index 5793430c88d..4cf5812e8a5 100644 > > --- a/gcc/cp/constexpr.c > > +++ b/gcc/cp/constexpr.c > > @@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, > > tree call, > > return cp_build_addr_expr (obj, complain); > > } > > +/* Data structure used by replace_result_decl and replace_result_decl_r. > > */ > > + > > +struct replace_result_decl_data > > +{ > > + /* The RESULT_DECL we want to replace. */ > > + tree decl; > > + /* The replacement for DECL. */ > > + tree replacement; > > + /* Whether we've performed any replacements. */ > > + bool changed; > > +}; > > + > > +/* Helper function for replace_result_decl, called through cp_walk_tree. > > */ > > + > > +static tree > > +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) > > +{ > > + replace_result_decl_data *d = (replace_result_decl_data *) data; > > + > > + if (*tp == d->decl) > > + { > > + *tp = unshare_expr (d->replacement); > > + d->changed = true; > > + *walk_subtrees = 0; > > + } > > + else if (TYPE_P (*tp)) > > + *walk_subtrees = 0; > > + > > + return NULL_TREE; > > +} > > + > > +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy > > of) > > + REPLACEMENT within *TP. Returns true iff a replacement was performed. > > */ > > + > > +static bool > > +replace_result_decl (tree *tp, tree decl, tree replacement) > > +{ > > + gcc_assert (TREE_CODE (decl) == RESULT_DECL); > > + replace_result_decl_data data = { decl, replacement, false }; > > + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); > > + return data.changed; > > +} > > + > > /* Subroutine of cxx_eval_constant_expression. > > Evaluate the call expression tree T in the context of OLD_CALL > > expression > > evaluation. */ > > @@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, > > tree t, > > break; > > } > > } > > + > > + /* Rewrite all occurrences of the function's RESULT_DECL with the > > + current object under construction. */ > > + if (ctx->object > > + && (same_type_ignoring_top_level_qualifiers_p > > + (TREE_TYPE (res), TREE_TYPE (ctx->object)))) > > When can they have different types? During prior testing I tried replacing the same_type_p check with an assert, i.e.: if (ctx->object && AGGREGATE_TYPE_P (TREE_TYPE (res))) { gcc_assert (same_type_ignoring_top_level_qualifiers_p (TREE_TYPE (res), TREE_TYPE (ctx->object)); ... } and IIRC I was able to trigger the assert only when the type of ctx->object and of the RESULT_DECL are distinct empty class types. Here's a testcase: struct empty1 { }; constexpr empty1 foo1() { return {}; } struct empty2 { }; constexpr empty2 foo2(empty1) { return {}; } constexpr empty2 a = foo2(foo1()); The initializer of 'a' has the form TARGET_EXPR <D.2389, foo2 (foo1 ();, <<< Unknown tree: empty_class_expr >>>;)>; and so during evaluation of 'foo1()', ctx->object still points to 'a' and we trip over the same_type_p assert. (Is it possible that the call to foo1 is missing a TARGET_EXPR?) Either way, it would be safe to skip empty class types. Should we change the condition to if (ctx->object && AGGREGATE_TYPE_P (TREE_TYPE (res)) && !is_empty_class (TREE_TYPE (res))) { ... } and turn the same_type_p check into an assert? > > > + if (replace_result_decl (&result, res, ctx->object)) > > + cacheable = false; > > } > > else > > /* Couldn't get a function copy to evaluate. */ > > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > > index d1192b7e094..1d49d43c229 100644 > > --- a/gcc/cp/tree.c > > +++ b/gcc/cp/tree.c > > @@ -669,6 +669,9 @@ build_aggr_init_expr (tree type, tree init) > > else > > rval = init; > > + if (location_t loc = EXPR_LOCATION (init)) > > + SET_EXPR_LOCATION (rval, loc); > > + > > return rval; > > } > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > > b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > > new file mode 100644 > > index 00000000000..bb844b952e2 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > > @@ -0,0 +1,26 @@ > > +// PR c++/94034 > > +// { dg-do compile { target c++14 } } > > + > > +struct A { > > + A *ap = this; > > +}; > > + > > +constexpr A foo() > > +{ > > + return {}; > > +} > > + > > +constexpr A bar() > > +{ > > + return foo(); > > +} > > + > > +void > > +baz() > > +{ > > + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant > > expression" } > > + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant > > expression" } > > +} > > + > > +constexpr A a = foo(); > > +constexpr A b = bar(); > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > > b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > > new file mode 100644 > > index 00000000000..f847fe9809f > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > > @@ -0,0 +1,27 @@ > > +// PR c++/94034 > > +// { dg-do compile { target c++14 } } > > + > > +struct A { > > + A() = default; A(const A&); > > + A *ap = this; > > +}; > > + > > +constexpr A foo() > > +{ > > + return {}; > > +} > > + > > +constexpr A bar() > > +{ > > + return foo(); > > +} > > + > > +void > > +baz() > > +{ > > + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant > > expression" } > > + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant > > expression" } > > +} > > + > > +constexpr A a = foo(); > > +constexpr A b = bar(); > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > > b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > > new file mode 100644 > > index 00000000000..5a40cd0b845 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > > @@ -0,0 +1,49 @@ > > +// PR c++/94034 > > +// { dg-do compile { target c++14 } } > > + > > +struct A > > +{ > > + A *p = this; > > + int n = 2; > > + int m = p->n++; > > +}; > > + > > +constexpr A > > +foo() > > +{ > > + return {}; > > +} > > + > > +constexpr A > > +bar() > > +{ > > + A a = foo(); > > + a.p->n = 5; > > + return a; > > +} > > + > > +static_assert(bar().n == 5, ""); > > + > > +constexpr int > > +baz() > > +{ > > + A b = foo(); > > + b.p->n = 10; > > + A c = foo(); > > + if (c.p->n != 3 || c.p->m != 2) > > + __builtin_abort(); > > + bar(); > > + return 0; > > +} > > + > > +static_assert(baz() == 0, ""); > > + > > +constexpr int > > +quux() > > +{ > > + const A d = foo(); > > + d.p->n++; // { dg-error "const object" } > > + return 0; > > +} > > + > > +static_assert(quux() == 0, ""); // { dg-error "non-constant" } > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > new file mode 100644 > > index 00000000000..86d8ab4e759 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > @@ -0,0 +1,48 @@ > > +// PR c++/94034 > > +// { dg-do compile { target c++14 } } > > + > > +struct A > > +{ > > + A() = default; A(const A&); > > + A *p = this; > > + int n = 2; > > + int m = p->n++; > > +}; > > + > > +constexpr A > > +foo() > > +{ > > + return {}; > > +} > > + > > +constexpr A > > +bar() > > +{ > > + A a = foo(); > > + a.p->n = 5; > > + return a; // { dg-error "non-.constexpr." } > > +} > > + > > +constexpr int > > +baz() > > +{ > > + A b = foo(); > > + b.p->n = 10; > > + A c = foo(); > > + if (c.p->n != 3 || c.p->m != 2) > > + __builtin_abort(); > > + foo(); > > + return 0; > > +} > > + > > +static_assert(baz() == 0, ""); > > + > > +constexpr int > > +quux() > > +{ > > + const A d = foo(); > > + d.p->n++; // { dg-error "const object" } > > + return 0; > > +} > > + > > +static_assert(quux() == 0, ""); // { dg-error "non-constant" } > > > >
On 4/13/20 2:49 PM, Patrick Palka wrote: > On Mon, 13 Apr 2020, Jason Merrill wrote: > >> On 4/12/20 9:43 AM, Patrick Palka wrote: >>> On Sat, 11 Apr 2020, Jason Merrill wrote: >>> >>>> On 4/10/20 5:47 PM, Patrick Palka wrote: >>>>> On Fri, 10 Apr 2020, Jason Merrill wrote: >>>>>> On 4/10/20 2:15 PM, Patrick Palka wrote: >>>>>>> On Fri, 10 Apr 2020, Patrick Palka wrote: >>>>>>> >>>>>>>> On Fri, 10 Apr 2020, Jason Merrill wrote: >>>>>>>> >>>>>>>>> On 4/10/20 1:04 PM, Patrick Palka wrote: >>>>>>>>>> On Thu, 9 Apr 2020, Patrick Palka wrote: >>>>>>>>>>> On Thu, 9 Apr 2020, Jason Merrill wrote: >>>>>>>>>>> >>>>>>>>>>>> On 4/8/20 7:49 PM, Patrick Palka wrote: >>>>>>>>>>>>> When evaluating the initializer of 'a' in the following >>>>>>>>>>>>> example >>>>>>>>>>>>> >>>>>>>>>>>>> struct A { A *p = this; }; >>>>>>>>>>>>> constexpr A foo() { return {}; } >>>>>>>>>>>>> constexpr A a = foo(); >>>>>>>>>>>>> >>>>>>>>>>>>> the PLACEHOLDER_EXPR for 'this' in the aggregate >>>>>>>>>>>>> initializer >>>>>>>>>>>>> returned >>>>>>>>>>>>> by foo >>>>>>>>>>>>> gets resolved to the RESULT_DECL of foo. But due to >>>>>>>>>>>>> guaranteed >>>>>>>>>>>>> RVO, >>>>>>>>>>>>> the >>>>>>>>>>>>> 'this' >>>>>>>>>>>>> should really be resolved to '&a'. >>>>>>>>>>>> >>>>>>>>>>>>> It seems to me that the right approach would be to >>>>>>>>>>>>> immediately >>>>>>>>>>>>> resolve >>>>>>>>>>>>> the >>>>>>>>>>>>> PLACEHOLDER_EXPR to the correct target object during >>>>>>>>>>>>> evaluation >>>>>>>>>>>>> of >>>>>>>>>>>>> 'foo()', >>>>>>>>>>>>> so >>>>>>>>>>>>> that we could use 'this' to access objects adjacent to >>>>>>>>>>>>> the >>>>>>>>>>>>> current >>>>>>>>>>>>> object in >>>>>>>>>>>>> the >>>>>>>>>>>>> ultimate storage location. (I think #c2 of PR c++/94537 >>>>>>>>>>>>> is >>>>>>>>>>>>> an >>>>>>>>>>>>> example >>>>>>>>>>>>> of >>>>>>>>>>>>> such >>>>>>>>>>>>> usage of 'this', which currently doesn't work. But as >>>>>>>>>>>>> #c1 >>>>>>>>>>>>> shows >>>>>>>>>>>>> we >>>>>>>>>>>>> don't >>>>>>>>>>>>> seem >>>>>>>>>>>>> to handle this case correctly in non-constexpr >>>>>>>>>>>>> initialization >>>>>>>>>>>>> either.) >>>>>>>>>>>> >>>>>>>>>>>> As I commented in the PR, the standard doesn't require >>>>>>>>>>>> this to >>>>>>>>>>>> work >>>>>>>>>>>> because A >>>>>>>>>>>> is trivially copyable, and our ABI makes it impossible. >>>>>>>>>>>> But >>>>>>>>>>>> there's >>>>>>>>>>>> still a >>>>>>>>>>>> constexpr bug when we add >>>>>>>>>>>> >>>>>>>>>>>> A() = default; A(const A&); >>>>>>>>>>>> >>>>>>>>>>>> clang doesn't require the constructors to make this work >>>>>>>>>>>> for >>>>>>>>>>>> constant >>>>>>>>>>>> initialization, but similarly can't make it work for >>>>>>>>>>>> non-constant >>>>>>>>>>>> initialization. >>>>>>>>>>> >>>>>>>>>>> That makes a lot of sense, thanks for the detailed >>>>>>>>>>> explanation. >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> I haven't yet been able to make a solution using the >>>>>>>>>>>>> above >>>>>>>>>>>>> approach >>>>>>>>>>>>> work -- >>>>>>>>>>>>> making sure we use the ultimate object instead of the >>>>>>>>>>>>> RESULT_DECL >>>>>>>>>>>>> whenever >>>>>>>>>>>>> we >>>>>>>>>>>>> access ctx->global->values is proving to be tricky and >>>>>>>>>>>>> subtle. >>>>>>>>>>>> >>>>>>>>>>>> Do we need to go through ctx->global->values? Would it >>>>>>>>>>>> work >>>>>>>>>>>> for >>>>>>>>>>>> the >>>>>>>>>>>> RESULT_DECL case in cxx_eval_constant_expression to go to >>>>>>>>>>>> straight >>>>>>>>>>>> to >>>>>>>>>>>> ctx->object or ctx->ctor instead? >>>>>>>>>>> >>>>>>>>>>> I attempted that at some point, but IIRC we still end up not >>>>>>>>>>> resolving >>>>>>>>>>> some RESULT_DECLs because not all of them get processed >>>>>>>>>>> through >>>>>>>>>>> cxx_eval_constant_expression before we manipulate >>>>>>>>>>> ctx->global->values >>>>>>>>>>> with them. I'll try this approach more carefully and report >>>>>>>>>>> back >>>>>>>>>>> with >>>>>>>>>>> more specifics. >>>>>>>>>> >>>>>>>>>> It turns out that immediately resolving RESULT_DECLs/'this' to >>>>>>>>>> the >>>>>>>>>> ultimate ctx->object would not interact well with >>>>>>>>>> constexpr_call >>>>>>>>>> caching: >>>>>>>>>> >>>>>>>>>> struct A { A() = default; A(const A&); A *ap = this; }; >>>>>>>>>> constexpr A foo() { return {}; } >>>>>>>>>> constexpr A a = foo(); >>>>>>>>>> constexpr A b = foo(); >>>>>>>>>> static_assert(b.ap == &b); // fails >>>>>>>>>> >>>>>>>>>> Evaluation of the first call to foo() returns {&a}, since we >>>>>>>>>> resolve >>>>>>>>>> 'this' to &a due to guaranteed RVO, and we cache this result. >>>>>>>>>> Evaluation of the second call to foo() just returns the cached >>>>>>>>>> result >>>>>>>>>> from the constexpr_call cache, and so we get {&a} again. >>>>>>>>>> >>>>>>>>>> So it seems we would have to mark the result of a constexpr >>>>>>>>>> call >>>>>>>>>> as >>>>>>>>>> not >>>>>>>>>> cacheable whenever this RVO applies during its evaluation, >>>>>>>>>> even >>>>>>>>>> when >>>>>>>>>> doing the RVO has no observable difference in the final result >>>>>>>>>> (i.e. >>>>>>>>>> the >>>>>>>>>> constructor does not try to save the 'this' pointer). >>>>>>>>>> >>>>>>>>>> Would the performance impact of disabling caching whenever RVO >>>>>>>>>> applies >>>>>>>>>> during constexpr call evaluation be worth it, or should we go >>>>>>>>>> with >>>>>>>>>> something like my first patch which "almost works," and which >>>>>>>>>> marks a >>>>>>>>>> constexpr call as not cacheable only when we replace a >>>>>>>>>> RESULT_DECL >>>>>>>>>> in >>>>>>>>>> the result of the call? >>>>>>>>> >>>>>>>>> Could we search through the result of the call for ctx->object >>>>>>>>> and >>>>>>>>> cache >>>>>>>>> if we >>>>>>>>> don't find it? >>>>>>>> >>>>>>>> Hmm, I think the result of the call could still depend on >>>>>>>> ctx->object >>>>>>>> without ctx->object explicitly appearing in the result. Consider >>>>>>>> the >>>>>>>> following testcase: >>>>>>>> >>>>>>>> struct A { >>>>>>>> A() = default; A(const A&); >>>>>>>> constexpr A(const A *q) : d{this - p} { } >>>>>>> >>>>>>> Oops sorry, that 'q' should be a 'p'. >>>>>>> >>>>>>>> long d = 0; >>>>>>>> }; >>>>>>>> >>>>>>>> constexpr A baz(const A *q) { return A(p); }; >>>>>>> >>>>>>> And same here. >>>>>>> >>>>>>>> constexpr A a = baz(&a); >>>>>>>> constexpr A b = baz(&a); // no error >>>>>>>> >>>>>>>> The initialization of 'b' should be ill-formed, but the result of >>>>>>>> the >>>>>>>> first call to baz(&a) would be {0}, so we would cache it and then >>>>>>>> reuse >>>>>>>> the result when initializing 'b'. >>>>>> >>>>>> Ah, true. Can we still cache if we're initializing something that >>>>>> isn't >>>>>> TREE_STATIC? >>>>> >>>>> Hmm, we correctly compile the analogous non-TREE_STATIC testcase >>>>> >>>>> struct A { >>>>> A() = default; A(const A&); >>>>> constexpr A(const A *p) : d{this == p} { } >>>>> bool d; >>>>> }; >>>>> >>>>> constexpr A baz(const A *p) { return A(p); } >>>>> >>>>> void foo() { >>>>> constexpr A x = baz(&x); >>>>> constexpr A y = baz(&x); >>>>> static_assert(!y.d); >>>>> } >>>>> >>>>> because the calls 'baz(&x)' are considered to have non-constant >>>>> arguments. >>>>> >>>>> >>>>> Unfortunately, we can come up with another trick to fool the >>>>> constexpr_call >>>>> cache in the presence of RVO even in the !TREE_STATIC case: >>>>> >>>>> struct B { >>>>> B() = default; B(const B&); >>>>> constexpr B(int) : q{!this[-1].q} { } >>>>> bool q = false; >>>>> }; >>>>> >>>>> constexpr B baz() { return B(0); } >>>>> >>>>> void foo() >>>>> { >>>>> constexpr B d[2] = { {}, baz() }; >>>>> constexpr B e = baz(); >>>>> } >>>>> >>>>> The initialization of the local variable 'e' should be invalid, but if >>>>> we >>>>> cached the first call to 'baz' then we would wrongly accept it. >>>> >>>> Right. >>>> >>>> We ought to be able to distinguish between uses of the RESULT_DECL only >>>> for >>>> storing to it (cacheable) and any other use, either reading from it or >>>> messing >>>> with its address. But I think that's a future direction, and we should >>>> stick >>>> with your patch that almost works for GCC 10. >>> >>> Sounds good. Does the following then look OK to commit? >>> >>> -- >8 -- >>> >>> Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call >>> [PR94034] >>> >>> When evaluating the initializer of 'a' in the following example >>> >>> struct A { >>> A() = default; A(const A&); >>> A *p = this; >>> }; >>> constexpr A foo() { return {}; } >>> constexpr A a = foo(); >>> >>> the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo >>> gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the >>> 'this' >>> should really be resolved to '&a'. >>> >>> Fixing this properly by immediately resolving 'this' and PLACEHOLDER_EXPRs >>> to >>> the ultimate object under construction would in general mean that we would >>> no >>> longer be able to cache constexpr calls for which RVO possibly applies, >>> because >>> the result of the call may now depend on the ultimate object under >>> construction. >>> >>> So as a mostly correct stopgap solution that retains cachability of RVO'd >>> constexpr calls, this patch fixes this issue by rewriting all occurrences of >>> the >>> RESULT_DECL in the result of a constexpr function call with the current >>> object >>> under construction, after the call returns. This means the 'this' pointer >>> during construction of the temporary will still point to the temporary >>> object >>> instead of the ultimate object, but besides that this approach seems >>> functionally equivalent to the proper approach. >>> >>> Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, does this >>> look >>> OK to commit? >>> >>> gcc/cp/ChangeLog: >>> >>> PR c++/94034 >>> * constexpr.c (replace_result_decl_data): New struct. >>> (replace_result_decl_data_r): New function. >>> (replace_result_decl): New function. >>> (cxx_eval_call_expression): Use it. >>> * tree.c (build_aggr_init_expr): Propagate the location of the >>> initializer to the AGGR_INIT_EXPR. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR c++/94034 >>> * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. >>> * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. >>> * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. >>> * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. >>> --- >>> gcc/cp/constexpr.c | 51 +++++++++++++++++++ >>> gcc/cp/tree.c | 3 ++ >>> .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++ >>> .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ >>> .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 ++++++++++++++++++ >>> .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ >>> 6 files changed, 204 insertions(+) >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C >>> >>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >>> index 5793430c88d..4cf5812e8a5 100644 >>> --- a/gcc/cp/constexpr.c >>> +++ b/gcc/cp/constexpr.c >>> @@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, >>> tree call, >>> return cp_build_addr_expr (obj, complain); >>> } >>> +/* Data structure used by replace_result_decl and replace_result_decl_r. >>> */ >>> + >>> +struct replace_result_decl_data >>> +{ >>> + /* The RESULT_DECL we want to replace. */ >>> + tree decl; >>> + /* The replacement for DECL. */ >>> + tree replacement; >>> + /* Whether we've performed any replacements. */ >>> + bool changed; >>> +}; >>> + >>> +/* Helper function for replace_result_decl, called through cp_walk_tree. >>> */ >>> + >>> +static tree >>> +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) >>> +{ >>> + replace_result_decl_data *d = (replace_result_decl_data *) data; >>> + >>> + if (*tp == d->decl) >>> + { >>> + *tp = unshare_expr (d->replacement); >>> + d->changed = true; >>> + *walk_subtrees = 0; >>> + } >>> + else if (TYPE_P (*tp)) >>> + *walk_subtrees = 0; >>> + >>> + return NULL_TREE; >>> +} >>> + >>> +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy >>> of) >>> + REPLACEMENT within *TP. Returns true iff a replacement was performed. >>> */ >>> + >>> +static bool >>> +replace_result_decl (tree *tp, tree decl, tree replacement) >>> +{ >>> + gcc_assert (TREE_CODE (decl) == RESULT_DECL); >>> + replace_result_decl_data data = { decl, replacement, false }; >>> + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); >>> + return data.changed; >>> +} >>> + >>> /* Subroutine of cxx_eval_constant_expression. >>> Evaluate the call expression tree T in the context of OLD_CALL >>> expression >>> evaluation. */ >>> @@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, >>> tree t, >>> break; >>> } >>> } >>> + >>> + /* Rewrite all occurrences of the function's RESULT_DECL with the >>> + current object under construction. */ >>> + if (ctx->object >>> + && (same_type_ignoring_top_level_qualifiers_p >>> + (TREE_TYPE (res), TREE_TYPE (ctx->object)))) >> >> When can they have different types? > > During prior testing I tried replacing the same_type_p check with an assert, i.e.: > > if (ctx->object > && AGGREGATE_TYPE_P (TREE_TYPE (res))) > { > gcc_assert (same_type_ignoring_top_level_qualifiers_p > (TREE_TYPE (res), TREE_TYPE (ctx->object)); > ... > } > > and IIRC I was able to trigger the assert only when the type of > ctx->object and of the RESULT_DECL are distinct empty class types. > Here's a testcase: > > struct empty1 { }; > constexpr empty1 foo1() { return {}; } > > struct empty2 { }; > constexpr empty2 foo2(empty1) { return {}; } > > constexpr empty2 a = foo2(foo1()); > > The initializer of 'a' has the form > TARGET_EXPR <D.2389, foo2 (foo1 ();, <<< Unknown tree: empty_class_expr >>>;)>; > and so during evaluation of 'foo1()', ctx->object still points to 'a' and we > trip over the same_type_p assert. (Is it possible that the call to foo1 > is missing a TARGET_EXPR?) Hmm, that would be because of > if (is_empty_class (TREE_TYPE (arg)) > && simple_empty_class_p (TREE_TYPE (arg), arg, INIT_EXPR)) > { > while (TREE_CODE (arg) == TARGET_EXPR) > /* We're disconnecting the initializer from its target, > don't create a temporary. */ > arg = TARGET_EXPR_INITIAL (arg); in build_call_a. > Either way, it would be safe to skip empty class types. Should we change the > condition to > > if (ctx->object > && AGGREGATE_TYPE_P (TREE_TYPE (res)) > && !is_empty_class (TREE_TYPE (res))) > { > ... > } > > and turn the same_type_p check into an assert? Sounds good. Let's say a checking_assert. Jason
On Mon, 13 Apr 2020, Jason Merrill wrote: > On 4/13/20 2:49 PM, Patrick Palka wrote: > > On Mon, 13 Apr 2020, Jason Merrill wrote: > > > > > On 4/12/20 9:43 AM, Patrick Palka wrote: > > > > On Sat, 11 Apr 2020, Jason Merrill wrote: > > > > > > > > > On 4/10/20 5:47 PM, Patrick Palka wrote: > > > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > > > On 4/10/20 2:15 PM, Patrick Palka wrote: > > > > > > > > On Fri, 10 Apr 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > > > > > On 4/10/20 1:04 PM, Patrick Palka wrote: > > > > > > > > > > > On Thu, 9 Apr 2020, Patrick Palka wrote: > > > > > > > > > > > > On Thu, 9 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > > > > > > > > > > > On 4/8/20 7:49 PM, Patrick Palka wrote: > > > > > > > > > > > > > > When evaluating the initializer of 'a' in the > > > > > > > > > > > > > > following > > > > > > > > > > > > > > example > > > > > > > > > > > > > > > > > > > > > > > > > > > > struct A { A *p = this; }; > > > > > > > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > > > > > > > constexpr A a = foo(); > > > > > > > > > > > > > > > > > > > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate > > > > > > > > > > > > > > initializer > > > > > > > > > > > > > > returned > > > > > > > > > > > > > > by foo > > > > > > > > > > > > > > gets resolved to the RESULT_DECL of foo. But due to > > > > > > > > > > > > > > guaranteed > > > > > > > > > > > > > > RVO, > > > > > > > > > > > > > > the > > > > > > > > > > > > > > 'this' > > > > > > > > > > > > > > should really be resolved to '&a'. > > > > > > > > > > > > > > > > > > > > > > > > > > > It seems to me that the right approach would be to > > > > > > > > > > > > > > immediately > > > > > > > > > > > > > > resolve > > > > > > > > > > > > > > the > > > > > > > > > > > > > > PLACEHOLDER_EXPR to the correct target object during > > > > > > > > > > > > > > evaluation > > > > > > > > > > > > > > of > > > > > > > > > > > > > > 'foo()', > > > > > > > > > > > > > > so > > > > > > > > > > > > > > that we could use 'this' to access objects adjacent > > > > > > > > > > > > > > to > > > > > > > > > > > > > > the > > > > > > > > > > > > > > current > > > > > > > > > > > > > > object in > > > > > > > > > > > > > > the > > > > > > > > > > > > > > ultimate storage location. (I think #c2 of PR > > > > > > > > > > > > > > c++/94537 > > > > > > > > > > > > > > is > > > > > > > > > > > > > > an > > > > > > > > > > > > > > example > > > > > > > > > > > > > > of > > > > > > > > > > > > > > such > > > > > > > > > > > > > > usage of 'this', which currently doesn't work. But > > > > > > > > > > > > > > as > > > > > > > > > > > > > > #c1 > > > > > > > > > > > > > > shows > > > > > > > > > > > > > > we > > > > > > > > > > > > > > don't > > > > > > > > > > > > > > seem > > > > > > > > > > > > > > to handle this case correctly in non-constexpr > > > > > > > > > > > > > > initialization > > > > > > > > > > > > > > either.) > > > > > > > > > > > > > > > > > > > > > > > > > > As I commented in the PR, the standard doesn't require > > > > > > > > > > > > > this to > > > > > > > > > > > > > work > > > > > > > > > > > > > because A > > > > > > > > > > > > > is trivially copyable, and our ABI makes it > > > > > > > > > > > > > impossible. > > > > > > > > > > > > > But > > > > > > > > > > > > > there's > > > > > > > > > > > > > still a > > > > > > > > > > > > > constexpr bug when we add > > > > > > > > > > > > > > > > > > > > > > > > > > A() = default; A(const A&); > > > > > > > > > > > > > > > > > > > > > > > > > > clang doesn't require the constructors to make this > > > > > > > > > > > > > work > > > > > > > > > > > > > for > > > > > > > > > > > > > constant > > > > > > > > > > > > > initialization, but similarly can't make it work for > > > > > > > > > > > > > non-constant > > > > > > > > > > > > > initialization. > > > > > > > > > > > > > > > > > > > > > > > > That makes a lot of sense, thanks for the detailed > > > > > > > > > > > > explanation. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I haven't yet been able to make a solution using the > > > > > > > > > > > > > > above > > > > > > > > > > > > > > approach > > > > > > > > > > > > > > work -- > > > > > > > > > > > > > > making sure we use the ultimate object instead of > > > > > > > > > > > > > > the > > > > > > > > > > > > > > RESULT_DECL > > > > > > > > > > > > > > whenever > > > > > > > > > > > > > > we > > > > > > > > > > > > > > access ctx->global->values is proving to be tricky > > > > > > > > > > > > > > and > > > > > > > > > > > > > > subtle. > > > > > > > > > > > > > > > > > > > > > > > > > > Do we need to go through ctx->global->values? Would > > > > > > > > > > > > > it > > > > > > > > > > > > > work > > > > > > > > > > > > > for > > > > > > > > > > > > > the > > > > > > > > > > > > > RESULT_DECL case in cxx_eval_constant_expression to go > > > > > > > > > > > > > to > > > > > > > > > > > > > straight > > > > > > > > > > > > > to > > > > > > > > > > > > > ctx->object or ctx->ctor instead? > > > > > > > > > > > > > > > > > > > > > > > > I attempted that at some point, but IIRC we still end up > > > > > > > > > > > > not > > > > > > > > > > > > resolving > > > > > > > > > > > > some RESULT_DECLs because not all of them get processed > > > > > > > > > > > > through > > > > > > > > > > > > cxx_eval_constant_expression before we manipulate > > > > > > > > > > > > ctx->global->values > > > > > > > > > > > > with them. I'll try this approach more carefully and > > > > > > > > > > > > report > > > > > > > > > > > > back > > > > > > > > > > > > with > > > > > > > > > > > > more specifics. > > > > > > > > > > > > > > > > > > > > > > It turns out that immediately resolving > > > > > > > > > > > RESULT_DECLs/'this' to > > > > > > > > > > > the > > > > > > > > > > > ultimate ctx->object would not interact well with > > > > > > > > > > > constexpr_call > > > > > > > > > > > caching: > > > > > > > > > > > > > > > > > > > > > > struct A { A() = default; A(const A&); A *ap = > > > > > > > > > > > this; }; > > > > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > > > > constexpr A a = foo(); > > > > > > > > > > > constexpr A b = foo(); > > > > > > > > > > > static_assert(b.ap == &b); // fails > > > > > > > > > > > > > > > > > > > > > > Evaluation of the first call to foo() returns {&a}, since > > > > > > > > > > > we > > > > > > > > > > > resolve > > > > > > > > > > > 'this' to &a due to guaranteed RVO, and we cache this > > > > > > > > > > > result. > > > > > > > > > > > Evaluation of the second call to foo() just returns the > > > > > > > > > > > cached > > > > > > > > > > > result > > > > > > > > > > > from the constexpr_call cache, and so we get {&a} again. > > > > > > > > > > > > > > > > > > > > > > So it seems we would have to mark the result of a > > > > > > > > > > > constexpr > > > > > > > > > > > call > > > > > > > > > > > as > > > > > > > > > > > not > > > > > > > > > > > cacheable whenever this RVO applies during its evaluation, > > > > > > > > > > > even > > > > > > > > > > > when > > > > > > > > > > > doing the RVO has no observable difference in the final > > > > > > > > > > > result > > > > > > > > > > > (i.e. > > > > > > > > > > > the > > > > > > > > > > > constructor does not try to save the 'this' pointer). > > > > > > > > > > > > > > > > > > > > > > Would the performance impact of disabling caching whenever > > > > > > > > > > > RVO > > > > > > > > > > > applies > > > > > > > > > > > during constexpr call evaluation be worth it, or should we > > > > > > > > > > > go > > > > > > > > > > > with > > > > > > > > > > > something like my first patch which "almost works," and > > > > > > > > > > > which > > > > > > > > > > > marks a > > > > > > > > > > > constexpr call as not cacheable only when we replace a > > > > > > > > > > > RESULT_DECL > > > > > > > > > > > in > > > > > > > > > > > the result of the call? > > > > > > > > > > > > > > > > > > > > Could we search through the result of the call for > > > > > > > > > > ctx->object > > > > > > > > > > and > > > > > > > > > > cache > > > > > > > > > > if we > > > > > > > > > > don't find it? > > > > > > > > > > > > > > > > > > Hmm, I think the result of the call could still depend on > > > > > > > > > ctx->object > > > > > > > > > without ctx->object explicitly appearing in the result. > > > > > > > > > Consider > > > > > > > > > the > > > > > > > > > following testcase: > > > > > > > > > > > > > > > > > > struct A { > > > > > > > > > A() = default; A(const A&); > > > > > > > > > constexpr A(const A *q) : d{this - p} { } > > > > > > > > > > > > > > > > Oops sorry, that 'q' should be a 'p'. > > > > > > > > > > > > > > > > > long d = 0; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > constexpr A baz(const A *q) { return A(p); }; > > > > > > > > > > > > > > > > And same here. > > > > > > > > > > > > > > > > > constexpr A a = baz(&a); > > > > > > > > > constexpr A b = baz(&a); // no error > > > > > > > > > > > > > > > > > > The initialization of 'b' should be ill-formed, but the result > > > > > > > > > of > > > > > > > > > the > > > > > > > > > first call to baz(&a) would be {0}, so we would cache it and > > > > > > > > > then > > > > > > > > > reuse > > > > > > > > > the result when initializing 'b'. > > > > > > > > > > > > > > Ah, true. Can we still cache if we're initializing something that > > > > > > > isn't > > > > > > > TREE_STATIC? > > > > > > > > > > > > Hmm, we correctly compile the analogous non-TREE_STATIC testcase > > > > > > > > > > > > struct A { > > > > > > A() = default; A(const A&); > > > > > > constexpr A(const A *p) : d{this == p} { } > > > > > > bool d; > > > > > > }; > > > > > > > > > > > > constexpr A baz(const A *p) { return A(p); } > > > > > > > > > > > > void foo() { > > > > > > constexpr A x = baz(&x); > > > > > > constexpr A y = baz(&x); > > > > > > static_assert(!y.d); > > > > > > } > > > > > > > > > > > > because the calls 'baz(&x)' are considered to have non-constant > > > > > > arguments. > > > > > > > > > > > > > > > > > > Unfortunately, we can come up with another trick to fool the > > > > > > constexpr_call > > > > > > cache in the presence of RVO even in the !TREE_STATIC case: > > > > > > > > > > > > struct B { > > > > > > B() = default; B(const B&); > > > > > > constexpr B(int) : q{!this[-1].q} { } > > > > > > bool q = false; > > > > > > }; > > > > > > > > > > > > constexpr B baz() { return B(0); } > > > > > > > > > > > > void foo() > > > > > > { > > > > > > constexpr B d[2] = { {}, baz() }; > > > > > > constexpr B e = baz(); > > > > > > } > > > > > > > > > > > > The initialization of the local variable 'e' should be invalid, but > > > > > > if > > > > > > we > > > > > > cached the first call to 'baz' then we would wrongly accept it. > > > > > > > > > > Right. > > > > > > > > > > We ought to be able to distinguish between uses of the RESULT_DECL > > > > > only > > > > > for > > > > > storing to it (cacheable) and any other use, either reading from it or > > > > > messing > > > > > with its address. But I think that's a future direction, and we > > > > > should > > > > > stick > > > > > with your patch that almost works for GCC 10. > > > > > > > > Sounds good. Does the following then look OK to commit? > > > > > > > > -- >8 -- > > > > > > > > Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call > > > > [PR94034] > > > > > > > > When evaluating the initializer of 'a' in the following example > > > > > > > > struct A { > > > > A() = default; A(const A&); > > > > A *p = this; > > > > }; > > > > constexpr A foo() { return {}; } > > > > constexpr A a = foo(); > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by > > > > foo > > > > gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the > > > > 'this' > > > > should really be resolved to '&a'. > > > > > > > > Fixing this properly by immediately resolving 'this' and > > > > PLACEHOLDER_EXPRs > > > > to > > > > the ultimate object under construction would in general mean that we > > > > would > > > > no > > > > longer be able to cache constexpr calls for which RVO possibly applies, > > > > because > > > > the result of the call may now depend on the ultimate object under > > > > construction. > > > > > > > > So as a mostly correct stopgap solution that retains cachability of > > > > RVO'd > > > > constexpr calls, this patch fixes this issue by rewriting all > > > > occurrences of > > > > the > > > > RESULT_DECL in the result of a constexpr function call with the current > > > > object > > > > under construction, after the call returns. This means the 'this' > > > > pointer > > > > during construction of the temporary will still point to the temporary > > > > object > > > > instead of the ultimate object, but besides that this approach seems > > > > functionally equivalent to the proper approach. > > > > > > > > Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, does > > > > this > > > > look > > > > OK to commit? > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > PR c++/94034 > > > > * constexpr.c (replace_result_decl_data): New struct. > > > > (replace_result_decl_data_r): New function. > > > > (replace_result_decl): New function. > > > > (cxx_eval_call_expression): Use it. > > > > * tree.c (build_aggr_init_expr): Propagate the location of the > > > > initializer to the AGGR_INIT_EXPR. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > PR c++/94034 > > > > * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. > > > > * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. > > > > * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. > > > > * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. > > > > --- > > > > gcc/cp/constexpr.c | 51 > > > > +++++++++++++++++++ > > > > gcc/cp/tree.c | 3 ++ > > > > .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++ > > > > .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ > > > > .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 ++++++++++++++++++ > > > > .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ > > > > 6 files changed, 204 insertions(+) > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > > > > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > > > index 5793430c88d..4cf5812e8a5 100644 > > > > --- a/gcc/cp/constexpr.c > > > > +++ b/gcc/cp/constexpr.c > > > > @@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx > > > > *ctx, > > > > tree call, > > > > return cp_build_addr_expr (obj, complain); > > > > } > > > > +/* Data structure used by replace_result_decl and > > > > replace_result_decl_r. > > > > */ > > > > + > > > > +struct replace_result_decl_data > > > > +{ > > > > + /* The RESULT_DECL we want to replace. */ > > > > + tree decl; > > > > + /* The replacement for DECL. */ > > > > + tree replacement; > > > > + /* Whether we've performed any replacements. */ > > > > + bool changed; > > > > +}; > > > > + > > > > +/* Helper function for replace_result_decl, called through > > > > cp_walk_tree. > > > > */ > > > > + > > > > +static tree > > > > +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) > > > > +{ > > > > + replace_result_decl_data *d = (replace_result_decl_data *) data; > > > > + > > > > + if (*tp == d->decl) > > > > + { > > > > + *tp = unshare_expr (d->replacement); > > > > + d->changed = true; > > > > + *walk_subtrees = 0; > > > > + } > > > > + else if (TYPE_P (*tp)) > > > > + *walk_subtrees = 0; > > > > + > > > > + return NULL_TREE; > > > > +} > > > > + > > > > +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared > > > > copy > > > > of) > > > > + REPLACEMENT within *TP. Returns true iff a replacement was > > > > performed. > > > > */ > > > > + > > > > +static bool > > > > +replace_result_decl (tree *tp, tree decl, tree replacement) > > > > +{ > > > > + gcc_assert (TREE_CODE (decl) == RESULT_DECL); > > > > + replace_result_decl_data data = { decl, replacement, false }; > > > > + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); > > > > + return data.changed; > > > > +} > > > > + > > > > /* Subroutine of cxx_eval_constant_expression. > > > > Evaluate the call expression tree T in the context of OLD_CALL > > > > expression > > > > evaluation. */ > > > > @@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const constexpr_ctx > > > > *ctx, > > > > tree t, > > > > break; > > > > } > > > > } > > > > + > > > > + /* Rewrite all occurrences of the function's RESULT_DECL with the > > > > + current object under construction. */ > > > > + if (ctx->object > > > > + && (same_type_ignoring_top_level_qualifiers_p > > > > + (TREE_TYPE (res), TREE_TYPE (ctx->object)))) > > > > > > When can they have different types? > > > > During prior testing I tried replacing the same_type_p check with an assert, > > i.e.: > > > > if (ctx->object > > && AGGREGATE_TYPE_P (TREE_TYPE (res))) > > { > > gcc_assert (same_type_ignoring_top_level_qualifiers_p > > (TREE_TYPE (res), TREE_TYPE (ctx->object)); > > ... > > } > > > > and IIRC I was able to trigger the assert only when the type of > > ctx->object and of the RESULT_DECL are distinct empty class types. > > Here's a testcase: > > > > struct empty1 { }; > > constexpr empty1 foo1() { return {}; } > > > > struct empty2 { }; > > constexpr empty2 foo2(empty1) { return {}; } > > > > constexpr empty2 a = foo2(foo1()); > > > > The initializer of 'a' has the form > > TARGET_EXPR <D.2389, foo2 (foo1 ();, <<< Unknown tree: empty_class_expr > > >>>;)>; > > and so during evaluation of 'foo1()', ctx->object still points to 'a' and we > > trip over the same_type_p assert. (Is it possible that the call to foo1 > > is missing a TARGET_EXPR?) > > Hmm, that would be because of > > > if (is_empty_class (TREE_TYPE (arg)) > > && simple_empty_class_p (TREE_TYPE (arg), arg, INIT_EXPR)) > > { > while (TREE_CODE (arg) == TARGET_EXPR) > > /* We're disconnecting the initializer from its target, > > don't create a temporary. */ > > arg = TARGET_EXPR_INITIAL (arg); > > in build_call_a. Ah okay. > > > Either way, it would be safe to skip empty class types. Should we change > > the > > condition to > > > > if (ctx->object > > && AGGREGATE_TYPE_P (TREE_TYPE (res)) > > && !is_empty_class (TREE_TYPE (res))) > > { > > ... > > } > > > > and turn the same_type_p check into an assert? > > Sounds good. Let's say a checking_assert. Great, I'm bootstrapping/regtesting the following overnight, does it look OK to commit if successful? -- >8 -- Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call [PR94034] gcc/cp/ChangeLog: PR c++/94034 * constexpr.c (replace_result_decl_data): New struct. (replace_result_decl_data_r): New function. (replace_result_decl): New function. (cxx_eval_call_expression): Use it. * tree.c (build_aggr_init_expr): Set the location of the AGGR_INIT_EXPR to that of its initializer. gcc/testsuite/ChangeLog: PR c++/94034 * g++.dg/cpp0x/constexpr-empty15.C: New test. * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. --- gcc/cp/constexpr.c | 53 +++++++++++++++++++ gcc/cp/tree.c | 3 ++ .../g++.dg/cpp0x/constexpr-empty15.C | 9 ++++ .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 +++++++++ .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 +++++++++++++++++ .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ 7 files changed, 215 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index d8636ddb92f..00e313178f9 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -2029,6 +2029,51 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call, return cp_build_addr_expr (obj, complain); } +/* Data structure used by replace_result_decl and replace_result_decl_r. */ + +struct replace_result_decl_data +{ + /* The RESULT_DECL we want to replace. */ + tree decl; + /* The replacement for DECL. */ + tree replacement; + /* Whether we've performed any replacements. */ + bool changed; +}; + +/* Helper function for replace_result_decl, called through cp_walk_tree. */ + +static tree +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) +{ + replace_result_decl_data *d = (replace_result_decl_data *) data; + + if (*tp == d->decl) + { + *tp = unshare_expr (d->replacement); + d->changed = true; + *walk_subtrees = 0; + } + else if (TYPE_P (*tp)) + *walk_subtrees = 0; + + return NULL_TREE; +} + +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy of) + REPLACEMENT within *TP. Returns true iff a replacement was performed. */ + +static bool +replace_result_decl (tree *tp, tree decl, tree replacement) +{ + gcc_checking_assert (TREE_CODE (decl) == RESULT_DECL + && (same_type_ignoring_top_level_qualifiers_p + (TREE_TYPE (decl), TREE_TYPE (replacement)))); + replace_result_decl_data data = { decl, replacement, false }; + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); + return data.changed; +} + /* Subroutine of cxx_eval_constant_expression. Evaluate the call expression tree T in the context of OLD_CALL expression evaluation. */ @@ -2536,6 +2581,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, break; } } + + /* Rewrite all occurrences of the function's RESULT_DECL with the + current object under construction. */ + if (ctx->object + && AGGREGATE_TYPE_P (TREE_TYPE (res)) + && !is_empty_class (TREE_TYPE (res))) + if (replace_result_decl (&result, res, ctx->object)) + cacheable = false; } else /* Couldn't get a function copy to evaluate. */ diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index 1d311b0fe61..8e4934c0009 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -669,6 +669,9 @@ build_aggr_init_expr (tree type, tree init) else rval = init; + if (location_t loc = EXPR_LOCATION (init)) + SET_EXPR_LOCATION (rval, loc); + return rval; } diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C new file mode 100644 index 00000000000..97863d4e1a7 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C @@ -0,0 +1,9 @@ +// { dg-do compile { target c++11 } } + +struct empty1 { }; +constexpr empty1 foo1() { return {}; } + +struct empty2 { }; +constexpr empty2 foo2(empty1) { return {}; } + +constexpr empty2 a = foo2(foo1()); diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C new file mode 100644 index 00000000000..bb844b952e2 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C @@ -0,0 +1,26 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A { + A *ap = this; +}; + +constexpr A foo() +{ + return {}; +} + +constexpr A bar() +{ + return foo(); +} + +void +baz() +{ + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } +} + +constexpr A a = foo(); +constexpr A b = bar(); diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C new file mode 100644 index 00000000000..f847fe9809f --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C @@ -0,0 +1,27 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A { + A() = default; A(const A&); + A *ap = this; +}; + +constexpr A foo() +{ + return {}; +} + +constexpr A bar() +{ + return foo(); +} + +void +baz() +{ + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } +} + +constexpr A a = foo(); +constexpr A b = bar(); diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C new file mode 100644 index 00000000000..5a40cd0b845 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C @@ -0,0 +1,49 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A +{ + A *p = this; + int n = 2; + int m = p->n++; +}; + +constexpr A +foo() +{ + return {}; +} + +constexpr A +bar() +{ + A a = foo(); + a.p->n = 5; + return a; +} + +static_assert(bar().n == 5, ""); + +constexpr int +baz() +{ + A b = foo(); + b.p->n = 10; + A c = foo(); + if (c.p->n != 3 || c.p->m != 2) + __builtin_abort(); + bar(); + return 0; +} + +static_assert(baz() == 0, ""); + +constexpr int +quux() +{ + const A d = foo(); + d.p->n++; // { dg-error "const object" } + return 0; +} + +static_assert(quux() == 0, ""); // { dg-error "non-constant" } diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C new file mode 100644 index 00000000000..86d8ab4e759 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C @@ -0,0 +1,48 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A +{ + A() = default; A(const A&); + A *p = this; + int n = 2; + int m = p->n++; +}; + +constexpr A +foo() +{ + return {}; +} + +constexpr A +bar() +{ + A a = foo(); + a.p->n = 5; + return a; // { dg-error "non-.constexpr." } +} + +constexpr int +baz() +{ + A b = foo(); + b.p->n = 10; + A c = foo(); + if (c.p->n != 3 || c.p->m != 2) + __builtin_abort(); + foo(); + return 0; +} + +static_assert(baz() == 0, ""); + +constexpr int +quux() +{ + const A d = foo(); + d.p->n++; // { dg-error "const object" } + return 0; +} + +static_assert(quux() == 0, ""); // { dg-error "non-constant" }
On Tue, 14 Apr 2020, Patrick Palka wrote: > On Mon, 13 Apr 2020, Jason Merrill wrote: > > > On 4/13/20 2:49 PM, Patrick Palka wrote: > > > On Mon, 13 Apr 2020, Jason Merrill wrote: > > > > > > > On 4/12/20 9:43 AM, Patrick Palka wrote: > > > > > On Sat, 11 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > On 4/10/20 5:47 PM, Patrick Palka wrote: > > > > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > > > > On 4/10/20 2:15 PM, Patrick Palka wrote: > > > > > > > > > On Fri, 10 Apr 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > > > > > > > On 4/10/20 1:04 PM, Patrick Palka wrote: > > > > > > > > > > > > On Thu, 9 Apr 2020, Patrick Palka wrote: > > > > > > > > > > > > > On Thu, 9 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On 4/8/20 7:49 PM, Patrick Palka wrote: > > > > > > > > > > > > > > > When evaluating the initializer of 'a' in the > > > > > > > > > > > > > > > following > > > > > > > > > > > > > > > example > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > struct A { A *p = this; }; > > > > > > > > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > > > > > > > > constexpr A a = foo(); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate > > > > > > > > > > > > > > > initializer > > > > > > > > > > > > > > > returned > > > > > > > > > > > > > > > by foo > > > > > > > > > > > > > > > gets resolved to the RESULT_DECL of foo. But due to > > > > > > > > > > > > > > > guaranteed > > > > > > > > > > > > > > > RVO, > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > 'this' > > > > > > > > > > > > > > > should really be resolved to '&a'. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It seems to me that the right approach would be to > > > > > > > > > > > > > > > immediately > > > > > > > > > > > > > > > resolve > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > PLACEHOLDER_EXPR to the correct target object during > > > > > > > > > > > > > > > evaluation > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > 'foo()', > > > > > > > > > > > > > > > so > > > > > > > > > > > > > > > that we could use 'this' to access objects adjacent > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > current > > > > > > > > > > > > > > > object in > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > ultimate storage location. (I think #c2 of PR > > > > > > > > > > > > > > > c++/94537 > > > > > > > > > > > > > > > is > > > > > > > > > > > > > > > an > > > > > > > > > > > > > > > example > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > such > > > > > > > > > > > > > > > usage of 'this', which currently doesn't work. But > > > > > > > > > > > > > > > as > > > > > > > > > > > > > > > #c1 > > > > > > > > > > > > > > > shows > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > don't > > > > > > > > > > > > > > > seem > > > > > > > > > > > > > > > to handle this case correctly in non-constexpr > > > > > > > > > > > > > > > initialization > > > > > > > > > > > > > > > either.) > > > > > > > > > > > > > > > > > > > > > > > > > > > > As I commented in the PR, the standard doesn't require > > > > > > > > > > > > > > this to > > > > > > > > > > > > > > work > > > > > > > > > > > > > > because A > > > > > > > > > > > > > > is trivially copyable, and our ABI makes it > > > > > > > > > > > > > > impossible. > > > > > > > > > > > > > > But > > > > > > > > > > > > > > there's > > > > > > > > > > > > > > still a > > > > > > > > > > > > > > constexpr bug when we add > > > > > > > > > > > > > > > > > > > > > > > > > > > > A() = default; A(const A&); > > > > > > > > > > > > > > > > > > > > > > > > > > > > clang doesn't require the constructors to make this > > > > > > > > > > > > > > work > > > > > > > > > > > > > > for > > > > > > > > > > > > > > constant > > > > > > > > > > > > > > initialization, but similarly can't make it work for > > > > > > > > > > > > > > non-constant > > > > > > > > > > > > > > initialization. > > > > > > > > > > > > > > > > > > > > > > > > > > That makes a lot of sense, thanks for the detailed > > > > > > > > > > > > > explanation. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I haven't yet been able to make a solution using the > > > > > > > > > > > > > > > above > > > > > > > > > > > > > > > approach > > > > > > > > > > > > > > > work -- > > > > > > > > > > > > > > > making sure we use the ultimate object instead of > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > RESULT_DECL > > > > > > > > > > > > > > > whenever > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > access ctx->global->values is proving to be tricky > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > subtle. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Do we need to go through ctx->global->values? Would > > > > > > > > > > > > > > it > > > > > > > > > > > > > > work > > > > > > > > > > > > > > for > > > > > > > > > > > > > > the > > > > > > > > > > > > > > RESULT_DECL case in cxx_eval_constant_expression to go > > > > > > > > > > > > > > to > > > > > > > > > > > > > > straight > > > > > > > > > > > > > > to > > > > > > > > > > > > > > ctx->object or ctx->ctor instead? > > > > > > > > > > > > > > > > > > > > > > > > > > I attempted that at some point, but IIRC we still end up > > > > > > > > > > > > > not > > > > > > > > > > > > > resolving > > > > > > > > > > > > > some RESULT_DECLs because not all of them get processed > > > > > > > > > > > > > through > > > > > > > > > > > > > cxx_eval_constant_expression before we manipulate > > > > > > > > > > > > > ctx->global->values > > > > > > > > > > > > > with them. I'll try this approach more carefully and > > > > > > > > > > > > > report > > > > > > > > > > > > > back > > > > > > > > > > > > > with > > > > > > > > > > > > > more specifics. > > > > > > > > > > > > > > > > > > > > > > > > It turns out that immediately resolving > > > > > > > > > > > > RESULT_DECLs/'this' to > > > > > > > > > > > > the > > > > > > > > > > > > ultimate ctx->object would not interact well with > > > > > > > > > > > > constexpr_call > > > > > > > > > > > > caching: > > > > > > > > > > > > > > > > > > > > > > > > struct A { A() = default; A(const A&); A *ap = > > > > > > > > > > > > this; }; > > > > > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > > > > > constexpr A a = foo(); > > > > > > > > > > > > constexpr A b = foo(); > > > > > > > > > > > > static_assert(b.ap == &b); // fails > > > > > > > > > > > > > > > > > > > > > > > > Evaluation of the first call to foo() returns {&a}, since > > > > > > > > > > > > we > > > > > > > > > > > > resolve > > > > > > > > > > > > 'this' to &a due to guaranteed RVO, and we cache this > > > > > > > > > > > > result. > > > > > > > > > > > > Evaluation of the second call to foo() just returns the > > > > > > > > > > > > cached > > > > > > > > > > > > result > > > > > > > > > > > > from the constexpr_call cache, and so we get {&a} again. > > > > > > > > > > > > > > > > > > > > > > > > So it seems we would have to mark the result of a > > > > > > > > > > > > constexpr > > > > > > > > > > > > call > > > > > > > > > > > > as > > > > > > > > > > > > not > > > > > > > > > > > > cacheable whenever this RVO applies during its evaluation, > > > > > > > > > > > > even > > > > > > > > > > > > when > > > > > > > > > > > > doing the RVO has no observable difference in the final > > > > > > > > > > > > result > > > > > > > > > > > > (i.e. > > > > > > > > > > > > the > > > > > > > > > > > > constructor does not try to save the 'this' pointer). > > > > > > > > > > > > > > > > > > > > > > > > Would the performance impact of disabling caching whenever > > > > > > > > > > > > RVO > > > > > > > > > > > > applies > > > > > > > > > > > > during constexpr call evaluation be worth it, or should we > > > > > > > > > > > > go > > > > > > > > > > > > with > > > > > > > > > > > > something like my first patch which "almost works," and > > > > > > > > > > > > which > > > > > > > > > > > > marks a > > > > > > > > > > > > constexpr call as not cacheable only when we replace a > > > > > > > > > > > > RESULT_DECL > > > > > > > > > > > > in > > > > > > > > > > > > the result of the call? > > > > > > > > > > > > > > > > > > > > > > Could we search through the result of the call for > > > > > > > > > > > ctx->object > > > > > > > > > > > and > > > > > > > > > > > cache > > > > > > > > > > > if we > > > > > > > > > > > don't find it? > > > > > > > > > > > > > > > > > > > > Hmm, I think the result of the call could still depend on > > > > > > > > > > ctx->object > > > > > > > > > > without ctx->object explicitly appearing in the result. > > > > > > > > > > Consider > > > > > > > > > > the > > > > > > > > > > following testcase: > > > > > > > > > > > > > > > > > > > > struct A { > > > > > > > > > > A() = default; A(const A&); > > > > > > > > > > constexpr A(const A *q) : d{this - p} { } > > > > > > > > > > > > > > > > > > Oops sorry, that 'q' should be a 'p'. > > > > > > > > > > > > > > > > > > > long d = 0; > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > constexpr A baz(const A *q) { return A(p); }; > > > > > > > > > > > > > > > > > > And same here. > > > > > > > > > > > > > > > > > > > constexpr A a = baz(&a); > > > > > > > > > > constexpr A b = baz(&a); // no error > > > > > > > > > > > > > > > > > > > > The initialization of 'b' should be ill-formed, but the result > > > > > > > > > > of > > > > > > > > > > the > > > > > > > > > > first call to baz(&a) would be {0}, so we would cache it and > > > > > > > > > > then > > > > > > > > > > reuse > > > > > > > > > > the result when initializing 'b'. > > > > > > > > > > > > > > > > Ah, true. Can we still cache if we're initializing something that > > > > > > > > isn't > > > > > > > > TREE_STATIC? > > > > > > > > > > > > > > Hmm, we correctly compile the analogous non-TREE_STATIC testcase > > > > > > > > > > > > > > struct A { > > > > > > > A() = default; A(const A&); > > > > > > > constexpr A(const A *p) : d{this == p} { } > > > > > > > bool d; > > > > > > > }; > > > > > > > > > > > > > > constexpr A baz(const A *p) { return A(p); } > > > > > > > > > > > > > > void foo() { > > > > > > > constexpr A x = baz(&x); > > > > > > > constexpr A y = baz(&x); > > > > > > > static_assert(!y.d); > > > > > > > } > > > > > > > > > > > > > > because the calls 'baz(&x)' are considered to have non-constant > > > > > > > arguments. > > > > > > > > > > > > > > > > > > > > > Unfortunately, we can come up with another trick to fool the > > > > > > > constexpr_call > > > > > > > cache in the presence of RVO even in the !TREE_STATIC case: > > > > > > > > > > > > > > struct B { > > > > > > > B() = default; B(const B&); > > > > > > > constexpr B(int) : q{!this[-1].q} { } > > > > > > > bool q = false; > > > > > > > }; > > > > > > > > > > > > > > constexpr B baz() { return B(0); } > > > > > > > > > > > > > > void foo() > > > > > > > { > > > > > > > constexpr B d[2] = { {}, baz() }; > > > > > > > constexpr B e = baz(); > > > > > > > } > > > > > > > > > > > > > > The initialization of the local variable 'e' should be invalid, but > > > > > > > if > > > > > > > we > > > > > > > cached the first call to 'baz' then we would wrongly accept it. > > > > > > > > > > > > Right. > > > > > > > > > > > > We ought to be able to distinguish between uses of the RESULT_DECL > > > > > > only > > > > > > for > > > > > > storing to it (cacheable) and any other use, either reading from it or > > > > > > messing > > > > > > with its address. But I think that's a future direction, and we > > > > > > should > > > > > > stick > > > > > > with your patch that almost works for GCC 10. > > > > > > > > > > Sounds good. Does the following then look OK to commit? > > > > > > > > > > -- >8 -- > > > > > > > > > > Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call > > > > > [PR94034] > > > > > > > > > > When evaluating the initializer of 'a' in the following example > > > > > > > > > > struct A { > > > > > A() = default; A(const A&); > > > > > A *p = this; > > > > > }; > > > > > constexpr A foo() { return {}; } > > > > > constexpr A a = foo(); > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by > > > > > foo > > > > > gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the > > > > > 'this' > > > > > should really be resolved to '&a'. > > > > > > > > > > Fixing this properly by immediately resolving 'this' and > > > > > PLACEHOLDER_EXPRs > > > > > to > > > > > the ultimate object under construction would in general mean that we > > > > > would > > > > > no > > > > > longer be able to cache constexpr calls for which RVO possibly applies, > > > > > because > > > > > the result of the call may now depend on the ultimate object under > > > > > construction. > > > > > > > > > > So as a mostly correct stopgap solution that retains cachability of > > > > > RVO'd > > > > > constexpr calls, this patch fixes this issue by rewriting all > > > > > occurrences of > > > > > the > > > > > RESULT_DECL in the result of a constexpr function call with the current > > > > > object > > > > > under construction, after the call returns. This means the 'this' > > > > > pointer > > > > > during construction of the temporary will still point to the temporary > > > > > object > > > > > instead of the ultimate object, but besides that this approach seems > > > > > functionally equivalent to the proper approach. > > > > > > > > > > Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, does > > > > > this > > > > > look > > > > > OK to commit? > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > PR c++/94034 > > > > > * constexpr.c (replace_result_decl_data): New struct. > > > > > (replace_result_decl_data_r): New function. > > > > > (replace_result_decl): New function. > > > > > (cxx_eval_call_expression): Use it. > > > > > * tree.c (build_aggr_init_expr): Propagate the location of the > > > > > initializer to the AGGR_INIT_EXPR. > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > PR c++/94034 > > > > > * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. > > > > > * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. > > > > > * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. > > > > > * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. > > > > > --- > > > > > gcc/cp/constexpr.c | 51 > > > > > +++++++++++++++++++ > > > > > gcc/cp/tree.c | 3 ++ > > > > > .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++ > > > > > .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ > > > > > .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 ++++++++++++++++++ > > > > > .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ > > > > > 6 files changed, 204 insertions(+) > > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > > > > > > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > > > > index 5793430c88d..4cf5812e8a5 100644 > > > > > --- a/gcc/cp/constexpr.c > > > > > +++ b/gcc/cp/constexpr.c > > > > > @@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx > > > > > *ctx, > > > > > tree call, > > > > > return cp_build_addr_expr (obj, complain); > > > > > } > > > > > +/* Data structure used by replace_result_decl and > > > > > replace_result_decl_r. > > > > > */ > > > > > + > > > > > +struct replace_result_decl_data > > > > > +{ > > > > > + /* The RESULT_DECL we want to replace. */ > > > > > + tree decl; > > > > > + /* The replacement for DECL. */ > > > > > + tree replacement; > > > > > + /* Whether we've performed any replacements. */ > > > > > + bool changed; > > > > > +}; > > > > > + > > > > > +/* Helper function for replace_result_decl, called through > > > > > cp_walk_tree. > > > > > */ > > > > > + > > > > > +static tree > > > > > +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) > > > > > +{ > > > > > + replace_result_decl_data *d = (replace_result_decl_data *) data; > > > > > + > > > > > + if (*tp == d->decl) > > > > > + { > > > > > + *tp = unshare_expr (d->replacement); > > > > > + d->changed = true; > > > > > + *walk_subtrees = 0; > > > > > + } > > > > > + else if (TYPE_P (*tp)) > > > > > + *walk_subtrees = 0; > > > > > + > > > > > + return NULL_TREE; > > > > > +} > > > > > + > > > > > +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared > > > > > copy > > > > > of) > > > > > + REPLACEMENT within *TP. Returns true iff a replacement was > > > > > performed. > > > > > */ > > > > > + > > > > > +static bool > > > > > +replace_result_decl (tree *tp, tree decl, tree replacement) > > > > > +{ > > > > > + gcc_assert (TREE_CODE (decl) == RESULT_DECL); > > > > > + replace_result_decl_data data = { decl, replacement, false }; > > > > > + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); > > > > > + return data.changed; > > > > > +} > > > > > + > > > > > /* Subroutine of cxx_eval_constant_expression. > > > > > Evaluate the call expression tree T in the context of OLD_CALL > > > > > expression > > > > > evaluation. */ > > > > > @@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const constexpr_ctx > > > > > *ctx, > > > > > tree t, > > > > > break; > > > > > } > > > > > } > > > > > + > > > > > + /* Rewrite all occurrences of the function's RESULT_DECL with the > > > > > + current object under construction. */ > > > > > + if (ctx->object > > > > > + && (same_type_ignoring_top_level_qualifiers_p > > > > > + (TREE_TYPE (res), TREE_TYPE (ctx->object)))) > > > > > > > > When can they have different types? > > > > > > During prior testing I tried replacing the same_type_p check with an assert, > > > i.e.: > > > > > > if (ctx->object > > > && AGGREGATE_TYPE_P (TREE_TYPE (res))) > > > { > > > gcc_assert (same_type_ignoring_top_level_qualifiers_p > > > (TREE_TYPE (res), TREE_TYPE (ctx->object)); > > > ... > > > } > > > > > > and IIRC I was able to trigger the assert only when the type of > > > ctx->object and of the RESULT_DECL are distinct empty class types. > > > Here's a testcase: > > > > > > struct empty1 { }; > > > constexpr empty1 foo1() { return {}; } > > > > > > struct empty2 { }; > > > constexpr empty2 foo2(empty1) { return {}; } > > > > > > constexpr empty2 a = foo2(foo1()); > > > > > > The initializer of 'a' has the form > > > TARGET_EXPR <D.2389, foo2 (foo1 ();, <<< Unknown tree: empty_class_expr > > > >>>;)>; > > > and so during evaluation of 'foo1()', ctx->object still points to 'a' and we > > > trip over the same_type_p assert. (Is it possible that the call to foo1 > > > is missing a TARGET_EXPR?) > > > > Hmm, that would be because of > > > > > if (is_empty_class (TREE_TYPE (arg)) > > > && simple_empty_class_p (TREE_TYPE (arg), arg, INIT_EXPR)) > > > { > while (TREE_CODE (arg) == TARGET_EXPR) > > > /* We're disconnecting the initializer from its target, > > > don't create a temporary. */ > > > arg = TARGET_EXPR_INITIAL (arg); > > > > in build_call_a. > > Ah okay. > > > > > > Either way, it would be safe to skip empty class types. Should we change > > > the > > > condition to > > > > > > if (ctx->object > > > && AGGREGATE_TYPE_P (TREE_TYPE (res)) > > > && !is_empty_class (TREE_TYPE (res))) > > > { > > > ... > > > } > > > > > > and turn the same_type_p check into an assert? > > > > Sounds good. Let's say a checking_assert. > > Great, I'm bootstrapping/regtesting the following overnight, does it > look OK to commit if successful? > > -- >8 -- > > Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call [PR94034] > > gcc/cp/ChangeLog: > > PR c++/94034 > * constexpr.c (replace_result_decl_data): New struct. > (replace_result_decl_data_r): New function. > (replace_result_decl): New function. > (cxx_eval_call_expression): Use it. > * tree.c (build_aggr_init_expr): Set the location of the AGGR_INIT_EXPR > to that of its initializer. > > gcc/testsuite/ChangeLog: > > PR c++/94034 > * g++.dg/cpp0x/constexpr-empty15.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. > --- > gcc/cp/constexpr.c | 53 +++++++++++++++++++ > gcc/cp/tree.c | 3 ++ > .../g++.dg/cpp0x/constexpr-empty15.C | 9 ++++ > .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 +++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 +++++++++++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ > 7 files changed, 215 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > index d8636ddb92f..00e313178f9 100644 > --- a/gcc/cp/constexpr.c > +++ b/gcc/cp/constexpr.c > @@ -2029,6 +2029,51 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call, > return cp_build_addr_expr (obj, complain); > } > > +/* Data structure used by replace_result_decl and replace_result_decl_r. */ > + > +struct replace_result_decl_data > +{ > + /* The RESULT_DECL we want to replace. */ > + tree decl; > + /* The replacement for DECL. */ > + tree replacement; > + /* Whether we've performed any replacements. */ > + bool changed; > +}; > + > +/* Helper function for replace_result_decl, called through cp_walk_tree. */ > + > +static tree > +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) > +{ > + replace_result_decl_data *d = (replace_result_decl_data *) data; > + > + if (*tp == d->decl) > + { > + *tp = unshare_expr (d->replacement); > + d->changed = true; > + *walk_subtrees = 0; > + } > + else if (TYPE_P (*tp)) > + *walk_subtrees = 0; > + > + return NULL_TREE; > +} > + > +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy of) > + REPLACEMENT within *TP. Returns true iff a replacement was performed. */ > + > +static bool > +replace_result_decl (tree *tp, tree decl, tree replacement) > +{ > + gcc_checking_assert (TREE_CODE (decl) == RESULT_DECL > + && (same_type_ignoring_top_level_qualifiers_p > + (TREE_TYPE (decl), TREE_TYPE (replacement)))); > + replace_result_decl_data data = { decl, replacement, false }; > + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); > + return data.changed; > +} > + > /* Subroutine of cxx_eval_constant_expression. > Evaluate the call expression tree T in the context of OLD_CALL expression > evaluation. */ > @@ -2536,6 +2581,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > break; > } > } > + > + /* Rewrite all occurrences of the function's RESULT_DECL with the > + current object under construction. */ > + if (ctx->object > + && AGGREGATE_TYPE_P (TREE_TYPE (res)) > + && !is_empty_class (TREE_TYPE (res))) > + if (replace_result_decl (&result, res, ctx->object)) > + cacheable = false; We should probably require !*non_constant_p here before doing the replacement because it doesn't make sense to do this transformation for non-constant calls to constexpr functions. And I think it also might be unsafe to mutate the result when !*non_constant_p because we might be modifying part of the DECL_SAVED_TREE of the function in that case, although I haven't been able to come up with a testcase for this. And requiring !*non_constant_p means that the result must be a reduced constant expression, which then allows replace_result_decl to make some assumptions about where within the result it could possibly find a RESULT_DECL, a minor optimization. I am testing the following (ChangeLog is the same as before): -- >8 -- --- gcc/cp/constexpr.c | 62 +++++++++++++++++++ gcc/cp/tree.c | 3 + .../g++.dg/cpp0x/constexpr-empty15.C | 9 +++ .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++ .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++ .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 +++++++++++++++ .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 ++++++++++++++ 7 files changed, 224 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index d8636ddb92f..387ee033214 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -2029,6 +2029,59 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call, return cp_build_addr_expr (obj, complain); } +/* Data structure used by replace_result_decl and replace_result_decl_r. */ + +struct replace_result_decl_data +{ + /* The RESULT_DECL we want to replace. */ + tree decl; + /* The replacement for DECL. */ + tree replacement; + /* Whether we've performed any replacements. */ + bool changed; +}; + +/* Helper function for replace_result_decl, called through cp_walk_tree. */ + +static tree +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) +{ + replace_result_decl_data *d = (replace_result_decl_data *) data; + + if (*tp == d->decl) + { + *tp = unshare_expr (d->replacement); + d->changed = true; + *walk_subtrees = 0; + } + else if (TYPE_P (*tp)) + *walk_subtrees = 0; + else if (TREE_CODE (*tp) != ADDR_EXPR + && !POINTER_TYPE_P (TREE_TYPE (*tp)) + && !AGGREGATE_TYPE_P (TREE_TYPE (*tp))) + /* Since *TP is part of a reduced constant expression, we know that a + RESULT_DECL can appear within it only as the operand of an ADDR_EXPR + within some aggregate. */ + *walk_subtrees = 0; + + return NULL_TREE; +} + +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy of) + REPLACEMENT within the reduced constant expression *TP. Returns true iff a + replacement was performed. */ + +static bool +replace_result_decl (tree *tp, tree decl, tree replacement) +{ + gcc_checking_assert (TREE_CODE (decl) == RESULT_DECL + && (same_type_ignoring_top_level_qualifiers_p + (TREE_TYPE (decl), TREE_TYPE (replacement)))); + replace_result_decl_data data = { decl, replacement, false }; + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); + return data.changed; +} + /* Subroutine of cxx_eval_constant_expression. Evaluate the call expression tree T in the context of OLD_CALL expression evaluation. */ @@ -2536,6 +2589,15 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, break; } } + + /* Rewrite all occurrences of the function's RESULT_DECL with the + current object under construction. */ + if (!*non_constant_p + && ctx->object + && AGGREGATE_TYPE_P (TREE_TYPE (res)) + && !is_empty_class (TREE_TYPE (res))) + if (replace_result_decl (&result, res, ctx->object)) + cacheable = false; } else /* Couldn't get a function copy to evaluate. */ diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index 1d311b0fe61..8e4934c0009 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -669,6 +669,9 @@ build_aggr_init_expr (tree type, tree init) else rval = init; + if (location_t loc = EXPR_LOCATION (init)) + SET_EXPR_LOCATION (rval, loc); + return rval; } diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C new file mode 100644 index 00000000000..97863d4e1a7 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C @@ -0,0 +1,9 @@ +// { dg-do compile { target c++11 } } + +struct empty1 { }; +constexpr empty1 foo1() { return {}; } + +struct empty2 { }; +constexpr empty2 foo2(empty1) { return {}; } + +constexpr empty2 a = foo2(foo1()); diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C new file mode 100644 index 00000000000..bb844b952e2 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C @@ -0,0 +1,26 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A { + A *ap = this; +}; + +constexpr A foo() +{ + return {}; +} + +constexpr A bar() +{ + return foo(); +} + +void +baz() +{ + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } +} + +constexpr A a = foo(); +constexpr A b = bar(); diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C new file mode 100644 index 00000000000..f847fe9809f --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C @@ -0,0 +1,27 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A { + A() = default; A(const A&); + A *ap = this; +}; + +constexpr A foo() +{ + return {}; +} + +constexpr A bar() +{ + return foo(); +} + +void +baz() +{ + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } +} + +constexpr A a = foo(); +constexpr A b = bar(); diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C new file mode 100644 index 00000000000..5a40cd0b845 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C @@ -0,0 +1,49 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A +{ + A *p = this; + int n = 2; + int m = p->n++; +}; + +constexpr A +foo() +{ + return {}; +} + +constexpr A +bar() +{ + A a = foo(); + a.p->n = 5; + return a; +} + +static_assert(bar().n == 5, ""); + +constexpr int +baz() +{ + A b = foo(); + b.p->n = 10; + A c = foo(); + if (c.p->n != 3 || c.p->m != 2) + __builtin_abort(); + bar(); + return 0; +} + +static_assert(baz() == 0, ""); + +constexpr int +quux() +{ + const A d = foo(); + d.p->n++; // { dg-error "const object" } + return 0; +} + +static_assert(quux() == 0, ""); // { dg-error "non-constant" } diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C new file mode 100644 index 00000000000..86d8ab4e759 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C @@ -0,0 +1,48 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A +{ + A() = default; A(const A&); + A *p = this; + int n = 2; + int m = p->n++; +}; + +constexpr A +foo() +{ + return {}; +} + +constexpr A +bar() +{ + A a = foo(); + a.p->n = 5; + return a; // { dg-error "non-.constexpr." } +} + +constexpr int +baz() +{ + A b = foo(); + b.p->n = 10; + A c = foo(); + if (c.p->n != 3 || c.p->m != 2) + __builtin_abort(); + foo(); + return 0; +} + +static_assert(baz() == 0, ""); + +constexpr int +quux() +{ + const A d = foo(); + d.p->n++; // { dg-error "const object" } + return 0; +} + +static_assert(quux() == 0, ""); // { dg-error "non-constant" }
On 4/14/20 12:29 AM, Patrick Palka wrote: > On Mon, 13 Apr 2020, Jason Merrill wrote: > >> On 4/13/20 2:49 PM, Patrick Palka wrote: >>> On Mon, 13 Apr 2020, Jason Merrill wrote: >>> >>>> On 4/12/20 9:43 AM, Patrick Palka wrote: >>>>> On Sat, 11 Apr 2020, Jason Merrill wrote: >>>>> >>>>>> On 4/10/20 5:47 PM, Patrick Palka wrote: >>>>>>> On Fri, 10 Apr 2020, Jason Merrill wrote: >>>>>>>> On 4/10/20 2:15 PM, Patrick Palka wrote: >>>>>>>>> On Fri, 10 Apr 2020, Patrick Palka wrote: >>>>>>>>> >>>>>>>>>> On Fri, 10 Apr 2020, Jason Merrill wrote: >>>>>>>>>> >>>>>>>>>>> On 4/10/20 1:04 PM, Patrick Palka wrote: >>>>>>>>>>>> On Thu, 9 Apr 2020, Patrick Palka wrote: >>>>>>>>>>>>> On Thu, 9 Apr 2020, Jason Merrill wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> On 4/8/20 7:49 PM, Patrick Palka wrote: >>>>>>>>>>>>>>> When evaluating the initializer of 'a' in the >>>>>>>>>>>>>>> following >>>>>>>>>>>>>>> example >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> struct A { A *p = this; }; >>>>>>>>>>>>>>> constexpr A foo() { return {}; } >>>>>>>>>>>>>>> constexpr A a = foo(); >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> the PLACEHOLDER_EXPR for 'this' in the aggregate >>>>>>>>>>>>>>> initializer >>>>>>>>>>>>>>> returned >>>>>>>>>>>>>>> by foo >>>>>>>>>>>>>>> gets resolved to the RESULT_DECL of foo. But due to >>>>>>>>>>>>>>> guaranteed >>>>>>>>>>>>>>> RVO, >>>>>>>>>>>>>>> the >>>>>>>>>>>>>>> 'this' >>>>>>>>>>>>>>> should really be resolved to '&a'. >>>>>>>>>>>>>> >>>>>>>>>>>>>>> It seems to me that the right approach would be to >>>>>>>>>>>>>>> immediately >>>>>>>>>>>>>>> resolve >>>>>>>>>>>>>>> the >>>>>>>>>>>>>>> PLACEHOLDER_EXPR to the correct target object during >>>>>>>>>>>>>>> evaluation >>>>>>>>>>>>>>> of >>>>>>>>>>>>>>> 'foo()', >>>>>>>>>>>>>>> so >>>>>>>>>>>>>>> that we could use 'this' to access objects adjacent >>>>>>>>>>>>>>> to >>>>>>>>>>>>>>> the >>>>>>>>>>>>>>> current >>>>>>>>>>>>>>> object in >>>>>>>>>>>>>>> the >>>>>>>>>>>>>>> ultimate storage location. (I think #c2 of PR >>>>>>>>>>>>>>> c++/94537 >>>>>>>>>>>>>>> is >>>>>>>>>>>>>>> an >>>>>>>>>>>>>>> example >>>>>>>>>>>>>>> of >>>>>>>>>>>>>>> such >>>>>>>>>>>>>>> usage of 'this', which currently doesn't work. But >>>>>>>>>>>>>>> as >>>>>>>>>>>>>>> #c1 >>>>>>>>>>>>>>> shows >>>>>>>>>>>>>>> we >>>>>>>>>>>>>>> don't >>>>>>>>>>>>>>> seem >>>>>>>>>>>>>>> to handle this case correctly in non-constexpr >>>>>>>>>>>>>>> initialization >>>>>>>>>>>>>>> either.) >>>>>>>>>>>>>> >>>>>>>>>>>>>> As I commented in the PR, the standard doesn't require >>>>>>>>>>>>>> this to >>>>>>>>>>>>>> work >>>>>>>>>>>>>> because A >>>>>>>>>>>>>> is trivially copyable, and our ABI makes it >>>>>>>>>>>>>> impossible. >>>>>>>>>>>>>> But >>>>>>>>>>>>>> there's >>>>>>>>>>>>>> still a >>>>>>>>>>>>>> constexpr bug when we add >>>>>>>>>>>>>> >>>>>>>>>>>>>> A() = default; A(const A&); >>>>>>>>>>>>>> >>>>>>>>>>>>>> clang doesn't require the constructors to make this >>>>>>>>>>>>>> work >>>>>>>>>>>>>> for >>>>>>>>>>>>>> constant >>>>>>>>>>>>>> initialization, but similarly can't make it work for >>>>>>>>>>>>>> non-constant >>>>>>>>>>>>>> initialization. >>>>>>>>>>>>> >>>>>>>>>>>>> That makes a lot of sense, thanks for the detailed >>>>>>>>>>>>> explanation. >>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>>> I haven't yet been able to make a solution using the >>>>>>>>>>>>>>> above >>>>>>>>>>>>>>> approach >>>>>>>>>>>>>>> work -- >>>>>>>>>>>>>>> making sure we use the ultimate object instead of >>>>>>>>>>>>>>> the >>>>>>>>>>>>>>> RESULT_DECL >>>>>>>>>>>>>>> whenever >>>>>>>>>>>>>>> we >>>>>>>>>>>>>>> access ctx->global->values is proving to be tricky >>>>>>>>>>>>>>> and >>>>>>>>>>>>>>> subtle. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Do we need to go through ctx->global->values? Would >>>>>>>>>>>>>> it >>>>>>>>>>>>>> work >>>>>>>>>>>>>> for >>>>>>>>>>>>>> the >>>>>>>>>>>>>> RESULT_DECL case in cxx_eval_constant_expression to go >>>>>>>>>>>>>> to >>>>>>>>>>>>>> straight >>>>>>>>>>>>>> to >>>>>>>>>>>>>> ctx->object or ctx->ctor instead? >>>>>>>>>>>>> >>>>>>>>>>>>> I attempted that at some point, but IIRC we still end up >>>>>>>>>>>>> not >>>>>>>>>>>>> resolving >>>>>>>>>>>>> some RESULT_DECLs because not all of them get processed >>>>>>>>>>>>> through >>>>>>>>>>>>> cxx_eval_constant_expression before we manipulate >>>>>>>>>>>>> ctx->global->values >>>>>>>>>>>>> with them. I'll try this approach more carefully and >>>>>>>>>>>>> report >>>>>>>>>>>>> back >>>>>>>>>>>>> with >>>>>>>>>>>>> more specifics. >>>>>>>>>>>> >>>>>>>>>>>> It turns out that immediately resolving >>>>>>>>>>>> RESULT_DECLs/'this' to >>>>>>>>>>>> the >>>>>>>>>>>> ultimate ctx->object would not interact well with >>>>>>>>>>>> constexpr_call >>>>>>>>>>>> caching: >>>>>>>>>>>> >>>>>>>>>>>> struct A { A() = default; A(const A&); A *ap = >>>>>>>>>>>> this; }; >>>>>>>>>>>> constexpr A foo() { return {}; } >>>>>>>>>>>> constexpr A a = foo(); >>>>>>>>>>>> constexpr A b = foo(); >>>>>>>>>>>> static_assert(b.ap == &b); // fails >>>>>>>>>>>> >>>>>>>>>>>> Evaluation of the first call to foo() returns {&a}, since >>>>>>>>>>>> we >>>>>>>>>>>> resolve >>>>>>>>>>>> 'this' to &a due to guaranteed RVO, and we cache this >>>>>>>>>>>> result. >>>>>>>>>>>> Evaluation of the second call to foo() just returns the >>>>>>>>>>>> cached >>>>>>>>>>>> result >>>>>>>>>>>> from the constexpr_call cache, and so we get {&a} again. >>>>>>>>>>>> >>>>>>>>>>>> So it seems we would have to mark the result of a >>>>>>>>>>>> constexpr >>>>>>>>>>>> call >>>>>>>>>>>> as >>>>>>>>>>>> not >>>>>>>>>>>> cacheable whenever this RVO applies during its evaluation, >>>>>>>>>>>> even >>>>>>>>>>>> when >>>>>>>>>>>> doing the RVO has no observable difference in the final >>>>>>>>>>>> result >>>>>>>>>>>> (i.e. >>>>>>>>>>>> the >>>>>>>>>>>> constructor does not try to save the 'this' pointer). >>>>>>>>>>>> >>>>>>>>>>>> Would the performance impact of disabling caching whenever >>>>>>>>>>>> RVO >>>>>>>>>>>> applies >>>>>>>>>>>> during constexpr call evaluation be worth it, or should we >>>>>>>>>>>> go >>>>>>>>>>>> with >>>>>>>>>>>> something like my first patch which "almost works," and >>>>>>>>>>>> which >>>>>>>>>>>> marks a >>>>>>>>>>>> constexpr call as not cacheable only when we replace a >>>>>>>>>>>> RESULT_DECL >>>>>>>>>>>> in >>>>>>>>>>>> the result of the call? >>>>>>>>>>> >>>>>>>>>>> Could we search through the result of the call for >>>>>>>>>>> ctx->object >>>>>>>>>>> and >>>>>>>>>>> cache >>>>>>>>>>> if we >>>>>>>>>>> don't find it? >>>>>>>>>> >>>>>>>>>> Hmm, I think the result of the call could still depend on >>>>>>>>>> ctx->object >>>>>>>>>> without ctx->object explicitly appearing in the result. >>>>>>>>>> Consider >>>>>>>>>> the >>>>>>>>>> following testcase: >>>>>>>>>> >>>>>>>>>> struct A { >>>>>>>>>> A() = default; A(const A&); >>>>>>>>>> constexpr A(const A *q) : d{this - p} { } >>>>>>>>> >>>>>>>>> Oops sorry, that 'q' should be a 'p'. >>>>>>>>> >>>>>>>>>> long d = 0; >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> constexpr A baz(const A *q) { return A(p); }; >>>>>>>>> >>>>>>>>> And same here. >>>>>>>>> >>>>>>>>>> constexpr A a = baz(&a); >>>>>>>>>> constexpr A b = baz(&a); // no error >>>>>>>>>> >>>>>>>>>> The initialization of 'b' should be ill-formed, but the result >>>>>>>>>> of >>>>>>>>>> the >>>>>>>>>> first call to baz(&a) would be {0}, so we would cache it and >>>>>>>>>> then >>>>>>>>>> reuse >>>>>>>>>> the result when initializing 'b'. >>>>>>>> >>>>>>>> Ah, true. Can we still cache if we're initializing something that >>>>>>>> isn't >>>>>>>> TREE_STATIC? >>>>>>> >>>>>>> Hmm, we correctly compile the analogous non-TREE_STATIC testcase >>>>>>> >>>>>>> struct A { >>>>>>> A() = default; A(const A&); >>>>>>> constexpr A(const A *p) : d{this == p} { } >>>>>>> bool d; >>>>>>> }; >>>>>>> >>>>>>> constexpr A baz(const A *p) { return A(p); } >>>>>>> >>>>>>> void foo() { >>>>>>> constexpr A x = baz(&x); >>>>>>> constexpr A y = baz(&x); >>>>>>> static_assert(!y.d); >>>>>>> } >>>>>>> >>>>>>> because the calls 'baz(&x)' are considered to have non-constant >>>>>>> arguments. >>>>>>> >>>>>>> >>>>>>> Unfortunately, we can come up with another trick to fool the >>>>>>> constexpr_call >>>>>>> cache in the presence of RVO even in the !TREE_STATIC case: >>>>>>> >>>>>>> struct B { >>>>>>> B() = default; B(const B&); >>>>>>> constexpr B(int) : q{!this[-1].q} { } >>>>>>> bool q = false; >>>>>>> }; >>>>>>> >>>>>>> constexpr B baz() { return B(0); } >>>>>>> >>>>>>> void foo() >>>>>>> { >>>>>>> constexpr B d[2] = { {}, baz() }; >>>>>>> constexpr B e = baz(); >>>>>>> } >>>>>>> >>>>>>> The initialization of the local variable 'e' should be invalid, but >>>>>>> if >>>>>>> we >>>>>>> cached the first call to 'baz' then we would wrongly accept it. >>>>>> >>>>>> Right. >>>>>> >>>>>> We ought to be able to distinguish between uses of the RESULT_DECL >>>>>> only >>>>>> for >>>>>> storing to it (cacheable) and any other use, either reading from it or >>>>>> messing >>>>>> with its address. But I think that's a future direction, and we >>>>>> should >>>>>> stick >>>>>> with your patch that almost works for GCC 10. >>>>> >>>>> Sounds good. Does the following then look OK to commit? >>>>> >>>>> -- >8 -- >>>>> >>>>> Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call >>>>> [PR94034] >>>>> >>>>> When evaluating the initializer of 'a' in the following example >>>>> >>>>> struct A { >>>>> A() = default; A(const A&); >>>>> A *p = this; >>>>> }; >>>>> constexpr A foo() { return {}; } >>>>> constexpr A a = foo(); >>>>> >>>>> the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by >>>>> foo >>>>> gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the >>>>> 'this' >>>>> should really be resolved to '&a'. >>>>> >>>>> Fixing this properly by immediately resolving 'this' and >>>>> PLACEHOLDER_EXPRs >>>>> to >>>>> the ultimate object under construction would in general mean that we >>>>> would >>>>> no >>>>> longer be able to cache constexpr calls for which RVO possibly applies, >>>>> because >>>>> the result of the call may now depend on the ultimate object under >>>>> construction. >>>>> >>>>> So as a mostly correct stopgap solution that retains cachability of >>>>> RVO'd >>>>> constexpr calls, this patch fixes this issue by rewriting all >>>>> occurrences of >>>>> the >>>>> RESULT_DECL in the result of a constexpr function call with the current >>>>> object >>>>> under construction, after the call returns. This means the 'this' >>>>> pointer >>>>> during construction of the temporary will still point to the temporary >>>>> object >>>>> instead of the ultimate object, but besides that this approach seems >>>>> functionally equivalent to the proper approach. >>>>> >>>>> Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, does >>>>> this >>>>> look >>>>> OK to commit? >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> PR c++/94034 >>>>> * constexpr.c (replace_result_decl_data): New struct. >>>>> (replace_result_decl_data_r): New function. >>>>> (replace_result_decl): New function. >>>>> (cxx_eval_call_expression): Use it. >>>>> * tree.c (build_aggr_init_expr): Propagate the location of the >>>>> initializer to the AGGR_INIT_EXPR. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> PR c++/94034 >>>>> * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. >>>>> * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. >>>>> * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. >>>>> * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. >>>>> --- >>>>> gcc/cp/constexpr.c | 51 >>>>> +++++++++++++++++++ >>>>> gcc/cp/tree.c | 3 ++ >>>>> .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++ >>>>> .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ >>>>> .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 ++++++++++++++++++ >>>>> .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ >>>>> 6 files changed, 204 insertions(+) >>>>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C >>>>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C >>>>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C >>>>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C >>>>> >>>>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >>>>> index 5793430c88d..4cf5812e8a5 100644 >>>>> --- a/gcc/cp/constexpr.c >>>>> +++ b/gcc/cp/constexpr.c >>>>> @@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx >>>>> *ctx, >>>>> tree call, >>>>> return cp_build_addr_expr (obj, complain); >>>>> } >>>>> +/* Data structure used by replace_result_decl and >>>>> replace_result_decl_r. >>>>> */ >>>>> + >>>>> +struct replace_result_decl_data >>>>> +{ >>>>> + /* The RESULT_DECL we want to replace. */ >>>>> + tree decl; >>>>> + /* The replacement for DECL. */ >>>>> + tree replacement; >>>>> + /* Whether we've performed any replacements. */ >>>>> + bool changed; >>>>> +}; >>>>> + >>>>> +/* Helper function for replace_result_decl, called through >>>>> cp_walk_tree. >>>>> */ >>>>> + >>>>> +static tree >>>>> +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) >>>>> +{ >>>>> + replace_result_decl_data *d = (replace_result_decl_data *) data; >>>>> + >>>>> + if (*tp == d->decl) >>>>> + { >>>>> + *tp = unshare_expr (d->replacement); >>>>> + d->changed = true; >>>>> + *walk_subtrees = 0; >>>>> + } >>>>> + else if (TYPE_P (*tp)) >>>>> + *walk_subtrees = 0; >>>>> + >>>>> + return NULL_TREE; >>>>> +} >>>>> + >>>>> +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared >>>>> copy >>>>> of) >>>>> + REPLACEMENT within *TP. Returns true iff a replacement was >>>>> performed. >>>>> */ >>>>> + >>>>> +static bool >>>>> +replace_result_decl (tree *tp, tree decl, tree replacement) >>>>> +{ >>>>> + gcc_assert (TREE_CODE (decl) == RESULT_DECL); >>>>> + replace_result_decl_data data = { decl, replacement, false }; >>>>> + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); >>>>> + return data.changed; >>>>> +} >>>>> + >>>>> /* Subroutine of cxx_eval_constant_expression. >>>>> Evaluate the call expression tree T in the context of OLD_CALL >>>>> expression >>>>> evaluation. */ >>>>> @@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const constexpr_ctx >>>>> *ctx, >>>>> tree t, >>>>> break; >>>>> } >>>>> } >>>>> + >>>>> + /* Rewrite all occurrences of the function's RESULT_DECL with the >>>>> + current object under construction. */ >>>>> + if (ctx->object >>>>> + && (same_type_ignoring_top_level_qualifiers_p >>>>> + (TREE_TYPE (res), TREE_TYPE (ctx->object)))) >>>> >>>> When can they have different types? >>> >>> During prior testing I tried replacing the same_type_p check with an assert, >>> i.e.: >>> >>> if (ctx->object >>> && AGGREGATE_TYPE_P (TREE_TYPE (res))) >>> { >>> gcc_assert (same_type_ignoring_top_level_qualifiers_p >>> (TREE_TYPE (res), TREE_TYPE (ctx->object)); >>> ... >>> } >>> >>> and IIRC I was able to trigger the assert only when the type of >>> ctx->object and of the RESULT_DECL are distinct empty class types. >>> Here's a testcase: >>> >>> struct empty1 { }; >>> constexpr empty1 foo1() { return {}; } >>> >>> struct empty2 { }; >>> constexpr empty2 foo2(empty1) { return {}; } >>> >>> constexpr empty2 a = foo2(foo1()); >>> >>> The initializer of 'a' has the form >>> TARGET_EXPR <D.2389, foo2 (foo1 ();, <<< Unknown tree: empty_class_expr >>>>>> ;)>; >>> and so during evaluation of 'foo1()', ctx->object still points to 'a' and we >>> trip over the same_type_p assert. (Is it possible that the call to foo1 >>> is missing a TARGET_EXPR?) >> >> Hmm, that would be because of >> >>> if (is_empty_class (TREE_TYPE (arg)) >>> && simple_empty_class_p (TREE_TYPE (arg), arg, INIT_EXPR)) >>> { > while (TREE_CODE (arg) == TARGET_EXPR) >>> /* We're disconnecting the initializer from its target, >>> don't create a temporary. */ >>> arg = TARGET_EXPR_INITIAL (arg); >> >> in build_call_a. > > Ah okay. > >> >>> Either way, it would be safe to skip empty class types. Should we change >>> the >>> condition to >>> >>> if (ctx->object >>> && AGGREGATE_TYPE_P (TREE_TYPE (res)) >>> && !is_empty_class (TREE_TYPE (res))) >>> { >>> ... >>> } >>> >>> and turn the same_type_p check into an assert? >> >> Sounds good. Let's say a checking_assert. > > Great, I'm bootstrapping/regtesting the following overnight, does it > look OK to commit if successful? OK. > -- >8 -- > > Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call [PR94034] > > gcc/cp/ChangeLog: > > PR c++/94034 > * constexpr.c (replace_result_decl_data): New struct. > (replace_result_decl_data_r): New function. > (replace_result_decl): New function. > (cxx_eval_call_expression): Use it. > * tree.c (build_aggr_init_expr): Set the location of the AGGR_INIT_EXPR > to that of its initializer. > > gcc/testsuite/ChangeLog: > > PR c++/94034 > * g++.dg/cpp0x/constexpr-empty15.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. > * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. > --- > gcc/cp/constexpr.c | 53 +++++++++++++++++++ > gcc/cp/tree.c | 3 ++ > .../g++.dg/cpp0x/constexpr-empty15.C | 9 ++++ > .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 +++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 +++++++++++++++++ > .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ > 7 files changed, 215 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > index d8636ddb92f..00e313178f9 100644 > --- a/gcc/cp/constexpr.c > +++ b/gcc/cp/constexpr.c > @@ -2029,6 +2029,51 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call, > return cp_build_addr_expr (obj, complain); > } > > +/* Data structure used by replace_result_decl and replace_result_decl_r. */ > + > +struct replace_result_decl_data > +{ > + /* The RESULT_DECL we want to replace. */ > + tree decl; > + /* The replacement for DECL. */ > + tree replacement; > + /* Whether we've performed any replacements. */ > + bool changed; > +}; > + > +/* Helper function for replace_result_decl, called through cp_walk_tree. */ > + > +static tree > +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) > +{ > + replace_result_decl_data *d = (replace_result_decl_data *) data; > + > + if (*tp == d->decl) > + { > + *tp = unshare_expr (d->replacement); > + d->changed = true; > + *walk_subtrees = 0; > + } > + else if (TYPE_P (*tp)) > + *walk_subtrees = 0; > + > + return NULL_TREE; > +} > + > +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy of) > + REPLACEMENT within *TP. Returns true iff a replacement was performed. */ > + > +static bool > +replace_result_decl (tree *tp, tree decl, tree replacement) > +{ > + gcc_checking_assert (TREE_CODE (decl) == RESULT_DECL > + && (same_type_ignoring_top_level_qualifiers_p > + (TREE_TYPE (decl), TREE_TYPE (replacement)))); > + replace_result_decl_data data = { decl, replacement, false }; > + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); > + return data.changed; > +} > + > /* Subroutine of cxx_eval_constant_expression. > Evaluate the call expression tree T in the context of OLD_CALL expression > evaluation. */ > @@ -2536,6 +2581,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > break; > } > } > + > + /* Rewrite all occurrences of the function's RESULT_DECL with the > + current object under construction. */ > + if (ctx->object > + && AGGREGATE_TYPE_P (TREE_TYPE (res)) > + && !is_empty_class (TREE_TYPE (res))) > + if (replace_result_decl (&result, res, ctx->object)) > + cacheable = false; > } > else > /* Couldn't get a function copy to evaluate. */ > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > index 1d311b0fe61..8e4934c0009 100644 > --- a/gcc/cp/tree.c > +++ b/gcc/cp/tree.c > @@ -669,6 +669,9 @@ build_aggr_init_expr (tree type, tree init) > else > rval = init; > > + if (location_t loc = EXPR_LOCATION (init)) > + SET_EXPR_LOCATION (rval, loc); > + > return rval; > } > > diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C > new file mode 100644 > index 00000000000..97863d4e1a7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C > @@ -0,0 +1,9 @@ > +// { dg-do compile { target c++11 } } > + > +struct empty1 { }; > +constexpr empty1 foo1() { return {}; } > + > +struct empty2 { }; > +constexpr empty2 foo2(empty1) { return {}; } > + > +constexpr empty2 a = foo2(foo1()); > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > new file mode 100644 > index 00000000000..bb844b952e2 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > @@ -0,0 +1,26 @@ > +// PR c++/94034 > +// { dg-do compile { target c++14 } } > + > +struct A { > + A *ap = this; > +}; > + > +constexpr A foo() > +{ > + return {}; > +} > + > +constexpr A bar() > +{ > + return foo(); > +} > + > +void > +baz() > +{ > + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } > + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } > +} > + > +constexpr A a = foo(); > +constexpr A b = bar(); > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > new file mode 100644 > index 00000000000..f847fe9809f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > @@ -0,0 +1,27 @@ > +// PR c++/94034 > +// { dg-do compile { target c++14 } } > + > +struct A { > + A() = default; A(const A&); > + A *ap = this; > +}; > + > +constexpr A foo() > +{ > + return {}; > +} > + > +constexpr A bar() > +{ > + return foo(); > +} > + > +void > +baz() > +{ > + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } > + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } > +} > + > +constexpr A a = foo(); > +constexpr A b = bar(); > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > new file mode 100644 > index 00000000000..5a40cd0b845 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > @@ -0,0 +1,49 @@ > +// PR c++/94034 > +// { dg-do compile { target c++14 } } > + > +struct A > +{ > + A *p = this; > + int n = 2; > + int m = p->n++; > +}; > + > +constexpr A > +foo() > +{ > + return {}; > +} > + > +constexpr A > +bar() > +{ > + A a = foo(); > + a.p->n = 5; > + return a; > +} > + > +static_assert(bar().n == 5, ""); > + > +constexpr int > +baz() > +{ > + A b = foo(); > + b.p->n = 10; > + A c = foo(); > + if (c.p->n != 3 || c.p->m != 2) > + __builtin_abort(); > + bar(); > + return 0; > +} > + > +static_assert(baz() == 0, ""); > + > +constexpr int > +quux() > +{ > + const A d = foo(); > + d.p->n++; // { dg-error "const object" } > + return 0; > +} > + > +static_assert(quux() == 0, ""); // { dg-error "non-constant" } > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > new file mode 100644 > index 00000000000..86d8ab4e759 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > @@ -0,0 +1,48 @@ > +// PR c++/94034 > +// { dg-do compile { target c++14 } } > + > +struct A > +{ > + A() = default; A(const A&); > + A *p = this; > + int n = 2; > + int m = p->n++; > +}; > + > +constexpr A > +foo() > +{ > + return {}; > +} > + > +constexpr A > +bar() > +{ > + A a = foo(); > + a.p->n = 5; > + return a; // { dg-error "non-.constexpr." } > +} > + > +constexpr int > +baz() > +{ > + A b = foo(); > + b.p->n = 10; > + A c = foo(); > + if (c.p->n != 3 || c.p->m != 2) > + __builtin_abort(); > + foo(); > + return 0; > +} > + > +static_assert(baz() == 0, ""); > + > +constexpr int > +quux() > +{ > + const A d = foo(); > + d.p->n++; // { dg-error "const object" } > + return 0; > +} > + > +static_assert(quux() == 0, ""); // { dg-error "non-constant" } >
On 4/14/20 9:01 AM, Patrick Palka wrote: > On Tue, 14 Apr 2020, Patrick Palka wrote: > >> On Mon, 13 Apr 2020, Jason Merrill wrote: >> >>> On 4/13/20 2:49 PM, Patrick Palka wrote: >>>> On Mon, 13 Apr 2020, Jason Merrill wrote: >>>> >>>>> On 4/12/20 9:43 AM, Patrick Palka wrote: >>>>>> On Sat, 11 Apr 2020, Jason Merrill wrote: >>>>>> >>>>>>> On 4/10/20 5:47 PM, Patrick Palka wrote: >>>>>>>> On Fri, 10 Apr 2020, Jason Merrill wrote: >>>>>>>>> On 4/10/20 2:15 PM, Patrick Palka wrote: >>>>>>>>>> On Fri, 10 Apr 2020, Patrick Palka wrote: >>>>>>>>>> >>>>>>>>>>> On Fri, 10 Apr 2020, Jason Merrill wrote: >>>>>>>>>>> >>>>>>>>>>>> On 4/10/20 1:04 PM, Patrick Palka wrote: >>>>>>>>>>>>> On Thu, 9 Apr 2020, Patrick Palka wrote: >>>>>>>>>>>>>> On Thu, 9 Apr 2020, Jason Merrill wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 4/8/20 7:49 PM, Patrick Palka wrote: >>>>>>>>>>>>>>>> When evaluating the initializer of 'a' in the >>>>>>>>>>>>>>>> following >>>>>>>>>>>>>>>> example >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> struct A { A *p = this; }; >>>>>>>>>>>>>>>> constexpr A foo() { return {}; } >>>>>>>>>>>>>>>> constexpr A a = foo(); >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> the PLACEHOLDER_EXPR for 'this' in the aggregate >>>>>>>>>>>>>>>> initializer >>>>>>>>>>>>>>>> returned >>>>>>>>>>>>>>>> by foo >>>>>>>>>>>>>>>> gets resolved to the RESULT_DECL of foo. But due to >>>>>>>>>>>>>>>> guaranteed >>>>>>>>>>>>>>>> RVO, >>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>> 'this' >>>>>>>>>>>>>>>> should really be resolved to '&a'. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> It seems to me that the right approach would be to >>>>>>>>>>>>>>>> immediately >>>>>>>>>>>>>>>> resolve >>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>> PLACEHOLDER_EXPR to the correct target object during >>>>>>>>>>>>>>>> evaluation >>>>>>>>>>>>>>>> of >>>>>>>>>>>>>>>> 'foo()', >>>>>>>>>>>>>>>> so >>>>>>>>>>>>>>>> that we could use 'this' to access objects adjacent >>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>> current >>>>>>>>>>>>>>>> object in >>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>> ultimate storage location. (I think #c2 of PR >>>>>>>>>>>>>>>> c++/94537 >>>>>>>>>>>>>>>> is >>>>>>>>>>>>>>>> an >>>>>>>>>>>>>>>> example >>>>>>>>>>>>>>>> of >>>>>>>>>>>>>>>> such >>>>>>>>>>>>>>>> usage of 'this', which currently doesn't work. But >>>>>>>>>>>>>>>> as >>>>>>>>>>>>>>>> #c1 >>>>>>>>>>>>>>>> shows >>>>>>>>>>>>>>>> we >>>>>>>>>>>>>>>> don't >>>>>>>>>>>>>>>> seem >>>>>>>>>>>>>>>> to handle this case correctly in non-constexpr >>>>>>>>>>>>>>>> initialization >>>>>>>>>>>>>>>> either.) >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> As I commented in the PR, the standard doesn't require >>>>>>>>>>>>>>> this to >>>>>>>>>>>>>>> work >>>>>>>>>>>>>>> because A >>>>>>>>>>>>>>> is trivially copyable, and our ABI makes it >>>>>>>>>>>>>>> impossible. >>>>>>>>>>>>>>> But >>>>>>>>>>>>>>> there's >>>>>>>>>>>>>>> still a >>>>>>>>>>>>>>> constexpr bug when we add >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> A() = default; A(const A&); >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> clang doesn't require the constructors to make this >>>>>>>>>>>>>>> work >>>>>>>>>>>>>>> for >>>>>>>>>>>>>>> constant >>>>>>>>>>>>>>> initialization, but similarly can't make it work for >>>>>>>>>>>>>>> non-constant >>>>>>>>>>>>>>> initialization. >>>>>>>>>>>>>> >>>>>>>>>>>>>> That makes a lot of sense, thanks for the detailed >>>>>>>>>>>>>> explanation. >>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I haven't yet been able to make a solution using the >>>>>>>>>>>>>>>> above >>>>>>>>>>>>>>>> approach >>>>>>>>>>>>>>>> work -- >>>>>>>>>>>>>>>> making sure we use the ultimate object instead of >>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>> RESULT_DECL >>>>>>>>>>>>>>>> whenever >>>>>>>>>>>>>>>> we >>>>>>>>>>>>>>>> access ctx->global->values is proving to be tricky >>>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>> subtle. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Do we need to go through ctx->global->values? Would >>>>>>>>>>>>>>> it >>>>>>>>>>>>>>> work >>>>>>>>>>>>>>> for >>>>>>>>>>>>>>> the >>>>>>>>>>>>>>> RESULT_DECL case in cxx_eval_constant_expression to go >>>>>>>>>>>>>>> to >>>>>>>>>>>>>>> straight >>>>>>>>>>>>>>> to >>>>>>>>>>>>>>> ctx->object or ctx->ctor instead? >>>>>>>>>>>>>> >>>>>>>>>>>>>> I attempted that at some point, but IIRC we still end up >>>>>>>>>>>>>> not >>>>>>>>>>>>>> resolving >>>>>>>>>>>>>> some RESULT_DECLs because not all of them get processed >>>>>>>>>>>>>> through >>>>>>>>>>>>>> cxx_eval_constant_expression before we manipulate >>>>>>>>>>>>>> ctx->global->values >>>>>>>>>>>>>> with them. I'll try this approach more carefully and >>>>>>>>>>>>>> report >>>>>>>>>>>>>> back >>>>>>>>>>>>>> with >>>>>>>>>>>>>> more specifics. >>>>>>>>>>>>> >>>>>>>>>>>>> It turns out that immediately resolving >>>>>>>>>>>>> RESULT_DECLs/'this' to >>>>>>>>>>>>> the >>>>>>>>>>>>> ultimate ctx->object would not interact well with >>>>>>>>>>>>> constexpr_call >>>>>>>>>>>>> caching: >>>>>>>>>>>>> >>>>>>>>>>>>> struct A { A() = default; A(const A&); A *ap = >>>>>>>>>>>>> this; }; >>>>>>>>>>>>> constexpr A foo() { return {}; } >>>>>>>>>>>>> constexpr A a = foo(); >>>>>>>>>>>>> constexpr A b = foo(); >>>>>>>>>>>>> static_assert(b.ap == &b); // fails >>>>>>>>>>>>> >>>>>>>>>>>>> Evaluation of the first call to foo() returns {&a}, since >>>>>>>>>>>>> we >>>>>>>>>>>>> resolve >>>>>>>>>>>>> 'this' to &a due to guaranteed RVO, and we cache this >>>>>>>>>>>>> result. >>>>>>>>>>>>> Evaluation of the second call to foo() just returns the >>>>>>>>>>>>> cached >>>>>>>>>>>>> result >>>>>>>>>>>>> from the constexpr_call cache, and so we get {&a} again. >>>>>>>>>>>>> >>>>>>>>>>>>> So it seems we would have to mark the result of a >>>>>>>>>>>>> constexpr >>>>>>>>>>>>> call >>>>>>>>>>>>> as >>>>>>>>>>>>> not >>>>>>>>>>>>> cacheable whenever this RVO applies during its evaluation, >>>>>>>>>>>>> even >>>>>>>>>>>>> when >>>>>>>>>>>>> doing the RVO has no observable difference in the final >>>>>>>>>>>>> result >>>>>>>>>>>>> (i.e. >>>>>>>>>>>>> the >>>>>>>>>>>>> constructor does not try to save the 'this' pointer). >>>>>>>>>>>>> >>>>>>>>>>>>> Would the performance impact of disabling caching whenever >>>>>>>>>>>>> RVO >>>>>>>>>>>>> applies >>>>>>>>>>>>> during constexpr call evaluation be worth it, or should we >>>>>>>>>>>>> go >>>>>>>>>>>>> with >>>>>>>>>>>>> something like my first patch which "almost works," and >>>>>>>>>>>>> which >>>>>>>>>>>>> marks a >>>>>>>>>>>>> constexpr call as not cacheable only when we replace a >>>>>>>>>>>>> RESULT_DECL >>>>>>>>>>>>> in >>>>>>>>>>>>> the result of the call? >>>>>>>>>>>> >>>>>>>>>>>> Could we search through the result of the call for >>>>>>>>>>>> ctx->object >>>>>>>>>>>> and >>>>>>>>>>>> cache >>>>>>>>>>>> if we >>>>>>>>>>>> don't find it? >>>>>>>>>>> >>>>>>>>>>> Hmm, I think the result of the call could still depend on >>>>>>>>>>> ctx->object >>>>>>>>>>> without ctx->object explicitly appearing in the result. >>>>>>>>>>> Consider >>>>>>>>>>> the >>>>>>>>>>> following testcase: >>>>>>>>>>> >>>>>>>>>>> struct A { >>>>>>>>>>> A() = default; A(const A&); >>>>>>>>>>> constexpr A(const A *q) : d{this - p} { } >>>>>>>>>> >>>>>>>>>> Oops sorry, that 'q' should be a 'p'. >>>>>>>>>> >>>>>>>>>>> long d = 0; >>>>>>>>>>> }; >>>>>>>>>>> >>>>>>>>>>> constexpr A baz(const A *q) { return A(p); }; >>>>>>>>>> >>>>>>>>>> And same here. >>>>>>>>>> >>>>>>>>>>> constexpr A a = baz(&a); >>>>>>>>>>> constexpr A b = baz(&a); // no error >>>>>>>>>>> >>>>>>>>>>> The initialization of 'b' should be ill-formed, but the result >>>>>>>>>>> of >>>>>>>>>>> the >>>>>>>>>>> first call to baz(&a) would be {0}, so we would cache it and >>>>>>>>>>> then >>>>>>>>>>> reuse >>>>>>>>>>> the result when initializing 'b'. >>>>>>>>> >>>>>>>>> Ah, true. Can we still cache if we're initializing something that >>>>>>>>> isn't >>>>>>>>> TREE_STATIC? >>>>>>>> >>>>>>>> Hmm, we correctly compile the analogous non-TREE_STATIC testcase >>>>>>>> >>>>>>>> struct A { >>>>>>>> A() = default; A(const A&); >>>>>>>> constexpr A(const A *p) : d{this == p} { } >>>>>>>> bool d; >>>>>>>> }; >>>>>>>> >>>>>>>> constexpr A baz(const A *p) { return A(p); } >>>>>>>> >>>>>>>> void foo() { >>>>>>>> constexpr A x = baz(&x); >>>>>>>> constexpr A y = baz(&x); >>>>>>>> static_assert(!y.d); >>>>>>>> } >>>>>>>> >>>>>>>> because the calls 'baz(&x)' are considered to have non-constant >>>>>>>> arguments. >>>>>>>> >>>>>>>> >>>>>>>> Unfortunately, we can come up with another trick to fool the >>>>>>>> constexpr_call >>>>>>>> cache in the presence of RVO even in the !TREE_STATIC case: >>>>>>>> >>>>>>>> struct B { >>>>>>>> B() = default; B(const B&); >>>>>>>> constexpr B(int) : q{!this[-1].q} { } >>>>>>>> bool q = false; >>>>>>>> }; >>>>>>>> >>>>>>>> constexpr B baz() { return B(0); } >>>>>>>> >>>>>>>> void foo() >>>>>>>> { >>>>>>>> constexpr B d[2] = { {}, baz() }; >>>>>>>> constexpr B e = baz(); >>>>>>>> } >>>>>>>> >>>>>>>> The initialization of the local variable 'e' should be invalid, but >>>>>>>> if >>>>>>>> we >>>>>>>> cached the first call to 'baz' then we would wrongly accept it. >>>>>>> >>>>>>> Right. >>>>>>> >>>>>>> We ought to be able to distinguish between uses of the RESULT_DECL >>>>>>> only >>>>>>> for >>>>>>> storing to it (cacheable) and any other use, either reading from it or >>>>>>> messing >>>>>>> with its address. But I think that's a future direction, and we >>>>>>> should >>>>>>> stick >>>>>>> with your patch that almost works for GCC 10. >>>>>> >>>>>> Sounds good. Does the following then look OK to commit? >>>>>> >>>>>> -- >8 -- >>>>>> >>>>>> Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call >>>>>> [PR94034] >>>>>> >>>>>> When evaluating the initializer of 'a' in the following example >>>>>> >>>>>> struct A { >>>>>> A() = default; A(const A&); >>>>>> A *p = this; >>>>>> }; >>>>>> constexpr A foo() { return {}; } >>>>>> constexpr A a = foo(); >>>>>> >>>>>> the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by >>>>>> foo >>>>>> gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the >>>>>> 'this' >>>>>> should really be resolved to '&a'. >>>>>> >>>>>> Fixing this properly by immediately resolving 'this' and >>>>>> PLACEHOLDER_EXPRs >>>>>> to >>>>>> the ultimate object under construction would in general mean that we >>>>>> would >>>>>> no >>>>>> longer be able to cache constexpr calls for which RVO possibly applies, >>>>>> because >>>>>> the result of the call may now depend on the ultimate object under >>>>>> construction. >>>>>> >>>>>> So as a mostly correct stopgap solution that retains cachability of >>>>>> RVO'd >>>>>> constexpr calls, this patch fixes this issue by rewriting all >>>>>> occurrences of >>>>>> the >>>>>> RESULT_DECL in the result of a constexpr function call with the current >>>>>> object >>>>>> under construction, after the call returns. This means the 'this' >>>>>> pointer >>>>>> during construction of the temporary will still point to the temporary >>>>>> object >>>>>> instead of the ultimate object, but besides that this approach seems >>>>>> functionally equivalent to the proper approach. >>>>>> >>>>>> Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, does >>>>>> this >>>>>> look >>>>>> OK to commit? >>>>>> >>>>>> gcc/cp/ChangeLog: >>>>>> >>>>>> PR c++/94034 >>>>>> * constexpr.c (replace_result_decl_data): New struct. >>>>>> (replace_result_decl_data_r): New function. >>>>>> (replace_result_decl): New function. >>>>>> (cxx_eval_call_expression): Use it. >>>>>> * tree.c (build_aggr_init_expr): Propagate the location of the >>>>>> initializer to the AGGR_INIT_EXPR. >>>>>> >>>>>> gcc/testsuite/ChangeLog: >>>>>> >>>>>> PR c++/94034 >>>>>> * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. >>>>>> * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. >>>>>> * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. >>>>>> * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. >>>>>> --- >>>>>> gcc/cp/constexpr.c | 51 >>>>>> +++++++++++++++++++ >>>>>> gcc/cp/tree.c | 3 ++ >>>>>> .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++ >>>>>> .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ >>>>>> .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 ++++++++++++++++++ >>>>>> .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ >>>>>> 6 files changed, 204 insertions(+) >>>>>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C >>>>>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C >>>>>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C >>>>>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C >>>>>> >>>>>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >>>>>> index 5793430c88d..4cf5812e8a5 100644 >>>>>> --- a/gcc/cp/constexpr.c >>>>>> +++ b/gcc/cp/constexpr.c >>>>>> @@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx >>>>>> *ctx, >>>>>> tree call, >>>>>> return cp_build_addr_expr (obj, complain); >>>>>> } >>>>>> +/* Data structure used by replace_result_decl and >>>>>> replace_result_decl_r. >>>>>> */ >>>>>> + >>>>>> +struct replace_result_decl_data >>>>>> +{ >>>>>> + /* The RESULT_DECL we want to replace. */ >>>>>> + tree decl; >>>>>> + /* The replacement for DECL. */ >>>>>> + tree replacement; >>>>>> + /* Whether we've performed any replacements. */ >>>>>> + bool changed; >>>>>> +}; >>>>>> + >>>>>> +/* Helper function for replace_result_decl, called through >>>>>> cp_walk_tree. >>>>>> */ >>>>>> + >>>>>> +static tree >>>>>> +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) >>>>>> +{ >>>>>> + replace_result_decl_data *d = (replace_result_decl_data *) data; >>>>>> + >>>>>> + if (*tp == d->decl) >>>>>> + { >>>>>> + *tp = unshare_expr (d->replacement); >>>>>> + d->changed = true; >>>>>> + *walk_subtrees = 0; >>>>>> + } >>>>>> + else if (TYPE_P (*tp)) >>>>>> + *walk_subtrees = 0; >>>>>> + >>>>>> + return NULL_TREE; >>>>>> +} >>>>>> + >>>>>> +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared >>>>>> copy >>>>>> of) >>>>>> + REPLACEMENT within *TP. Returns true iff a replacement was >>>>>> performed. >>>>>> */ >>>>>> + >>>>>> +static bool >>>>>> +replace_result_decl (tree *tp, tree decl, tree replacement) >>>>>> +{ >>>>>> + gcc_assert (TREE_CODE (decl) == RESULT_DECL); >>>>>> + replace_result_decl_data data = { decl, replacement, false }; >>>>>> + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); >>>>>> + return data.changed; >>>>>> +} >>>>>> + >>>>>> /* Subroutine of cxx_eval_constant_expression. >>>>>> Evaluate the call expression tree T in the context of OLD_CALL >>>>>> expression >>>>>> evaluation. */ >>>>>> @@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const constexpr_ctx >>>>>> *ctx, >>>>>> tree t, >>>>>> break; >>>>>> } >>>>>> } >>>>>> + >>>>>> + /* Rewrite all occurrences of the function's RESULT_DECL with the >>>>>> + current object under construction. */ >>>>>> + if (ctx->object >>>>>> + && (same_type_ignoring_top_level_qualifiers_p >>>>>> + (TREE_TYPE (res), TREE_TYPE (ctx->object)))) >>>>> >>>>> When can they have different types? >>>> >>>> During prior testing I tried replacing the same_type_p check with an assert, >>>> i.e.: >>>> >>>> if (ctx->object >>>> && AGGREGATE_TYPE_P (TREE_TYPE (res))) >>>> { >>>> gcc_assert (same_type_ignoring_top_level_qualifiers_p >>>> (TREE_TYPE (res), TREE_TYPE (ctx->object)); >>>> ... >>>> } >>>> >>>> and IIRC I was able to trigger the assert only when the type of >>>> ctx->object and of the RESULT_DECL are distinct empty class types. >>>> Here's a testcase: >>>> >>>> struct empty1 { }; >>>> constexpr empty1 foo1() { return {}; } >>>> >>>> struct empty2 { }; >>>> constexpr empty2 foo2(empty1) { return {}; } >>>> >>>> constexpr empty2 a = foo2(foo1()); >>>> >>>> The initializer of 'a' has the form >>>> TARGET_EXPR <D.2389, foo2 (foo1 ();, <<< Unknown tree: empty_class_expr >>>>>>> ;)>; >>>> and so during evaluation of 'foo1()', ctx->object still points to 'a' and we >>>> trip over the same_type_p assert. (Is it possible that the call to foo1 >>>> is missing a TARGET_EXPR?) >>> >>> Hmm, that would be because of >>> >>>> if (is_empty_class (TREE_TYPE (arg)) >>>> && simple_empty_class_p (TREE_TYPE (arg), arg, INIT_EXPR)) >>>> { > while (TREE_CODE (arg) == TARGET_EXPR) >>>> /* We're disconnecting the initializer from its target, >>>> don't create a temporary. */ >>>> arg = TARGET_EXPR_INITIAL (arg); >>> >>> in build_call_a. >> >> Ah okay. >> >>> >>>> Either way, it would be safe to skip empty class types. Should we change >>>> the >>>> condition to >>>> >>>> if (ctx->object >>>> && AGGREGATE_TYPE_P (TREE_TYPE (res)) >>>> && !is_empty_class (TREE_TYPE (res))) >>>> { >>>> ... >>>> } >>>> >>>> and turn the same_type_p check into an assert? >>> >>> Sounds good. Let's say a checking_assert. >> >> Great, I'm bootstrapping/regtesting the following overnight, does it >> look OK to commit if successful? >> >> -- >8 -- >> >> Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call [PR94034] >> >> gcc/cp/ChangeLog: >> >> PR c++/94034 >> * constexpr.c (replace_result_decl_data): New struct. >> (replace_result_decl_data_r): New function. >> (replace_result_decl): New function. >> (cxx_eval_call_expression): Use it. >> * tree.c (build_aggr_init_expr): Set the location of the AGGR_INIT_EXPR >> to that of its initializer. >> >> gcc/testsuite/ChangeLog: >> >> PR c++/94034 >> * g++.dg/cpp0x/constexpr-empty15.C: New test. >> * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. >> * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. >> * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. >> * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. >> --- >> gcc/cp/constexpr.c | 53 +++++++++++++++++++ >> gcc/cp/tree.c | 3 ++ >> .../g++.dg/cpp0x/constexpr-empty15.C | 9 ++++ >> .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 +++++++++ >> .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ >> .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 +++++++++++++++++ >> .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ >> 7 files changed, 215 insertions(+) >> create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C >> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C >> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C >> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C >> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C >> >> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >> index d8636ddb92f..00e313178f9 100644 >> --- a/gcc/cp/constexpr.c >> +++ b/gcc/cp/constexpr.c >> @@ -2029,6 +2029,51 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call, >> return cp_build_addr_expr (obj, complain); >> } >> >> +/* Data structure used by replace_result_decl and replace_result_decl_r. */ >> + >> +struct replace_result_decl_data >> +{ >> + /* The RESULT_DECL we want to replace. */ >> + tree decl; >> + /* The replacement for DECL. */ >> + tree replacement; >> + /* Whether we've performed any replacements. */ >> + bool changed; >> +}; >> + >> +/* Helper function for replace_result_decl, called through cp_walk_tree. */ >> + >> +static tree >> +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) >> +{ >> + replace_result_decl_data *d = (replace_result_decl_data *) data; >> + >> + if (*tp == d->decl) >> + { >> + *tp = unshare_expr (d->replacement); >> + d->changed = true; >> + *walk_subtrees = 0; >> + } >> + else if (TYPE_P (*tp)) >> + *walk_subtrees = 0; >> + >> + return NULL_TREE; >> +} >> + >> +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy of) >> + REPLACEMENT within *TP. Returns true iff a replacement was performed. */ >> + >> +static bool >> +replace_result_decl (tree *tp, tree decl, tree replacement) >> +{ >> + gcc_checking_assert (TREE_CODE (decl) == RESULT_DECL >> + && (same_type_ignoring_top_level_qualifiers_p >> + (TREE_TYPE (decl), TREE_TYPE (replacement)))); >> + replace_result_decl_data data = { decl, replacement, false }; >> + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); >> + return data.changed; >> +} >> + >> /* Subroutine of cxx_eval_constant_expression. >> Evaluate the call expression tree T in the context of OLD_CALL expression >> evaluation. */ >> @@ -2536,6 +2581,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, >> break; >> } >> } >> + >> + /* Rewrite all occurrences of the function's RESULT_DECL with the >> + current object under construction. */ >> + if (ctx->object >> + && AGGREGATE_TYPE_P (TREE_TYPE (res)) >> + && !is_empty_class (TREE_TYPE (res))) >> + if (replace_result_decl (&result, res, ctx->object)) >> + cacheable = false; > > We should probably require !*non_constant_p here before doing the > replacement because it doesn't make sense to do this transformation for > non-constant calls to constexpr functions. Sounds good. > And requiring !*non_constant_p means that the result must be a reduced > constant expression No, it doesn't: it means that the result doesn't contain anything that makes it not a valid sub- constant expression. Indeed, if there's a RESULT_DECL in there that makes it not a reduced constant expression. Jason
On Tue, 14 Apr 2020, Jason Merrill wrote: > On 4/14/20 9:01 AM, Patrick Palka wrote: > > On Tue, 14 Apr 2020, Patrick Palka wrote: > > > > > On Mon, 13 Apr 2020, Jason Merrill wrote: > > > > > > > On 4/13/20 2:49 PM, Patrick Palka wrote: > > > > > On Mon, 13 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > On 4/12/20 9:43 AM, Patrick Palka wrote: > > > > > > > On Sat, 11 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > On 4/10/20 5:47 PM, Patrick Palka wrote: > > > > > > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > > > > > > On 4/10/20 2:15 PM, Patrick Palka wrote: > > > > > > > > > > > On Fri, 10 Apr 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > > > > > > > > > On Fri, 10 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > > > > > > > > > > > On 4/10/20 1:04 PM, Patrick Palka wrote: > > > > > > > > > > > > > > On Thu, 9 Apr 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > On Thu, 9 Apr 2020, Jason Merrill wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 4/8/20 7:49 PM, Patrick Palka wrote: > > > > > > > > > > > > > > > > > When evaluating the initializer of 'a' in the > > > > > > > > > > > > > > > > > following > > > > > > > > > > > > > > > > > example > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > struct A { A *p = this; }; > > > > > > > > > > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > > > > > > > > > > constexpr A a = foo(); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the > > > > > > > > > > > > > > > > > aggregate > > > > > > > > > > > > > > > > > initializer > > > > > > > > > > > > > > > > > returned > > > > > > > > > > > > > > > > > by foo > > > > > > > > > > > > > > > > > gets resolved to the RESULT_DECL of foo. But > > > > > > > > > > > > > > > > > due to > > > > > > > > > > > > > > > > > guaranteed > > > > > > > > > > > > > > > > > RVO, > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > 'this' > > > > > > > > > > > > > > > > > should really be resolved to '&a'. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It seems to me that the right approach would > > > > > > > > > > > > > > > > > be to > > > > > > > > > > > > > > > > > immediately > > > > > > > > > > > > > > > > > resolve > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > PLACEHOLDER_EXPR to the correct target object > > > > > > > > > > > > > > > > > during > > > > > > > > > > > > > > > > > evaluation > > > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > 'foo()', > > > > > > > > > > > > > > > > > so > > > > > > > > > > > > > > > > > that we could use 'this' to access objects > > > > > > > > > > > > > > > > > adjacent > > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > current > > > > > > > > > > > > > > > > > object in > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > ultimate storage location. (I think #c2 of PR > > > > > > > > > > > > > > > > > c++/94537 > > > > > > > > > > > > > > > > > is > > > > > > > > > > > > > > > > > an > > > > > > > > > > > > > > > > > example > > > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > such > > > > > > > > > > > > > > > > > usage of 'this', which currently doesn't work. > > > > > > > > > > > > > > > > > But > > > > > > > > > > > > > > > > > as > > > > > > > > > > > > > > > > > #c1 > > > > > > > > > > > > > > > > > shows > > > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > > > don't > > > > > > > > > > > > > > > > > seem > > > > > > > > > > > > > > > > > to handle this case correctly in non-constexpr > > > > > > > > > > > > > > > > > initialization > > > > > > > > > > > > > > > > > either.) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > As I commented in the PR, the standard doesn't > > > > > > > > > > > > > > > > require > > > > > > > > > > > > > > > > this to > > > > > > > > > > > > > > > > work > > > > > > > > > > > > > > > > because A > > > > > > > > > > > > > > > > is trivially copyable, and our ABI makes it > > > > > > > > > > > > > > > > impossible. > > > > > > > > > > > > > > > > But > > > > > > > > > > > > > > > > there's > > > > > > > > > > > > > > > > still a > > > > > > > > > > > > > > > > constexpr bug when we add > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > A() = default; A(const A&); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > clang doesn't require the constructors to make > > > > > > > > > > > > > > > > this > > > > > > > > > > > > > > > > work > > > > > > > > > > > > > > > > for > > > > > > > > > > > > > > > > constant > > > > > > > > > > > > > > > > initialization, but similarly can't make it work > > > > > > > > > > > > > > > > for > > > > > > > > > > > > > > > > non-constant > > > > > > > > > > > > > > > > initialization. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That makes a lot of sense, thanks for the detailed > > > > > > > > > > > > > > > explanation. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I haven't yet been able to make a solution > > > > > > > > > > > > > > > > > using the > > > > > > > > > > > > > > > > > above > > > > > > > > > > > > > > > > > approach > > > > > > > > > > > > > > > > > work -- > > > > > > > > > > > > > > > > > making sure we use the ultimate object instead > > > > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > RESULT_DECL > > > > > > > > > > > > > > > > > whenever > > > > > > > > > > > > > > > > > we > > > > > > > > > > > > > > > > > access ctx->global->values is proving to be > > > > > > > > > > > > > > > > > tricky > > > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > subtle. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Do we need to go through ctx->global->values? > > > > > > > > > > > > > > > > Would > > > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > work > > > > > > > > > > > > > > > > for > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > RESULT_DECL case in cxx_eval_constant_expression > > > > > > > > > > > > > > > > to go > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > straight > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > ctx->object or ctx->ctor instead? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I attempted that at some point, but IIRC we still > > > > > > > > > > > > > > > end up > > > > > > > > > > > > > > > not > > > > > > > > > > > > > > > resolving > > > > > > > > > > > > > > > some RESULT_DECLs because not all of them get > > > > > > > > > > > > > > > processed > > > > > > > > > > > > > > > through > > > > > > > > > > > > > > > cxx_eval_constant_expression before we manipulate > > > > > > > > > > > > > > > ctx->global->values > > > > > > > > > > > > > > > with them. I'll try this approach more carefully > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > report > > > > > > > > > > > > > > > back > > > > > > > > > > > > > > > with > > > > > > > > > > > > > > > more specifics. > > > > > > > > > > > > > > > > > > > > > > > > > > > > It turns out that immediately resolving > > > > > > > > > > > > > > RESULT_DECLs/'this' to > > > > > > > > > > > > > > the > > > > > > > > > > > > > > ultimate ctx->object would not interact well with > > > > > > > > > > > > > > constexpr_call > > > > > > > > > > > > > > caching: > > > > > > > > > > > > > > > > > > > > > > > > > > > > struct A { A() = default; A(const A&); A *ap > > > > > > > > > > > > > > = > > > > > > > > > > > > > > this; }; > > > > > > > > > > > > > > constexpr A foo() { return {}; } > > > > > > > > > > > > > > constexpr A a = foo(); > > > > > > > > > > > > > > constexpr A b = foo(); > > > > > > > > > > > > > > static_assert(b.ap == &b); // fails > > > > > > > > > > > > > > > > > > > > > > > > > > > > Evaluation of the first call to foo() returns {&a}, > > > > > > > > > > > > > > since > > > > > > > > > > > > > > we > > > > > > > > > > > > > > resolve > > > > > > > > > > > > > > 'this' to &a due to guaranteed RVO, and we cache > > > > > > > > > > > > > > this > > > > > > > > > > > > > > result. > > > > > > > > > > > > > > Evaluation of the second call to foo() just returns > > > > > > > > > > > > > > the > > > > > > > > > > > > > > cached > > > > > > > > > > > > > > result > > > > > > > > > > > > > > from the constexpr_call cache, and so we get {&a} > > > > > > > > > > > > > > again. > > > > > > > > > > > > > > > > > > > > > > > > > > > > So it seems we would have to mark the result of a > > > > > > > > > > > > > > constexpr > > > > > > > > > > > > > > call > > > > > > > > > > > > > > as > > > > > > > > > > > > > > not > > > > > > > > > > > > > > cacheable whenever this RVO applies during its > > > > > > > > > > > > > > evaluation, > > > > > > > > > > > > > > even > > > > > > > > > > > > > > when > > > > > > > > > > > > > > doing the RVO has no observable difference in the > > > > > > > > > > > > > > final > > > > > > > > > > > > > > result > > > > > > > > > > > > > > (i.e. > > > > > > > > > > > > > > the > > > > > > > > > > > > > > constructor does not try to save the 'this' > > > > > > > > > > > > > > pointer). > > > > > > > > > > > > > > > > > > > > > > > > > > > > Would the performance impact of disabling caching > > > > > > > > > > > > > > whenever > > > > > > > > > > > > > > RVO > > > > > > > > > > > > > > applies > > > > > > > > > > > > > > during constexpr call evaluation be worth it, or > > > > > > > > > > > > > > should we > > > > > > > > > > > > > > go > > > > > > > > > > > > > > with > > > > > > > > > > > > > > something like my first patch which "almost works," > > > > > > > > > > > > > > and > > > > > > > > > > > > > > which > > > > > > > > > > > > > > marks a > > > > > > > > > > > > > > constexpr call as not cacheable only when we replace > > > > > > > > > > > > > > a > > > > > > > > > > > > > > RESULT_DECL > > > > > > > > > > > > > > in > > > > > > > > > > > > > > the result of the call? > > > > > > > > > > > > > > > > > > > > > > > > > > Could we search through the result of the call for > > > > > > > > > > > > > ctx->object > > > > > > > > > > > > > and > > > > > > > > > > > > > cache > > > > > > > > > > > > > if we > > > > > > > > > > > > > don't find it? > > > > > > > > > > > > > > > > > > > > > > > > Hmm, I think the result of the call could still depend > > > > > > > > > > > > on > > > > > > > > > > > > ctx->object > > > > > > > > > > > > without ctx->object explicitly appearing in the result. > > > > > > > > > > > > Consider > > > > > > > > > > > > the > > > > > > > > > > > > following testcase: > > > > > > > > > > > > > > > > > > > > > > > > struct A { > > > > > > > > > > > > A() = default; A(const A&); > > > > > > > > > > > > constexpr A(const A *q) : d{this - p} { } > > > > > > > > > > > > > > > > > > > > > > Oops sorry, that 'q' should be a 'p'. > > > > > > > > > > > > > > > > > > > > > > > long d = 0; > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > constexpr A baz(const A *q) { return A(p); }; > > > > > > > > > > > > > > > > > > > > > > And same here. > > > > > > > > > > > > > > > > > > > > > > > constexpr A a = baz(&a); > > > > > > > > > > > > constexpr A b = baz(&a); // no error > > > > > > > > > > > > > > > > > > > > > > > > The initialization of 'b' should be ill-formed, but the > > > > > > > > > > > > result > > > > > > > > > > > > of > > > > > > > > > > > > the > > > > > > > > > > > > first call to baz(&a) would be {0}, so we would cache it > > > > > > > > > > > > and > > > > > > > > > > > > then > > > > > > > > > > > > reuse > > > > > > > > > > > > the result when initializing 'b'. > > > > > > > > > > > > > > > > > > > > Ah, true. Can we still cache if we're initializing > > > > > > > > > > something that > > > > > > > > > > isn't > > > > > > > > > > TREE_STATIC? > > > > > > > > > > > > > > > > > > Hmm, we correctly compile the analogous non-TREE_STATIC > > > > > > > > > testcase > > > > > > > > > > > > > > > > > > struct A { > > > > > > > > > A() = default; A(const A&); > > > > > > > > > constexpr A(const A *p) : d{this == p} { } > > > > > > > > > bool d; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > constexpr A baz(const A *p) { return A(p); } > > > > > > > > > > > > > > > > > > void foo() { > > > > > > > > > constexpr A x = baz(&x); > > > > > > > > > constexpr A y = baz(&x); > > > > > > > > > static_assert(!y.d); > > > > > > > > > } > > > > > > > > > > > > > > > > > > because the calls 'baz(&x)' are considered to have > > > > > > > > > non-constant > > > > > > > > > arguments. > > > > > > > > > > > > > > > > > > > > > > > > > > > Unfortunately, we can come up with another trick to fool the > > > > > > > > > constexpr_call > > > > > > > > > cache in the presence of RVO even in the !TREE_STATIC case: > > > > > > > > > > > > > > > > > > struct B { > > > > > > > > > B() = default; B(const B&); > > > > > > > > > constexpr B(int) : q{!this[-1].q} { } > > > > > > > > > bool q = false; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > constexpr B baz() { return B(0); } > > > > > > > > > > > > > > > > > > void foo() > > > > > > > > > { > > > > > > > > > constexpr B d[2] = { {}, baz() }; > > > > > > > > > constexpr B e = baz(); > > > > > > > > > } > > > > > > > > > > > > > > > > > > The initialization of the local variable 'e' should be > > > > > > > > > invalid, but > > > > > > > > > if > > > > > > > > > we > > > > > > > > > cached the first call to 'baz' then we would wrongly accept > > > > > > > > > it. > > > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > We ought to be able to distinguish between uses of the > > > > > > > > RESULT_DECL > > > > > > > > only > > > > > > > > for > > > > > > > > storing to it (cacheable) and any other use, either reading from > > > > > > > > it or > > > > > > > > messing > > > > > > > > with its address. But I think that's a future direction, and we > > > > > > > > should > > > > > > > > stick > > > > > > > > with your patch that almost works for GCC 10. > > > > > > > > > > > > > > Sounds good. Does the following then look OK to commit? > > > > > > > > > > > > > > -- >8 -- > > > > > > > > > > > > > > Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr > > > > > > > call > > > > > > > [PR94034] > > > > > > > > > > > > > > When evaluating the initializer of 'a' in the following example > > > > > > > > > > > > > > struct A { > > > > > > > A() = default; A(const A&); > > > > > > > A *p = this; > > > > > > > }; > > > > > > > constexpr A foo() { return {}; } > > > > > > > constexpr A a = foo(); > > > > > > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer > > > > > > > returned by > > > > > > > foo > > > > > > > gets resolved to the RESULT_DECL of foo. But due to guaranteed > > > > > > > RVO, the > > > > > > > 'this' > > > > > > > should really be resolved to '&a'. > > > > > > > > > > > > > > Fixing this properly by immediately resolving 'this' and > > > > > > > PLACEHOLDER_EXPRs > > > > > > > to > > > > > > > the ultimate object under construction would in general mean that > > > > > > > we > > > > > > > would > > > > > > > no > > > > > > > longer be able to cache constexpr calls for which RVO possibly > > > > > > > applies, > > > > > > > because > > > > > > > the result of the call may now depend on the ultimate object under > > > > > > > construction. > > > > > > > > > > > > > > So as a mostly correct stopgap solution that retains cachability > > > > > > > of > > > > > > > RVO'd > > > > > > > constexpr calls, this patch fixes this issue by rewriting all > > > > > > > occurrences of > > > > > > > the > > > > > > > RESULT_DECL in the result of a constexpr function call with the > > > > > > > current > > > > > > > object > > > > > > > under construction, after the call returns. This means the 'this' > > > > > > > pointer > > > > > > > during construction of the temporary will still point to the > > > > > > > temporary > > > > > > > object > > > > > > > instead of the ultimate object, but besides that this approach > > > > > > > seems > > > > > > > functionally equivalent to the proper approach. > > > > > > > > > > > > > > Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, > > > > > > > does > > > > > > > this > > > > > > > look > > > > > > > OK to commit? > > > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > > > PR c++/94034 > > > > > > > * constexpr.c (replace_result_decl_data): New struct. > > > > > > > (replace_result_decl_data_r): New function. > > > > > > > (replace_result_decl): New function. > > > > > > > (cxx_eval_call_expression): Use it. > > > > > > > * tree.c (build_aggr_init_expr): Propagate the location of the > > > > > > > initializer to the AGGR_INIT_EXPR. > > > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > > > PR c++/94034 > > > > > > > * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. > > > > > > > * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. > > > > > > > * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. > > > > > > > * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. > > > > > > > --- > > > > > > > gcc/cp/constexpr.c | 51 > > > > > > > +++++++++++++++++++ > > > > > > > gcc/cp/tree.c | 3 ++ > > > > > > > .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++ > > > > > > > .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ > > > > > > > .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 > > > > > > > ++++++++++++++++++ > > > > > > > .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 > > > > > > > +++++++++++++++++ > > > > > > > 6 files changed, 204 insertions(+) > > > > > > > create mode 100644 > > > > > > > gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > > > > > > > create mode 100644 > > > > > > > gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > > > > > > > create mode 100644 > > > > > > > gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > > > > > > > create mode 100644 > > > > > > > gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > > > > > > > > > > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > > > > > > index 5793430c88d..4cf5812e8a5 100644 > > > > > > > --- a/gcc/cp/constexpr.c > > > > > > > +++ b/gcc/cp/constexpr.c > > > > > > > @@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const > > > > > > > constexpr_ctx > > > > > > > *ctx, > > > > > > > tree call, > > > > > > > return cp_build_addr_expr (obj, complain); > > > > > > > } > > > > > > > +/* Data structure used by replace_result_decl and > > > > > > > replace_result_decl_r. > > > > > > > */ > > > > > > > + > > > > > > > +struct replace_result_decl_data > > > > > > > +{ > > > > > > > + /* The RESULT_DECL we want to replace. */ > > > > > > > + tree decl; > > > > > > > + /* The replacement for DECL. */ > > > > > > > + tree replacement; > > > > > > > + /* Whether we've performed any replacements. */ > > > > > > > + bool changed; > > > > > > > +}; > > > > > > > + > > > > > > > +/* Helper function for replace_result_decl, called through > > > > > > > cp_walk_tree. > > > > > > > */ > > > > > > > + > > > > > > > +static tree > > > > > > > +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) > > > > > > > +{ > > > > > > > + replace_result_decl_data *d = (replace_result_decl_data *) > > > > > > > data; > > > > > > > + > > > > > > > + if (*tp == d->decl) > > > > > > > + { > > > > > > > + *tp = unshare_expr (d->replacement); > > > > > > > + d->changed = true; > > > > > > > + *walk_subtrees = 0; > > > > > > > + } > > > > > > > + else if (TYPE_P (*tp)) > > > > > > > + *walk_subtrees = 0; > > > > > > > + > > > > > > > + return NULL_TREE; > > > > > > > +} > > > > > > > + > > > > > > > +/* Replace every occurrence of DECL, a RESULT_DECL, with (an > > > > > > > unshared > > > > > > > copy > > > > > > > of) > > > > > > > + REPLACEMENT within *TP. Returns true iff a replacement was > > > > > > > performed. > > > > > > > */ > > > > > > > + > > > > > > > +static bool > > > > > > > +replace_result_decl (tree *tp, tree decl, tree replacement) > > > > > > > +{ > > > > > > > + gcc_assert (TREE_CODE (decl) == RESULT_DECL); > > > > > > > + replace_result_decl_data data = { decl, replacement, false }; > > > > > > > + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, > > > > > > > &data); > > > > > > > + return data.changed; > > > > > > > +} > > > > > > > + > > > > > > > /* Subroutine of cxx_eval_constant_expression. > > > > > > > Evaluate the call expression tree T in the context of > > > > > > > OLD_CALL > > > > > > > expression > > > > > > > evaluation. */ > > > > > > > @@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const > > > > > > > constexpr_ctx > > > > > > > *ctx, > > > > > > > tree t, > > > > > > > break; > > > > > > > } > > > > > > > } > > > > > > > + > > > > > > > + /* Rewrite all occurrences of the function's RESULT_DECL > > > > > > > with the > > > > > > > + current object under construction. */ > > > > > > > + if (ctx->object > > > > > > > + && (same_type_ignoring_top_level_qualifiers_p > > > > > > > + (TREE_TYPE (res), TREE_TYPE (ctx->object)))) > > > > > > > > > > > > When can they have different types? > > > > > > > > > > During prior testing I tried replacing the same_type_p check with an > > > > > assert, > > > > > i.e.: > > > > > > > > > > if (ctx->object > > > > > && AGGREGATE_TYPE_P (TREE_TYPE (res))) > > > > > { > > > > > gcc_assert (same_type_ignoring_top_level_qualifiers_p > > > > > (TREE_TYPE (res), TREE_TYPE (ctx->object)); > > > > > ... > > > > > } > > > > > > > > > > and IIRC I was able to trigger the assert only when the type of > > > > > ctx->object and of the RESULT_DECL are distinct empty class types. > > > > > Here's a testcase: > > > > > > > > > > struct empty1 { }; > > > > > constexpr empty1 foo1() { return {}; } > > > > > > > > > > struct empty2 { }; > > > > > constexpr empty2 foo2(empty1) { return {}; } > > > > > > > > > > constexpr empty2 a = foo2(foo1()); > > > > > > > > > > The initializer of 'a' has the form > > > > > TARGET_EXPR <D.2389, foo2 (foo1 ();, <<< Unknown tree: > > > > > empty_class_expr > > > > > > > > ;)>; > > > > > and so during evaluation of 'foo1()', ctx->object still points to 'a' > > > > > and we > > > > > trip over the same_type_p assert. (Is it possible that the call to > > > > > foo1 > > > > > is missing a TARGET_EXPR?) > > > > > > > > Hmm, that would be because of > > > > > > > > > if (is_empty_class (TREE_TYPE (arg)) > > > > > && simple_empty_class_p (TREE_TYPE (arg), arg, > > > > > INIT_EXPR)) > > > > > { > while (TREE_CODE (arg) == TARGET_EXPR) > > > > > /* We're disconnecting the initializer from its target, > > > > > don't create a temporary. */ > > > > > arg = TARGET_EXPR_INITIAL (arg); > > > > > > > > in build_call_a. > > > > > > Ah okay. > > > > > > > > > > > > Either way, it would be safe to skip empty class types. Should we > > > > > change > > > > > the > > > > > condition to > > > > > > > > > > if (ctx->object > > > > > && AGGREGATE_TYPE_P (TREE_TYPE (res)) > > > > > && !is_empty_class (TREE_TYPE (res))) > > > > > { > > > > > ... > > > > > } > > > > > > > > > > and turn the same_type_p check into an assert? > > > > > > > > Sounds good. Let's say a checking_assert. > > > > > > Great, I'm bootstrapping/regtesting the following overnight, does it > > > look OK to commit if successful? > > > > > > -- >8 -- > > > > > > Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call > > > [PR94034] > > > > > > gcc/cp/ChangeLog: > > > > > > PR c++/94034 > > > * constexpr.c (replace_result_decl_data): New struct. > > > (replace_result_decl_data_r): New function. > > > (replace_result_decl): New function. > > > (cxx_eval_call_expression): Use it. > > > * tree.c (build_aggr_init_expr): Set the location of the > > > AGGR_INIT_EXPR > > > to that of its initializer. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR c++/94034 > > > * g++.dg/cpp0x/constexpr-empty15.C: New test. > > > * g++.dg/cpp1y/constexpr-nsdmi6a.C: New test. > > > * g++.dg/cpp1y/constexpr-nsdmi6b.C: New test. > > > * g++.dg/cpp1y/constexpr-nsdmi7a.C: New test. > > > * g++.dg/cpp1y/constexpr-nsdmi7b.C: New test. > > > --- > > > gcc/cp/constexpr.c | 53 +++++++++++++++++++ > > > gcc/cp/tree.c | 3 ++ > > > .../g++.dg/cpp0x/constexpr-empty15.C | 9 ++++ > > > .../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 +++++++++ > > > .../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++ > > > .../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 +++++++++++++++++ > > > .../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++ > > > 7 files changed, 215 insertions(+) > > > create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C > > > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > > index d8636ddb92f..00e313178f9 100644 > > > --- a/gcc/cp/constexpr.c > > > +++ b/gcc/cp/constexpr.c > > > @@ -2029,6 +2029,51 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, > > > tree call, > > > return cp_build_addr_expr (obj, complain); > > > } > > > +/* Data structure used by replace_result_decl and > > > replace_result_decl_r. */ > > > + > > > +struct replace_result_decl_data > > > +{ > > > + /* The RESULT_DECL we want to replace. */ > > > + tree decl; > > > + /* The replacement for DECL. */ > > > + tree replacement; > > > + /* Whether we've performed any replacements. */ > > > + bool changed; > > > +}; > > > + > > > +/* Helper function for replace_result_decl, called through cp_walk_tree. > > > */ > > > + > > > +static tree > > > +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) > > > +{ > > > + replace_result_decl_data *d = (replace_result_decl_data *) data; > > > + > > > + if (*tp == d->decl) > > > + { > > > + *tp = unshare_expr (d->replacement); > > > + d->changed = true; > > > + *walk_subtrees = 0; > > > + } > > > + else if (TYPE_P (*tp)) > > > + *walk_subtrees = 0; > > > + > > > + return NULL_TREE; > > > +} > > > + > > > +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared > > > copy of) > > > + REPLACEMENT within *TP. Returns true iff a replacement was performed. > > > */ > > > + > > > +static bool > > > +replace_result_decl (tree *tp, tree decl, tree replacement) > > > +{ > > > + gcc_checking_assert (TREE_CODE (decl) == RESULT_DECL > > > + && (same_type_ignoring_top_level_qualifiers_p > > > + (TREE_TYPE (decl), TREE_TYPE (replacement)))); > > > + replace_result_decl_data data = { decl, replacement, false }; > > > + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); > > > + return data.changed; > > > +} > > > + > > > /* Subroutine of cxx_eval_constant_expression. > > > Evaluate the call expression tree T in the context of OLD_CALL > > > expression > > > evaluation. */ > > > @@ -2536,6 +2581,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, > > > tree t, > > > break; > > > } > > > } > > > + > > > + /* Rewrite all occurrences of the function's RESULT_DECL with the > > > + current object under construction. */ > > > + if (ctx->object > > > + && AGGREGATE_TYPE_P (TREE_TYPE (res)) > > > + && !is_empty_class (TREE_TYPE (res))) > > > + if (replace_result_decl (&result, res, ctx->object)) > > > + cacheable = false; > > > > We should probably require !*non_constant_p here before doing the > > replacement because it doesn't make sense to do this transformation for > > non-constant calls to constexpr functions. > > Sounds good. > > > And requiring !*non_constant_p means that the result must be a reduced > > constant expression > > No, it doesn't: it means that the result doesn't contain anything that makes > it not a valid sub- constant expression. Indeed, if there's a RESULT_DECL in > there that makes it not a reduced constant expression. Ah okay, that makes sense. I'll add the !*non_constant_p check to the approved patch and commit after another round of testing. > > Jason
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 96497ab85d7..c011f2c7722 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -2029,6 +2029,50 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call, return cp_build_addr_expr (obj, complain); } +/* Data structure used by replace_result_decl and replace_result_decl_r. */ + +struct replace_result_decl_data +{ + /* The RESULT_DECL we want to replace. */ + tree decl; + /* What we're replacing every occurrence of DECL with. */ + tree to; + /* Whether we've performed any replacements. */ + bool changed; +}; + +/* Helper function for replace_result_decl, called through cp_walk_tree. */ + +static tree +replace_result_decl_r (tree *tp, int *walk_subtrees, void *data) +{ + replace_result_decl_data *d = (replace_result_decl_data *) data; + + if (*tp == d->decl) + { + *tp = unshare_expr (d->to); + d->changed = true; + } + else if (TYPE_P (*tp)) + *walk_subtrees = 0; + + return NULL_TREE; +} + +/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy of) + TO within *TP. Returns true iff a replacement was performed. */ + +static bool +replace_result_decl (tree *tp, tree decl, tree to) +{ + gcc_assert (TREE_CODE (decl) == RESULT_DECL + && same_type_ignoring_top_level_qualifiers_p (TREE_TYPE (decl), + TREE_TYPE (to))); + replace_result_decl_data data = { decl, to, false }; + cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data); + return data.changed; +} + /* Subroutine of cxx_eval_constant_expression. Evaluate the call expression tree T in the context of OLD_CALL expression evaluation. */ @@ -2381,6 +2425,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, } else { + tree result_decl = NULL_TREE; bool cacheable = true; if (result && result != error_mark_node) /* OK */; @@ -2395,13 +2440,13 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, } else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) { - tree body, parms, res; + tree body, parms; releasing_vec ctors; /* Reuse or create a new unshared copy of this function's body. */ body = TREE_PURPOSE (copy); parms = TREE_VALUE (copy); - res = TREE_TYPE (copy); + result_decl = TREE_TYPE (copy); /* Associate the bindings with the remapped parms. */ tree bound = new_call.bindings; @@ -2428,8 +2473,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, remapped = DECL_CHAIN (remapped); } /* Add the RESULT_DECL to the values map, too. */ - gcc_assert (!DECL_BY_REFERENCE (res)); - ctx->global->values.put (res, NULL_TREE); + gcc_assert (!DECL_BY_REFERENCE (result_decl)); + ctx->global->values.put (result_decl, NULL_TREE); /* Track the callee's evaluated SAVE_EXPRs and TARGET_EXPRs so that we can forget their values after the call. */ @@ -2452,11 +2497,11 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, value by evaluating *this, but we don't bother; there's no need to put such a call in the hash table. */ result = lval ? ctx->object : ctx->ctor; - else if (VOID_TYPE_P (TREE_TYPE (res))) + else if (VOID_TYPE_P (TREE_TYPE (result_decl))) result = void_node; else { - result = *ctx->global->values.get (res); + result = *ctx->global->values.get (result_decl); if (result == NULL_TREE && !*non_constant_p) { if (!ctx->quiet) @@ -2495,7 +2540,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, bothering to do this when the map itself is only live for one constexpr evaluation? If so, maybe also clear out other vars from call, maybe in BIND_EXPR handling? */ - ctx->global->values.remove (res); + ctx->global->values.remove (result_decl); for (tree parm = parms; parm; parm = TREE_CHAIN (parm)) ctx->global->values.remove (parm); @@ -2547,6 +2592,15 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, result = error_mark_node; else if (!result) result = void_node; + + /* Adjust any references to the function's RESULT_DECL inside the result + to point to the current object under construction. */ + if (result_decl && ctx->object + && same_type_ignoring_top_level_qualifiers_p (TREE_TYPE (result_decl), + TREE_TYPE (ctx->object))) + if (replace_result_decl (&result, result_decl, ctx->object)) + cacheable = false; + if (entry) entry->result = cacheable ? result : error_mark_node; } diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7.C new file mode 100644 index 00000000000..55aef277d82 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7.C @@ -0,0 +1,21 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A { A *ap = this; }; + +constexpr A foo() +{ + return {}; +} + +constexpr A bar() +{ + return foo(); +} + +void +baz() +{ + constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" } + constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" } +} diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi8.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi8.C new file mode 100644 index 00000000000..7b7a48e694d --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi8.C @@ -0,0 +1,45 @@ +// PR c++/94034 +// { dg-do compile { target c++14 } } + +struct A { A *p = this; int n = 2; int m = p->n++; }; + +constexpr A +foo() +{ + return {}; +} + +constexpr A +bar() +{ + A a = foo(); + a.p->n = 5; + return a; +} + +static_assert(bar().n == 5, ""); + +constexpr int +baz() +{ + A b = foo(); + b.p->n = 10; + A c = foo(); + if (c.p->n != 3 || c.p->m != 2) + __builtin_abort(); + bar(); + return 0; +} + +static_assert(baz() == 0, ""); + +constexpr int +quux() +{ + const A d = foo(); + d.p->n++; // { dg-error "const object" } + return 0; +} + +static_assert(quux() == 0, ""); // { dg-error "non-constant" } +
When evaluating the initializer of 'a' in the following example struct A { A *p = this; }; constexpr A foo() { return {}; } constexpr A a = foo(); the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the 'this' should really be resolved to '&a'. It seems to me that the right approach would be to immediately resolve the PLACEHOLDER_EXPR to the correct target object during evaluation of 'foo()', so that we could use 'this' to access objects adjacent to the current object in the ultimate storage location. (I think #c2 of PR c++/94537 is an example of such usage of 'this', which currently doesn't work. But as #c1 shows we don't seem to handle this case correctly in non-constexpr initialization either.) I haven't yet been able to make a solution using the above approach work -- making sure we use the ultimate object instead of the RESULT_DECL whenever we access ctx->global->values is proving to be tricky and subtle. As a simpler stopgap solution, this patch fixes this stray RESULT_DECL issue by replacing all occurrences of the RESULT_DECL in the result of a constexpr function call with the current object under construction. This doesn't allow one to access adjacent objects using the 'this' pointer, but besides that it seems functionally equivalent to the above approach. Is this stopgap solution adequate, or should I continue working on a more correct approach? This patch passes "make check-c++", and a full bootstrap/regtest is in progress. gcc/cp/ChangeLog: PR c++/94034 * constexpr.c (replace_result_decl_data): New struct. (replace_result_decl_data_r): New function. (replace_result_decl): New function. (cxx_eval_call_expression): Rename local variable 'res' to 'result_decl' and move its declaration to an outer scope. Use replace_result_decl. gcc/testsuite/ChangeLog: PR c++/94034 * g++.dg/cpp1y/constexpr-nsdmi7.C: New test. * g++.dg/cpp1y/constexpr-nsdmi8.C: New test. --- gcc/cp/constexpr.c | 68 +++++++++++++++++-- gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7.C | 21 ++++++ gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi8.C | 45 ++++++++++++ 3 files changed, 127 insertions(+), 7 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi8.C