https://github.com/hnrklssn created https://github.com/llvm/llvm-project/pull/143739
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 >From f5b43df377864a462f5782ef2b477eb4ed801e9e Mon Sep 17 00:00:00 2001 From: "Henrik G. Olsson" <h_ols...@apple.com> Date: Wed, 11 Jun 2025 17:36:29 +0200 Subject: [PATCH] [ASTReader] Remove assert This assert was reportedly added to "Defensively ensure that GetExternalDeclStmt protects itself from nested deserialization". Given that this triggers without nested deserialization we should remove this assert. The assert blocks https://github.com/swiftlang/swift/pull/81859 rdar://153085264 --- clang/lib/Serialization/ASTReader.cpp | 2 -- 1 file changed, 2 deletions(-) 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); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
