[Modules] fix assert in hasInitWithSideEffects #146468
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Henrik G. Olsson (hnrklssn) ChangesThis fixes another instance of rdar://154717930 Full diff: https://github.com/llvm/llvm-project/pull/146468.diff 1 Files Affected:
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 5cdf75d71e4d7..5e91566e87f74 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -2444,20 +2444,14 @@ bool VarDecl::hasInitWithSideEffects() const {
if (!hasInit())
return false;
- // Check if we can get the initializer without deserializing
- const Expr *E = nullptr;
+ // Check if we can get the initializer directly without an external source
if (auto *S = dyn_cast<Stmt *>(Init)) {
- E = cast<Expr>(S);
- } else {
- E = cast_or_null<Expr>(getEvaluatedStmt()->Value.getWithoutDeserializing());
- }
-
- if (E)
+ const Expr *E = cast<Expr>(S);
return E->HasSideEffects(getASTContext()) &&
// We can get a value-dependent initializer during error recovery.
(E->isValueDependent() || !evaluateValue());
+ }
- assert(getEvaluatedStmt()->Value.isOffset());
// ASTReader tracks this without having to deserialize the initializer
if (auto Source = getASTContext().getExternalSource())
return Source->hasInitializerWithSideEffects(this);
|
ChuanqiXu9
reviewed
Jul 1, 2025
Yup, working on it |
hnrklssn
force-pushed
the
fix-hasInitWithSideEffects
branch
from
49b4689 to
610416f
Compare
hnrklssn
changed the title
|
Found a reduced test case, at last |
ChuanqiXu9
approved these changes
Jul 3, 2025
hnrklssn
force-pushed
the
fix-hasInitWithSideEffects
branch
from
610416f to
39fa1a4
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
All deserialized VarDecl initializers are EvaluatedStmt, but not all EvaluatedStmt initializers are from a PCH. Calling `VarDecl::hasInitWithSideEffects` can trigger constant evaluation, but it's hard to know ahead of time whether that will trigger deserialization - even if the initializer is fully deserialized, it may contain a call to a constructor whose body is not deserialized. By caching the result of `VarDecl::hasInitWithSideEffects` and populating that cache during deserialization we can guarantee that calling it won't trigger deserialization regardless of the state of the initializer. This also reduces memory usage by removing the `InitSideEffectVars` set in `ASTReader`. rdar://154717930
hnrklssn
force-pushed
the
fix-hasInitWithSideEffects
branch
from
39fa1a4 to
c7d7419
Compare
|
@ChuanqiXu9 After investigating the test failures for my previous fix I realised I'd misunderstood |
hnrklssn
added a commit
to hnrklssn/llvm-project
that referenced
this pull request
Jul 3, 2025
All deserialized VarDecl initializers are EvaluatedStmt, but not all EvaluatedStmt initializers are from a PCH. Calling `VarDecl::hasInitWithSideEffects` can trigger constant evaluation, but it's hard to know ahead of time whether that will trigger deserialization - even if the initializer is fully deserialized, it may contain a call to a constructor whose body is not deserialized. By caching the result of `VarDecl::hasInitWithSideEffects` and populating that cache during deserialization we can guarantee that calling it won't trigger deserialization regardless of the state of the initializer. This also reduces memory usage by removing the `InitSideEffectVars` set in `ASTReader`. rdar://154717930 (cherry picked from commit 2910c24)
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/20434 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/211/builds/216 Here is the relevant piece of the build log for the reference |
hnrklssn
added a commit
to hnrklssn/llvm-project
that referenced
this pull request
Jul 7, 2025
All deserialized VarDecl initializers are EvaluatedStmt, but not all EvaluatedStmt initializers are from a PCH. Calling `VarDecl::hasInitWithSideEffects` can trigger constant evaluation, but it's hard to know ahead of time whether that will trigger deserialization - even if the initializer is fully deserialized, it may contain a call to a constructor whose body is not deserialized. By caching the result of `VarDecl::hasInitWithSideEffects` and populating that cache during deserialization we can guarantee that calling it won't trigger deserialization regardless of the state of the initializer. This also reduces memory usage by removing the `InitSideEffectVars` set in `ASTReader`. rdar://154717930 (cherry picked from commit 2910c24)
hokein
pushed a commit
to hokein/llvm-project
that referenced
this pull request
Jul 10, 2025
All deserialized VarDecl initializers are EvaluatedStmt, but not all EvaluatedStmt initializers are from a PCH. Calling `VarDecl::hasInitWithSideEffects` can trigger constant evaluation, but it's hard to know ahead of time whether that will trigger deserialization - even if the initializer is fully deserialized, it may contain a call to a constructor whose body is not deserialized. By caching the result of `VarDecl::hasInitWithSideEffects` and populating that cache during deserialization we can guarantee that calling it won't trigger deserialization regardless of the state of the initializer. This also reduces memory usage by removing the `InitSideEffectVars` set in `ASTReader`. rdar://154717930
hokein
pushed a commit
to hokein/llvm-project
that referenced
this pull request
Jul 16, 2025
All deserialized VarDecl initializers are EvaluatedStmt, but not all EvaluatedStmt initializers are from a PCH. Calling `VarDecl::hasInitWithSideEffects` can trigger constant evaluation, but it's hard to know ahead of time whether that will trigger deserialization - even if the initializer is fully deserialized, it may contain a call to a constructor whose body is not deserialized. By caching the result of `VarDecl::hasInitWithSideEffects` and populating that cache during deserialization we can guarantee that calling it won't trigger deserialization regardless of the state of the initializer. This also reduces memory usage by removing the `InitSideEffectVars` set in `ASTReader`. rdar://154717930
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes another instance of
Assertion failed: (NumCurrentElementsDeserializing == 0 && "should not be called while already deserializing"). I ran into it while importing clang modules from Swift, but I haven't been able to reproduce it in a test case yet. The error seems to be that an initializer expression can be deserialized and contain a call to a function whose function body is not yet deserialized. WhenevaluateValue()is called the constexpr evaluation triggers deserialization of the function body.rdar://154717930