[Clang] Instantiate functions from constant evaluation. - #205557
Conversation
| isConstexpr() ? | ||
| Init->EvaluateAsMandatedConstantInitializer(EStatus, Ctx, *SP, this) | ||
| : Init->EvaluateAsInitializer(Ctx, this, EStatus, | ||
| IsConstantInitialization); |
There was a problem hiding this comment.
Just passing a SemaProxy when there is one seems much easier...
There was a problem hiding this comment.
Yeah, I wasn't happy how this particular line ended up either.
I pursued a design in which entrance to the constant evaluator is always via either
- an evaluation function that does not affect the AST, which has no capacity to accept a
SemaProxy, or - an evaluation function that can affect the AST, which must accept a
SemaProxy.
Open to changing that design if others dislike how this ended up.
| CurrentInstantiationScope(nullptr), NonInstantiationEntries(0), | ||
| ArgPackSubstIndex(std::nullopt), SatisfactionCache(Context) { | ||
| ArgPackSubstIndex(std::nullopt), SatisfactionCache(Context), | ||
| ProxyForEval(makeProxyForEval(*this)) { |
There was a problem hiding this comment.
This would probably explode if the proxy is used before init - you probably rely on no one doing that/ Maybe that's fine
There was a problem hiding this comment.
Yeah - if you expand the diff upwards, there's precedent for passing *this to the initializers of other Sema subobjects. Seemed okay.
| SemaProxy *SP = Info.getSemaProxy(); | ||
| if (SP && FunctionDefinitionCanBeLazilyInstantiated(FD) && | ||
| Info.InConstantContext) | ||
| SP->instantiateFunctionDefinition(Loc, const_cast<FunctionDecl *>(FD)); | ||
| } | ||
|
|
There was a problem hiding this comment.
So, in your approach, do we still need Info.InConstantContext or is that true by construction and it should be an assert?
There was a problem hiding this comment.
Good question - judging by the comment
Whether or not we're in a context where the front end requires a constant value.
we could probably get away with changing this to some State::inConstantContext() const that returns whether State::Sema is nullptr. I'd be fine either way.
What about external consumers of AST - we don't expect them to be able to pass a Sema, but maybe they still want to evaluate expressions in a constant context? Sort of at the edge of my clang knowledge here, so apologies if that's nonsense.
There was a problem hiding this comment.
Well, Sema is a tool to construct the AST - once the AST is complete, Sema disappear.
Consumer of the AST get an immutable AST.
I do not think it will ever be possible to change that without very big redesign of the whole compiler.
i.e calling define_aggregate from lldb's expression evaluator will probably never work.
And I think that's perfectly fine.
But from there, it's not clear to me that State::Sema should imply inConstantContext() ( - Maybe it makes sense in your model. Does that mean we will never want to modify some sema state on overflow or constant folding? i don't know. I lack imagination either way
There was a problem hiding this comment.
I don't think it makes sense to allow evaluation in a constant context without a Sema object. Instantiating function definitions is just the tip of the iceberg here -- reflection support adds a whole host of additional AST mutation operations that evaluation in a constant context can perform. The other option would be to add some kind of "evaluate in a constant context but without Sema" mode, that is not used by anything in-tree and would presumably bit-rot as a consequence. Speaking as a maintainer of an external project that uses Clang as a library (the Carbon toolchain) and does want to evaluate expressions, the burden of having a live Sema object to do this, for us, is just not a big deal.
The opposite direction, of allowing there to be a Sema proxy when not in a constant context, just seems fundamentally error-prone. Allowing that is precisely why I was concerned about prior approaches here -- it makes it too easy for code that just wants to perform a side-effect-free to result in leaking persistent side-effects into type-checking. I don't think there's no use cases that would benefit, but I do think the risk is likely to outweigh the benefit.
I think it would make sense to remove InConstantContext and instead have the invariant that we are in a constant context if and only if we have a proxy. I think we should also rename the proxy (SemaProxy is very vague) to something more like SemaConstantContextProxy to make it clearer that it encapsulates the things that Sema can be asked to do when evaluating in a constant context.
There was a problem hiding this comment.
As we discuss removing InConstantContext, it would be helpful for me to understand how "stable" the API surface area of the constant evaluator must remain - what do we promise external users of these APIs? In particular:
- Is it acceptable to remove the
InConstantContextparameter from e.g.,EvaluateAsRValue(which could break external users of that function)? - Is it acceptable to add to the set of overloads for a given name - for instance, would it be okay to add a new member function also named
EvaluateAsRValuethat accepts an additionalSemaProxyargument (which could break external users that do&Expr::EvaluateAsRValue)?
Answers here will inform how the shape of these new constant evaluator "entry points" might look.
There was a problem hiding this comment.
We don't promise the C++ API is stable; you can refactor it if you want.
| // when the specialization is referenced in a context that requires a function | ||
| // definition to exist or if the existence of the definition affects the | ||
| // semantics of the program. | ||
| if (FunctionDefinitionCanBeLazilyInstantiated(Fn) && S.inConstantContext()) { |
There was a problem hiding this comment.
I made the same comment elsewhere, do we still need S.inConstantContext() ?
cor3ntin
added
clang:frontend
llvmorg-github-actions
Bot
added
the
clang
|
@llvm/pr-subscribers-clang Author: Daniel M. Katz (katzdm) ChangesThis PR is a rework of #173537, which was a rework of #115168; the sole goal remains to address #73232. I used #173537 as a starting point (for both code and tests), but attempted to follow the alternative design requested in the comments therein. A quick design overview:
The tests are mostly from #173537, although I've added some to more explicitly cover certain kinds of manifestly constant-evaluated expressions (e.g., immediate invocations). I commented one test from #173537 that fails an assertion due to an unrelated issue (i.e., #199347). #17537 included some other changes to existing tests due to changes to diagnostic messages; I was able to avoid these changes by checking whether the template of a specialization has a definition, prior to trying to instantiate it (so the diagnostic continues to refer to "undefined function As with #173537, this PR neither addresses #59966 nor provides scaffolding for Reflection, though the scaffolding introduced here will help to address both use-cases. In particular, the availability of It was brought to my attention that the Clang 23 branch is scheduled for a few weeks from now; seeing as this is a fairly significant architectural change, I have no intention to target that release. Patch is 65.01 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/205557.diff 27 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 42c5dc16ea2e1..d27ba186a98df 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -804,6 +804,7 @@ Bug Fixes to C++ Support
- Fixed a crash in constant evaluation using placement new on an array which was later initialized. (#GH196450)
- Fixed an issue where Clang incorrectly accepted invalid unqualified uses of local nested class names outside their declaring scope. (#GH184622)
- Fixed a crash when parsing invalid friend declaration with storage-class specifier. (#GH186569)
+- Instantiate constexpr functions as needed before they are evaluated. (#GH73232) (#GH35052) (#GH100897)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index a4ed852d36442..6c157cc8755a5 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -129,6 +129,7 @@ class ParentMapContext;
struct ParsedTargetAttr;
class Preprocessor;
class ProfileList;
+class SemaProxy;
class StoredDeclsMap;
class TargetAttr;
class TargetInfo;
@@ -808,7 +809,7 @@ class ASTContext : public RefCountedBase<ASTContext> {
ASTMutationListener *Listener = nullptr;
/// Returns the clang bytecode interpreter context.
- interp::Context &getInterpContext() const;
+ interp::Context &getInterpContext(SemaProxy *Sema) const;
struct CUDAConstantEvalContext {
/// Do not allow wrong-sided variables in constant expressions.
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index e200b8f06ec4b..8c3effac13255 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -70,6 +70,7 @@ class Module;
class NamespaceDecl;
class ParmVarDecl;
class RecordDecl;
+class SemaProxy;
class Stmt;
class StringLiteral;
class TagDecl;
@@ -1435,6 +1436,7 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
private:
APValue *evaluateValueImpl(SmallVectorImpl<PartialDiagnosticAt> *Notes,
+ SemaProxy *SP,
bool IsConstantInitialization) const;
public:
@@ -1451,6 +1453,15 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
/// not.
bool evaluateDestruction(SmallVectorImpl<PartialDiagnosticAt> &Notes) const;
+ /// Evaluate the destruction of this variable, the destruction of which is
+ /// required by language rules to be constant.
+ ///
+ /// \pre hasConstantInitialization()
+ /// \return \c true if this variable has constant destruction, \c false if
+ /// not.
+ bool evaluateMandatedConstantDestruction(
+ SmallVectorImpl<PartialDiagnosticAt> &Notes, SemaProxy &SP) const;
+
/// Determine whether this variable has constant initialization.
///
/// This is only set in two cases: when the language semantics require
@@ -1468,7 +1479,7 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
/// constant initializer. Should only be called once, after completing the
/// definition of the variable.
bool checkForConstantInitialization(
- SmallVectorImpl<PartialDiagnosticAt> &Notes) const;
+ SmallVectorImpl<PartialDiagnosticAt> &Notes, SemaProxy *SP) const;
void setInitStyle(InitializationStyle Style) {
VarDeclBits.InitStyle = Style;
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 7c94c4d35641c..6ca1eb96b651a 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -55,6 +55,7 @@ namespace clang {
class ObjCPropertyRefExpr;
class OpaqueValueExpr;
class ParmVarDecl;
+ class SemaProxy;
class StringLiteral;
class TargetInfo;
class ValueDecl;
@@ -667,6 +668,13 @@ class Expr : public ValueStmt {
bool EvaluateAsRValue(EvalResult &Result, const ASTContext &Ctx,
bool InConstantContext = false) const;
+ /// Evaluate an expression that is required by the language to be a constant
+ /// expression, and fold the resulting rvalue constant into Result. If the
+ /// expression is a glvalue, an lvalue-to-rvalue conversion will be applied.
+ bool EvaluateAsMandatedConstantRValue(EvalResult &Result,
+ const ASTContext &Ctx,
+ SemaProxy &SP) const;
+
/// EvaluateAsBooleanCondition - Return true if this is a constant
/// which we can fold and convert to a boolean condition using
/// any crazy technique that we want to, even if the expression has
@@ -743,6 +751,12 @@ class Expr : public ValueStmt {
EvalResult &Result,
bool IsConstantInitializer) const;
+ /// Evaluate an expression that is required by the language to be a constant
+ /// expression.
+ bool EvaluateAsMandatedConstantInitializer(
+ EvalResult &Result, const ASTContext &Ctx, SemaProxy &SP,
+ const VarDecl *VD) const;
+
/// EvaluateWithSubstitution - Evaluate an expression as if from the context
/// of a call to the given function with the given arguments, inside an
/// unevaluated context. Returns true if the expression could be folded to a
@@ -773,6 +787,12 @@ class Expr : public ValueStmt {
EvalResult &Result, const ASTContext &Ctx,
ConstantExprKind Kind = ConstantExprKind::Normal) const;
+ /// Evaluate an expression that is required by the language to be a constant
+ /// expression.
+ bool EvaluateAsMandatedConstantExpr(
+ EvalResult &Result, const ASTContext &Ctx, SemaProxy &SP,
+ ConstantExprKind Kind = ConstantExprKind::Normal) const;
+
/// If the current Expr is a pointer, this will try to statically
/// determine the number of bytes available where the pointer is pointing.
/// Returns true if all of the above holds and we were able to figure out the
diff --git a/clang/include/clang/AST/SemaProxy.h b/clang/include/clang/AST/SemaProxy.h
new file mode 100644
index 0000000000000..ab6a87f1e32a5
--- /dev/null
+++ b/clang/include/clang/AST/SemaProxy.h
@@ -0,0 +1,40 @@
+//===--- SemaProxy.h - Interface to language semantics ---------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the SemaProxy interface, used during language-mandated
+// constant evaluation to act on, and query, the representation of the program
+// according to language-defined semantics.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_AST_SEMA_PROXY_H
+#define LLVM_CLANG_AST_SEMA_PROXY_H
+
+#include "clang/Basic/SourceLocation.h"
+
+namespace clang {
+
+class FunctionDecl;
+
+/// Classes implementing SemaProxy present a restricted view of the (possibly
+/// mutating) actions and queries defined by language semantics against the
+/// representation of the program (i.e., the AST). Such a view is required in
+/// order to evaluate certain expressions (e.g., C++'s manifestly
+/// constant-evaluated expressions) according to language rules.
+class SemaProxy {
+public:
+ virtual ~SemaProxy() = default;
+
+ virtual void
+ instantiateFunctionDefinition(SourceLocation PointOfInstantiation,
+ FunctionDecl *Function) = 0;
+};
+
+} // end namespace clang
+
+#endif // LLVM_CLANG_AST_SEMA_PROXY_H
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index b8d760e7e0975..97b951bf6d672 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -30,6 +30,7 @@
#include "clang/AST/ExternalASTSource.h"
#include "clang/AST/NestedNameSpecifier.h"
#include "clang/AST/OperationKinds.h"
+#include "clang/AST/SemaProxy.h"
#include "clang/AST/StmtCXX.h"
#include "clang/AST/Type.h"
#include "clang/AST/TypeLoc.h"
@@ -907,6 +908,7 @@ class Sema final : public SemaBase {
// 33. Types (SemaType.cpp)
// 34. FixIt Helpers (SemaFixItUtils.cpp)
// 35. Function Effects (SemaFunctionEffects.cpp)
+ // 36. Language-Mandated Constant Evaluation (SemaEval.cpp)
/// \name Semantic Analysis
/// Implementations are in Sema.cpp
@@ -15811,6 +15813,30 @@ class Sema final : public SemaBase {
void performFunctionEffectAnalysis(TranslationUnitDecl *TU);
///@}
+
+ //
+ //
+ // -------------------------------------------------------------------------
+ //
+ //
+
+ /// \name Language-Mandated Constant Evaluation
+ /// Implementations are in SemaEval.cpp
+ ///@{
+public:
+ SemaProxy &getProxyForEval() const {
+ assert(ProxyForEval);
+ return *ProxyForEval;
+ }
+
+private:
+ std::unique_ptr<SemaProxy> ProxyForEval;
+
+ static SemaProxy *makeProxyForEval(Sema &SemaRef);
+
+ ///@}
+public:
+
};
DeductionFailureInfo
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index abf0cd5e18c2b..f58e7c394ad18 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -40,6 +40,7 @@
#include "clang/AST/ParentMapContext.h"
#include "clang/AST/RawCommentList.h"
#include "clang/AST/RecordLayout.h"
+#include "clang/AST/SemaProxy.h"
#include "clang/AST/Stmt.h"
#include "clang/AST/TemplateBase.h"
#include "clang/AST/TemplateName.h"
@@ -901,9 +902,10 @@ CXXABI *ASTContext::createCXXABI(const TargetInfo &T) {
llvm_unreachable("Invalid CXXABI type!");
}
-interp::Context &ASTContext::getInterpContext() const {
+interp::Context &ASTContext::getInterpContext(SemaProxy *Sema) const {
if (!InterpContext) {
- InterpContext.reset(new interp::Context(const_cast<ASTContext &>(*this)));
+ InterpContext.reset(
+ new interp::Context(const_cast<ASTContext &>(*this), Sema));
}
return *InterpContext;
}
diff --git a/clang/lib/AST/ByteCode/Context.cpp b/clang/lib/AST/ByteCode/Context.cpp
index 4beb35a9a7b43..eaf077a3eae92 100644
--- a/clang/lib/AST/ByteCode/Context.cpp
+++ b/clang/lib/AST/ByteCode/Context.cpp
@@ -26,7 +26,8 @@
using namespace clang;
using namespace clang::interp;
-Context::Context(ASTContext &Ctx) : Ctx(Ctx), P(new Program(*this)) {
+Context::Context(ASTContext &Ctx, SemaProxy *Sema)
+ : Ctx(Ctx), Sema(Sema), P(new Program(*this)) {
this->ShortWidth = Ctx.getTargetInfo().getShortWidth();
this->IntWidth = Ctx.getTargetInfo().getIntWidth();
this->LongWidth = Ctx.getTargetInfo().getLongWidth();
diff --git a/clang/lib/AST/ByteCode/Context.h b/clang/lib/AST/ByteCode/Context.h
index 789f72ae34f73..085883b1d729d 100644
--- a/clang/lib/AST/ByteCode/Context.h
+++ b/clang/lib/AST/ByteCode/Context.h
@@ -25,6 +25,7 @@ class FunctionDecl;
class VarDecl;
class APValue;
class BlockExpr;
+class SemaProxy;
namespace interp {
class Function;
@@ -47,7 +48,7 @@ class EvalIDScope;
class Context final {
public:
/// Initialises the constexpr VM.
- explicit Context(ASTContext &Ctx);
+ explicit Context(ASTContext &Ctx, SemaProxy *Sema);
/// Cleans up the constexpr VM.
~Context();
@@ -99,6 +100,8 @@ class Context final {
/// Returns the AST context.
ASTContext &getASTContext() const { return Ctx; }
+ /// Returns the (possibly null) pointer to the language semantics proxy.
+ SemaProxy *getSemaProxy() const { return Sema; }
/// Returns the language options.
const LangOptions &getLangOpts() const;
/// Returns CHAR_BIT.
@@ -191,6 +194,8 @@ class Context final {
/// Current compilation context.
ASTContext &Ctx;
+ /// Current proxy to language semantics.
+ SemaProxy *Sema;
/// Interpreter stack, shared across invocations.
InterpStack Stk;
/// Constexpr program.
diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp
index 3772def47408f..13de84a26eedc 100644
--- a/clang/lib/AST/ByteCode/Interp.cpp
+++ b/clang/lib/AST/ByteCode/Interp.cpp
@@ -21,6 +21,7 @@
#include "clang/AST/DeclObjC.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/SemaProxy.h"
#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/TargetInfo.h"
#include "llvm/ADT/StringExtras.h"
@@ -1776,15 +1777,28 @@ bool CheckBitCast(InterpState &S, CodePtr OpPC, const Type *TargetType,
return true;
}
-static void compileFunction(InterpState &S, const Function *Func) {
- const FunctionDecl *Definition;
- if (!Func->getDecl()->getBody(Definition))
- return;
- if (!Definition)
+static void compileFunction(InterpState &S, const Function *Func,
+ CodePtr OpPC) {
+ const FunctionDecl *Fn = Func->getDecl();
+
+ // [C++26] [temp.inst] p5
+ // [...] the function template specialization is implicitly instantiated
+ // when the specialization is referenced in a context that requires a function
+ // definition to exist or if the existence of the definition affects the
+ // semantics of the program.
+ if (FunctionDefinitionCanBeLazilyInstantiated(Fn) && S.inConstantContext()) {
+ SemaProxy *SP = S.getSemaProxy();
+ if (!SP)
+ return;
+ SP->instantiateFunctionDefinition(S.Current->getLocation(OpPC),
+ const_cast<FunctionDecl *>(Fn));
+ }
+ Fn = Fn->getDefinition();
+ if (!Fn)
return;
Compiler<ByteCodeEmitter>(S.getContext(), S.P)
- .compileFunc(Definition, const_cast<Function *>(Func));
+ .compileFunc(Fn, const_cast<Function *>(Func));
}
bool CallVar(InterpState &S, CodePtr OpPC, const Function *Func,
@@ -1811,7 +1825,7 @@ bool CallVar(InterpState &S, CodePtr OpPC, const Function *Func,
}
if (!Func->isFullyCompiled())
- compileFunction(S, Func);
+ compileFunction(S, Func, OpPC);
if (!CheckCallable(S, OpPC, Func))
return false;
@@ -1898,7 +1912,7 @@ bool Call(InterpState &S, CodePtr OpPC, const Function *Func,
}
if (!Func->isFullyCompiled())
- compileFunction(S, Func);
+ compileFunction(S, Func, OpPC);
if (!CheckCallable(S, OpPC, Func))
return cleanup();
@@ -2316,7 +2330,7 @@ bool CallPtr(InterpState &S, CodePtr OpPC, uint32_t ArgSize,
// because the Call/CallVirt below might access the instance pointer
// but the Function's information about them is wrong.
if (!F->isFullyCompiled())
- compileFunction(S, F);
+ compileFunction(S, F, OpPC);
if (!CheckCallable(S, OpPC, F))
return false;
diff --git a/clang/lib/AST/ByteCode/InterpState.cpp b/clang/lib/AST/ByteCode/InterpState.cpp
index 2d6ed98e6b52c..ac6d45df2b9e6 100644
--- a/clang/lib/AST/ByteCode/InterpState.cpp
+++ b/clang/lib/AST/ByteCode/InterpState.cpp
@@ -19,9 +19,9 @@ using namespace clang::interp;
InterpState::InterpState(const State &Parent, Program &P, InterpStack &Stk,
Context &Ctx, SourceMapper *M)
- : State(Ctx.getASTContext(), Parent.getEvalStatus()), M(M), P(P), Stk(Stk),
- Ctx(Ctx), BottomFrame(*this), Current(&BottomFrame),
- StepsLeft(Ctx.getLangOpts().ConstexprStepLimit),
+ : State(Ctx.getASTContext(), Parent.getSemaProxy(), Parent.getEvalStatus()),
+ M(M), P(P), Stk(Stk), Ctx(Ctx), BottomFrame(*this),
+ Current(&BottomFrame), StepsLeft(Ctx.getLangOpts().ConstexprStepLimit),
InfiniteSteps(StepsLeft == 0), EvalID(Ctx.getEvalID()) {
InConstantContext = Parent.InConstantContext;
CheckingPotentialConstantExpression =
@@ -32,9 +32,9 @@ InterpState::InterpState(const State &Parent, Program &P, InterpStack &Stk,
InterpState::InterpState(const State &Parent, Program &P, InterpStack &Stk,
Context &Ctx, const Function *Func)
- : State(Ctx.getASTContext(), Parent.getEvalStatus()), M(nullptr), P(P),
- Stk(Stk), Ctx(Ctx), BottomFrame(*this), Current(&BottomFrame),
- StepsLeft(Ctx.getLangOpts().ConstexprStepLimit),
+ : State(Ctx.getASTContext(), Parent.getSemaProxy(), Parent.getEvalStatus()),
+ M(nullptr), P(P), Stk(Stk), Ctx(Ctx), BottomFrame(*this),
+ Current(&BottomFrame), StepsLeft(Ctx.getLangOpts().ConstexprStepLimit),
InfiniteSteps(StepsLeft == 0), EvalID(Ctx.getEvalID()) {
InConstantContext = Parent.InConstantContext;
CheckingPotentialConstantExpression =
diff --git a/clang/lib/AST/ByteCode/State.h b/clang/lib/AST/ByteCode/State.h
index df3afdf8cbc24..e9e94d4ffeb9b 100644
--- a/clang/lib/AST/ByteCode/State.h
+++ b/clang/lib/AST/ByteCode/State.h
@@ -20,6 +20,7 @@
namespace clang {
class OptionalDiagnostic;
+class SemaProxy;
/// Kinds of access we can perform on an object, for diagnostics. Note that
/// we consider a member function call to be a kind of access, even though
@@ -80,8 +81,8 @@ class SourceInfo;
/// Interface for the VM to interact with the AST walker's context.
class State {
public:
- State(ASTContext &ASTCtx, Expr::EvalStatus &EvalStatus)
- : Ctx(ASTCtx), EvalStatus(EvalStatus) {}
+ State(ASTContext &ASTCtx, SemaProxy *Sema, Expr::EvalStatus &EvalStatus)
+ : Ctx(ASTCtx), Sema(Sema), EvalStatus(EvalStatus) {}
virtual ~State();
virtual const Frame *getCurrentFrame() = 0;
@@ -90,6 +91,7 @@ class State {
Expr::EvalStatus &getEvalStatus() const { return EvalStatus; }
ASTContext &getASTContext() const { return Ctx; }
+ SemaProxy *getSemaProxy() const { return Sema; }
const LangOptions &getLangOpts() const { return Ctx.getLangOpts(); }
/// Note that we have had a side-effect, and determine whether we should
@@ -188,6 +190,7 @@ class State {
EvaluationMode EvalMode;
ASTContext &Ctx;
+ SemaProxy *Sema;
Expr::EvalStatus &EvalStatus;
private:
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 7ab4235717dde..0382a26f0f692 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -32,6 +32,7 @@
#include "clang/AST/Randstruct.h"
#include "clang/AST/RecordLayout.h"
#include "clang/AST/Redeclarable.h"
+#include "clang/AST/SemaProxy.h"
#include "clang/AST/Stmt.h"
#include "clang/AST/TemplateBase.h"
#include "clang/AST/Type.h"
@@ -2552,10 +2553,12 @@ EvaluatedStmt *VarDecl::getEvaluatedStmt() const {
}
APValue *VarDecl::evaluateValue() const {
- return evaluateValueImpl(/*Notes=*/nullptr, hasConstantInitialization());
+ return evaluateValueImpl(/*Notes=*/nullptr, /*Sema=*/nullptr,
+ hasConstantInitialization());
}
APValue *VarDecl::evaluateValueImpl(SmallVectorImpl<PartialDiagnosticAt> *Notes,
+ SemaProxy *SP,
bool IsConstantInitialization) const {
EvaluatedStmt *Eval = ensureEvaluatedStmt();
@@ -2579,7 +2582,10 @@ APValue *VarDecl::evaluateValueImpl(SmallVectorImpl<PartialDiagnosticAt> *Notes,
Expr::EvalResult EStatus;
EStatus.Diag = Notes;
bool Result =
- Init->EvaluateAsInitializer(Ctx, this, EStatus, IsConstantInitialization);
+ isConstexpr() ?
+ Init->EvaluateAsMandatedConstantInitializer(EStatus, Ctx, *SP, this)
+ : Init->EvaluateAsInitializer(Ctx, this, EStatus,
+ IsConstantInitialization);
Eval->Evaluated = std::move(EStatus.Val);
// In C++, or in C23 if we're initialising a 'constexpr' variable, this isn't
@@ -2643,7 +2649,7 @@ bool VarDecl::hasConstantInitialization() const {
}
bool VarDecl::checkForConstantInitialization(
- SmallVectorImpl<PartialDiagnosticAt> &Notes) const {
+ SmallVectorImpl<PartialDiagnosticAt> &Notes, SemaProxy *SP) const {
EvaluatedStmt *Eval = ensureEvaluatedStmt();
// If we ask for the value before we know whether we have a constant
// initializer, we can compute the wrong value (for example, due to
@@ -2658,7 +2664,7 @@ bool VarDecl::checkForConstantInitialization(
// Evaluate the initializer to check whether it's a constant expression.
Eval->HasConstantInitialization =
- evaluateValueImpl(&Notes, true) && Notes.empty();
+ evaluateValueImpl(&Notes, SP, true) && Notes.empty();
// If evaluation as a constant initializer failed, allow re-evaluation as a
// non-constant initializer if we later find we want the value.
diff --git a/clang/lib/AST/ExprConstShared.h b/clang/lib/AST/ExprConstShared.h
index 619c79a1408f3..45c7db846fc70 100644
--- a/clang/lib/AST/ExprConstShared.h
+++ b/clang/lib/AST/ExprConstShared.h
@@ -29,6 +29,7 @@ class LangOptions;
class ASTContext;
class CharUni...
[truncated]
|
2074fb5 to
67c2baf
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Co-authored-by: Corentin Jabot <corentinjabot@gmail.com>
Also change pointer to ref in checkForConstantInitialization.
1e30611 to
2b5b4a1
Compare
|
A few open questions for reviewers:
Second - Now that the suggested API is in front of us - how do we feel about its shape? It achieves the "bright line visible at every call" that can mutate the AST (as asked for by @zygoloid). It's also nice that it avoids changes to the existing APIs (e.g., One other idea could be to pursue something like what @cor3ntin tried (i.e., shove the // e.g., SemaDecl.cpp
bool HasConstInit;
SmallVector<PartialDiagnosticAt, 8> Notes;
{
ManifestlyConstantEvaluatedContext mcce(Context, getProxyForEval());
HasConstInit = var->checkForConstantInitialization(Notes);
}What I like about this direction is that it ties the call-site more closely to the language reason for allowing side-effects: The expression is manifestly constant-evaluated, and it is clear from the call-site that we're evaluating it as such. My concern with the existing direction is that identifying which evaluations are side-effectual requires remembering which 8 or so evaluation-functions allow side-effects, and which don't - which feels like a high-ish cognitive load. Finding side-effectual evaluations could instead be a matter of greping for Alternatively, we could tie this into the existing // e.g., SemaDecl.cpp
bool HasConstInit;
SmallVector<PartialDiagnosticAt, 8> Notes;
{
EnterExpressionEvaluationContext(*this, ExpressionEvaluationContext::ManifestlyConstant);
HasConstInit = var->checkForConstantInitialization(Notes);
}Something like this was admittedly also present in Corentin's design (which checked for Happy to stick with the existing direction or change course, whatever folks feel is best. |
zygoloid
left a comment
zygoloid
left a comment
There was a problem hiding this comment.
The approach here makes sense to me, and addresses my concerns with prior approaches for this issue. Thanks!
| SemaProxy *SP = Info.getSemaProxy(); | ||
| if (SP && FunctionDefinitionCanBeLazilyInstantiated(FD) && | ||
| Info.InConstantContext) | ||
| SP->instantiateFunctionDefinition(Loc, const_cast<FunctionDecl *>(FD)); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think it makes sense to allow evaluation in a constant context without a Sema object. Instantiating function definitions is just the tip of the iceberg here -- reflection support adds a whole host of additional AST mutation operations that evaluation in a constant context can perform. The other option would be to add some kind of "evaluate in a constant context but without Sema" mode, that is not used by anything in-tree and would presumably bit-rot as a consequence. Speaking as a maintainer of an external project that uses Clang as a library (the Carbon toolchain) and does want to evaluate expressions, the burden of having a live Sema object to do this, for us, is just not a big deal.
The opposite direction, of allowing there to be a Sema proxy when not in a constant context, just seems fundamentally error-prone. Allowing that is precisely why I was concerned about prior approaches here -- it makes it too easy for code that just wants to perform a side-effect-free to result in leaking persistent side-effects into type-checking. I don't think there's no use cases that would benefit, but I do think the risk is likely to outweigh the benefit.
I think it would make sense to remove InConstantContext and instead have the invariant that we are in a constant context if and only if we have a proxy. I think we should also rename the proxy (SemaProxy is very vague) to something more like SemaConstantContextProxy to make it clearer that it encapsulates the things that Sema can be asked to do when evaluating in a constant context.
…ates (#209693) This fixes another case of member functions where we overlooked template depths when only abbreviated template parameters are involved. This mirrors previous fix cfb2520, but I don't intend to put it in ParseTrailingRequiresClause because we might want the similar fix for e.g. noexcept expressions, so let's keep it inline for future refactor. The example comes from #205557.
…ates (llvm#209693) This fixes another case of member functions where we overlooked template depths when only abbreviated template parameters are involved. This mirrors previous fix cfb2520, but I don't intend to put it in ParseTrailingRequiresClause because we might want the similar fix for e.g. noexcept expressions, so let's keep it inline for future refactor. The example comes from llvm#205557.
| EStatus.Diag = Notes; | ||
| bool Result = | ||
| Init->EvaluateAsInitializer(Ctx, this, EStatus, IsConstantInitialization); | ||
| isConstexpr() |
There was a problem hiding this comment.
isConstexpr() doesn't seem right.
Consider a global variable definition like the following:
int a = f();
If f() is constexpr, we need to instantiate it: we need the instantiated function body to determine whether the variable is constant-initialized. even though it's not actually an error if the resulting expression isn't constant. At least, that's my understanding of the rules here.
…ates (llvm#209693) This fixes another case of member functions where we overlooked template depths when only abbreviated template parameters are involved. This mirrors previous fix cfb2520, but I don't intend to put it in ParseTrailingRequiresClause because we might want the similar fix for e.g. noexcept expressions, so let's keep it inline for future refactor. The example comes from llvm#205557. (cherry picked from commit 3485d85)
|
Since, with the current PR, you only can see issues in edge-cases (e.g. where the constexpr template is defined after it's mentioned by a constexpr function), in order to better test all the behaviors, I've deleted the eager instantiation via the following patch, and then run ninja check-clang. IIUC, with a correct change here, lazy instantiation should be triggered in all cases where it's necessary, and so deleting the above should only change some of the notes around where instantiation was triggered, but not otherwise break any code. However, that's not the case. There's a lot of errors reported. Looking at them, I see at least the following seemingly-distinct issues:
|
|
@jyknight right, there are more work to be done to fix all of the issues - i do not think it needs to be in that PR - they are related but distinct issues |
…ates (llvm#209693) This fixes another case of member functions where we overlooked template depths when only abbreviated template parameters are involved. This mirrors previous fix cfb2520, but I don't intend to put it in ParseTrailingRequiresClause because we might want the similar fix for e.g. noexcept expressions, so let's keep it inline for future refactor. The example comes from llvm#205557. (cherry picked from commit 3485d85)
|
It shouldn't all be in the same PR, sure, but do we need to fix the issues before we enable this by default? It sounds like the errors could be triggered by code which previously compiled successfully. |
|
I don't really agree they are distinct issues. (Or, at least, the ones I mentioned don't seem so. Certainly there could also be some independent issues in there). While the diff disabling eager instantiation I used for testing doesn't necessarily need to be part of this PR, I believe the failures detected by testing with it are pointing out actual bugs which this PR has as-is. And that carefully crafting some slightly more-complex test-cases would also show those same failures, without that diff. This PR's intent is to instantiate constexpr functions on-demand when it's required. It adds new APIs and updates all the appropriate callers to use those new APIs. Except: it's only updated some of the required callers, not all of them. We could submit without addressing the bugs in this PR first, but ISTM it probably isn't that hard (by using this testing strategy), to find and update the rest of the identified callers that need to be able to do instantiation, to call the new API too? Also, the "friend" failure might be pointing out that the instantiation code in TryInstantiateFunctionBeforeCall is buggy by trying to instantiate the friend-decl instead of the actual definition, or something like that. (Does it need to search the redecls chain to find the right thing to instantiate or something? I don't really know.) |
|
We are trying to establish the mechanism to instantiate functions/mutate the ast from constant evaluation - which turn out to be more contentious than anyone hoped for. I would prefer we focus on that, and then fix the code you commented. That code was always load bearing to a series of workaround. I agree it should go. But maybe we can make incremental progress? it's the 3rd PR in 3 years trying to gain consensus here. |
|
Can we put this behind a flag for now? I don't want to block progress here, but it also seems likely to cause problems, and I don't want it bouncing in and out of the tree. |
I think a flag makes a fair amount of sense, particularly because I think we're still going to find corner cases that need adjusting. I think it'd make sense for the flag to start out as opt-in, then once we think we're ready, transition to opt-out, and hopefully we can eventually deprecate and remove the flag. |
|
I'd really, really like to see the lazy-instantiation feature land, but it'll only be useful once it's actually working. Any method to most-efficiently get to that final working state is fine with me...whatever the maintainers would like. I'm not sure this PR would actually break any compilations which work today, since currently we fail to build anything which would require instantiation during constant evaluation. With this PR, we will accept some such code that would've previously failed, but continue to reject (or potentially crash the compiler) on other such code. So, from that POV, it might be fine to submit this in the current half-way state, without a flag. Though such inconsistency in behavior doesn't really seem great: it'd be pretty difficult to explain to users why things seemingly-arbitrarily sometimes work and other times don't. But, I do think it's also probably useful to at least do initial triage on the discovered errors, to see whether the assumptions made here are valid or not. For example: So, apparently we're supposed to validate that it is "a constant expression" but in some weird special way, pretending we're not doing constant evaluation? That feels REALLY darn sketchy to me, but OK... Anyways, if we still buy the logic discussed on that issue, presumably for this oddball case (and are there any other cases like it?), we must be able to trigger template instantiation (and thus provide a SemaProxy) while simultaneously setting |
If you write something like the following: This triggers constant evaluation of f(), but it's not an error if constant evaluation fails. So if the attempted constant evaluation triggers an error or crash, that's a problem. Not sure how likely that is in practice. |
|
@AaronBallman @efriedma-quic can you expand on the set of breakages you expect?
Yes, that's the intent
This is not a constant context - and also we do not want that behavior in constant initialization, so in that case we should not instantiate consider for example https://compiler-explorer.com/z/sdv153zx8 - that's not something we want to break - even though the standard is not clear on that point. (similarly, this PR does not instantiate on constant folding or any context where the standard does not require f() to have been instantiated) |
I don't have a code example at hand, just painful life experience that makes me conservative: if this can cause instantiations to happen at a different point in the TU, it's likely to break code somewhere because it seems like most times we modify point-of-instantiation, it has that kind of subtle fallout. So my concerns are likely more FUD at the moment. |
From the standard, "the initializer of a variable that is usable in constant expressions or has constant initialization" is manifestly constant-evaluated. (And a variable has constant initialization if it's constant-initialized, and it's constant-initialized if it's constant-initializable.) So it is a constant context as long as the evaluation succeeds, which means we need to perform instantiation to see whether the evaluation is successful. |
8 participants
This PR is a rework of #173537, which was a rework of #115168; the sole goal remains to address #73232. I used #173537 as a starting point (for both code and tests), but attempted to follow the alternative design requested in the comments therein.
A quick design overview:
SemaProxyinterface is introduced toAST, which defines a set of (possibly mutable) semantic operations that can be triggered by the evaluation of certain expressions; this class finds its implementation inSema/SemaEval.cpp.Semacreates a uniqueSemaProxyto itself at construction time, which is made available viaSema::getproxyForEval().Expr::EvaluateAsConstantExpr) has no access toSemaProxy, and therefore cannot act on the AST: instead, variants of these functions (e.g.,Expr::EvaluateAsMandatedConstantExpr) are introduced, which accept aSemaProxy &. This makes clear from the call-side whether an operation can or cannot act on the AST (according to language semantic rules).SemaProxyis stored ininterp::State; this isnullptrif evaluation began through a non-Mandatedevaluation function (again, e.g.,EvaluateAsConstantExpr). The proxy, if present, is leveraged during evaluation to call back through toSema(e.g., to instantiate a specialization of a templated function).Expr::EvaluateAsMandatedConstant*forms of evaluation. I'd appreciate help from reviewers in auditing whether I've made the correct choices here.The tests are mostly from #173537, although I've added some to more explicitly cover certain kinds of manifestly constant-evaluated expressions (e.g., immediate invocations). I commented one test from #173537 that fails an assertion due to an unrelated issue (i.e., #199347). #17537 included some other changes to existing tests due to changes to diagnostic messages; I was able to avoid these changes by checking whether the template of a specialization has a definition, prior to trying to instantiate it (so the diagnostic continues to refer to "undefined function
f<int>", rather than to an explicit specialization).As with #173537, this PR neither addresses #59966 nor provides scaffolding for Reflection, though the scaffolding introduced here will help to address both use-cases. In particular, the availability of
Semathrough the constant evaluator will unblock the implementation of many of C++26 Reflection's metafunctions, which can explicitly act on the AST during evaluation.It was brought to my attention that the Clang 23 branch is scheduled for a few weeks from now; seeing as this is a fairly significant architectural change, I have no intention to target that release.