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

Reply via email to