Re: [PATCH] c++: Fix constexpr evaluation of self-modifying CONSTRUCTORs [PR94066]

Tue, 17 Mar 2020 14:14:50 -0700 

On 3/16/20 1:39 PM, Patrick Palka wrote:

In this PR, we are performing constexpr evaluation of a CONSTRUCTOR of type
union U which looks like

   {.a=foo (&<PLACEHOLDER_EXPR union U>)}.

Since the function foo takes a reference to the CONSTRUCTOR we're building, it
could potentially modify the CONSTRUCTOR from under us.  In particular since U
is a union, the evaluation of a's initializer could change the active member
from a to another member -- something which cxx_eval_bare_aggregate doesn't
expect to happen.

Upon further investigation, it turns out this issue is not limited to
constructors of UNION_TYPE and not limited to cxx_eval_bare_aggregate either.
For example, within cxx_eval_store_expression we may be evaluating an assignment
such as (this comes from the test pr94066-2.C):

   ((union U *) this)->a = TARGET_EXPR <D.2163, foo ((union U *) this)>;
I assume this is actually an INIT_EXPR, or we would have preevaluated and not had this problem. 


where evaluation of foo could change the active member of *this, which was set
earlier in cxx_eval_store_expression to 'a'.  And if U is a RECORD_TYPE, then
evaluation of foo could add new fields to *this, thereby making stale the 'valp'
pointer to the target constructor_elt through which we're later assigning.

So in short, it seems that both cxx_eval_bare_aggregate and
cxx_eval_store_expression do not anticipate that a constructor_elt's initializer
could modify the underlying CONSTRUCTOR as a side-effect.
Oof. If this is well-formed, it's because initialization of a doesn't actually start until the return statement of foo, so we're probably wrong to create a CONSTRUCTOR to hold the value of 'a' before evaluating foo. Perhaps init_subob_ctx shouldn't preemptively create a CONSTRUCTOR, and similarly for the cxx_eval_store_expression !preeval code. 


To fix this problem, this patch makes cxx_eval_bare_aggregate and
cxx_eval_store_expression recompute the pointer to the constructor_elt through
which we're assigning, after the initializer has been evaluated.

I am worried that the use of get_or_insert_ctor_field in cxx_eval_bare_aggregate
might introduce quadratic behavior where there wasn't before.  I wonder if
there's a cheap heuristic we can use in cxx_eval_bare_aggregate to determine
whether "self-modification" took place, and in which case we could avoid calling
get_or_insert_ctor_field and do the fast thing we that we were doing before the
patch.
We could remember the (integer) index of the constructor element and verify that our sub-ctors are still at those indices before doing a new search. 


Tested on x86_64-pc-linux-gnu, but a full bootstrap is in progress.  Does this
look like the right approach?

gcc/cp/ChangeLog:

        PR c++/94066
        * constexpr.c (get_or_insert_ctor_field): Split out (while adding
        handling for VECTOR_TYPEs) from ...
        (cxx_eval_store_expression): ... here.  Record the sequence of indexes
        into INDEXES that yields the suboject we're assigning to.  After
        evaluating the initializer of the store expression, recompute valp using
        INDEXES.
        (cxx_eval_bare_aggregate): Use get_or_insert_ctor_field to recompute the
        pointer to the constructor_elt we're assigning through after evaluating
        each initializer.

gcc/testsuite/ChangeLog:

        PR c++/94066
        * g++.dg/cpp1y/pr94066.C: New test.
        * g++.dg/cpp1y/pr94066-2.C: New test.
        * g++.dg/cpp1y/pr94066-3.C: New test.
        * g++.dg/cpp1y/pr94066-4.C: New test.
        * g++.dg/cpp1y/pr94066-5.C: New test.
