[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 &amp;&amp;
         "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-&gt;getImportedOwningModule();
    if (M &amp;&amp; M-&gt;isModuleMapModule() &amp;&amp;
        getContext().DeclMustBeEmitted(D))
      return false;
  }
```
in DeclMustBeEmitted we have:
```
  // Variables that have initialization with side-effects are required.
  if (VD-&gt;getInit() &amp;&amp; VD-&gt;getInit()-&gt;HasSideEffects(*this) 
&amp;&amp;
      // We can get a value-dependent initializer during error recovery.
      (VD-&gt;getInit()-&gt;isValueDependent() || !VD-&gt;evaluateValue()))
    return true;
```
in VarDecl::getInit we have:
```
  auto *Eval = getEvaluatedStmt();

  return cast&lt;Expr&gt;(Eval-&gt;Value.get(
      Eval-&gt;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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits