[clang] [C++20][Modules] Do not update the declaration generation number if the redeclaration chain completion was delayed. (PR #129982)

[Michael Park via cfe-commits]

================
@@ -10475,6 +10468,12 @@ void ASTReader::finishPendingActions() {
   for (auto *ND : PendingMergedDefinitionsToDeduplicate)
     getContext().deduplicateMergedDefinitonsFor(ND);
   PendingMergedDefinitionsToDeduplicate.clear();
+
+  // For each decl chain that we wanted to complete while deserializing, mark
+  // it as "still needs to be completed".
+  for (Decl *D : PendingIncompleteDeclChains)
+    markIncompleteDeclChain(D);
+  PendingIncompleteDeclChains.clear();
----------------
mpark wrote:

> Sorry, I've been away from this code for a long time and have forgotten a lot about how it's supposed to work.

Not a problem at all. In trying to dig through some of the history, I've seen that a bunch of this code is from 2015 😄

> We're just marking the chains as incomplete here, not actually completing them, and we won't do the work to complete them until someone actually calls `redecls()` or similar.

What I'm uncertain about is the expectation of the parts **within** `finishPendingActions` that call `redecls()` or similar. Because for those, we **still** won't do the work to complete them. More specifically, the parts within `finishPendingActions` that say "we require the redecl chain to be fully wired" seem to me like they wouldn't have the requirement guaranteed to be satisfied.

> I would expect that when we do go and wire up the redecl chains, we're propagating the `Definition` value along the chain. Also, when chains aren't wired up properly, each declaration still does correctly point to its canonical declaration, so we will still detect things like multiple definitions for an entity because each of those will try to update the canonical declaration to have a different definition.

Ah, I see! This is great. Thanks for sharing. It's exactly this kind of high-level architectural understanding is what I'm needing more of.

> Moving the marking of pending incomplete decl chains as incomplete to the end of the function makes sense to me.

This is exactly what I had done in #121245, but unfortunately it caused #126973 so it got reverted. The reduced test case of #126973 is [pr129982.cpp](https://github.com/llvm/llvm-project/pull/129982/files#diff-972832d26c9b2be8952ac137e86510a1eaf0197f98df6245688b1c3b5303ad65).

My gathering of that situation so far is that `RD->addedMember(MD);` (near the end of `finishPendingActions`) effectively calls `RD->data()` which calls `RD->getMostRecentDecl()`. Anyway, for some reason leaving the `RD` in `PendingIncompleteDeclChains` as before is okay, but marking it incomplete caused another issue. The other issue in short:
1. There is a call to `getMostRecentDecl()` which calls `CompleteRedeclChain` which calls `DC->lookup(Name)`. During this process, the definition of `Name` is not found. [`Lookups[DC].Table[Name]`](https://github.com/llvm/llvm-project/blob/c6d95c441a29a45782ff72d6cb82839b86fd0e4a/clang/lib/Serialization/ASTReader.cpp#L8494-L8501) doesn't have the definition ID.
2. A pending update record populates `Lookups[DC].Table[Name]` with the definition ID.
3. Another call to `getMostRecentDecl()` which, if it were to call `CompleteRedeclChain` would call `DC->lookup(Name)` and find the definition. However, we updated the lazy ptr generation number in step (1), so we do not invoke `CompleteRedeclChain`, and we can't find the definition.

> Can we also add an assert that all of the other "pending" lists are still empty at that point? (Or do we already have an assert for that somewhere?)

Yes, I can certainly do that. No, we don't have that assertion currently.

https://github.com/llvm/llvm-project/pull/129982