Skip to content

Cherry-pick "[Modules] Record whether VarDecl initializers contain side effects (#143739)" #10925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

hnrklssn
Copy link

Calling DeclMustBeEmitted should not lead to more deserialization, as
it may occur before previous deserialization has finished.
When passed a VarDecl with an initializer however, DeclMustBeEmitted
needs to know whether that initializer contains side effects. When the
VarDecl is deserialized but the initializer is not, this triggers
deserialization of the initializer. To avoid this we add a bit to the
serialization format for VarDecls, indicating whether its initializer
contains side effects or not, so that the ASTReader can query this
information directly without deserializing the initializer.

rdar://153085264

This is a cherry-pick of 319a51a

This unblocks swiftlang/swift#81859

hnrklssn and others added 2 commits June 28, 2025 16:47
…lvm#143739)

Calling `DeclMustBeEmitted` should not lead to more deserialization, as
it may occur before previous deserialization has finished.
When passed a `VarDecl` with an initializer however, `DeclMustBeEmitted`
needs to know whether that initializer contains side effects. When the
`VarDecl` is deserialized but the initializer is not, this triggers
deserialization of the initializer. To avoid this we add a bit to the
serialization format for `VarDecl`s, indicating whether its initializer
contains side effects or not, so that the `ASTReader` can query this
information directly without deserializing the initializer.

rdar://153085264

This is a cherry-pick of 319a51a
Normally expressions passed to EvaluateInPlace have a type, but not when
a VarDecl initializer is evaluated before the untyped ParenListExpr is
replaced with a CXXConstructExpr. This can happen in LLDB. In these
cases consteval fails.
@hnrklssn hnrklssn requested a review from a team as a code owner June 28, 2025 23:53
@hnrklssn
Copy link
Author

@swift-ci please test

@hnrklssn
Copy link
Author

@swift-ci please test llvm

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks safe to me

@@ -2529,12 +2531,12 @@ void ASTWriter::WriteDeclAbbrevs() {
// VarDecl
Abv->Add(BitCodeAbbrevOp(
BitCodeAbbrevOp::Fixed,
21)); // Packed Var Decl bits: Linkage, ModulesCodegen,
22)); // Packed Var Decl bits: Linkage, ModulesCodegen,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect this kind of change to also involve an update to VERSION_MAJOR, but it seems that doesn't happen much for Clang serialization changes? If it's not needed, that's fine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we've ever written down a policy for this, and we've been inconsistent about it in the past. In general, we don't try to read a pch/module if getClangFullRepositoryVersion doesn't match, and we check that in the same record that we check VERSION_MAJOR. @Bigcheese or @jansvoboda11 any opinion?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've always treated Clang as needing an exact commit hash match to consider modules as valid. This probably should have updated VERSION_MAJOR, but also I don't think it's a problem for us if it doesn't.

@ravikandhadai ravikandhadai merged commit 144f270 into swiftlang:swift/release/6.2 Jun 30, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants