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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
16 changes: 16 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 Expand Up @@ -429,6 +434,17 @@ struct LazyOffsetPtr {
return GetPtr();
}

/// Retrieve the pointer to the AST node that this lazy pointer points to,
/// if it can be done without triggering deserialization.
///
/// \returns a pointer to the AST node, or null if not yet deserialized.
T *getWithoutDeserializing() const {
if (isOffset()) {
return nullptr;
}
return GetPtr();
}

/// Retrieve the address of the AST node pointer. Deserializes the pointee if
/// necessary.
T **getAddressOfPointer(ExternalASTSource *Source) const {
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Sema/MultiplexExternalSemaSource.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ class MultiplexExternalSemaSource : public ExternalSemaSource {

bool wasThisDeclarationADefinition(const FunctionDecl *FD) override;

bool hasInitializerWithSideEffects(const VarDecl *VD) const override;

/// Find all declarations with the given name in the
/// given context.
bool FindExternalVisibleDeclsByName(const DeclContext *DC,
Expand Down
8 changes: 8 additions & 0 deletions clang/include/clang/Serialization/ASTReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -1442,6 +1442,12 @@ class ASTReader
const StringRef &operator*() && = delete;
};

/// VarDecls with initializers containing side effects must be emitted,
/// but DeclMustBeEmitted is not allowed to deserialize the intializer.
/// FIXME: Lower memory usage by removing VarDecls once the initializer
/// is deserialized.
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 +2398,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
24 changes: 24 additions & 0 deletions clang/lib/AST/Decl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2434,6 +2434,30 @@ 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 {
E = cast_or_null<Expr>(getEvaluatedStmt()->Value.getWithoutDeserializing());
}

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
if (auto Source = getASTContext().getExternalSource())
return Source->hasInitializerWithSideEffects(this);
return false;
}

bool VarDecl::isOutOfLine() const {
if (Decl::isOutOfLine())
return true;
Expand Down
8 changes: 8 additions & 0 deletions clang/lib/Sema/MultiplexExternalSemaSource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,14 @@ bool MultiplexExternalSemaSource::wasThisDeclarationADefinition(
return false;
}

bool MultiplexExternalSemaSource::hasInitializerWithSideEffects(
const VarDecl *VD) const {
for (const auto &S : Sources)
if (S->hasInitializerWithSideEffects(VD))
return true;
return false;
}

bool MultiplexExternalSemaSource::FindExternalVisibleDeclsByName(
const DeclContext *DC, DeclarationName Name,
const DeclContext *OriginalDC) {
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
3 changes: 3 additions & 0 deletions clang/lib/Serialization/ASTReaderDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1632,6 +1632,9 @@ RedeclarableResult ASTDeclReader::VisitVarDeclImpl(VarDecl *VD) {
VD->NonParmVarDeclBits.PreviousDeclInSameBlockScope =
VarDeclBits.getNextBit();

if (VarDeclBits.getNextBit())
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;
}