[Clang] Instantiate constexpr function when they are needed.#173537
[Clang] Instantiate constexpr function when they are needed.#173537
Conversation
|
@llvm/pr-subscribers-clang Author: Corentin Jabot (cor3ntin) ChangesThis is a rework of #115168, with the sole aim of fixing #73232. The scaffolding is introduced here is necessary - but not sufficient for reflection. Reading the comments on #115168, I think we need to separate multiple concerns:
This does not fix #59966, as the change is complicated enough that i want to explore it in a separate PR. Fixes #73232 Patch is 20.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/173537.diff 14 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2319ff13f7864..2718dd61c139d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -331,8 +331,8 @@ Non-comprehensive list of changes in this release
allocator-level heap organization strategies. A feature to instrument all
allocation functions with a token ID can be enabled via the
``-fsanitize=alloc-token`` flag.
-
-- A new generic byte swap builtin function ``__builtin_bswapg`` that extends the existing
+
+- A new generic byte swap builtin function ``__builtin_bswapg`` that extends the existing
__builtin_bswap{16,32,64} function family to support all standard integer types.
- A builtin ``__builtin_infer_alloc_token(<args>, ...)`` is provided to allow
@@ -491,12 +491,12 @@ Improvements to Clang's diagnostics
Objective-C method and block declarations when calling format functions. It is part
of the format-nonliteral diagnostic (#GH60718)
-- Fixed a crash when enabling ``-fdiagnostics-format=sarif`` and the output
+- Fixed a crash when enabling ``-fdiagnostics-format=sarif`` and the output
carries messages like 'In file included from ...' or 'In module ...'.
Now the include/import locations are written into `sarif.run.result.relatedLocations`.
-- Clang now generates a fix-it for C++20 designated initializers when the
- initializers do not match the declaration order in the structure.
+- Clang now generates a fix-it for C++20 designated initializers when the
+ initializers do not match the declaration order in the structure.
Improvements to Clang's time-trace
----------------------------------
@@ -592,6 +592,7 @@ Bug Fixes to C++ Support
- Fixed a bug where our ``member-like constrained friend`` checking caused an incorrect analysis of lambda captures. (#GH156225)
- Fixed a crash when implicit conversions from initialize list to arrays of
unknown bound during constant evaluation. (#GH151716)
+- Instantiate constexpr functions as needed before they are evaluated. (#GH73232)
- Support the dynamic_cast to final class optimization with pointer
authentication enabled. (#GH152601)
- Fix the check for narrowing int-to-float conversions, so that they are detected in
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 68205dd1c1fd9..206e482237342 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -44,6 +44,7 @@
#include "llvm/ADT/StringSet.h"
#include "llvm/ADT/TinyPtrVector.h"
#include "llvm/Support/TypeSize.h"
+#include <memory>
#include <optional>
namespace llvm {
@@ -215,6 +216,16 @@ struct TypeInfoChars {
}
};
+/// Interface that allows constant evaluator to call Sema
+/// and mutate the AST.
+struct SemaProxy {
+ virtual ~SemaProxy() = default;
+
+ virtual void
+ InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
+ FunctionDecl *Function) = 0;
+};
+
/// Holds long-lived AST nodes (such as types and decls) that can be
/// referred to throughout the semantic analysis of a file.
class ASTContext : public RefCountedBase<ASTContext> {
@@ -786,6 +797,8 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// Keeps track of the deallocated DeclListNodes for future reuse.
DeclListNode *ListNodeFreeList = nullptr;
+ std::unique_ptr<SemaProxy> SemaProxyPtr;
+
public:
IdentifierTable &Idents;
SelectorTable &Selectors;
@@ -1409,6 +1422,10 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// with this AST context, if any.
ASTMutationListener *getASTMutationListener() const { return Listener; }
+ SemaProxy *getSemaProxy();
+
+ void setSemaProxy(std::unique_ptr<SemaProxy> Proxy);
+
void PrintStats() const;
const SmallVectorImpl<Type *>& getTypes() const { return Types; }
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index c9ad6860dc625..bd1277a271906 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -909,6 +909,9 @@ class Sema final : public SemaBase {
/// initialized but before it parses anything.
void Initialize();
+ void RegisterSemaProxy();
+ void UnregisterSemaProxy();
+
/// This virtual key function only exists to limit the emission of debug info
/// describing the Sema class. GCC and Clang only emit debug info for a class
/// with a vtable when the vtable is emitted. Sema is final and not
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index f52470a4d7458..a2768a9dc06b1 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -926,6 +926,13 @@ ASTContext::setExternalSource(IntrusiveRefCntPtr<ExternalASTSource> Source) {
ExternalSource = std::move(Source);
}
+SemaProxy *ASTContext::getSemaProxy() { return SemaProxyPtr.get(); }
+
+void ASTContext::setSemaProxy(std::unique_ptr<SemaProxy> Proxy) {
+ assert((!SemaProxyPtr || !Proxy) && "SemaProxy already set");
+ SemaProxyPtr = std::move(Proxy);
+}
+
void ASTContext::PrintStats() const {
llvm::errs() << "\n*** AST Context Stats:\n";
llvm::errs() << " " << Types.size() << " types total.\n";
diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp
index 889ac1e1a9a7e..f8e10ec2c9d39 100644
--- a/clang/lib/AST/ByteCode/Interp.cpp
+++ b/clang/lib/AST/ByteCode/Interp.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "Interp.h"
+#include "ByteCode/Source.h"
#include "Compiler.h"
#include "Function.h"
#include "InterpFrame.h"
@@ -1526,13 +1527,30 @@ bool CheckFunctionDecl(InterpState &S, CodePtr OpPC, const FunctionDecl *FD) {
return diagnoseCallableDecl(S, OpPC, FD);
}
-static void compileFunction(InterpState &S, const Function *Func) {
- const FunctionDecl *Definition = Func->getDecl()->getDefinition();
- if (!Definition)
+static void compileFunction(InterpState &S, const Function *Func,
+ SourceLocation Loc) {
+
+ 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 (!Fn->isDefined() && Fn->isImplicitlyInstantiable() && Fn->isConstexpr() &&
+ S.inConstantContext() && !S.TryConstantInitialization &&
+ !S.checkingPotentialConstantExpression()) {
+ SemaProxy *SP = S.getASTContext().getSemaProxy();
+ if (!SP)
+ return;
+ SP->InstantiateFunctionDefinition(Loc, 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,
@@ -1558,7 +1576,7 @@ bool CallVar(InterpState &S, CodePtr OpPC, const Function *Func,
}
if (!Func->isFullyCompiled())
- compileFunction(S, Func);
+ compileFunction(S, Func, S.Current->getLocation(OpPC));
if (!CheckCallable(S, OpPC, Func))
return false;
@@ -1630,7 +1648,7 @@ bool Call(InterpState &S, CodePtr OpPC, const Function *Func,
}
if (!Func->isFullyCompiled())
- compileFunction(S, Func);
+ compileFunction(S, Func, S.Current->getLocation(OpPC));
if (!CheckCallable(S, OpPC, Func))
return cleanup();
diff --git a/clang/lib/AST/ByteCode/InterpState.cpp b/clang/lib/AST/ByteCode/InterpState.cpp
index a95916cd63981..ff201cb6151b1 100644
--- a/clang/lib/AST/ByteCode/InterpState.cpp
+++ b/clang/lib/AST/ByteCode/InterpState.cpp
@@ -22,6 +22,7 @@ InterpState::InterpState(State &Parent, Program &P, InterpStack &Stk,
: Parent(Parent), M(M), P(P), Stk(Stk), Ctx(Ctx), BottomFrame(*this),
Current(&BottomFrame) {
InConstantContext = Parent.InConstantContext;
+ TryConstantInitialization = Parent.TryConstantInitialization;
CheckingPotentialConstantExpression =
Parent.CheckingPotentialConstantExpression;
CheckingForUndefinedBehavior = Parent.CheckingForUndefinedBehavior;
@@ -34,6 +35,7 @@ InterpState::InterpState(State &Parent, Program &P, InterpStack &Stk,
BottomFrame(*this, Func, nullptr, CodePtr(), Func->getArgSize()),
Current(&BottomFrame) {
InConstantContext = Parent.InConstantContext;
+ TryConstantInitialization = Parent.TryConstantInitialization;
CheckingPotentialConstantExpression =
Parent.CheckingPotentialConstantExpression;
CheckingForUndefinedBehavior = Parent.CheckingForUndefinedBehavior;
diff --git a/clang/lib/AST/ByteCode/State.h b/clang/lib/AST/ByteCode/State.h
index 0695c61c07a05..376d120c3a9d9 100644
--- a/clang/lib/AST/ByteCode/State.h
+++ b/clang/lib/AST/ByteCode/State.h
@@ -168,6 +168,8 @@ class State {
/// is set; this is used when evaluating ICEs in C.
bool CheckingForUndefinedBehavior = false;
+ bool TryConstantInitialization = false;
+
EvaluationMode EvalMode;
private:
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 6ccd57bdc4df8..d9c1c8d6d6b8a 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -43,6 +43,7 @@
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/CharUnits.h"
#include "clang/AST/CurrentSourceLocExprScope.h"
+#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
#include "clang/AST/InferAlloc.h"
#include "clang/AST/OSLog.h"
@@ -53,6 +54,7 @@
#include "clang/AST/TypeLoc.h"
#include "clang/Basic/Builtins.h"
#include "clang/Basic/DiagnosticSema.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/TargetBuiltins.h"
#include "clang/Basic/TargetInfo.h"
#include "llvm/ADT/APFixedPoint.h"
@@ -7064,6 +7066,28 @@ static bool handleTrivialCopy(EvalInfo &Info, const ParmVarDecl *Param,
CopyObjectRepresentation);
}
+static void InstantiateFunctionBeforeCall(const FunctionDecl *FD,
+ EvalInfo &Info, SourceLocation Loc) {
+
+ // [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 (!FD->isDefined() && FD->isImplicitlyInstantiable() && FD->isConstexpr() &&
+ Info.InConstantContext && !Info.TryConstantInitialization &&
+ !Info.checkingPotentialConstantExpression()) {
+
+ SemaProxy *SP = Info.getASTContext().getSemaProxy();
+ // Try to instantiate the definition if Sema is available
+ // (i.e during the initial parse of the TU).
+ if (SP) {
+ SP->InstantiateFunctionDefinition(Loc, const_cast<FunctionDecl *>(FD));
+ }
+ }
+}
+
/// Evaluate a function call.
static bool HandleFunctionCall(SourceLocation CallLoc,
const FunctionDecl *Callee,
@@ -7457,6 +7481,8 @@ static bool HandleDestructionImpl(EvalInfo &Info, SourceRange CallRange,
if (!Info.CheckCallLimit(CallRange.getBegin()))
return false;
+ InstantiateFunctionBeforeCall(DD, Info, CallRange.getBegin());
+
const FunctionDecl *Definition = nullptr;
const Stmt *Body = DD->getBody(Definition);
@@ -8922,10 +8948,13 @@ class ExprEvaluatorBase
CallScope.destroy();
}
- const FunctionDecl *Definition = nullptr;
- Stmt *Body = FD->getBody(Definition);
SourceLocation Loc = E->getExprLoc();
+ InstantiateFunctionBeforeCall(FD, Info, Loc);
+
+ const FunctionDecl *Definition = nullptr;
+ const Stmt *Body = FD->getBody(Definition);
+
// Treat the object argument as `this` when evaluating defaulted
// special menmber functions
if (FD->hasCXXExplicitFunctionObjectParameter())
@@ -11443,8 +11472,10 @@ bool RecordExprEvaluator::VisitCXXConstructExpr(const CXXConstructExpr *E,
return handleDefaultInitValue(T, Result);
}
+ InstantiateFunctionBeforeCall(FD, Info, E->getBeginLoc());
+
const FunctionDecl *Definition = nullptr;
- auto Body = FD->getBody(Definition);
+ const Stmt *Body = FD->getBody(Definition);
if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition, Body))
return false;
@@ -11484,8 +11515,9 @@ bool RecordExprEvaluator::VisitCXXInheritedCtorInitExpr(
if (FD->isInvalidDecl() || FD->getParent()->isInvalidDecl())
return false;
+ InstantiateFunctionBeforeCall(FD, Info, E->getBeginLoc());
const FunctionDecl *Definition = nullptr;
- auto Body = FD->getBody(Definition);
+ const Stmt *Body = FD->getBody(Definition);
if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition, Body))
return false;
@@ -20707,6 +20739,8 @@ bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
: EvaluationMode::ConstantFold);
Info.setEvaluatingDecl(VD, Value);
Info.InConstantContext = IsConstantInitialization;
+ Info.TryConstantInitialization =
+ !VD->isConstexpr() && !VD->hasAttr<ConstInitAttr>();
SourceLocation DeclLoc = VD->getLocation();
QualType DeclTy = VD->getType();
diff --git a/clang/lib/Interpreter/IncrementalParser.cpp b/clang/lib/Interpreter/IncrementalParser.cpp
index bf08911e23533..5a24342e73ca4 100644
--- a/clang/lib/Interpreter/IncrementalParser.cpp
+++ b/clang/lib/Interpreter/IncrementalParser.cpp
@@ -42,9 +42,13 @@ IncrementalParser::IncrementalParser(CompilerInstance &Instance,
External->StartTranslationUnit(Consumer);
P->Initialize();
+ S.RegisterSemaProxy();
}
-IncrementalParser::~IncrementalParser() { P.reset(); }
+IncrementalParser::~IncrementalParser() {
+ S.UnregisterSemaProxy();
+ P.reset();
+}
llvm::Expected<TranslationUnitDecl *>
IncrementalParser::ParseOrWrapTopLevelDecl() {
diff --git a/clang/lib/Parse/ParseAST.cpp b/clang/lib/Parse/ParseAST.cpp
index c8ee625eb57ad..1cea282f13a00 100644
--- a/clang/lib/Parse/ParseAST.cpp
+++ b/clang/lib/Parse/ParseAST.cpp
@@ -21,6 +21,7 @@
#include "clang/Sema/Sema.h"
#include "clang/Sema/SemaConsumer.h"
#include "clang/Sema/TemplateInstCallback.h"
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/Support/CrashRecoveryContext.h"
#include "llvm/Support/TimeProfiler.h"
#include <cstdio>
@@ -161,6 +162,11 @@ void clang::ParseAST(Sema &S, bool PrintStats, bool SkipFunctionBodies) {
return M;
});
P.Initialize();
+
+ auto Unregister =
+ llvm::make_scope_exit([&S]() { S.UnregisterSemaProxy(); });
+ S.RegisterSemaProxy();
+
Parser::DeclGroupPtrTy ADecl;
Sema::ModuleImportState ImportState;
EnterExpressionEvaluationContext PotentiallyEvaluated(
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 6eea8f6e9d97e..e148c23f15516 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -75,11 +75,14 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/Support/TimeProfiler.h"
+#include <memory>
#include <optional>
using namespace clang;
using namespace sema;
+static std::unique_ptr<SemaProxy> getSemaProxyImplementation(Sema &SemaRef);
+
SourceLocation Sema::getLocForEndOfToken(SourceLocation Loc, unsigned Offset) {
return Lexer::getLocForEndOfToken(Loc, Offset, SourceMgr, LangOpts);
}
@@ -622,6 +625,15 @@ Sema::~Sema() {
SemaPPCallbackHandler->reset();
}
+void Sema::RegisterSemaProxy() {
+ // Let the AST context relies on Sema for
+ // ast mutations features that require semantic analysis
+ // (lazy instantiation, reflection, etc).
+ Context.setSemaProxy(getSemaProxyImplementation(*this));
+}
+
+void Sema::UnregisterSemaProxy() { Context.setSemaProxy({}); }
+
void Sema::runWithSufficientStackSpace(SourceLocation Loc,
llvm::function_ref<void()> Fn) {
StackHandler.runWithSufficientStackSpace(Loc, Fn);
@@ -2958,3 +2970,21 @@ Attr *Sema::CreateAnnotationAttr(const ParsedAttr &AL) {
return CreateAnnotationAttr(AL, Str, Args);
}
+
+class SemaProxyImplementation final : public SemaProxy {
+private:
+ Sema &SemaRef;
+
+public:
+ SemaProxyImplementation(Sema &SemaRef) : SemaRef(SemaRef) {}
+ void InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
+ FunctionDecl *Function) override {
+ SemaRef.InstantiateFunctionDefinition(
+ PointOfInstantiation, Function, /*Recursive=*/true,
+ /*DefinitionRequired=*/true, /*AtEndOfTU=*/false);
+ }
+};
+
+std::unique_ptr<SemaProxy> getSemaProxyImplementation(Sema &SemaRef) {
+ return std::make_unique<SemaProxyImplementation>(SemaRef);
+}
diff --git a/clang/test/SemaCXX/constexpr-late-instantiation.cpp b/clang/test/SemaCXX/constexpr-late-instantiation.cpp
index 9aec0c90e61dc..59cfbfdff0e03 100644
--- a/clang/test/SemaCXX/constexpr-late-instantiation.cpp
+++ b/clang/test/SemaCXX/constexpr-late-instantiation.cpp
@@ -1,16 +1,90 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify
-// RUN: %clang_cc1 %s -fexperimental-new-constant-interpreter -fsyntax-only -verify
+// RUN: %clang_cc1 %s -std=c++14 -fsyntax-only -verify
+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -verify
+// RUN: %clang_cc1 %s -std=c++2c -fsyntax-only -verify
+
+// RUN: %clang_cc1 %s -std=c++14 -fsyntax-only -fexperimental-new-constant-interpreter -verify
+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -fexperimental-new-constant-interpreter -verify
+// RUN: %clang_cc1 %s -std=c++2c -fsyntax-only -fexperimental-new-constant-interpreter -verify
template <typename T>
-constexpr T foo(T a); // expected-note {{declared here}}
+constexpr T foo(T a); // expected-note {{explicit instantiation refers here}}
int main() {
int k = foo<int>(5); // Ok
- constexpr int j = // expected-error {{constexpr variable 'j' must be initialized by a constant expression}}
- foo<int>(5); // expected-note {{undefined function 'foo<int>' cannot be used in a constant expression}}
+ constexpr int j =
+ foo<int>(5); // expected-error {{explicit instantiation of undefined function template 'foo'}} \
+ // expected-error {{constexpr variable 'j' must be initialized by a constant expression}}
}
template <typename T>
constexpr T foo(T a) {
return a;
}
+
+
+namespace GH73232 {
+namespace ex1 {
+template <typename T>
+constexpr void g(T);
+
+constexpr int f() {
+ g(0);
+ return 0;
+}
+
+template <typename T>
+constexpr void g(T) {}
+
+constexpr auto z = f();
+}
+
+namespace ex2 {
+template <typename> constexpr static void fromType();
+
+void registerConverter() { fromType<int>(); }
+template <typename> struct QMetaTypeId {};
+template <typename T> constexpr void fromType() {
+ (void)QMetaTypeId<T>{};
+}
+template <> struct QMetaTypeId<int> {};
+} // namespace ex2
+
+namespace ex3 {
+
+#if __cplusplus > 202302L
+struct A {
+ consteval A(int i) {
+ chk(i);
+ }
+ constexpr void chk(auto) {}
+};
+A a{1};
+
+#endif
+
+}
+
+} // namespace GH73232
+
+
+namespace GH156255 {
+
+class X
+{
+public:
+ constexpr int f( int x ) const
+ {
+ return g( x );
+ }
+
+private:
+
+ template<class T>
+ constexpr T g( T x ) const
+ {
+ return x;
+ }
+};
+
+constexpr int x = X().f( 1 );
+}
diff --git a/clang/test/SemaCXX/constexpr-subobj-initialization.cpp b/clang/test/SemaCXX/constexpr-subobj-initialization.cpp
index f0252df1e2ce1..c2e51d3920f6a 100644
--- a/clang/test/SemaCXX/constexpr-subobj-initialization.cpp
+++ b/clang/test/SemaCXX/constexpr-subobj-initialization.cpp
@@ -47,12 +47,13 @@ constexpr Bar bb; // expected-error {{must be initialized by a constant expressi
template <typename Ty>
struct Baz {
- constexpr Baz(); // expected-note {{declared here}}
+ constexpr Baz(); // expected-note {{explicit instantiation refers here}}
};
struct Quux : Baz<Foo>, private Bar {
int i;
- constexpr Quux() : i(12) {} // expected-note {{undefined constructor 'Baz' cannot be used in a constant expression}}
+ constexpr Quux() : i(12) {} // expected-error {{explicit instantiation of undefined member function 'Baz' of class template 'Baz<Foo>'}} \
+ // expected-note {{subexpression not valid in a constant expression}}
};
constexpr Quux qx; // expect...
[truncated]
|
| Info.setEvaluatingDecl(VD, Value); | ||
| Info.InConstantContext = IsConstantInitialization; | ||
| Info.TryConstantInitialization = | ||
| !VD->isConstexpr() && !VD->hasAttr<ConstInitAttr>(); |
There was a problem hiding this comment.
Doesn't this mean we only do it for initializers? What if I want to call such a function in a static_assert call? Shouldn't that work too?
There was a problem hiding this comment.
It's the other way around - when trying to initialize a constant-initialized non-constexpr variable we do not want to force an instantiation (it may not yet be available or a more specialized definition may be provided later)
clang/lib/AST/ByteCode/Interp.cpp
Outdated
| if (!Fn->isDefined() && Fn->isImplicitlyInstantiable() && Fn->isConstexpr() && | ||
| S.inConstantContext() && !S.TryConstantInitialization && | ||
| !S.checkingPotentialConstantExpression()) { |
There was a problem hiding this comment.
Can you move this entire condition into a helper function somewhere (maybe make it a member of State)?
There was a problem hiding this comment.
Heh, I'm not sure I can find a nice spot for it - we do it once per constant evaluation engine.
There was a problem hiding this comment.
I commented above, I think there is value in this being TWO functions for htis condition.
ONE for hte function condition and one for the State. I'm not clever enough/nor understand it well enough to come up with good names though. BUT something where this is:
if (Fn->canBeAndNeedsConstexprEvaluationTimeInstantationOrSomethingOfThatSort() && S.evaluatingConstantExprSituationWhereWeProbablyShouldInstantiateConstexprFunctionsSometimesWhoKnowsWhatever())
might be nice.
clang/lib/AST/ExprConstant.cpp
Outdated
| // (i.e during the initial parse of the TU). | ||
| if (SP) { | ||
| SP->InstantiateFunctionDefinition(Loc, const_cast<FunctionDecl *>(FD)); | ||
| } |
There was a problem hiding this comment.
I wonder if we should have a warning for the else branch, so that users know the behavior might be wrong (generally a missed instantiation should be our bug)
There was a problem hiding this comment.
The else case is tooling (ie calling evaluate from tidy or lldb) - I don;t know if a warning make sense in that case
There was a problem hiding this comment.
I think so? For status-quo, missed instantiation would always result in failures, and having extra warnings does give toolings a chance to inspect the issue.
| return CreateAnnotationAttr(AL, Str, Args); | ||
| } | ||
|
|
||
| class SemaProxyImplementation final : public SemaProxy { |
There was a problem hiding this comment.
How about SemaProxyForConstantEvaluator?
Thanks for letting me know about these. They are fixed, I added tests |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
67c3a9a to
d136f8a
Compare
erichkeane
left a comment
There was a problem hiding this comment.
This seems reaosnable to me? Or at least as we can be for what sort of stuff this needs to do.
Though I DO wonder if the property:
!FD->isDefined() && FD->isImplicitlyInstantiable() && FD->isConstexpr()
should be a function inside of FunctionDecl with its own descriptive name. Those conditions there are pretty horrific to read.
clang/lib/AST/ExprConstant.cpp
Outdated
| CopyObjectRepresentation); | ||
| } | ||
|
|
||
| static void InstantiateFunctionBeforeCall(const FunctionDecl *FD, |
There was a problem hiding this comment.
There is perhaps a try that should be in the name of some sort.
d136f8a to
ea6211a
Compare
|
@erichkeane @Sirraide I realized we have a |
Oh I didn’t realise that that was a thing either; stgm. |
This is a rework of llvm#115168, with the sole aim of fixing llvm#73232. The scaffolding is introduced here is necessary - but not sufficient for reflection. Reading the comments on llvm#115168, I think we need to separate multiple concerns: - Do we have access to Sema (which this PR provides) - Can we mutate the AST - which is something individual callers will have to figure out. This does **not** fix llvm#59966, as the change is complicated enough that i want to explore it in a separate PR. Fixes llvm#73232
ea6211a to
95e3c7d
Compare
95e3c7d to
8daac7b
Compare
|
I had chats with both @hubert-reinterpretcast and @katzdm We concluded that this is consistent with proposed resolutions to CWG2166 and CWG2497 (for those who have access https://lists.isocpp.org/core/2025/12/19097.php). So while at a previous clang WG (late 2025?) there was hope for a solution that would preserve the immutability of constant evaluation, I think we converged toward an acceptance (with more or less reluctancy) than this shipped had sailed and therefore the solution presented here was both acceptable and forward-consistent with future tweaks for reflection. |
| /// Keeps track of the deallocated DeclListNodes for future reuse. | ||
| DeclListNode *ListNodeFreeList = nullptr; | ||
|
|
||
| std::unique_ptr<SemaProxy> SemaProxyPtr; |
There was a problem hiding this comment.
There was a problem hiding this comment.
See the description of the PR.
For reflection, we will need to pass around (or put on the stack or something) some sort of additional context (and/or modify EvaluationContext, which would be better).
It's important to separate "do we have access to sema" from "can we do that specific thing" - notably because sema can do different things some but not all of which can be valid in a particular context - here we need to check for example that we are in a constant context, not doing trial evaluation, not checking for overflow, etc
| template <typename T> | ||
| constexpr void g(T) {} | ||
|
|
||
| constexpr auto z = f(); |
There was a problem hiding this comment.
For the working code, I think comments narrating the behavior and why it should be working would be very helpful for all these additional tests. So we can see specific behavior changes and we can see any implementation decisions documents explicitly in the tests. This will help us later on when we go back and try and build any rationale back for changes.
Right now, I am just walking through the tests but I am not sure what lines are key and whether I should be surprised or not.
clang/include/clang/AST/ASTContext.h
Outdated
| SemaProxy *getSemaProxy(); | ||
|
|
||
| void setSemaProxy(std::unique_ptr<SemaProxy> Proxy); | ||
|
|
There was a problem hiding this comment.
We might want to document these two functions (like what getASTMutationListener has), as ASTContext is exposed to (out-of-tree) users.
clang/lib/AST/ASTContext.cpp
Outdated
| SemaProxy *ASTContext::getSemaProxy() { return SemaProxyPtr.get(); } | ||
|
|
||
| void ASTContext::setSemaProxy(std::unique_ptr<SemaProxy> Proxy) { | ||
| assert((!SemaProxyPtr || !Proxy) && "SemaProxy already set"); | ||
| SemaProxyPtr = std::move(Proxy); | ||
| } |
There was a problem hiding this comment.
Can we inline the getter/setter?
There was a problem hiding this comment.
setSemaProxy is only called once, but i inlined getSemaProxy, that might be beneficial indeed!
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This PR does resolve some of the errors in this area, but does not seem to resolve my previously-reported issue: With this patch applied,
Here's a test-case without library headers: |
My reading of this is:
An implementation is free to choose either, so a program (like the one above) for which the choice matters is IFNDR. Clang is choosing (1) when the function is But if we wish to pursue greater compatibility with GCC, then Clang is, of course, also free to choose (2) - it's worth noting the documented rationale (linked above) for why (1) has up until this time been preferred:
@cor3ntin - Now that the expression evaluator can call back into |
|
The plan is to do a follow up once this lands ( as mentioned in the description) |
zyn0217
left a comment
There was a problem hiding this comment.
Thanks, LGTM plus some nits.
| virtual void | ||
| instantiateFunctionDefinition(SourceLocation PointOfInstantiation, | ||
| FunctionDecl *Function) = 0; |
There was a problem hiding this comment.
Maybe it's less necessary to make it an interface?
Users are not likely going to have their own instantiation logic. And the no-op behavior is already defaulted with null SemaProxy. WDYT?
| /// with this AST context, if any. | ||
| ASTMutationListener *getASTMutationListener() const { return Listener; } | ||
|
|
||
| /// Returns a SemaProxy*, i.e an object through which interact with Sema. |
There was a problem hiding this comment.
| /// Returns a SemaProxy*, i.e an object through which interact with Sema. | |
| /// Returns a SemaProxy*, i.e. an object through which to interact with Sema. |
| /// This will return null, unless setSemaProxy has been called. | ||
| /// As Sema is only available during parsing, getSemaProxy will return null | ||
| /// during CodeGen and when using the AST from tooling. |
There was a problem hiding this comment.
| /// This will return null, unless setSemaProxy has been called. | |
| /// As Sema is only available during parsing, getSemaProxy will return null | |
| /// during CodeGen and when using the AST from tooling. | |
| /// This will return a SemaProxy if available, e.g. during parsing. | |
| /// This is currently unavailable during CodeGen or when using the AST from tooling. |
| /// during CodeGen and when using the AST from tooling. | ||
| SemaProxy *getSemaProxy() { return SemaProxyPtr.get(); } | ||
|
|
||
| /// Set the SemaProxy instance |
There was a problem hiding this comment.
| /// Set the SemaProxy instance | |
| /// Set the SemaProxy instance. |
Ah, sorry! I saw that, but I didn't understand that my example was an instance of the not-expected-to-be-fixed issue, rather than an example of the expected-to-be-fixed issue. At least for me, it was unclear what the distinguishing factors are between "fixed in this PR" vs "deferred to followup PR" were. |
This is a rework of #115168, with the sole aim of fixing #73232. The scaffolding is introduced here is necessary - but not sufficient for reflection.
Reading the comments on #115168, I think we need to separate multiple concerns:
This does not address #59966, as the change is complicated enough that i want to explore it in a separate PR.
Fixes #73232
Fixes #35052
Fixes #100897