c++: Refrain from using replace_placeholders in constexpr evaluation [PR94205]

Message ID 20200402234014.2942365-1-ppalka@redhat.com
State New
Headers show
Series
  • c++: Refrain from using replace_placeholders in constexpr evaluation [PR94205]
Related show

Commit Message

Aaron Sawdey via Gcc-patches April 2, 2020, 11:40 p.m.
This removes the use of replace_placeholders in cxx_eval_constant_expression
(which causes the new test lambda-this6.C to ICE due to replace_placeholders
mutating the shared TARGET_EXPR_INITIAL tree which then trips up the
gimplifier).

In its place, this patch adds a 'parent' field to constexpr_ctx which is used to
store a pointer to an outer constexpr_ctx that points to another object under
construction.  With this new field, we can beef up lookup_placeholder to resolve
PLACEHOLDER_EXPRs that refer to former objects under construction, which fixes
PR94205 without needing to do replace_placeholders.  Also we can now respect the
CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag which fixes the constexpr analogue of
PR79937.

Does this look OK to commit after testing?

gcc/cp/ChangeLog:

	PR c++/94205
	PR c++/79937
	* constexpr.c (struct constexpr_ctx): New field 'parent'.
	(cxx_eval_bare_aggregate): Propagate CONSTRUCTOR_PLACEHOLDER_BOUNDARY
	flag from the original constructor to the reduced constructor.
	(lookup_placeholder): Prefer to return the outermost matching object
	by recursively calling lookup_placeholder on the 'parent' context,
	but don't cross CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructors.
	(cxx_eval_constant_expression): Link the 'ctx' context to the 'new_ctx'
	context via 'new_ctx.parent' when being expanded without an explicit
	target.  Don't call replace_placeholders.
	(cxx_eval_outermost_constant_expr): Initialize 'ctx.parent' to NULL.

gcc/testsuite/ChangeLog:

	PR c++/94205
	PR c++/79937
	* g++.dg/cpp1y/pr79937-5.C: New test.
	* g++.dg/cpp1z/lambda-this6.C: New test.
---
 gcc/cp/constexpr.c                        | 25 +++++++++-----
 gcc/testsuite/g++.dg/cpp1y/pr79937-5.C    | 42 +++++++++++++++++++++++
 gcc/testsuite/g++.dg/cpp1z/lambda-this6.C | 12 +++++++
 3 files changed, 71 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr79937-5.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1z/lambda-this6.C

-- 
2.26.0.106.g9fadedd637

Comments

Aaron Sawdey via Gcc-patches April 3, 2020, 7:20 p.m. | #1
On 4/2/20 7:40 PM, Patrick Palka wrote:
> +  /* Prefer the outermost matching object, but don't cross

> +     CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructors.  */

> +  if (ctx->ctor && !CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ctx->ctor))

> +    if (tree parent_ob = lookup_placeholder (ctx->parent, lval, type))

> +      return parent_ob;


Instead of recursing here, would it work to replace the loop at the end 
with tail recursion?

Jason
Aaron Sawdey via Gcc-patches April 3, 2020, 8:07 p.m. | #2
On Fri, 3 Apr 2020, Jason Merrill wrote:

> On 4/2/20 7:40 PM, Patrick Palka wrote:

> > +  /* Prefer the outermost matching object, but don't cross

> > +     CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructors.  */

> > +  if (ctx->ctor && !CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ctx->ctor))

> > +    if (tree parent_ob = lookup_placeholder (ctx->parent, lval, type))

> > +      return parent_ob;

> 

> Instead of recursing here, would it work to replace the loop at the end with

> tail recursion?


Do you mean something like the following?

  static tree
  lookup_placeholder (const constexpr_ctx *ctx, bool lval, tree type)
  {
    if (!ctx)
      return NULL_TREE;

    tree ob = ctx->object;
    if (ob && same_type_ignoring_top_level_qualifiers_p (TREE_TYPE (ob), type))
      return obj;
    else if (ctx->ctor && !CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ctx->ctor))
      return lookup_placeholder (ctx->parent, lval, type);
    else
      return NULL_TREE;
  }

I think we would need to set ctx->parent in more places in order for this to
work, so that each node in the path to the innermost suboject is represented by
a constexpr_ctx.

Besides that, using tail recursion would, IIUC, mean that we would return the
innermost matching object, but I think we want to return the outermost matching
object (without crossing CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructors) when
there might be more than one matching object.  For if a PLACEHOLDER_EXPR really
was meant to refer to an object other than this outermost object, then
presumably the CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag would have already been
set on some suboject constructor and so we wouldn't have recursed to
this outermost object in the first place, if I'm understanding the flag
correctly.
Aaron Sawdey via Gcc-patches April 4, 2020, 4:40 a.m. | #3
On 4/3/20 4:07 PM, Patrick Palka wrote:
> On Fri, 3 Apr 2020, Jason Merrill wrote:

