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?
> 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
>