---
  gcc/cp/constexpr.c                     | 184 +++++++++++++++----------
  gcc/testsuite/g++.dg/cpp1y/pr94066-2.C |  21 +++
  gcc/testsuite/g++.dg/cpp1y/pr94066-3.C |  21 +++
  gcc/testsuite/g++.dg/cpp1y/pr94066-4.C |  22 +++
  gcc/testsuite/g++.dg/cpp1y/pr94066-5.C |  18 +++
  gcc/testsuite/g++.dg/cpp1y/pr94066.C   |  20 +++
  6 files changed, 210 insertions(+), 76 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
  create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066.C

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 192face9a3a..9fcf9f33457 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3144,6 +3144,88 @@ find_array_ctor_elt (tree ary, tree dindex, bool insert)
    return -1;
  }
+/* Return a pointer to the constructor_elt of CTOR which matches INDEX. If no 
+   matching constructor_elt exists, then add it to CTOR.  Emit an appropriate
+   diagnostic if CTOR constructs a UNION_TYPE and adding INDEX would change the
+   active member of the union, unless QUIET is true.   */
+
+static constructor_elt *
+get_or_insert_ctor_field (tree ctor, tree index,
+                         bool quiet, bool *non_constant_p)
+{
+  tree type = TREE_TYPE (ctor);
+  if (TREE_CODE (type) == VECTOR_TYPE && index == NULL_TREE)
+    {
+      CONSTRUCTOR_APPEND_ELT (CONSTRUCTOR_ELTS (ctor), index, NULL_TREE);
+      return &CONSTRUCTOR_ELTS (ctor)->last();
+    }
+  else if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == VECTOR_TYPE)
+    {
+      HOST_WIDE_INT i = find_array_ctor_elt (ctor, index, /*insert*/true);
+      gcc_assert (i >= 0);
+      constructor_elt *cep = CONSTRUCTOR_ELT (ctor, i);
+      gcc_assert (cep->index == NULL_TREE
+                 || TREE_CODE (cep->index) != RANGE_EXPR);
+      return cep;
+    }
+  else
+    {
+      gcc_assert (TREE_CODE (index) == FIELD_DECL);
+
+      /* We must keep the CONSTRUCTOR's ELTS in FIELD order.
+        Usually we meet initializers in that order, but it is
+        possible for base types to be placed not in program
+        order.  */
+      tree fields = TYPE_FIELDS (DECL_CONTEXT (index));
+      unsigned HOST_WIDE_INT idx;
+
+      if (TREE_CODE (type) == UNION_TYPE && CONSTRUCTOR_NELTS (ctor)
+         && CONSTRUCTOR_ELT (ctor, 0)->index != index)
+       {
+         if (cxx_dialect < cxx2a)
+           {
+             if (!quiet)
+               error ("change of the active member of a union from %qD to %qD",
+                       CONSTRUCTOR_ELT (ctor, 0)->index,
+                       index);
+             *non_constant_p = true;
+           }
+         /* Changing active member.  */
+         vec_safe_truncate (CONSTRUCTOR_ELTS (ctor), 0);
+       }
+
+      constructor_elt *cep = NULL;
+      for (idx = 0;
+          vec_safe_iterate (CONSTRUCTOR_ELTS (ctor), idx, &cep);
+          idx++, fields = DECL_CHAIN (fields))
+       {
+         if (index == cep->index)
+           goto found;
+
+         /* The field we're initializing must be on the field
+            list.  Look to see if it is present before the
+            field the current ELT initializes.  */
+         for (; fields != cep->index; fields = DECL_CHAIN (fields))
+           if (index == fields)
+             goto insert;
+       }
+
+      /* We fell off the end of the CONSTRUCTOR, so insert a new
+        entry at the end.  */
+    insert:
+      {
+       constructor_elt ce = { index, NULL_TREE };
+
+       vec_safe_insert (CONSTRUCTOR_ELTS (ctor), idx, ce);
+       cep = CONSTRUCTOR_ELT (ctor, idx);
+      }
+    found:;
+
+      return cep;
+    }
+}
+
+
  /* Under the control of CTX, issue a detailed diagnostic for
     an out-of-bounds subscript INDEX into the expression ARRAY.  */
