Hi Jason,
On 15 Jan 2025, at 22:42, Jason Merrill wrote:
> On 12/12/24 2:07 PM, Simon Martin wrote:
>> We currently ICE upon the following valid (under -Wno-vla) code
>>
>> === cut here ===
>> void f(int c) {
>> constexpr int r = 4;
>> [&](auto) { int t[r * c]; }(0);
>> }
>> === cut here ===
>>
>> The problem is that when parsing the lambda body, and more
>> specifically
>> the multiplication, we mark the lambda as
>> LAMBDA_EXPR_CAPTURE_OPTIMIZED
>> even though the replacement of r by 4 is "undone" by the call to
>> build_min_non_dep in build_x_binary_op. This makes
>> prune_lambda_captures
>> remove the proxy declaration while it should not, and we trip on an
>> assert at instantiation time.
>
> Why does prune_lambda_captures remove the declaration if it's still
> used in the function body? Setting LAMBDA_EXPR_CAPTURE_OPTIMIZED just
> means "we might have optimized away some captures", the tree walk
> should have found the use put back by build_x_binary_op.
I think that this is due to a bug in mark_const_cap_r, that’s been
here since the beginning, i.e. r8-7213-g1577f10a637352: it decides NOT
to walk sub-trees when encountering a DECL_EXPR for a constant capture
proxy (at lambda.cc:1769). I don’t understand why we wouldn’t want
to continue.
Removing that line fixes the PR, but breaks 3 existing tests
(lambda-capture1.C, lambda-const11.C and lambda-const11a.C, that all
assert that we optimise out the capture); that’s why I did not pursue
it in the first place.
But taking another look, it might not be that big a deal that we don’t
optimise those out: as soon as we use -O1 or above, the assignment to
the closure field actually disappears.
So the updated attached patch removes that line, and was successfully
tested on x86_64-pc-linux-gnu. OK for trunk? And if so, OK for active
branches as well since it’s a reject-valid regression from GCC 7?
Thanks, SimonFrom 567da917ca98b235e1dc65029ef25e4daa058215 Mon Sep 17 00:00:00 2001
From: Simon Martin <si...@nasilyan.com>
Date: Mon, 20 Jan 2025 13:51:50 +0100
Subject: [PATCH] c++: Only prune capture proxies for constant variables at
instantiation time [PR114292]
We currently ICE upon the following valid (under -Wno-vla) code
=== cut here ===
void f(int c) {
constexpr int r = 4;
[&](auto) { int t[r * c]; }(0);
}
=== cut here ===
The problem is that when parsing the lambda body, and more specifically
the multiplication, we mark the lambda as LAMBDA_EXPR_CAPTURE_OPTIMIZED
even though the replacement of r by 4 is "undone" by the call to
build_min_non_dep in build_x_binary_op. This makes prune_lambda_captures
remove the proxy declaration while it should not, and we trip on an
assert at instantiation time.
This patch fixes an issue in mark_const_cap_r that's been present since
its inception in r8-7213-g1577f10a637352: it decides to NOT walk
sub-trees when encountering a DECL_EXPR for a constant capture proxy,
and I don't see why not.
Removing this fixes the PR, but breaks 3 tests that asserted that the
capture was optimized (they all have the same pattern as this PR, i.e.
the use of the captured variable is in a sub-tree); I think that those
assert are actually "bogus", and can be removed.
Successfully tested on x86_64-pc-linux-gnu.
PR c++/114292
gcc/cp/ChangeLog:
* lambda.cc (mark_const_cap_r): Walk sub-trees even when
encountering a DECL_EXPR for a constant capture proxy.
gcc/testsuite/ChangeLog:
* g++.dg/abi/lambda-capture1.C: Remove incorrect expectation
about lambda capture removal.
* g++.dg/cpp0x/lambda/lambda-const11.C:
* g++.dg/cpp0x/lambda/lambda-const11a.C:
* g++.dg/cpp1y/lambda-ice4.C: New test.
---
gcc/cp/lambda.cc | 7 +--
gcc/testsuite/g++.dg/abi/lambda-capture1.C | 3 --
.../g++.dg/cpp0x/lambda/lambda-const11.C | 2 -
.../g++.dg/cpp0x/lambda/lambda-const11a.C | 2 -
gcc/testsuite/g++.dg/cpp1y/lambda-ice4.C | 44 +++++++++++++++++++
5 files changed, 46 insertions(+), 12 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/lambda-ice4.C
diff --git a/gcc/cp/lambda.cc b/gcc/cp/lambda.cc
index be8a0fe01cb..e52435fbd08 100644
--- a/gcc/cp/lambda.cc
+++ b/gcc/cp/lambda.cc
@@ -1755,7 +1755,7 @@ var_to_maybe_prune (tree cap)
see it, but replace it with any other use. */
static tree
-mark_const_cap_r (tree *t, int *walk_subtrees, void *data)
+mark_const_cap_r (tree *t, int *, void *data)
{
hash_map<tree,tree*> &const_vars = *(hash_map<tree,tree*>*)data;
@@ -1764,10 +1764,7 @@ mark_const_cap_r (tree *t, int *walk_subtrees, void
*data)
{
tree decl = DECL_EXPR_DECL (*t);
if (is_constant_capture_proxy (decl))
- {
- var = DECL_CAPTURED_VARIABLE (decl);
- *walk_subtrees = 0;
- }
+ var = DECL_CAPTURED_VARIABLE (decl);
}
else if (!location_wrapper_p (*t) /* is_capture_proxy dislikes them. */
&& is_constant_capture_proxy (*t))
diff --git a/gcc/testsuite/g++.dg/abi/lambda-capture1.C
b/gcc/testsuite/g++.dg/abi/lambda-capture1.C
index ad86eff876b..39ef34c5702 100644
--- a/gcc/testsuite/g++.dg/abi/lambda-capture1.C
+++ b/gcc/testsuite/g++.dg/abi/lambda-capture1.C
@@ -1,11 +1,8 @@
// PR c++/84726
// { dg-do compile { target c++11 } }
-#define SA(X) static_assert (X, #X)
-
int main()
{
const int i = 42;
auto l = [=]{return i+i;};
- SA(sizeof(l) == 1);
}
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11.C
b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11.C
index 26af75bf132..0d087b8d0b2 100644
--- a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11.C
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11.C
@@ -7,8 +7,6 @@ void f() {
auto l = [&] {
int n[dim * 1];
};
- // In f<int>, we shouldn't actually capture dim.
- static_assert (sizeof(l) == 1, "");
}
template void f<int>();
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11a.C
b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11a.C
index 7fc3d48a8dd..3de910c8ec8 100644
--- a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11a.C
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11a.C
@@ -14,8 +14,6 @@ void f() {
using ty2 = A<dim * 3>;
};
l(0);
- // In f<int>, we shouldn't actually capture dim.
- static_assert (sizeof(l) == 1, "");
}
template void f<int>();
diff --git a/gcc/testsuite/g++.dg/cpp1y/lambda-ice4.C
b/gcc/testsuite/g++.dg/cpp1y/lambda-ice4.C
new file mode 100644
index 00000000000..fe8df52b827
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/lambda-ice4.C
@@ -0,0 +1,44 @@
+// PR c++/114292
+// { dg-do "compile" { target c++14 } }
+// { dg-additional-options "-Wno-vla" }
+
+#define ASSERT_CAPTURE_NUMBER(Lambda, NumCaptures) \
+ { \
+ auto oneCapture = [&](auto) { int t[c]; }; \
+ const auto sizeOneCapture = sizeof (oneCapture); \
+ const auto expected = NumCaptures ? NumCaptures * sizeOneCapture : 1; \
+ static_assert (sizeof (Lambda) == expected, ""); \
+ }
+
+void foo (int c)
+{
+ constexpr int r = 4;
+
+ // This used to ICE.
+ [&](auto) { int t[c * r]; }(0);
+
+ // All those worked already, but were not covered by any test - do it here.
+ auto ok_1 = [&](auto) { int t[c]; };
+ ok_1 (0);
+ ASSERT_CAPTURE_NUMBER (ok_1, 1);
+
+ auto ok_2 = [&](auto) { int n = r * c; int t[n]; };
+ ok_2 (0);
+ ASSERT_CAPTURE_NUMBER (ok_2, 2);
+
+ auto ok_3 = [&](auto) { int t[r]; };
+ ok_3 (0);
+ ASSERT_CAPTURE_NUMBER (ok_3, 1); // Was 0 before the patch.
+
+ auto ok_4 = [&](auto) { int t[c * 4]; };
+ ok_4 (0);
+ ASSERT_CAPTURE_NUMBER (ok_4, 1);
+
+ auto ok_5 = [&](auto) { int t[1]; };
+ ok_5 (0);
+ ASSERT_CAPTURE_NUMBER (ok_5, 0);
+
+ auto ok_6 = [&](auto) { int t[c * r]; };
+ ok_6 (0);
+ ASSERT_CAPTURE_NUMBER (ok_6, 2);
+}
--
2.44.0
