On 10/22/25 6:11 AM, Nathaniel Shead wrote:
On Sun, Oct 19, 2025 at 09:35:28PM +0300, Jason Merrill wrote:
On 10/19/25 2:14 AM, Nathaniel Shead wrote:
On Sat, Oct 18, 2025 at 06:41:15PM +0300, Jason Merrill wrote:
On 10/18/25 4:13 PM, Nathaniel Shead wrote:
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/15?
-- >8 --
The ICE in the linked PR occurs because we first stream the lambda type
before its keyed decl has been streamed, but the key decl's type depends
on the lambda. And so when streaming the key decl to check for an
existing decl to merge with, merging the key decl itself crashes because
its type has only been partially streamed.
This patch fixes the issue by generalising the existing FIELD_DECL
handling to any class member using the outermost containing TYPE_DECL as
its key type. This way we can guarantee that the key decl has been
streamed before the lambda type is otherwise needed.
Why doesn't the existing FIELD_DECL handling already key the lambda to the
class, since "do_nothing" is a data member?
Because 'do_nothing' is a static member it's a VAR_DECL, not a
FIELD_DECL.
Ah, indeed.
Why is the outermost class needed? It looks like the A/B case is testing
that, but why doesn't it work to start streaming the lambda and check with
MoreInner for a merge candidate?
A more minimal example of this case that also fails in GCC 15:
struct X {
struct Inner {
union MoreInner {
decltype([]{}) y;
};
};
using B = decltype(Inner::MoreInner::y);
};
Here we crash when streaming in the attached decls for 'MoreInner'.
The type of 'y' is attached to 'MoreInner' directly, but we currently
stream members in reverse order of declaration, so we see 'B' before
streaming 'Inner' or 'MoreInner'. So similarly to the case in the PR,
we end up reading 'MoreInner' first while finding the key entity for the
lambda, see that's it a duplicate, read its attached entities, and then
fail the check here:
if (DECL_LANG_SPECIFIC (inner)
&& DECL_MODULE_KEYED_DECLS_P (inner))
{
/* Read and maybe install the attached entities. */
bool existed;
auto &set = keyed_table->get_or_insert (STRIP_TEMPLATE (existing),
&existed);
unsigned num = u ();
if (is_new == existed)
set_overrun ();
if (is_new)
set.reserve (num);
for (unsigned ix = 0; !get_overrun () && ix != num; ix++)
{
tree attached = tree_node ();
dump (dumper::MERGE)
&& dump ("Read %d[%u] %s attached decl %N", tag, ix,
is_new ? "new" : "matched", attached);
if (is_new)
set.quick_push (attached);
else if (set[ix] != attached)
set_overrun (); // <--- fail here
}
}
because the partially-streamed, not-yet-deduplicated type of the lambda
isn't the same type as the existing lambda keyed to the existing
MoreInner type.
By keying to the outermost class we avoid any potential issues with
out-of-order member dependencies. These specific cases could possibly
also be fixed by reversing the order that we stream members, but I'm not
sure if that would expose other possible issues (with e.g. lazily
declared members or instantiations or whatnot).
I would imagine that in general, streaming members in declaration order
would improve how often a reference points to something already loaded. Is
there any rationale for reverse order?
On closer inspection, looks like I misunderstood; the ordering of class
members is redone by 'sort_cluster' which effectively randomises
non-field deps unless they have ordering dependencies between them
(which was what I was looking at...).
This implies that another possible solution for this particular bug
would be to add some additional ordering guarantee so that intra-cluster
decls always emit the key decl before the lambda. This still doesn't
work for FIELD_DECLs though (as they have no dep for sort_cluster to
order), and I wasn't able to find a neat way to enforce this ordering
for other decls without adding additional data to stream in
'decl_container'.
Still OK for trunk/15 or would you prefer me to try to find another
approach to guarantee this ordering?
Still OK with a comment explaining the above.
In the meantime, this patch is OK with the tweaks below.
There's also the very
very slight benefit that we need to allocate less vecs in the
keyed_decls map if more decls reuse the same key here.
@@ -21506,9 +21493,11 @@ maybe_key_decl (tree ctx, tree decl)
&& TREE_CODE (ctx) != CONCEPT_DECL)
return;
- /* For fields, key it to the containing type to handle deduplication
- correctly. */
- if (TREE_CODE (ctx) == FIELD_DECL)
+ /* For members, key it to the containing type to handle deduplication
+ correctly: otherwise we may run into issues when loading the lambda
+ type before its keyed decl, if the member itself depends on the
+ lambda type (PR c++/122310). */
+ while (RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (ctx)))
DECL_CLASS_SCOPE_P
ctx = TYPE_NAME (DECL_CONTEXT (ctx));
if (!keyed_table)
@@ -21523,6 +21512,30 @@ maybe_key_decl (tree ctx, tree decl)
vec.safe_push (decl);
}
+/* Find the scope that the lambda DECL is keyed to, if any. */
+
+static tree
+get_keyed_decl_scope (tree decl)
+{
+ gcc_checking_assert (LAMBDA_TYPE_P (TREE_TYPE (decl)));
+ tree scope = LAMBDA_TYPE_EXTRA_SCOPE (TREE_TYPE (decl));
+ if (!scope)
+ return NULL_TREE;
+
+ gcc_checking_assert (TREE_CODE (scope) == VAR_DECL
+ || TREE_CODE (scope) == FIELD_DECL
+ || TREE_CODE (scope) == PARM_DECL
+ || TREE_CODE (scope) == TYPE_DECL
+ || TREE_CODE (scope) == CONCEPT_DECL);
+
+ while (RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (scope)))
Likewise.
Jason
