Re: [PATCH] Fix PR c++/70121 (premature folding of const var that was implicitly captured)

[Patrick Palka]

On Thu, Mar 10, 2016 at 6:06 PM, Patrick Palka  wrote:
> On Thu, Mar 10, 2016 at 5:58 PM, Patrick Palka  wrote:
>> Within a lambda we should implicitly capture an outer const variable
>> only if it's odr-used in the body of the lambda.  But we are currently
>> making the decision of whether to capture such a variable, or else to
>> fold it to a constant, too early -- before we can know whether it's
>> being odr-used or not.  So we currently always fold a const variable to
>> a constant if possible instead of otherwise capturing it, but of course
>> doing this is wrong if e.g. the address of this variable is taken inside
>> the lambda's body.
>>
>> This patch reverses the behavior of process_outer_var_ref, so that we
>> always implicitly capture a const variable if it's capturable, instead
>> of always trying to first fold it to a constant.  This behavior however
>> is wrong too, and introduces a different but perhaps less important
>> regression: if we implicitly capture by value a const object that is not
>> actually odr-used within the body of the lambda, we may introduce a
>> redundant call to its copy/move constructor, see pr70121-2.C.
>>
>> Ideally we should be capturing a variable only if it's not odr-used
>
> Er, this sentence should read
>
>   Ideally we should be _implicitly_ capturing a variable only if it
> _is_ odr-used ...

Ping.

Re: [PATCH] Fix PR c++/70121 (premature folding of const var that was implicitly captured)

[Jason Merrill]

On 03/10/2016 05:58 PM, Patrick Palka wrote:

This patch reverses the behavior of process_outer_var_ref, so that we
always implicitly capture a const variable if it's capturable, instead
of always trying to first fold it to a constant.  This behavior however
is wrong too, and introduces a different but perhaps less important
regression: if we implicitly capture by value a const object that is not
actually odr-used within the body of the lambda, we may introduce a
redundant call to its copy/move constructor, see pr70121-2.C.


In general I'm disinclined to trade one bug for another, and I'm 
skeptical that the different regression is less important; I imagine 
that most uses of const variables will be for their constant value 
rather than their address.


Jason

Re: [PATCH] Fix PR c++/70121 (premature folding of const var that was implicitly captured)

[Patrick Palka]

On Fri, Mar 18, 2016 at 11:14 AM, Jason Merrill  wrote:
> On 03/10/2016 05:58 PM, Patrick Palka wrote:
>>
>> This patch reverses the behavior of process_outer_var_ref, so that we
>> always implicitly capture a const variable if it's capturable, instead
>> of always trying to first fold it to a constant.  This behavior however
>> is wrong too, and introduces a different but perhaps less important
>> regression: if we implicitly capture by value a const object that is not
>> actually odr-used within the body of the lambda, we may introduce a
>> redundant call to its copy/move constructor, see pr70121-2.C.
>
>
> In general I'm disinclined to trade one bug for another, and I'm skeptical
> that the different regression is less important; I imagine that most uses of
> const variables will be for their constant value rather than their address.

Okay, I may try to implement a complete fix for GCC 7 then.

[PATCH] Fix PR c++/70121 (premature folding of const var that was implicitly captured)

[Patrick Palka]

Within a lambda we should implicitly capture an outer const variable
only if it's odr-used in the body of the lambda.  But we are currently
making the decision of whether to capture such a variable, or else to
fold it to a constant, too early -- before we can know whether it's
being odr-used or not.  So we currently always fold a const variable to
a constant if possible instead of otherwise capturing it, but of course
doing this is wrong if e.g. the address of this variable is taken inside
the lambda's body.

This patch reverses the behavior of process_outer_var_ref, so that we
always implicitly capture a const variable if it's capturable, instead
of always trying to first fold it to a constant.  This behavior however
is wrong too, and introduces a different but perhaps less important
regression: if we implicitly capture by value a const object that is not
actually odr-used within the body of the lambda, we may introduce a
redundant call to its copy/move constructor, see pr70121-2.C.