> 

>> On 4/2/20 7:40 PM, Patrick Palka wrote:

>>> +  /* Prefer the outermost matching object, but don't cross

>>> +     CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructors.  */

>>> +  if (ctx->ctor && !CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ctx->ctor))

>>> +    if (tree parent_ob = lookup_placeholder (ctx->parent, lval, type))

>>> +      return parent_ob;

>>

>> Instead of recursing here, would it work to replace the loop at the end with

>> tail recursion?

> 

> Do you mean something like the following?

> 

>    static tree

>    lookup_placeholder (const constexpr_ctx *ctx, bool lval, tree type)

>    {

>      if (!ctx)

>        return NULL_TREE;

> 

>      tree ob = ctx->object;

>      if (ob && same_type_ignoring_top_level_qualifiers_p (TREE_TYPE (ob), type))

>        return obj;

>      else if (ctx->ctor && !CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ctx->ctor))

>        return lookup_placeholder (ctx->parent, lval, type);

>      else

>        return NULL_TREE;

>    }

> 

> I think we would need to set ctx->parent in more places in order for this to

> work, so that each node in the path to the innermost suboject is represented by

> a constexpr_ctx.


Hmm, true.  But given how it's currently set, the comment

> +  /* The constexpr expansion context inside which this one is nested.


seems overly general; perhaps "the aggregate initialization context..."?

OK with that change.

> Besides that, using tail recursion would, IIUC, mean that we would return the

> innermost matching object, but I think we want to return the outermost matching

> object (without crossing CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructors) when

> there might be more than one matching object.  For if a PLACEHOLDER_EXPR really

> was meant to refer to an object other than this outermost object, then

> presumably the CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag would have already been

> set on some suboject constructor and so we wouldn't have recursed to

> this outermost object in the first place, if I'm understanding the flag

> correctly.


Good point.

Jason
Aaron Sawdey via Gcc-patches April 4, 2020, 4:19 p.m. | #4
On Sat, 4 Apr 2020, Jason Merrill wrote:

> On 4/3/20 4:07 PM, Patrick Palka wrote:

> > On Fri, 3 Apr 2020, Jason Merrill wrote:

> > 

> > > On 4/2/20 7:40 PM, Patrick Palka wrote:

> > > > +  /* Prefer the outermost matching object, but don't cross

> > > > +     CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructors.  */

> > > > +  if (ctx->ctor && !CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ctx->ctor))

> > > > +    if (tree parent_ob = lookup_placeholder (ctx->parent, lval, type))

> > > > +      return parent_ob;

> > > 

> > > Instead of recursing here, would it work to replace the loop at the end

> > > with

> > > tail recursion?

> > 

> > Do you mean something like the following?

> > 

> >    static tree

> >    lookup_placeholder (const constexpr_ctx *ctx, bool lval, tree type)

> >    {

> >      if (!ctx)

> >        return NULL_TREE;

> > 

> >      tree ob = ctx->object;

> >      if (ob && same_type_ignoring_top_level_qualifiers_p (TREE_TYPE (ob),

> > type))

> >        return obj;

> >      else if (ctx->ctor && !CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ctx->ctor))

> >        return lookup_placeholder (ctx->parent, lval, type);

> >      else

> >        return NULL_TREE;

> >    }

> > 

> > I think we would need to set ctx->parent in more places in order for this to

> > work, so that each node in the path to the innermost suboject is represented

> > by

> > a constexpr_ctx.

> 

> Hmm, true.  But given how it's currently set, the comment

> 

> > +  /* The constexpr expansion context inside which this one is nested.

> 

> seems overly general; perhaps "the aggregate initialization context..."?


Oops, fixed.  A preliminary version of the patch set new_ctx->parent to
point to the old constexpr_ctx whenever we would create a
sub-constexpr_ctx, consistent with the comment, but later I ended up
just setting it in that one place since it was sufficient and seemed
easier to reason about.

> 

> OK with that change.


Thanks, patch committed just now with the comment adjusted.

> 

> > Besides that, using tail recursion would, IIUC, mean that we would return

> > the

> > innermost matching object, but I think we want to return the outermost

> > matching

> > object (without crossing CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructors) when

> > there might be more than one matching object.  For if a PLACEHOLDER_EXPR

> > really

> > was meant to refer to an object other than this outermost object, then

> > presumably the CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag would have already been

> > set on some suboject constructor and so we wouldn't have recursed to

> > this outermost object in the first place, if I'm understanding the flag

> > correctly.

> 

> Good point.

> 

> Jason

> 

>

Patch

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 613a769e642..c72305715e4 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -1076,6 +1076,9 @@  struct constexpr_ctx {
   tree object;
   /* If inside SWITCH_EXPR.  */
   constexpr_switch_state *css_state;
