[clang] [ASTReader] Remove assert in GetExternalDeclStmt (PR #143739)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-modules

Author: Henrik G. Olsson (hnrklssn)

<details>
<summary>Changes</summary>

This assert was reportedly added to "Defensively ensure that GetExternalDeclStmt protects itself from nested deserialization". In the tests for https://github.com/swiftlang/swift/pull/81859 I was able to trigger this assert without nested deserialization. In `FinishedDeserializing` we have this:
```
void ASTReader::FinishedDeserializing() {
  assert(NumCurrentElementsDeserializing &&
         "FinishedDeserializing not paired with StartedDeserializing");
  if (NumCurrentElementsDeserializing == 1) {
    // We decrease NumCurrentElementsDeserializing only after pending actions
    // are finished, to avoid recursively re-calling finishPendingActions().
    finishPendingActions();
  }
  --NumCurrentElementsDeserializing;
```
where `NumCurrentElementsDeserializing` is clearly 1 when calling `finishPendingActions`. Through this we end up in `loadDeclUpdateRecords`, which has:
```
in loadDeclUpdateRecords we have:
    // Check if this decl was interesting to the consumer. If we just loaded
    // the declaration, then we know it was interesting and we skip the call
    // to isConsumerInterestedIn because it is unsafe to call in the
    // current ASTReader state.
    bool WasInteresting = Record.JustLoaded || isConsumerInterestedIn(D);
```
In this case `Record.JustLoaded` is false, so we end up calling `isConsumerInterestedIn`.
In isConsumerInterestedIn we have:
```
  // An ImportDecl or VarDecl imported from a module map module will get
  // emitted when we import the relevant module.
  if (isPartOfPerModuleInitializer(D)) {
    auto *M = D->getImportedOwningModule();
    if (M && M->isModuleMapModule() &&
        getContext().DeclMustBeEmitted(D))
      return false;
  }
```
in DeclMustBeEmitted we have:
```
  // Variables that have initialization with side-effects are required.
  if (VD->getInit() && VD->getInit()->HasSideEffects(*this) &&
      // We can get a value-dependent initializer during error recovery.
      (VD->getInit()->isValueDependent() || !VD->evaluateValue()))
    return true;
```
in VarDecl::getInit we have:
```
  auto *Eval = getEvaluatedStmt();

  return cast<Expr>(Eval->Value.get(
      Eval->Value.isOffset() ? getASTContext().getExternalSource() : nullptr));
```
which ends up calling `ASTReader::GetExternalDeclStmt`.

At first I considered whether `bool WasInteresting = Record.JustLoaded || isConsumerInterestedIn(D);` should guard against calling `isConsumerInterestedIn` in even more cases, but I didn't know what that would be. Instead I tried removing the assert, and my test passed, so I assume calling `isConsumerInterestedIn` was safe in this case.

rdar://153085264

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


1 Files Affected:

- (modified) clang/lib/Serialization/ASTReader.cpp (-2) 


``````````diff
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 70b54b7296882..aaa64b06e1cee 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8310,8 +8310,6 @@ Stmt *ASTReader::GetExternalDeclStmt(uint64_t Offset) {
     Error(std::move(Err));
     return nullptr;
   }
-  assert(NumCurrentElementsDeserializing == 0 &&
-         "should not be called while already deserializing");
   Deserializing D(this);
   return ReadStmtFromStream(*Loc.F);
 }

``````````

</details>


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