[clang] Delay marking pending incomplete decl chains until the end of `finishPendingActions`. (PR #121245)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Michael Park (mpark)

<details>
<summary>Changes</summary>

This is fix for an unreachable code path being reached, for an internal use case at Meta. I'm unfortunately still not able to reproduce a minimal example that I can share 😞

# Description

There is a defaulted constructor `_Hashtable_alloc() = default;` which ends up in [`CodeGenFunction::GenerateCode`](https://github.com/llvm/llvm-project/blob/48bf0a9457fd60d0872d9b9b4804a95c833a72e1/clang/lib/CodeGen/CodeGenFunction.cpp#L1448) and eventually calls [`FunctionProtoType::canThrow`](https://github.com/llvm/llvm-project/blob/48bf0a9457fd60d0872d9b9b4804a95c833a72e1/clang/lib/AST/Type.cpp#L3758) with `EST_Unevaluated`.

In the "good" cases I observed, I see that the decl is also loaded with the `noexcept-unevaluated` state, but it gets fixed up later by a [call to `adjustExceptionSpec`](https://github.com/llvm/llvm-project/blob/48bf0a9457fd60d0872d9b9b4804a95c833a72e1/clang/lib/Serialization/ASTReader.cpp#L10656). The update gets [added to `PendingExceptionSpecUpdates` here](https://github.com/llvm/llvm-project/blob/48bf0a9457fd60d0872d9b9b4804a95c833a72e1/clang/lib/Serialization/ASTReaderDecl.cpp#L4749-L4752).

In the "bad" case, the update does not get added to `PendingExceptionSpecUpdates` and hence the call to `adjustExceptionSpec` also does not occur.

# Observations

I made two observations in Clang Discord: [[1](https://discord.com/channels/636084430946959380/636732781086638081/1317290104431185921)] and [[2](https://discord.com/channels/636084430946959380/636732781086638081/1317291980413206608)].

1. [FinishedDeserializating](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10641) can exit with stuff still in `PendingIncompleteDeclChains`.

`FinishedDeserializing` [calls `finishPendingActions` only if `NumCurrentElementsDeserializing == 1`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10647). In [`finishPendingActions`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10028), it tries to [clear out `PendingIncompleteDeclChains`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10082-L10087). This is done in a loop, which is fine. But, later in `finishPendingActions`, it calls things like `hasBody` [here](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10296). These can call `getMostRecentDecl` / `getNextRedeclaration` that ultimately calls [`CompleteDeclChain`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L7719). `CompleteDeclChain` is "called each time we need the most recent declaration of a declaration after the generation count is incremented." according to [this comment](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/ExternalASTSource.h#L213-L215). Anyway, since we're still in `finishPendingActions` with `NumCurrentElementsDeserializing == 1`, calls to `CompleteDeclChain` simply [adds more stuff to `PendingIncompleteDeclChains`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L7725). ~~I think the emptying out of `PendingIncompleteDeclChains` should actually happen in `FinishedDeserializing`, in [this loop](https://github.com/llvm/llvm-project/blob/main/clang/lib/Serialization/ASTReader.cpp#L10657-L10658) instead.~~

2. `LazyGenerationalUpdatePtr::get` seems a bit dubious. In the `LazyData` case, it does [the following](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/ExternalASTSource.h#L482-L486):

```cpp
      if (LazyVal->LastGeneration != LazyVal->ExternalSource->getGeneration()) {
        LazyVal->LastGeneration = LazyVal->ExternalSource->getGeneration();
        (LazyVal->ExternalSource->*Update)(O);
      }
      return LazyVal->LastValue;
```

so for example, after `markIncomplete`, `LastGeneration` gets set to `0` to force the update. For example, `LazyVal->LastGeneration == 0` and `LazyVal->ExternalSource->getGeneration() == 6`. The `Update` function pointer calls `CompleteDeclChain`, which, if we're in the middle of deserialization, do nothing and just add the decl to `PendingIncompleteDeclChains`. So the update was not completed, but the `LastGeneration` is updated to `6` now... that seems potentially problematic, since subsequent calls will simply return `LazyVal->LastValue` since the generation numbers match now. I would've maybe expected something like:
```
      if (LazyVal->LastGeneration != LazyVal->ExternalSource->getGeneration() &&
          (LazyVal->ExternalSource->*Update)(O)) {
        LazyVal->LastGeneration = LazyVal->ExternalSource->getGeneration();
      }
      return LazyVal->LastValue;
```

where the generation is updated once the intended update actually happens.

# Solution

The proposed solution is to perform the marking of incomplete decl chains at the end of `finishPendingActions`. We know that calls such as `hasBody` can add entries to `PendingIncompleteDeclChains` and change the generation counter without actually performing the update. By clearing out the `PendingIncompleteDeclChains` at the end of `finishPendingActions`, we reset the generation counter to force reload post-`finishPendingActions`. It's also safe to delay this operation since any operation being done within `finishPendingActions` has `NumCurrentElementsDeserializing == 1`, which means that any calls to `CompleteDeclChain` would simply add to the `PendingIncompleteDeclChains` without doing anything anyway.

---
Full diff: https://github.com/llvm/llvm-project/pull/121245.diff


1 Files Affected:

- (modified) clang/lib/Serialization/ASTReader.cpp (+13-13) 


``````````diff
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index ec85fad3389a1c..842d00951a2675 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9807,12 +9807,12 @@ void ASTReader::visitTopLevelModuleMaps(
 }
 
 void ASTReader::finishPendingActions() {
-  while (
-      !PendingIdentifierInfos.empty() || !PendingDeducedFunctionTypes.empty() ||
-      !PendingDeducedVarTypes.empty() || !PendingIncompleteDeclChains.empty() ||
-      !PendingDeclChains.empty() || !PendingMacroIDs.empty() ||
-      !PendingDeclContextInfos.empty() || !PendingUpdateRecords.empty() ||
-      !PendingObjCExtensionIvarRedeclarations.empty()) {
+  while (!PendingIdentifierInfos.empty() ||
+         !PendingDeducedFunctionTypes.empty() ||
+         !PendingDeducedVarTypes.empty() || !PendingDeclChains.empty() ||
+         !PendingMacroIDs.empty() || !PendingDeclContextInfos.empty() ||
+         !PendingUpdateRecords.empty() ||
+         !PendingObjCExtensionIvarRedeclarations.empty()) {
     // If any identifiers with corresponding top-level declarations have
     // been loaded, load those declarations now.
     using TopLevelDeclsMap =
@@ -9860,13 +9860,6 @@ void ASTReader::finishPendingActions() {
     }
     PendingDeducedVarTypes.clear();
 
-    // For each decl chain that we wanted to complete while deserializing, mark
-    // it as "still needs to be completed".
-    for (unsigned I = 0; I != PendingIncompleteDeclChains.size(); ++I) {
-      markIncompleteDeclChain(PendingIncompleteDeclChains[I]);
-    }
-    PendingIncompleteDeclChains.clear();
-
     // Load pending declaration chains.
     for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
       loadPendingDeclChain(PendingDeclChains[I].first,
@@ -10117,6 +10110,13 @@ 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 (unsigned I = 0; I != PendingIncompleteDeclChains.size(); ++I) {
+    markIncompleteDeclChain(PendingIncompleteDeclChains[I]);
+  }
+  PendingIncompleteDeclChains.clear();
 }
 
 void ASTReader::diagnoseOdrViolations() {

``````````

</details>


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