[C++] c++/61636 generic lambdas and this capture

Message ID 65f1a240-adf2-4b04-5839-3f378de40f1d@acm.org
State New
Headers show

Commit Message

Nathan Sidwell Jan. 13, 2017, 1:33 p.m.
On 12/21/2016 03:27 PM, Nathan Sidwell wrote:
> This patch addresses bug 61636, which is an ICE during generic lambda

> instantiation due to unexpected this capture.


Here's an updated patch.
Firstly I added the worker function to lambda.c, with a more descriptive 
name.  Secondly, this implements a check that the overload set contains 
at least one non-static member fn.  That's what Clang does, and 
discussion on the std list seems to be heading that way as the right 
approach.

ok?

nathan

-- 
Nathan Sidwell

Comments

Jason Merrill Jan. 17, 2017, 4:57 p.m. | #1
On 01/13/2017 08:33 AM, Nathan Sidwell wrote:
> 	* lambda.c (resolvable_dummy): New, broken out of ...


Maybe resolvable_dummy_lambda, since that's what it returns?

OK with that change.

Jason

Patch hide | download patch | download mbox

2016-12-21  Nathan Sidwell  <nathan@acm.org>

	PR c++/61636
	* cp-tree.h (maybe_generic_this_capture): Declare.
	* lambda.c (resolvable_dummy): New, broken out of ...
	(maybe_resolve_dummy): ... here.  Call it.
	(maybe_generic_this_capture): New.
	* parser.c (cp_parser_postfix_expression): Speculatively capture
	this in generic lambda in unresolved member function call.
	* pt.c (tsubst_copy_and_build): Force hard error from failed
	member function lookup in generic lambda.

	PR c++/61636
	* g++.dg/cpp1y/pr61636-1.C: New.
	* g++.dg/cpp1y/pr61636-2.C: New.
	* g++.dg/cpp1y/pr61636-3.C: New.

Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 244382)
+++ cp/cp-tree.h	(working copy)
@@ -6551,6 +6551,7 @@  extern bool is_capture_proxy			(tree);
 extern bool is_normal_capture_proxy             (tree);
 extern void register_capture_members		(tree);
 extern tree lambda_expr_this_capture            (tree, bool);
+extern void maybe_generic_this_capture		(tree, tree);
 extern tree maybe_resolve_dummy			(tree, bool);
 extern tree current_nonlambda_function		(void);
 extern tree nonlambda_method_basetype		(void);
Index: cp/lambda.c
===================================================================
--- cp/lambda.c	(revision 244382)
+++ cp/lambda.c	(working copy)
@@ -793,16 +793,14 @@  lambda_expr_this_capture (tree lambda, b
   return result;
 }
 
-/* We don't want to capture 'this' until we know we need it, i.e. after
-   overload resolution has chosen a non-static member function.  At that
-   point we call this function to turn a dummy object into a use of the
-   'this' capture.  */
+/* Return the current LAMBDA_EXPR, if this is a resolvable dummy
+   object.  NULL otherwise..  */
 
-tree
-maybe_resolve_dummy (tree object, bool add_capture_p)
+static tree
+resolvable_dummy (tree object)
 {
   if (!is_dummy_object (object))
-    return object;
+    return NULL_TREE;
 
   tree type = TYPE_MAIN_VARIANT (TREE_TYPE (object));
   gcc_assert (!TYPE_PTR_P (type));
@@ -812,18 +810,55 @@  maybe_resolve_dummy (tree object, bool a
       && LAMBDA_TYPE_P (current_class_type)
       && lambda_function (current_class_type)
       && DERIVED_FROM_P (type, current_nonlambda_class_type ()))
-    {
-      /* In a lambda, need to go through 'this' capture.  */
-      tree lam = CLASSTYPE_LAMBDA_EXPR (current_class_type);
-      tree cap = lambda_expr_this_capture (lam, add_capture_p);
-      if (cap && cap != error_mark_node)
+    return CLASSTYPE_LAMBDA_EXPR (current_class_type);
+
+  return NULL_TREE;
+}
+
+/* We don't want to capture 'this' until we know we need it, i.e. after
+   overload resolution has chosen a non-static member function.  At that
+   point we call this function to turn a dummy object into a use of the
+   'this' capture.  */
+
+tree
+maybe_resolve_dummy (tree object, bool add_capture_p)
+{
+  if (tree lam = resolvable_dummy (object))
+    if (tree cap = lambda_expr_this_capture (lam, add_capture_p))
+      if (cap != error_mark_node)
 	object = build_x_indirect_ref (EXPR_LOCATION (object), cap,
 				       RO_NULL, tf_warning_or_error);
-    }
 
   return object;
 }
 
