On 1/27/25 2:34 PM, Simon Martin wrote:
Hi Jason,
On 27 Jan 2025, at 15:23, Jason Merrill wrote:
On 1/27/25 8:14 AM, Simon Martin wrote:
Hi Jason,
On 24 Jan 2025, at 16:51, Jason Merrill wrote:
On 1/24/25 6:19 AM, Simon Martin wrote:
Hi Jason,
On 23 Jan 2025, at 22:56, Jason Merrill wrote:
On 1/23/25 12:06 PM, Simon Martin wrote:
Hi Marek,
On 23 Jan 2025, at 16:45, Marek Polacek wrote:
On Thu, Jan 23, 2025 at 02:40:09PM +0000, Simon Martin wrote:
Hi Jason,
On 20 Jan 2025, at 22:49, Jason Merrill wrote:
On 1/20/25 2:11 PM, Simon Martin wrote:
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.
Because that's the declaration of the capture proxy, not a use
of
it.
Indeed, thanks for clarifying.
Why aren't we finding the use in the declaration of t?
After further investigation, the problem is rather that neither
walk_tree_1 nor cp_walk_subtrees walk the dimensions of array
types,
so
we miss the uses.
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.
Right.
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.
Completely breaking this optimization is a big deal,
particularly
since it affects the layout of closure objects. We can't
always
optimize everything away.
ACK.
The attached updated patch makes cp_walk_subtrees walk array
type
dimensions, which fixes the initial PR without those 3
regressions.
Successfully tested on x86_64-pc-linux-gnu. Is it OK?
Simon
PS: In theory I think it’d make sense to do do this change in
walk_tree_1 since C also supports VLAs, but doing so breaks
some
OMP
tests. OMP can do interesting things with array bounds (see
r9-5354-gda972c05f48637), and fixing those would require
teaching
walk_tree_1 about the “omp dummy var” array bounds, which I
think
would be a bit ugly. And I’m not aware of any C case that
would
be
improved/fixed by doing this change, so we’re probably fine
not
doing
it.
From e19bb6c943a422b1201c5c9a2a1d4f32141baf84 Mon Sep 17
00:00:00
2001
From: Simon Martin <si...@nasilyan.com>
Date: Wed, 22 Jan 2025 16:19:47 +0100
Subject: [PATCH] c++: Don't prune constant capture proxies only
used
in array
dimensions [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 ===
When parsing the lambda body, and more specifically the
multiplication,
we mark the lambda as LAMBDA_EXPR_CAPTURE_OPTIMIZED, which
indicates
to
prune_lambda_captures that it might be possible to optimize out
some
captures.
The problem is that prune_lambda_captures then misses the use
of
the
r
capture (because neither walk_tree_1 nor cp_walk_subtrees walks
the
dimensions of array types - here "r * c"), hence believes the
capture
can be pruned... and we trip on an assert when instantiating
the
lambda.
This patch changes cp_walk_subtrees so that when walking a
declaration
with an array type, it walks that array type's dimensions.
Since
C
also
supports VLAs, I thought it'd make sense to do this in
walk_tree_1,
but
this breaks some omp-low related test cases (because OMP can do
funky
things with array bounds when adjust_array_error_bounds is
set),
and
I
don't really want to add code to walk_tree_1 to skips arrays
whose
dimension is a temporary variable with the "omp dummy var"
attribute.
Successfully tested on x86_64-pc-linux-gnu.
PR c++/114292
gcc/cp/ChangeLog:
* tree.cc (cp_walk_subtrees): Walk array type dimensions.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1y/lambda-ice4.C: New test.
---
gcc/cp/tree.cc | 11 ++++++
gcc/testsuite/g++.dg/cpp1y/lambda-ice4.C | 44
++++++++++++++++++++++++
2 files changed, 55 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/lambda-ice4.C
diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
index 36581865a17..fc9a2fbff32 100644
--- a/gcc/cp/tree.cc
+++ b/gcc/cp/tree.cc
@@ -5844,6 +5844,17 @@ cp_walk_subtrees (tree *tp, int
*walk_subtrees_p, walk_tree_fn func,
break;
default:
+ /* If this is an array, walk its dimensions. */
+ if (DECL_P (t) && TREE_TYPE (t)
+ && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
+ {
+ tree domain = TYPE_DOMAIN (TREE_TYPE (t));
+ if (domain)
+ {
+ WALK_SUBTREE (TYPE_MIN_VALUE (domain));
+ WALK_SUBTREE (TYPE_MAX_VALUE (domain));
+ }
+ }
return NULL_TREE;
}
Is there a reason I'm missing not to put this into the TYPE_P
block
above?
Do you mean put it in that block as well, or instead of where I
put
it?
If the latter, we don’t see any TYPE_P node for “int[r*n]”,
so
we
don’t go into that block, and continue ICE’ing without the
change
in
the “default:”.
Anyway it’s a very good call (thanks!), because it got me to
check
what we get if we change the lambda body to just do “typedef
int
MyT[c*r];”, and we still ICE. And from a quick look, doing
something
similar in the TYPE_P block does not fix it.
I’ll work something out and report back.
I would think this should go in the DECL_EXPR handling, so we
don't
walk into the type every time we see a use of an array variable.
Indeed, we can do the check there, thanks a lot.
And concerning the case of a single “typedef int MyT[r*c];”,
there
already is code in walk_tree_1 that covers such typedefs, but that
does
not walk array type dimensions.
So the result is the attached updated patch, successfully tested on
x86_64-pc-linux-gnu. OK for trunk? And if so, also for branches?
* tree.cc (walk_tree_1): Walk array type dimensions, if any, for
TYPE_DECLs.
Hmm, why isn't this covered by the existing call to
walk_type_fields?
Ah, I guess because that just walks into TYPE_DOMAIN, and nothing
walks from there into the TYPE_MIN/MAX_VALUE.
What if instead of touching walk_tree_1, mark_const_cap_r handled
INTEGER_TYPE?
I don’t think we can look into TYPE_{MIN,MAX}_VALUE from
mark_const_cap_r since we might need to recurse (here we have a
MINUS_EXPR in TYPE_MAX_VALUE) and I don’t think we can from there?
Several tree walk functions recurse, in which case they need to handle
the pointer_set themselves instead of using
cp_walk_tree_without_duplicates. For instance,
find_parameter_packs_r. But no need for this patch.
I know walk_tree used to walk into VLA bounds, which is why we have
stabilize_vla_size to split out any relevant expressions into
temporaries; I'm reluctant to re-add that without a lot of
investigation into the history.
Yeah, makes sense. I learnt that touching walk_tree requires caution
through all that I broke with my few attempts at changing it :-)
OK, I looked it up: this goes back to r119481
(r0-77782-g8f6e6bf375959d), the bounds walking was removed because it
caused trouble for Ada. That patch added bounds walking to
for_each_template_parm_r instead to avoid regressing C++.
Thanks for digging into this and providing extra context.
In cp_walk_subtrees, I think we also want to go through the typedef
handling at the top, so in the DECL_EXPR handling, let's just walk
into the type of the decl. So we walk into the ARRAY_TYPE, check
whether it's a typedef, if not walk_type_fields walks into the
TYPE_DOMAIN, and the mark_const_cap_r adjustment I suggested above
checks the TYPE_MAX_VALUE from there.
I think that the typedef handling is already done in walk_tree (at
least
I was unable to craft a test case where doing something extra for
typedefs in cp_walk_subtrees was required).
I agree that nothing extra is required; I was referring to the
typedef_variant_p code at the top of cp_walk_subtree.
And following up on your suggestion, things end up pretty simple:
just
walk the type of declarations in DECL_EXPRs, and explicitly handle
the
min/max value of INTEGER_TYPEs in cp_walk_subtree.
At least that’s what the updated patch does, successfully tested on
x86_64-pc-linux-gnu. Ok for trunk?
@@ -5843,6 +5844,11 @@ cp_walk_subtrees (tree *tp, int
*walk_subtrees_p, walk_tree_fn func,
WALK_SUBTREE (STATIC_ASSERT_MESSAGE (t));
break;
+ case INTEGER_TYPE:
+ WALK_SUBTREE (TYPE_MIN_VALUE (t));
+ WALK_SUBTREE (TYPE_MAX_VALUE (t));
+ break;
Let's add a comment
/* Removed from walk_type_fields in r119481. */
and remove the now-redundant INTEGER_TYPE handling from
for_each_template_parm_r.
OK with those adjustments.
Thanks a lot. Merged to trunk as r15-7238-gceabea405ffdc8.
Even though it’s an “old” regression (from GCC 7), it’s a
reject-valid and I’m tempted to backport; OK for active branches as
well?
For backports I'd be inclined to condition the INTEGER_TYPE handing on
if (processing_template_decl) to reduce its impact. But first let's
wait and see if this breaks anything on trunk.
Jason
