On Fri, Nov 01, 2024 at 12:08:45PM -0400, Jason Merrill wrote: > On 10/31/24 6:09 AM, Nathaniel Shead wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > -- >8 -- > > > > In cases like the linked PR we sometimes get mutually recursive > > dependencies that both rely on the other to have been streamed as part > > of their merge key information. In the linked PR, this causes an ICE. > > > > The root cause is that 'sort_cluster' is not correctly ordering the > > dependencies; both the element_t specialisation and the > > reverse_adaptor::first function decl depend on each other, but by > > streaming element_t first it ends up trying to stream itself recursively > > as part of calculating its own merge key, which apart from the checking > > ICE will also cause issues on stream-in, as the merge key will not > > properly stream. > > > > There is a comment already in 'sort_cluster' describing this issue, but > > it says: > > > > Finding the single cluster entry dep is very tricky and > > expensive. Let's just not do that. It's harmless in this case > > anyway. > > > > However in this case it was not harmless: it's just somewhat luck that > > the sorting happened to work for the existing cases in the testsuite. > > > > This patch solves the issue by noting any declarations that rely on deps > > first seen within their own merge key. This declaration gets marked as > > an "entry" dep; any of these deps that end up recursively referring back > > to that entry dep as part of their own merge key do not. > > > > Then within sort_cluster we can ensure that the entry dep is written to > > be streamed first of its cluster; this will ensure that any other deps > > are just emitted as back-references, and the mergeable dep itself will > > structurally decompose. > > Do you also want to adjust the comment before sort_cluster? > > > // FIXME: I am not convinced this is needed and, if needed, > > // sufficient. We emit the decls in this order but that emission > > // could walk into later decls (from the body of the decl, or default > > // arg-like things). Why doesn't that walk do the right thing? And > > // if it DTRT why do we need to sort here -- won't things naturally > > // work? I think part of the issue is that when we're going to refer > > // to an entity by name, and that entity is in the same cluster as us, > > // we need to actually walk that entity, if we've not already walked > > // it. > > OK either way. >
Thanks; I've elected to keep the comment for now (it was still helpful for me when I was also wondering about why this function was needed) but I might reword it from a FIXME later perhaps. > Jason >
