On Mon, 27 Apr 2020, Patrick Palka wrote: > On Mon, 27 Apr 2020, Jason Merrill wrote: > > > On 4/26/20 6:48 PM, Patrick Palka wrote: > > > In the testcase below, the call to the target constructor foo{} from foo's > > > delegating constructor is encoded as the INIT_EXPR > > > > > > *(struct foo *) this = AGGR_INIT_EXPR <4, __ct_comp, D.2140, ...>; > > > > > > During initialization of the variable 'bar', we prematurely set > > > TREE_READONLY on > > > bar's CONSTRUCTOR in two places before the outer delegating constructor > > > has > > > returned: first, at the end of cxx_eval_call_expression after evaluating > > > the > > > RHS > > > of the above INIT_EXPR, and second, at the end of > > > cxx_eval_store_expression > > > after having finished evaluating the above INIT_EXPR. This then prevents > > > the > > > rest of the outer delegating constructor from mutating 'bar'. > > > > > > This (hopefully minimally risky) patch makes cxx_eval_call_expression > > > refrain > > > from setting TREE_READONLY when evaluating the target constructor of a > > > delegating constructor. It also makes cxx_eval_store_expression refrain > > > from > > > setting TREE_READONLY when the object being initialized is "*this', on the > > > basis > > > that it should be the responsibility of the routine that set 'this' in the > > > first > > > place to set the object's TREE_READONLY appropriately. > > > > > > Passes 'make check-c++', does this look OK to commit after full > > > bootstrap/regtest? > > > > > > gcc/cp/ChangeLog: > > > > > > PR c++/94772 > > > * constexpr.c (cxx_eval_call_expression): Don't set new_obj if we're > > > evaluating the target constructor of a delegating constructor. > > > (cxx_eval_store_expression): Don't set TREE_READONLY if the LHS of the > > > INIT_EXPR is '*this'. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR c++/94772 > > > * g++.dg/cpp1y/constexpr-tracking-const23.C: New test. > > > --- > > > gcc/cp/constexpr.c | 29 +++++++++++++++---- > > > .../g++.dg/cpp1y/constexpr-tracking-const23.C | 21 ++++++++++++++ > > > 2 files changed, 45 insertions(+), 5 deletions(-) > > > create mode 100644 > > > gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const23.C > > > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > > index 6b3e514398b..a9ddd861195 100644 > > > --- a/gcc/cp/constexpr.c > > > +++ b/gcc/cp/constexpr.c > > > @@ -2367,10 +2367,20 @@ cxx_eval_call_expression (const constexpr_ctx > > > *ctx, > > > tree t, > > > /* In a constructor, it should be the first `this' argument. > > > At this point it has already been evaluated in the call > > > to cxx_bind_parameters_in_call. */ > > > - new_obj = TREE_VEC_ELT (new_call.bindings, 0); > > > - STRIP_NOPS (new_obj); > > > - if (TREE_CODE (new_obj) == ADDR_EXPR) > > > - new_obj = TREE_OPERAND (new_obj, 0); > > > + > > > + if (ctx->call && ctx->call->fundef > > > + && DECL_CONSTRUCTOR_P (ctx->call->fundef->decl) > > > + && (TREE_VEC_ELT (ctx->call->bindings, 0) > > > + == TREE_VEC_ELT (new_call.bindings, 0))) > > > + /* We're calling the target constructor of a delegating constructor, > > > so > > > + there is no new object. */; > > > + else > > > + { > > > + new_obj = TREE_VEC_ELT (new_call.bindings, 0); > > > + STRIP_NOPS (new_obj); > > > + if (TREE_CODE (new_obj) == ADDR_EXPR) > > > + new_obj = TREE_OPERAND (new_obj, 0); > > > + } > > > } > > > tree result = NULL_TREE; > > > @@ -4950,7 +4960,16 @@ cxx_eval_store_expression (const constexpr_ctx > > > *ctx, > > > tree t, > > > if (TREE_CODE (t) == INIT_EXPR > > > && TREE_CODE (*valp) == CONSTRUCTOR > > > && TYPE_READONLY (type)) > > > - TREE_READONLY (*valp) = true; > > > + { > > > + if (INDIRECT_REF_P (target) > > > + && (is_this_parameter > > > + (tree_strip_nop_conversions (TREE_OPERAND (target, 0))))) > > > + /* We've just initialized '*this' (perhaps via the target constructor > > > of > > > + a delegating constructor). Leave it up to the caller that set > > > 'this' > > > + to set TREE_READONLY appropriately. */; > > > > Let's checking_assert that target and *this are > > same_type_ignoring_top_level_qualifiers_p. > > Like this? Bootstrap and regtest in progress. > > -- >8 -- > > gcc/cp/ChangeLog: > > PR c++/94772 > * constexpr.c (cxx_eval_call_expression): Don't set new_obj if we're > evaluating the target constructor of a delegating constructor. > (cxx_eval_store_expression): Don't set TREE_READONLY if the LHS of the > INIT_EXPR is '*this'. > > gcc/testsuite/ChangeLog: > > PR c++/94772 > * g++.dg/cpp1y/constexpr-tracking-const23.C: New test. > --- > gcc/cp/constexpr.c | 31 ++++++++++++++++--- > .../g++.dg/cpp1y/constexpr-tracking-const23.C | 21 +++++++++++++ > 2 files changed, 47 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const23.C > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > index 6b3e514398b..c7923897e23 100644 > --- a/gcc/cp/constexpr.c > +++ b/gcc/cp/constexpr.c > @@ -2367,10 +2367,20 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, > tree t, > /* In a constructor, it should be the first `this' argument. > At this point it has already been evaluated in the call > to cxx_bind_parameters_in_call. */ > - new_obj = TREE_VEC_ELT (new_call.bindings, 0); > - STRIP_NOPS (new_obj); > - if (TREE_CODE (new_obj) == ADDR_EXPR) > - new_obj = TREE_OPERAND (new_obj, 0); > + > + if (ctx->call && ctx->call->fundef > + && DECL_CONSTRUCTOR_P (ctx->call->fundef->decl) > + && (TREE_VEC_ELT (ctx->call->bindings, 0) > + == TREE_VEC_ELT (new_call.bindings, 0))) > + /* We're calling the target constructor of a delegating constructor, so > + there is no new object. */;
Further experimentation revealed that testing the 'this' arguments for pointer equality here is too strict because the target constructor could belong to a base class, in which case its 'this' argument would be (base *)&bar instead of (foo *)&bar, as in the new testcase below. Fixed by comparing the objects pointed to by the 'this' arguments more directly. Bootstrap and regtest is in progress.. -- >8 -- gcc/cp/ChangeLog: PR c++/94772 * constexpr.c (cxx_eval_call_expression): Don't set new_obj if we're evaluating the target constructor of a delegating constructor. (cxx_eval_store_expression): Don't set TREE_READONLY if the LHS of the INIT_EXPR is '*this'. gcc/testsuite/ChangeLog: PR c++/94772 * g++.dg/cpp1y/constexpr-tracking-const23.C: New test. * g++.dg/cpp1y/constexpr-tracking-const24.C: New test. --- gcc/cp/constexpr.c | 26 ++++++++++++++++++- .../g++.dg/cpp1y/constexpr-tracking-const23.C | 21 +++++++++++++++ .../g++.dg/cpp1y/constexpr-tracking-const24.C | 26 +++++++++++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const23.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const24.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 6b3e514398b..5d9b10c63d4 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -2371,6 +2371,19 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, STRIP_NOPS (new_obj); if (TREE_CODE (new_obj) == ADDR_EXPR) new_obj = TREE_OPERAND (new_obj, 0); + + if (ctx->call && ctx->call->fundef + && DECL_CONSTRUCTOR_P (ctx->call->fundef->decl)) + { + tree cur_obj = TREE_VEC_ELT (ctx->call->bindings, 0); + STRIP_NOPS (cur_obj); + if (TREE_CODE (cur_obj) == ADDR_EXPR) + cur_obj = TREE_OPERAND (cur_obj, 0); + if (new_obj == cur_obj) + /* We're calling the target constructor of a delegating constructor, + so there is no new object. */ + new_obj = NULL_TREE; + } } tree result = NULL_TREE; @@ -4950,7 +4963,18 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, if (TREE_CODE (t) == INIT_EXPR && TREE_CODE (*valp) == CONSTRUCTOR && TYPE_READONLY (type)) - TREE_READONLY (*valp) = true; + { + if (INDIRECT_REF_P (target) + && (is_this_parameter + (tree_strip_nop_conversions (TREE_OPERAND (target, 0))))) + /* We've just initialized '*this' (perhaps via the target constructor of + a delegating constructor). Leave it up to the caller that set 'this' + to set TREE_READONLY appropriately. */ + gcc_checking_assert (same_type_ignoring_top_level_qualifiers_p + (TREE_TYPE (target), type)); + else + TREE_READONLY (*valp) = true; + } /* Update TREE_CONSTANT and TREE_SIDE_EFFECTS on enclosing CONSTRUCTORs, if any. */ diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const23.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const23.C new file mode 100644 index 00000000000..c6643c78a6f --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const23.C @@ -0,0 +1,21 @@ +// PR c++/94772 +// { dg-do compile { target c++14 } } + +struct foo +{ + int x{}; + + constexpr foo() noexcept = default; + + constexpr foo(int a) : foo{} + { x = -a; } + + constexpr foo(int a, int b) : foo{a} + { x += a + b; } +}; + +int main() +{ + constexpr foo bar{1, 2}; + static_assert(bar.x == 2, ""); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const24.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const24.C new file mode 100644 index 00000000000..2c923f69cf4 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const24.C @@ -0,0 +1,26 @@ +// PR c++/94772 +// { dg-do compile { target c++14 } } + +struct base +{ + base() = default; + + constexpr base(int) : base{} { } +}; + +struct foo : base +{ + int x{}; + + constexpr foo(int a) : base{a} + { x = -a; } + + constexpr foo(int a, int b) : foo{a} + { x += a + b; } +}; + +int main() +{ + constexpr foo bar{1, 2}; + static_assert(bar.x == 2, ""); +} -- 2.26.2.266.ge870325ee8