+/* When parsing a generic lambda containing an argument-dependent
+   member function call we defer overload resolution to instantiation
+   time.  But we have to know now whether to capture this or not.
+   Do that if FNS contains any non-static fns.
+   The std doesn't anticipate this case, but I expect this to be the
+   outcome of discussion.  */
+
+void
+maybe_generic_this_capture (tree object, tree fns)
+{
+  if (tree lam = resolvable_dummy (object))
+    if (!LAMBDA_EXPR_THIS_CAPTURE (lam))
+      {
+	/* We've not yet captured, so look at the function set of
+	   interest.  */
+	if (BASELINK_P (fns))
+	  fns = BASELINK_FUNCTIONS (fns);
+	for (; fns; fns = OVL_NEXT (fns))
+	  if (DECL_NONSTATIC_MEMBER_FUNCTION_P (OVL_CURRENT (fns)))
+	    {
+	      /* Found a non-static member.  Capture this.  */
+	      lambda_expr_this_capture (lam, true);
+	      break;
+	    }
+      }
+}
+
 /* Returns the innermost non-lambda function.  */
 
 tree
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 244382)
+++ cp/parser.c	(working copy)
@@ -6971,6 +6971,7 @@  cp_parser_postfix_expression (cp_parser
 			|| type_dependent_expression_p (fn)
 			|| any_type_dependent_arguments_p (args)))
 		  {
+		    maybe_generic_this_capture (instance, fn);
 		    postfix_expression
 		      = build_nt_call_vec (postfix_expression, args);
 		    release_tree_vector (args);
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 244382)
+++ cp/pt.c	(working copy)
@@ -17142,19 +17142,34 @@  tsubst_copy_and_build (tree t,
 
 		if (unq != function)
 		  {
-		    tree fn = unq;
-		    if (INDIRECT_REF_P (fn))
-		      fn = TREE_OPERAND (fn, 0);
-		    if (TREE_CODE (fn) == COMPONENT_REF)
-		      fn = TREE_OPERAND (fn, 1);
-		    if (is_overloaded_fn (fn))
-		      fn = get_first_fn (fn);
-		    if (permerror (EXPR_LOC_OR_LOC (t, input_location),
-				   "%qD was not declared in this scope, "
-				   "and no declarations were found by "
-				   "argument-dependent lookup at the point "
-				   "of instantiation", function))
+		    /* In a lambda fn, we have to be careful to not
+		       introduce new this captures.  Legacy code can't
+		       be using lambdas anyway, so it's ok to be
+		       stricter.  */
+		    bool in_lambda = (current_class_type
+				      && LAMBDA_TYPE_P (current_class_type));
+		    char const *msg = "%qD was not declared in this scope, "
+		      "and no declarations were found by "
+		      "argument-dependent lookup at the point "
+		      "of instantiation";
+
+		    bool diag = true;
+		    if (in_lambda)
+		      error_at (EXPR_LOC_OR_LOC (t, input_location),
+				msg, function);
+		    else
+		      diag = permerror (EXPR_LOC_OR_LOC (t, input_location),
+					msg, function);
+		    if (diag)
 		      {
+			tree fn = unq;
+			if (INDIRECT_REF_P (fn))
+			  fn = TREE_OPERAND (fn, 0);
+			if (TREE_CODE (fn) == COMPONENT_REF)
+			  fn = TREE_OPERAND (fn, 1);
+			if (is_overloaded_fn (fn))
+			  fn = get_first_fn (fn);
+
 			if (!DECL_P (fn))
 			  /* Can't say anything more.  */;
 			else if (DECL_CLASS_SCOPE_P (fn))
@@ -17177,7 +17192,13 @@  tsubst_copy_and_build (tree t,
 			  inform (DECL_SOURCE_LOCATION (fn),
 				  "%qD declared here, later in the "
 				  "translation unit", fn);
+			if (in_lambda)
+			  {
+			    release_tree_vector (call_args);
+			    RETURN (error_mark_node);
+			  }
 		      }
+
 		    function = unq;
 		  }
 	      }
Index: testsuite/g++.dg/cpp1y/pr61636-1.C
===================================================================
--- testsuite/g++.dg/cpp1y/pr61636-1.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/pr61636-1.C	(working copy)
@@ -0,0 +1,31 @@ 
+// PR c++/61636
+// { dg-do compile { target c++14 } }
+
+// ICE because we figure this capture too late.
+
+struct Base
+{
+  void Bar (int);
+};
+
+struct A : Base {
+  void b ();
+  void Foo (int);
+  using Base::Bar;
+  template <typename T> void Baz (T);
+};
+
+void A::b() {
+
+  auto lam = [&](auto asdf) { Foo (asdf); };
+
+  lam (0);
+
+  auto lam1 = [&](auto asdf) { Bar (asdf); };
+
+  lam1 (0);
+
+  auto lam2 = [&](auto asdf) { Baz (asdf); };
+
+  lam2 (0);
+}
Index: testsuite/g++.dg/cpp1y/pr61636-2.C
===================================================================
--- testsuite/g++.dg/cpp1y/pr61636-2.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/pr61636-2.C	(working copy)
@@ -0,0 +1,72 @@ 
+// PR c++/61636
+// { dg-do run { target c++14 } }
+
+// Check we don't capture this (too) unnecessarily
+
+struct A {
+  int b ();
+  void f (int) {}
+  static void f (double) {}
+
+  static void g (int) {}
+  static void g (double) {}
+};
+
+struct O {
+  void x (int) {}
+  static void x (double) {}
+};
+
+namespace N {
+  void y (double) {}
+}
+
+int Check (bool expect, unsigned size)
+{
+  return (expect ? sizeof (void *) : 1) != size;
+}
+
+int A::b() {
+  int r = 0;
+
+  // one of the functions is non-static
+  auto l0 = [&](auto z) { f (z); };
+  r += Check (true, sizeof l0);
+  l0(0.0); // doesn't need this capture for A::f(double), but too late
+  l0 (0); // Needs this capture for A::f(int)
+
+  // no fn is non-static.
+  auto l00 = [&](auto z) { g (z); };
+  r += Check (false, sizeof l00);
+  l00(0.0); 
+  l00 (0);
+
+  // sizeof isn't an evaluation context, so no this capture
+  auto l1 = [&](auto z) { sizeof (f (z), 1); };
+  r += Check (false, sizeof l1);
+  l1(0.0); l1 (0); 
+
+  auto l2 = [&](auto) { f (2.4); };
+  auto l3 = [&](auto) { f (0); };
+  l2(0); l3(0); l2(0.0); l3 (0.0);
+  r += Check (false, sizeof l2);
+  r += Check (true, sizeof l3);
+
+  auto l4 = [&](auto) { O::x (2.4); };
+  auto l5 = [&](auto) { N::y (2.4); };
+  auto l6 = [&](auto) { };
+  l4(0); l5(0); l6(0);
+  l4(0.0); l5(0.0); l6(0.0);
+  r += Check (false, sizeof l4);
+  r += Check (false, sizeof l5);
+  r += Check (false, sizeof l6);
+
+  return r;
+}
+
+int main ()
+{
+  A a;
+
+  return a.b () ? 1 : 0;
+}
Index: testsuite/g++.dg/cpp1y/pr61636-3.C
===================================================================
--- testsuite/g++.dg/cpp1y/pr61636-3.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/pr61636-3.C	(working copy)
@@ -0,0 +1,25 @@ 
+// PR c++/61636
+// { dg-do compile { target c++14 } }
+// permissiveness doesn't make this permitted
+// { dg-additional-options "-fpermissive" }
+
+// ICE because we attempt to use dependent Foo during error recovery
+// and die with an unexpected this capture need.
+
+template <typename T> struct Base
+{
+  void Foo (int);
+};
+
+template <typename T> struct A : Base<T> {
+  void b ();
+};
+
+template <typename T> void A<T>::b() {
+
+  auto lam = [&](auto asdf) { Foo (asdf); }; // { dg-error "not declared" }
+
+  lam (T(0));
+}
+
+template void A<int>::b ();