[Modules] Record whether VarDecl initializers contain side effects

[hnrklssn]

This assert was reportedly added to "Defensively ensure that GetExternalDeclStmt protects itself from nested deserialization". In the tests for swiftlang/swift#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

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Henrik G. Olsson (hnrklssn)

Changes

This assert was reportedly added to "Defensively ensure that GetExternalDeclStmt protects itself from nested deserialization". In the tests for swiftlang/swift#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 --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);
 }

[ChuanqiXu9]

I think we shouldn't remove the assertion. Your test passes with the removal of the assertion since the initializers are not complex. So it ends quickly. But if it is a complex initialization which triggers more deserialization, I feel it will be problematic.

I think the point is in DeclMustBeEmitted, this should be a "pure" method but it triggers deserialization.

I think, the proper solution may be:

1. When we write a VarDecl, use a bit to record whether the var decl has an initialization with side effects.
2. When we read a var decl with the above information, let's record it in a set in ASTReader.
3. When we decide if a VarDecl needs to be emitted in DeclMustBeEmitted, let's lookup it in the above set.

[hnrklssn]

I think we shouldn't remove the assertion. Your test passes with the removal of the assertion since the initializers are not complex. So it ends quickly. But if it is a complex initialization which triggers more deserialization, I feel it will be problematic.

I think the point is in DeclMustBeEmitted, this should be a "pure" method but it triggers deserialization.

I think, the proper solution may be:

1. When we write a VarDecl, use a bit to record whether the var decl has an initialization with side effects.

2. When we read a var decl with the above information, let's record it in a set in ASTReader.

3. When we decide if a VarDecl needs to be emitted in `DeclMustBeEmitted`, let's lookup it in the above set.

Thanks for providing context! I'm not very familiar with this part of clang. I have a few clarifying questions:
DeclMustBeEmitted is part of ASTContext rather than ASTReader, so unless I missed something it wouldn't have access to this set. We could check the "has initializer with side effect" set in ASTReader before the call to DeclMustBeEmitted, but unless we removed that piece of code from DeclMustBeEmitted it would still result in the initializer being deserialized, right? There are other callers that would not necessarily have anASTReader in context, so I'm not sure how to go about that.

Actually, looking at the callers, one of them is isRequiredDecl, which is used to determine whether decls go into the EagerlyDeserializedDecls record. I'm not fully clear on how eagerly they are deserialized, but if the initializer is also eagerly deserialized, then maybe the ASTReader could skip calling DeclMustBeEmitted for VarDecls that aren't fully deserialized, since they would have been in the EagerlyDeserializedDecls record if DeclMustBeEmitted returned true.

Another option would be to set some bits in VarDecl::NonParmVarDeclBits rather than creating a set in ASTReader.
WDYT?

[ChuanqiXu9]

I think we shouldn't remove the assertion. Your test passes with the removal of the assertion since the initializers are not complex. So it ends quickly. But if it is a complex initialization which triggers more deserialization, I feel it will be problematic.
I think the point is in DeclMustBeEmitted, this should be a "pure" method but it triggers deserialization.
I think, the proper solution may be:

1. When we write a VarDecl, use a bit to record whether the var decl has an initialization with side effects.

2. When we read a var decl with the above information, let's record it in a set in ASTReader.

3. When we decide if a VarDecl needs to be emitted in `DeclMustBeEmitted`, let's lookup it in the above set.

Thanks for providing context! I'm not very familiar with this part of clang. I have a few clarifying questions: DeclMustBeEmitted is part of ASTContext rather than ASTReader, so unless I missed something it wouldn't have access to this set. We could check the "has initializer with side effect" set in ASTReader before the call to DeclMustBeEmitted, but unless we removed that piece of code from DeclMustBeEmitted it would still result in the initializer being deserialized, right? There are other callers that would not necessarily have anASTReader in context, so I'm not sure how to go about that.

Actually, looking at the callers, one of them is isRequiredDecl, which is used to determine whether decls go into the EagerlyDeserializedDecls record. I'm not fully clear on how eagerly they are deserialized, but if the initializer is also eagerly deserialized, then maybe the ASTReader could skip calling DeclMustBeEmitted for VarDecls that aren't fully deserialized, since they would have been in the EagerlyDeserializedDecls record if DeclMustBeEmitted returned true.

Semantically it is better to do this in DeclMustBeEmitted since this is what we're deciding.

We can relate ASTContext and ASTReader by ExternalASTSource generally. We can add an interface in ExternalASTSource and call it in ASTContext and implement it in ASTReader.

Another option would be to set some bits in VarDecl::NonParmVarDeclBits rather than creating a set in ASTReader. WDYT?

Generally we don't do this, since it will add additional cost for non-module users. Although it is cheaper and simpler for module users, generally we prefer to give non-module users higher precedence.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[hnrklssn]

@ChuanqiXu9 I pushed a new fix based on your feedback. Please let me know what you think. I don't really know how to test this, so if you think it needs testing I'm open for suggestions.

[benlangmuir]

I think this new function should be called from ASTContext::DeclMustBeEmitted, no?

[benlangmuir]

Not a big deal, but do we have any kind of data informing us how many of these we typically see?

[hnrklssn]

No idea!

[ChuanqiXu9]

The direction meets my expectation. I think you already have an existing test for swift. Maybe you can try to reduce a lit test from it.

[hnrklssn]

The direction meets my expectation. I think you already have an existing test for swift. Maybe you can try to reduce a lit test from it.

Yeah, I've tried reducing a lit test, but my reduced case doesn't trigger the original assert because the initializer is already deserialized. I'm trying to figure out what the difference is.

[hnrklssn]

The direction meets my expectation. I think you already have an existing test for swift. Maybe you can try to reduce a lit test from it.

Yeah, I've tried reducing a lit test, but my reduced case doesn't trigger the original assert because the initializer is already deserialized. I'm trying to figure out what the difference is.

Managed to create a reduced test case now that triggered the original assert, but doesn't with the current patch.

[hnrklssn]

I think this new function should be called from ASTContext::DeclMustBeEmitted, no?

Of course, yes! Fixed now.