On 4/27/20 3:33 PM, Patrick Palka wrote:
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.

OK, thanks.

-- >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 );
+}


Reply via email to