+  /* The constexpr expansion context inside which this one is nested.  This is
+     used by lookup_placeholder to resolve PLACEHOLDER_EXPRs.  */
+  const constexpr_ctx *parent;
 
   /* Whether we should error on a non-constant expression or fail quietly.
      This flag needs to be here, but some of the others could move to global
@@ -3843,6 +3846,9 @@  cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
   vec<constructor_elt, va_gc> **p = &CONSTRUCTOR_ELTS (ctx->ctor);
   vec_alloc (*p, vec_safe_length (v));
 
+  if (CONSTRUCTOR_PLACEHOLDER_BOUNDARY (t))
+    CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ctx->ctor) = 1;
+
   unsigned i;
   tree index, value;
   bool constant_p = true;
@@ -5305,6 +5311,12 @@  lookup_placeholder (const constexpr_ctx *ctx, bool lval, tree type)
   if (!ctx)
     return NULL_TREE;
 
+  /* Prefer the outermost matching object, but don't cross
+     CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructors.  */
+  if (ctx->ctor && !CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ctx->ctor))
+    if (tree parent_ob = lookup_placeholder (ctx->parent, lval, type))
+      return parent_ob;
+
   /* We could use ctx->object unconditionally, but using ctx->ctor when we
      can is a minor optimization.  */
   if (!lval && ctx->ctor && same_type_p (TREE_TYPE (ctx->ctor), type))
@@ -5608,19 +5620,16 @@  cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
 	    r = *p;
 	    break;
 	  }
-	tree init = TARGET_EXPR_INITIAL (t);
 	if ((AGGREGATE_TYPE_P (type) || VECTOR_TYPE_P (type)))
 	  {
-	    if (ctx->object)
-	      /* If the initializer contains any PLACEHOLDER_EXPR, we need to
-		 resolve them before we create a new CONSTRUCTOR for the
-		 temporary.  */
-	      init = replace_placeholders (init, ctx->object);
-
 	    /* We're being expanded without an explicit target, so start
 	       initializing a new object; expansion with an explicit target
 	       strips the TARGET_EXPR before we get here.  */
 	    new_ctx = *ctx;
+	    /* Link CTX to NEW_CTX so that lookup_placeholder can resolve
+	       any PLACEHOLDER_EXPR within the initializer that refers to the
+	       former object under construction.  */
+	    new_ctx.parent = ctx;
 	    new_ctx.ctor = build_constructor (type, NULL);
 	    CONSTRUCTOR_NO_CLEARING (new_ctx.ctor) = true;
 	    new_ctx.object = slot;
@@ -6474,7 +6483,7 @@  cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant,
   bool overflow_p = false;
 
   constexpr_global_ctx global_ctx;
-  constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL,
+  constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL,
 			allow_non_constant, strict,
 			manifestly_const_eval || !allow_non_constant,
 			uid_sensitive };
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr79937-5.C b/gcc/testsuite/g++.dg/cpp1y/pr79937-5.C
new file mode 100644
index 00000000000..cbf0115d5bb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr79937-5.C
@@ -0,0 +1,42 @@ 
+// PR c++/79937
+// This is a constexpr adaptation of pr79937-3.C and pr79937-4.C.
+// { dg-do compile { target c++14 } }
+
+struct X {
+  unsigned i;
+  unsigned n = i;
+};
+
+constexpr X bar(X x) {
+  return x;
+}
+
+struct Y
+{
+  static constexpr Y bar(Y y) { return y; }
+  unsigned i;
+  unsigned n = bar(Y{2,i}).n;
+};
+
+constexpr X x { 1, bar(X{2}).n };
+static_assert(x.n == 2, "");
+
+constexpr Y y { 1 };
+static_assert(y.n == 1, "");
+
+struct Z {
+  unsigned i;
+  unsigned n = i;
+  unsigned m = i;
+};
+
+constexpr Z
+baz (Z z)
+{
+  if (z.i != 1 || z.n != 2 || z.m != 1)
+    __builtin_abort ();
+  return z;
+}
+
+constexpr Z z = baz (Z {1, Z {2}.n});
+static_assert(z.i == 1 && z.n == 2 && z.m == 1, "");
diff --git a/gcc/testsuite/g++.dg/cpp1z/lambda-this6.C b/gcc/testsuite/g++.dg/cpp1z/lambda-this6.C
new file mode 100644
index 00000000000..76f067d374d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/lambda-this6.C
@@ -0,0 +1,12 @@ 
+// { dg-do compile { target c++17 } }
+
+struct S
+{
+  int a = [this] { return 6; } ();
+};
+
+S
+foo()
+{
+  return {};
+}