Ideally we should be capturing a variable only if it's not odr-used
within the entire body of the lambda, but doing that would be a
significantly less trivial patch I think.  So I wonder if this patch is
a good tradeoff for GCC 6.  But I mostly wonder if my analysis and
proposed ideal solution is correct :)  Either way I'm planning to
bootstrap + regtest this patch and also test it against Boost.

gcc/cp/ChangeLog:

PR c++/70121
* semantics.c (process_outer_var_ref): Don't fold DECL to a
constant if it's otherwise going to get implicitly captured.

gcc/testsuite/ChangeLog:

PR c++/70121
* g++.dg/cpp0x/lambda/pr70121.C: New test.
* g++.dg/cpp0x/lambda/pr70121-2.C: New test.
---
 gcc/cp/semantics.c|  6 +
 gcc/testsuite/g++.dg/cpp0x/lambda/pr70121-2.C | 22 
 gcc/testsuite/g++.dg/cpp0x/lambda/pr70121.C   | 37 +++
 3 files changed, 65 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/lambda/pr70121-2.C
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/lambda/pr70121.C

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index ea72e0e..a9dbf16 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -3332,6 +3332,12 @@ process_outer_var_ref (tree decl, tsubst_flags_t 
complain)
   the closure type when instantiating the lambda context.  That is
   probably also the way to handle lambdas within pack expansions.  */
return decl;
+  /* Don't try to fold DECL to a constant if we are otherwise going to
+implicitly capture it.  FIXME we should avoid folding DECL to a
+constant only if it's odr-used within the lambda body, but we can't
+determine that at this point.  */
+  else if (lambda_expr && context == containing_function)
+   ;
   else if (decl_constant_var_p (decl))
{
  tree t = maybe_constant_value (convert_from_reference (decl));
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/pr70121-2.C 
b/gcc/testsuite/g++.dg/cpp0x/lambda/pr70121-2.C
new file mode 100644
index 000..4676068
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/pr70121-2.C
@@ -0,0 +1,22 @@
+// PR c++/70121
+// { dg-do compile { target c++11 } }
+
+struct X
+{
+  int a;
+
+  constexpr X () : a (28) { };
+  X (const X&) = delete;
+  X (const X&&) = delete;
+};
+
+void
+baz (void)
+{
+  constexpr X x;
+  // These are non-odr usages of x.a so they should each be folded to a
+  // constant expression without having to capture and copy x.
+  auto ff1 = [=] { return x.a; }; // { dg-bogus "deleted" "" { xfail *-*-* } }
+  const auto  = [=] { return x.a; }; // { dg-bogus "deleted" "" { xfail 
*-*-* } }
+  const auto & = [=] { return x.a; }; // { dg-bogus "deleted" "" { xfail 
*-*-* } }
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/pr70121.C 
b/gcc/testsuite/g++.dg/cpp0x/lambda/pr70121.C
new file mode 100644
index 000..db8a4ca
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/pr70121.C
@@ -0,0 +1,37 @@
+// PR c++/70121
+// { dg-do compile { target c++11 } }
+
+static void
+foo (void)
+{
+  const int val = 28;
+  auto ff_v = [=]() -> const int& { return val; }; // { dg-bogus "temporary" }
+  auto ff_r = [&]() -> const int& { return val; }; // { dg-bogus "temporary" }
+
+  if (_r () != )
+__builtin_abort ();
+
+  if (ff_v () + ff_r () != 56)
+__builtin_abort ();
+}
+
+static void
+bar (void)
+{
+  const int val = 28;
+  static const int *p;
+  auto ff_v = [=] { p =  }; // { dg-bogus "lvalue required" }
+  auto ff_r = [&] { p =  }; // { dg-bogus "lvalue required" }
+
+  ff_v ();
+  ff_r ();
+  if (p != )
+__builtin_abort ();
+}
+
+int
+main ()
+{
+  foo ();
+  bar ();
+}
-- 
2.8.0.rc1.12.gfce6d53