Skip to content

[Modules] Record whether VarDecl initializers contain side effects #143739

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

Merged
merged 2 commits into from
Jun 23, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clang/include/clang/AST/Decl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1351,6 +1351,11 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
return const_cast<VarDecl *>(this)->getInitializingDeclaration();
}

/// Checks whether this declaration has an initializer with side effects,
/// without triggering deserialization if the initializer is not yet
/// deserialized.
bool hasInitWithSideEffects() const;

/// Determine whether this variable's value might be usable in a
/// constant expression, according to the relevant language standard.
/// This only checks properties of the declaration, and does not check
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/AST/ExternalASTSource.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ class RecordDecl;
class Selector;
class Stmt;
class TagDecl;
class VarDecl;

/// Abstract interface for external sources of AST nodes.
///
Expand Down Expand Up @@ -195,6 +196,10 @@ class ExternalASTSource : public RefCountedBase<ExternalASTSource> {
/// module.
virtual bool wasThisDeclarationADefinition(const FunctionDecl *FD);

virtual bool hasInitializerWithSideEffects(const VarDecl *VD) const {
return false;
}

/// Finds all declarations lexically contained within the given
/// DeclContext, after applying an optional filter predicate.
///
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Serialization/ASTReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -1442,6 +1442,10 @@ class ASTReader
const StringRef &operator*() && = delete;
};

/// VarDecls with initializers containing side effects must be emitted,
/// but DeclMustBeEmitted is not allowed to deserialize the intializer.
llvm::SmallPtrSet<Decl *, 16> InitSideEffectVars;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea!

Copy link
Member

Choose a reason for hiding this comment

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

I think we can erase elements from InitSideEffectVars if the initializer of the variable got loaded. This is helpful for memory. But given I feel it might not be significant, so if you don't want to do it right now, please leave a FIXME.


public:
/// Get the buffer for resolving paths.
SmallString<0> &getPathBuf() { return PathBuf; }
Expand Down Expand Up @@ -2392,6 +2396,8 @@ class ASTReader

bool wasThisDeclarationADefinition(const FunctionDecl *FD) override;

bool hasInitializerWithSideEffects(const VarDecl *VD) const override;

