On Mon, 27 Apr 2020, Jason Merrill wrote:
> On 4/27/20 10:45 AM, Patrick Palka wrote:
> > 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.
>
> Well, in that case it's not a delegating constructor, it's normal base
> construction. But it's certainly true that we don't want to treat a base
> subobject as a whole new object.
Ah okay, noted.
>
> > 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. */
>
> ...so you'll want to update this comment.
Done.
>
> What happens if we get to 'base' by COMPONENT_REF rather than NOP_EXPR?
It looks like in this case the TREE_TYPE of the evaluated COMPONENT_REF
is non-const regardless of the constness of the parent object, because
in cxx_eval_component_reference the built COMPONENT_REF inherits the
constness of the original tree
((struct foo *) this)->D.2414;
that is passed as the 'this' argument to the base class constructor (and
is evaluated by cxx_bind_parameters_in_call).
So because its TREE_TYPE is non-const, we don't consider setting
TREE_READONLY on the CONSTRUCTOR of the parent object at the end of
cxx_eval_call_expression, and likewise in cxx_eval_store_expression. So
when constructing a base subobject through a COMPONENT_REF it luckily
seems we don't have this constness problem with or without this patch.
Here's the updated patch, with the comment updated and a new test added.
-- >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.
* g++.dg/cpp1y/constexpr-tracking-const25.C: New test.
---
gcc/cp/constexpr.c | 28 +++++++-
.../g++.dg/cpp1y/constexpr-tracking-const23.C | 21 ++++++
.../g++.dg/cpp1y/constexpr-tracking-const24.C | 26 ++++++++
.../g++.dg/cpp1y/constexpr-tracking-const25.C | 66 +++++++++++++++++++
4 files changed, 140 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
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const25.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 6b3e514398b..637cb746576 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -2371,6 +2371,21 @@ 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, or accessing a base subobject through a
+ NOP_EXPR as part of a call to a base constructor, so
+ there is no new (sub)object. */
+ new_obj = NULL_TREE;
+ }
}
tree result = NULL_TREE;
@@ -4950,7 +4965,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, "");
+}
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const25.C
b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const25.C
new file mode 100644
index 00000000000..662a6f93642
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const25.C
@@ -0,0 +1,66 @@
+// PR c++/94772
+// { dg-do compile { target c++14 } }
+
+template<int>
+struct base
+{
+ int y{};
+
+ base() = default;
+
+ constexpr base(int a) : base{}
+ { y = a; }
+};
+
+struct foo : base<1>, base<2>
+{
+ int x{};
+
+ constexpr foo() : base<2>{}
+ {
+ x = x;
+ ++base<1>::y;
+ ++base<2>::y;
+ }
+
+ constexpr foo(int a) : base<2>{a}
+ {
+ x = -base<2>::y;
+ ++base<1>::y;
+ ++base<2>::y;
+ }
+
+ constexpr foo(int a, int b) : foo{a}
+ {
+ x += a + b;
+ ++base<1>::y;
+ ++base<2>::y;
+ }
+
+ constexpr foo(int a, int b, int c) : base<1>{a}
+ {
+ x += a + b + c;
+ ++base<1>::y;
+ ++base<2>::y;
+ }
+};
+
+#define SA(X) static_assert(X, #X)
+
+int main()
+{
+ constexpr foo bar1{1, 2};
+ SA( bar1.x == 2 );
+ SA( bar1.base<1>::y == 2 );
+ SA( bar1.base<2>::y == 3 );
+
+ constexpr foo bar2{1, 2, 3};
+ SA( bar2.x == 6 );
+ SA( bar2.base<1>::y == 2 );
+ SA( bar2.base<2>::y == 1 );
+
+ constexpr foo bar3{};
+ SA( bar3.x == 0 );
+ SA( bar3.base<1>::y == 1 );
+ SA( bar3.base<2>::y == 1 );
+}
--
2.26.2.266.ge870325ee8
