[PATCH][C++] Fix constant reference in a lambda (PR c++/53488)

[PATCH][C++] Fix constant reference in a lambda (PR c++/53488)

[Jiří Paleček]

[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]

Hello,

I've been investigating a bug in gcc I came across recently and after some  
difficulties, I have produced a patch that actually fixes that behavior.  
However, I don't think the patch is very good and I would really  
appreciate your help in making it better.

The problem is that when a const(expr) variable is used in a lambda, it  
yields garbage values in gcc when the initializer is dependent. Moreover,  
it should not be captured if it fulfills the requirements of a constant  
expression (at least that's how I understand the definition of "odr-used"  
along with the definition of default captures). finish_id_expression did  
return VAR_DECL for the corresponding automatic variable in that case, but  
didn't create the capture.

The idea of the fix is to postpone the decision whether or not to capture  
later to the template instantiation. This is because until then, we cannot  
know if the variable fulfills the conditions for a constant expression or  
not. So the patch assumes that if the constant value is a VAR_DECL (which  
is what I've found to happen in the debugger, although I'm not sure this  
is the only case) and we are in a template declaration, it just leaves the  
id expression there as is.

Then the patch fiddles with the capture list in template instantiation  
code. Because new captures can be added while processing the template, we  
need to concatenate them with the original list from the template, plus  
there's a small hack to avoid producing the lambda-to-function-pointer  
conversion operator while we still don't have the final capture list.

I have produced two tests (in the patch) and ran the testsuite in the gcc  
subdirectory successfully. I've also tested the behavior on a testcase in  
the attached test.cpp file (it should print ok with all possible  
combinations of #defined macros).

I'd be grateful on your comments on this, especially

- whether the approach taken is a good one, and
- whether my assumptions about what's going on in the code (expressed  
either in this mail and in the comments in code) are true

Regards
     Jiri Palecek

