On Mon, Oct 27, 2025 at 04:07:30PM +0200, Jason Merrill wrote:
> 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.
>
Thanks, I'll push with the following comment:
@@ -21511,9 +21498,21 @@ 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. For fields, this is necessary as FIELD_DECLs have no
+ dep and so would only be streamed after the lambda type, defeating
+ our ability to merge them.
+
+ Other class-scope key decls might depend on the type of the lambda
+ but be within the same cluster; we need to ensure that we never
+ first see the key decl while streaming the lambda type as merging
+ would then fail when comparing the partially-streamed lambda type
+ of the key decl with the existing (PR c++/122310).
+
+ Perhaps sort_cluster can be adjusted to handle this better, but
+ this is a simple workaround (and might down on the number of
+ entries in keyed_table as a bonus). */
+ while (DECL_CLASS_SCOPE_P (ctx))
ctx = TYPE_NAME (DECL_CONTEXT (ctx));
if (!keyed_table)
> > > 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
> > >
> >
>