@@ -3784,14 +3866,12 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t, 
        }
        else
        {
-         if (new_ctx.ctor != ctx->ctor)
-           {
-             /* We appended this element above; update the value.  */
-             gcc_assert ((*p)->last().index == index);
-             (*p)->last().value = elt;
-           }
-         else
-           CONSTRUCTOR_APPEND_ELT (*p, index, elt);
+         iloc_sentinel ils (cp_expr_location (t));
+         constructor_elt *cep
+           = get_or_insert_ctor_field (ctx->ctor, index,
+                                       ctx->quiet, non_constant_p);
+         cep->value = elt;
+
          /* Adding or replacing an element might change the ctor's flags.  */
          TREE_CONSTANT (ctx->ctor) = constant_p;
          TREE_SIDE_EFFECTS (ctx->ctor) = side_effects_p;
@@ -4566,7 +4646,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree 
t,
    type = TREE_TYPE (object);
    bool no_zero_init = true;
- releasing_vec ctors; 
+  releasing_vec ctors, indexes;
    while (!refs->is_empty ())
      {
        if (*valp == NULL_TREE)
@@ -4607,77 +4687,21 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, 
tree t,
         subobjects will also be zero-initialized.  */
        no_zero_init = CONSTRUCTOR_NO_CLEARING (*valp);
- vec_safe_push (ctors, *valp); 
-
        enum tree_code code = TREE_CODE (type);
        type = refs->pop();
        tree index = refs->pop();
- constructor_elt *cep = NULL; 
-      if (code == ARRAY_TYPE)
-       {
-         HOST_WIDE_INT i
-           = find_array_ctor_elt (*valp, index, /*insert*/true);
-         gcc_assert (i >= 0);
-         cep = CONSTRUCTOR_ELT (*valp, i);
-         gcc_assert (cep->index == NULL_TREE
-                     || TREE_CODE (cep->index) != RANGE_EXPR);
-       }
-      else
-       {
-         gcc_assert (TREE_CODE (index) == FIELD_DECL);
-
-         /* We must keep the CONSTRUCTOR's ELTS in FIELD order.
-            Usually we meet initializers in that order, but it is
-            possible for base types to be placed not in program
-            order.  */
-         tree fields = TYPE_FIELDS (DECL_CONTEXT (index));
-         unsigned HOST_WIDE_INT idx;
-
-         if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
-             && CONSTRUCTOR_ELT (*valp, 0)->index != index)
-           {
-             if (cxx_dialect < cxx2a)
-               {
-                 if (!ctx->quiet)
-                   error_at (cp_expr_loc_or_input_loc (t),
-                             "change of the active member of a union "
-                             "from %qD to %qD",
-                             CONSTRUCTOR_ELT (*valp, 0)->index,
-                             index);
-                 *non_constant_p = true;
-               }
-             /* Changing active member.  */
-             vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
-             no_zero_init = true;
-           }
+      vec_safe_push (ctors, *valp);
+      vec_safe_push (indexes, index);
- for (idx = 0; 
-              vec_safe_iterate (CONSTRUCTOR_ELTS (*valp), idx, &cep);
-              idx++, fields = DECL_CHAIN (fields))
-           {
-             if (index == cep->index)
-               goto found;
-
-             /* The field we're initializing must be on the field
-                list.  Look to see if it is present before the
-                field the current ELT initializes.  */
-             for (; fields != cep->index; fields = DECL_CHAIN (fields))
-               if (index == fields)
-                 goto insert;
-           }
+      if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
+         && CONSTRUCTOR_ELT (*valp, 0)->index != index)
+       no_zero_init = true;
- /* We fell off the end of the CONSTRUCTOR, so insert a new 
-            entry at the end.  */
-       insert:
-         {
-           constructor_elt ce = { index, NULL_TREE };
+      iloc_sentinel ils (cp_expr_location (t));
+      constructor_elt *cep
+       = get_or_insert_ctor_field (*valp, index, ctx->quiet, non_constant_p);
- vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce); 
-           cep = CONSTRUCTOR_ELT (*valp, idx);
-         }
-       found:;
-       }
        valp = &cep->value;
      }