[-- Attachment #2: gcc.diff --]
[-- Type: application/octet-stream, Size: 4330 bytes --]

commit 67e2b85a7a28a739e25068f9e5eebabd827cd1af
Author: Jiri Palecek <jirka@debian>
Date:   Sat Aug 18 13:45:29 2012 +0200

    Fix capturing constants in lambda expressions in templates

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index ad81bab..5008361 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -14102,14 +14102,30 @@ tsubst_copy_and_build (tree t,
 
 	/* Do this again now that LAMBDA_EXPR_EXTRA_SCOPE is set.  */
 	determine_visibility (TYPE_NAME (type));
+
+        /* This is a hack:
+
+           We absolutely need the capture list to be nonempty if the
+           template had it nonempty, otherwise, we will have the
+           conversion-to-function-pointer operator erroneously
+           added. We use a dummy list with a single element that we
+           can get rid of easily later
+        */
+	LAMBDA_EXPR_CAPTURE_LIST (r)
+          = LAMBDA_EXPR_CAPTURE_LIST (t) != NULL_TREE ? tree_cons(NULL_TREE, NULL_TREE, NULL_TREE) : NULL_TREE;
 	/* Now that we know visibility, instantiate the type so we have a
 	   declaration of the op() for later calls to lambda_function.  */
 	complete_type (type);
 
 	/* The capture list refers to closure members, so this needs to
 	   wait until after we finish instantiating the type.  */
+        /* The dummy element, if any, is now at the end of the capture
+           list. There might have been some default captures added, so
+           we reverse the list, remove the dummy element and append
+           the list to the capture list from the template */
+        tree reverse_list = LAMBDA_EXPR_CAPTURE_LIST(r)!= NULL_TREE ? nreverse(LAMBDA_EXPR_CAPTURE_LIST(r)) : NULL_TREE;
 	LAMBDA_EXPR_CAPTURE_LIST (r)
-	  = RECUR (LAMBDA_EXPR_CAPTURE_LIST (t));
+          = chainon(RECUR (LAMBDA_EXPR_CAPTURE_LIST (t)), (reverse_list!=NULL_TREE && TREE_PURPOSE(reverse_list) == NULL_TREE) ? TREE_CHAIN(reverse_list) : reverse_list);
 
 	return build_lambda_object (r);
       }
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index ebac960..d856f7a 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -2961,8 +2961,20 @@ finish_id_expression (tree id_expression,
 	     the complexity of the problem"
 
 	     FIXME update for final resolution of core issue 696.  */
-	  if (decl_constant_var_p (decl))
-	    return integral_constant_value (decl);
+          if (decl_constant_var_p (decl)) {
+	    tree const_value = integral_constant_value (decl);
+            if (TREE_CODE(const_value) != VAR_DECL)
+              return const_value;
+            /* If it didn't work out, it means we don't actually know
+               what will is the initializer (and if it is a constant
+               expression). We postpone the resolution to
+               instantiation time. This should be safe assuming we
+               find the same variable on instantiation time as here */
+            if (processing_template_decl)
+              return id_expression;
+            /* OK, I HOPE this can't happen */
+            gcc_unreachable();
+          }
 
 	  /* If we are in a lambda function, we can move out until we hit
 	     1. the context,
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-capture-const.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-capture-const.C
new file mode 100644
index 0000000..92f98fa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-capture-const.C
@@ -0,0 +1,24 @@
+// { dg-do run }
+// { dg-options "-std=c++0x" }
+#include <cassert>
+
+template <class T>
+void f(T t, unsigned size)
+{
+  int i = 1, j = -1;
+  const int ci = sizeof(T);
+  [&] () { j = ci; } ();
+  assert(i == 1);
+  assert(j == size);
+  j = -1;
+  j = [=] (T) { return ci; } (T());
+  assert(j == size);
+  //[&ci] () -> void { ci = 0; } (); { dg-error: cannot assign to const int }
+}
+
+int main() {
+  f(1, sizeof(int));
+
+  return 0;
+}
+
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-capture-const2.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-capture-const2.C
new file mode 100644
index 0000000..0943280
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-capture-const2.C
@@ -0,0 +1,20 @@
+// { dg-options "-std=c++0x" }
+
+struct A
+{
+  static int x;
+};
+
+int A::x;
+
+template <class T>
+void f()
+{
+  const int x = T::x;
+  [] { return x; }; // { dg-error "x.*not.*captured" }
+}
+
+int main()
+{
+  f<A>();
+}

[-- Attachment #3: test.cpp --]
[-- Type: application/octet-stream, Size: 2160 bytes --]

#include <iostream>
#ifdef CONST_MEMBER
#undef CONST_MEMBER
#define CONST_MEMBER const
#else
#define CONST_MEMBER
#endif

#ifdef TEMPLATE
#define TEMPLATE_DEF(A1, A2) template <A1, A2>
#define TEMPLATE_USE(X, A1, A2) X<A1, A2>
#else
#define TEMPLATE_DEF(A1, A2)
#define TEMPLATE_USE(X, A1, A2) X
#endif

struct A
{
  static CONST_MEMBER int value;
};

CONST_MEMBER int A::value=2;

struct B
{
  static CONST_MEMBER int value;
};

CONST_MEMBER int B::value = 3;

struct no {};
struct yes { no no_[2]; };

yes test(int(*)());
no test(...);

#define MY_ASSERT(TEST, STRING) \
  std::cout << ((TEST) ? "ok " : "bad ") << STRING << "\n";

TEMPLATE_DEF(class A, class B)
int func(const A&, const B&)
{
  int nonconst =0;
  const int ci = 1;
  auto a = [=] { return ci; };
  MY_ASSERT(sizeof(test(a)) == sizeof(yes) , "const1");
  MY_ASSERT(a() == 1, "const val1");
  auto b = [] { return ci; };
  MY_ASSERT(sizeof(test(b)) == sizeof(yes) , "const2");
  MY_ASSERT(b() == 1, "const val2");
  auto c = [&] { return ci; };
  MY_ASSERT(sizeof(test(c)) == sizeof(yes) , "const3");
  MY_ASSERT(c() == 1, "const val3");
  auto d = [=] { return nonconst; };
  MY_ASSERT(sizeof(test(d)) != sizeof(yes) , "nc1");
  MY_ASSERT(d() == 0, "nc val1");
  auto e = [&] { return nonconst; };
  MY_ASSERT(sizeof(test(e)) != sizeof(yes) , "nc2");
  MY_ASSERT(e() == 0, "nc val2");
  const int expr = A::value;
  auto f = [&] { return expr; };
  MY_ASSERT((sizeof(test(f)) == sizeof(yes)) == std::is_const<decltype(A::value)>::value , "expr1");
  MY_ASSERT(f() == 2, "expr val1");
  auto g = [=] { return expr; };
  MY_ASSERT((sizeof(test(g)) == sizeof(yes)) == std::is_const<decltype(A::value)>::value , "expr2");
  MY_ASSERT(g() == 2, "expr val2");
  const int expr2 = A::value*B::value;
  auto h = [&] { return expr2; };
  MY_ASSERT((sizeof(test(h)) == sizeof(yes)) == std::is_const<decltype(A::value)>::value , "expr3");
  MY_ASSERT(h() == 6, "expr val3");
  auto i = [=] { return expr2; };
  MY_ASSERT((sizeof(test(i)) == sizeof(yes)) == std::is_const<decltype(A::value)>::value , "expr4");
  MY_ASSERT(i() == 6, "expr val4");
}

int main()
{
  TEMPLATE_USE(func, A, B)(A(), B());
}

Re: [PATCH][C++] Fix constant reference in a lambda (PR c++/53488)

[Jason Merrill]

Hi, thanks for your patch, and sorry it took me so long to respond.

On 08/18/2012 11:02 AM, Jiří Paleček wrote:
> The idea of the fix is to postpone the decision whether or not to
> capture later to the template instantiation. This is because until then,
> we cannot know if the variable fulfills the conditions for a constant
> expression or not. So the patch assumes that if the constant value is a
> VAR_DECL (which is what I've found to happen in the debugger, although
> I'm not sure this is the only case) and we are in a template
> declaration, it just leaves the id expression there as is.

Thanks, I've now committed a simpler version of this that doesn't even 
bother calling integral_constant_value in templates.

> Then the patch fiddles with the capture list in template instantiation
> code. Because new captures can be added while processing the template,
> we need to concatenate them with the original list from the template,
> plus there's a small hack to avoid producing the
> lambda-to-function-pointer conversion operator while we still don't have
> the final capture list.

I think a cleaner way to deal with the conversion issue would be to 
instantiate the capture list in instantiate_class_template_1 instead of 
here, so that we can do it in between the members and the conversion op.

> I have produced two tests (in the patch) and ran the testsuite in the
> gcc subdirectory successfully. I've also tested the behavior on a
> testcase in the attached test.cpp file (it should print ok with all
> possible combinations of #defined macros).

Could you adjust the testcase so that it will work in the testsuite?

Thanks,
Jason