================
@@ -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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