@@ -4758,9 +4782,17 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, 
          init = tinit;
        init = cxx_eval_constant_expression (&new_ctx, init, false,
                                           non_constant_p, overflow_p);
-      if (ctors->is_empty())
-       /* The hash table might have moved since the get earlier.  */
-       valp = ctx->global->values.get (object);
+      /* The hash table might have moved since the get earlier.  */
+      valp = ctx->global->values.get (object);
+      /* And the underlying CONSTRUCTORs may have been modified by the
+        initializer.  */
+      for (unsigned i = 0; i < vec_safe_length (indexes); i++)
+       {
+         constructor_elt *cep
+           = get_or_insert_ctor_field (*valp, indexes[i],
+                                       /*quiet=*/true, non_constant_p);
+         valp = &cep->value;
+       }
      }
/* Don't share a CONSTRUCTOR that might be changed later. */ 
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C 
b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
new file mode 100644
index 00000000000..ec8210a9d73
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
@@ -0,0 +1,21 @@
+// PR c++/94066
+// { dg-do compile { target c++14 } }
+
+struct A { long x; };
+
+union U;
+constexpr A foo(U *up);
+
+union U {
+  U() = default;
+  A a = foo(this); int y;
+};
+
+constexpr A foo(U *up) {
+  up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
+  return {42};
+}
+
+extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target 
c++17_down } }
+
+static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target 
c++17_down } }
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C 
b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
new file mode 100644
index 00000000000..b68c975d569
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
@@ -0,0 +1,21 @@
+// PR c++/94066
+// { dg-do compile { target c++14 } }
+
+struct A { long x; };
+
+struct U;
+constexpr A foo(U *up);
+
+struct U {
+  A a = foo(this); int y;
+};
+
+constexpr A foo(U *up) {
+  up->y = 11;
+  return {42};
+}
+
+extern constexpr U u = {};
+
+static_assert(u.y == 11, "");
+static_assert(u.a.x == 42, "");
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C 
b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
new file mode 100644
index 00000000000..0362c939faf
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
@@ -0,0 +1,22 @@
+// PR c++/94066
+// { dg-do compile { target c++14 } }
+
+struct A { long x; };
+
+struct U;
+constexpr A foo(U *up);
+
+struct U {
+  U() = default;
+  int y; A a = foo(this);
+};
+
+constexpr A foo(U *up) {
+  up->y = 11;
+  return {42};
+}
+
+extern constexpr U u = {};
+
+static_assert(u.y == 11, "");
+static_assert(u.a.x == 42, "");
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C 
b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
new file mode 100644
index 00000000000..63069f3ea5f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
@@ -0,0 +1,18 @@
+// PR c++/94066
+// { dg-do compile { target c++14 } }
+
+union U;
+constexpr int foo(U *up);
+
+union U {
+  int a = foo(this); int y;
+};
+
+constexpr int foo(U *up) {
+  up->y = 11;
+  return 42;
+}
+
+extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target 
c++17_down } }
+
+static_assert(u.a == 42, ""); // { dg-error "non-constant" "" { target 
c++17_down } }
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C 
b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
new file mode 100644
index 00000000000..11912199406
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
@@ -0,0 +1,20 @@
+// PR c++/94066
+// { dg-do compile { target c++14 } }
+
+struct A { long x; };
+
+union U;
+constexpr A foo(U *up);
+
+union U {
+  A a = foo(this); int y;
+};
+
+constexpr A foo(U *up) {
+  up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
+  return {42};
+}
+
+extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target 
c++17_down } }
+
+static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target 
c++17_down } }

Reply via email to