-
Notifications
You must be signed in to change notification settings - Fork 15.1k
Reland "[Modules] Record whether VarDecl initializers contain side effects" #145447
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
…fects" (llvm#145407)" This reverts commit 329ae86.
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Henrik G. Olsson (hnrklssn) ChangesOriginal commit in f2e9f88, fix triggered assert in f9272dd360b93f85b7fea6acb82d631f074200d2 Full diff: https://github.com/llvm/llvm-project/pull/145447.diff 12 Files Affected:
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 58209f4601422..0da940883b6f5 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -1352,6 +1352,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
diff --git a/clang/include/clang/AST/ExternalASTSource.h b/clang/include/clang/AST/ExternalASTSource.h
index f45e3af7602c1..e91d5132da10f 100644
--- a/clang/include/clang/AST/ExternalASTSource.h
+++ b/clang/include/clang/AST/ExternalASTSource.h
@@ -51,6 +51,7 @@ class RecordDecl;
class Selector;
class Stmt;
class TagDecl;
+class VarDecl;
/// Abstract interface for external sources of AST nodes.
///
@@ -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.
///
@@ -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 {
diff --git a/clang/include/clang/Sema/MultiplexExternalSemaSource.h b/clang/include/clang/Sema/MultiplexExternalSemaSource.h
index 391c2177d75ec..7c66c26a17a13 100644
--- a/clang/include/clang/Sema/MultiplexExternalSemaSource.h
+++ b/clang/include/clang/Sema/MultiplexExternalSemaSource.h
@@ -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,
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index be1c6e0817593..7a4b7d21bb20e 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -1455,6 +1455,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;
+
public:
/// Get the buffer for resolving paths.
SmallString<0> &getPathBuf() { return PathBuf; }
@@ -2406,6 +2412,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);
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index e851e8c3d8143..eba3c3de3d092 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -13110,9 +13110,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
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 2f01c715ae4ba..35c41859595da 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -2441,6 +2441,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;
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 18ad326942273..c19e50c9aacb2 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -16719,6 +16719,9 @@ static bool EvaluateInPlace(APValue &Result, EvalInfo &Info, const LValue &This,
const Expr *E, bool AllowNonLiteralTypes) {
assert(!E->isValueDependent());
+ if (E->getType().isNull())
+ return false;
+
if (!AllowNonLiteralTypes && !CheckLiteralType(Info, E, &This))
return false;
diff --git a/clang/lib/Sema/MultiplexExternalSemaSource.cpp b/clang/lib/Sema/MultiplexExternalSemaSource.cpp
index fbfb242598c24..9f19f13592e86 100644
--- a/clang/lib/Sema/MultiplexExternalSemaSource.cpp
+++ b/clang/lib/Sema/MultiplexExternalSemaSource.cpp
@@ -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) {
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index a3fbc3d25acab..6f082fe840b4c 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9722,6 +9722,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));
}
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 7f7882654b9d1..0ffd78424be0d 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1628,6 +1628,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 =
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 2d93832a9ac31..2e390dbe79ec6 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1306,6 +1306,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();
@@ -1355,10 +1356,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;
@@ -2731,12 +2733,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
diff --git a/clang/test/Modules/var-init-side-effects.cpp b/clang/test/Modules/var-init-side-effects.cpp
new file mode 100644
index 0000000000000..438a9eb204275
--- /dev/null
+++ b/clang/test/Modules/var-init-side-effects.cpp
@@ -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;
+}
+
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between this one and the previous PR?
The second commit, where I prevent constant evaluation if the expression type is null |
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.
f9272dd to
e8db4a4
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/12004 Here is the relevant piece of the build log for the reference |
…fects" (llvm#145447) This reverts commit 329ae86 and adds an early exit for EvaluateInPlace when the expression's type is null.
…fects" (llvm#145447) This reverts commit 329ae86 and adds an early exit for EvaluateInPlace when the expression's type is null.
|
@hnrklssn we (at google) have bisected a clang crash at the original commit (319a51a) that also reproduces at this revision. Here's the stack trace: Can you please take a look? |
Is it fixed by #146468? |
I patched #146468 after this change, rebuilt the compiler and reproduced the same crash. So I guess that patch does not fix the issue I see here. Does #146468 depend on some other change? The assertion looks similar: |
It does not. Do you have a reproducer you can share? Does the type that's being casted to |
Working to get a smaller repro. But this only reproduces for builds with modules enabled so it will take some time.
The compiler seems to choke on serializing a const member initialized with a value of the same type as the member (but the type is the result of multiple macro expansions and I didn't have yet the time to investigate thoroughly)
Not sure what you mean by this. We're using the unmodified compiler and testing its correctness on google internal code. |
Then you're likely not. I was just making sure, because the constant evaluation takes a different path if the new (experimental) constant interpreter is enabled. |
|
Here's a small repro: export module Foo;
export template <class Float>
struct Wrapper {
double value;
};
export constexpr Wrapper<double> Compute() {
return Wrapper<double>{1.0};
}
export template <typename Float>
Wrapper<Float> ComputeInFloat() {
const Wrapper<Float> a = Compute();
return a;
}it fails when assertions are on: https://gcc.godbolt.org/z/vsW69z9Pd The problem is that type of the My gut feeling is that the right fix would be to avoid running |
Thanks! Opened a PR with a fix in #147378 |
EvaluateAsInitializer does not support evaluating values with dependent types. This was previously guarded with a check for the initializer expression, but it is possible for the VarDecl to have a dependent type without the initializer having a dependent type, when the initializer is a specialized template type and the VarDecl has the unspecialized type. This adds a guard checking for dependence in the VarDecl type as well. This fixes the issue raised by Google in #145447
EvaluateAsInitializer does not support evaluating values with dependent types. This was previously guarded with a check for the initializer expression, but it is possible for the VarDecl to have a dependent type without the initializer having a dependent type, when the initializer is a specialized template type and the VarDecl has the unspecialized type. This adds a guard checking for dependence in the VarDecl type as well. This fixes the issue raised by Google in llvm#145447 (cherry picked from commit c885005)
…147378) EvaluateAsInitializer does not support evaluating values with dependent types. This was previously guarded with a check for the initializer expression, but it is possible for the VarDecl to have a dependent type without the initializer having a dependent type, when the initializer is a specialized template type and the VarDecl has the unspecialized type. This adds a guard checking for dependence in the VarDecl type as well. This fixes the issue raised by Google in llvm/llvm-project#145447
EvaluateAsInitializer does not support evaluating values with dependent types. This was previously guarded with a check for the initializer expression, but it is possible for the VarDecl to have a dependent type without the initializer having a dependent type, when the initializer is a specialized template type and the VarDecl has the unspecialized type. This adds a guard checking for dependence in the VarDecl type as well. This fixes the issue raised by Google in llvm#145447
EvaluateAsInitializer does not support evaluating values with dependent types. This was previously guarded with a check for the initializer expression, but it is possible for the VarDecl to have a dependent type without the initializer having a dependent type, when the initializer is a specialized template type and the VarDecl has the unspecialized type. This adds a guard checking for dependence in the VarDecl type as well. This fixes the issue raised by Google in llvm#145447
EvaluateAsInitializer does not support evaluating values with dependent types. This was previously guarded with a check for the initializer expression, but it is possible for the VarDecl to have a dependent type without the initializer having a dependent type, when the initializer is a specialized template type and the VarDecl has the unspecialized type. This adds a guard checking for dependence in the VarDecl type as well. This fixes the issue raised by Google in llvm#145447 (cherry picked from commit c885005)
Original commit in f2e9f88, fix triggered assert in f9272dd360b93f85b7fea6acb82d631f074200d2