/// Retrieve a selector from the given module with its local ID
/// number.
Selector getLocalSelector(ModuleFile &M, unsigned LocalID);
Expand Down
4 changes: 1 addition & 3 deletions clang/lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12889,9 +12889,7 @@ bool ASTContext::DeclMustBeEmitted(const Decl *D) {
return true;

// 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()))
if (VD->hasInitWithSideEffects())
return true;

// Likewise, variables with tuple-like bindings are required if their
Expand Down
25 changes: 25 additions & 0 deletions clang/lib/AST/Decl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2434,6 +2434,31 @@ VarDecl *VarDecl::getInitializingDeclaration() {
return Def;
}

bool VarDecl::hasInitWithSideEffects() const {
if (!hasInit())
return false;

// Check if we can get the initializer without deserializing
const Expr *E = nullptr;
if (auto *S = dyn_cast<Stmt *>(Init)) {
E = cast<Expr>(S);
} else {
auto *Eval = getEvaluatedStmt();
if (!Eval->Value.isOffset())
E = cast<Expr>(Eval->Value.get(nullptr));
}

if (E)
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
return getASTContext().getExternalSource()->hasInitializerWithSideEffects(
Copy link
Member

Choose a reason for hiding this comment

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

We don't assume getASTContext().getExternalSource() exists generally. So,

if (getASTContext().getExternalSource())
   return getASTContext().getExternalSource()->hasInitializerWithSideEffects(

return false;

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with this change, but I'm curious if you know of any case when hasInit() && !isa<Stmt>(Init) && !getASTContext().getExternalSource() would be true?

Copy link
Member

Choose a reason for hiding this comment

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

Currently I don't know but I prefer to guard getASTContext().getExternalSource() generally, it is consistent and easier to maintain. I don't think the additional branch is critical.

this);
}

bool VarDecl::isOutOfLine() const {
if (Decl::isOutOfLine())
return true;
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/Serialization/ASTReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9712,6 +9712,10 @@ bool ASTReader::wasThisDeclarationADefinition(const FunctionDecl *FD) {
return ThisDeclarationWasADefinitionSet.contains(FD);
}

bool ASTReader::hasInitializerWithSideEffects(const VarDecl *VD) const {
return InitSideEffectVars.count(VD);
}

Selector ASTReader::getLocalSelector(ModuleFile &M, unsigned LocalID) {
return DecodeSelector(getGlobalSelectorID(M, LocalID));
}
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/Serialization/ASTReaderDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1632,6 +1632,10 @@ RedeclarableResult ASTDeclReader::VisitVarDeclImpl(VarDecl *VD) {
VD->NonParmVarDeclBits.PreviousDeclInSameBlockScope =
VarDeclBits.getNextBit();

bool HasInitWithSideEffect = VarDeclBits.getNextBit();
if (HasInitWithSideEffect)
Reader.InitSideEffectVars.insert(VD);

VD->NonParmVarDeclBits.EscapingByref = VarDeclBits.getNextBit();
HasDeducedType = VarDeclBits.getNextBit();
VD->NonParmVarDeclBits.ImplicitParamKind =
Expand Down
14 changes: 8 additions & 6 deletions clang/lib/Serialization/ASTWriterDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1259,6 +1259,7 @@ void ASTDeclWriter::VisitVarDecl(VarDecl *D) {
VarDeclBits.addBit(D->isConstexpr());
VarDeclBits.addBit(D->isInitCapture());
VarDeclBits.addBit(D->isPreviousDeclInSameBlockScope());
VarDeclBits.addBit(D->hasInitWithSideEffects());

VarDeclBits.addBit(D->isEscapingByref());
HasDeducedType = D->getType()->getContainedDeducedType();
Expand Down Expand Up @@ -1308,10 +1309,11 @@ void ASTDeclWriter::VisitVarDecl(VarDecl *D) {
!D->hasExtInfo() && D->getFirstDecl() == D->getMostRecentDecl() &&
D->getKind() == Decl::Var && !D->isInline() && !D->isConstexpr() &&
!D->isInitCapture() && !D->isPreviousDeclInSameBlockScope() &&
!D->isEscapingByref() && !HasDeducedType &&
D->getStorageDuration() != SD_Static && !D->getDescribedVarTemplate() &&
!D->getMemberSpecializationInfo() && !D->isObjCForDecl() &&
!isa<ImplicitParamDecl>(D) && !D->isEscapingByref())
!D->hasInitWithSideEffects() && !D->isEscapingByref() &&
!HasDeducedType && D->getStorageDuration() != SD_Static &&
!D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
!D->isObjCForDecl() && !isa<ImplicitParamDecl>(D) &&
!D->isEscapingByref())
AbbrevToUse = Writer.getDeclVarAbbrev();

Code = serialization::DECL_VAR;
Expand Down Expand Up @@ -2693,12 +2695,12 @@ void ASTWriter::WriteDeclAbbrevs() {
// VarDecl
Abv->Add(BitCodeAbbrevOp(
BitCodeAbbrevOp::Fixed,
21)); // Packed Var Decl bits: Linkage, ModulesCodegen,
22)); // Packed Var Decl bits: Linkage, ModulesCodegen,
// SClass, TSCSpec, InitStyle,
// isARCPseudoStrong, IsThisDeclarationADemotedDefinition,
// isExceptionVariable, isNRVOVariable, isCXXForRangeDecl,
// isInline, isInlineSpecified, isConstexpr,
// isInitCapture, isPrevDeclInSameScope,
// isInitCapture, isPrevDeclInSameScope, hasInitWithSideEffects,
// EscapingByref, HasDeducedType, ImplicitParamKind, isObjCForDecl
Abv->Add(BitCodeAbbrevOp(0)); // VarKind (local enum)
// Type Source Info
Expand Down
37 changes: 37 additions & 0 deletions clang/test/Modules/var-init-side-effects.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Tests referencing variable with initializer containing side effect across module boundary
//
// RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file %s %t

// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/Foo.cppm \
// RUN: -o %t/Foo.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface \
// RUN: -fmodule-file=Foo=%t/Foo.pcm \
// RUN: %t/Bar.cppm \
// RUN: -o %t/Bar.pcm

// RUN: %clang_cc1 -std=c++20 -emit-obj \
// RUN: -main-file-name Bar.cppm \
// RUN: -fmodule-file=Foo=%t/Foo.pcm \
// RUN: -x pcm %t/Bar.pcm \
// RUN: -o %t/Bar.o

//--- Foo.cppm
export module Foo;

export {
class S {};

inline S s = S{};
}

//--- Bar.cppm
export module Bar;
import Foo;

S bar() {
return s;
}