[Clang] Reapply CWG2369 "Ordering between constraints and substitution" #122423
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesThe previous approach broke code generation for the MS ABI due to an unintended code path during constraint substitution. This time we address the issue by inspecting the CodeSynthesisContexts and thereby avoiding that code path. This reapplies 96eced6. Patch is 41.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/122423.diff 25 Files Affected:
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a41f16f6dc8c9b..87d9a335763e31 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13062,6 +13062,7 @@ class Sema final : public SemaBase {
///
/// \param SkipForSpecialization when specified, any template specializations
/// in a traversal would be ignored.
+ ///
/// \param ForDefaultArgumentSubstitution indicates we should continue looking
/// when encountering a specialized member function template, rather than
/// returning immediately.
@@ -13073,6 +13074,17 @@ class Sema final : public SemaBase {
bool SkipForSpecialization = false,
bool ForDefaultArgumentSubstitution = false);
+ /// Apart from storing the result to \p Result, this behaves the same as
+ /// another overload.
+ void getTemplateInstantiationArgs(
+ MultiLevelTemplateArgumentList &Result, const NamedDecl *D,
+ const DeclContext *DC = nullptr, bool Final = false,
+ std::optional<ArrayRef<TemplateArgument>> Innermost = std::nullopt,
+ bool RelativeToPrimary = false, const FunctionDecl *Pattern = nullptr,
+ bool ForConstraintInstantiation = false,
+ bool SkipForSpecialization = false,
+ bool ForDefaultArgumentSubstitution = false);
+
/// RAII object to handle the state changes required to synthesize
/// a function body.
class SynthesizedFunctionScope {
@@ -13169,6 +13181,10 @@ class Sema final : public SemaBase {
// FIXME: Should we have a similar limit for other forms of synthesis?
unsigned NonInstantiationEntries;
+ /// The number of \p CodeSynthesisContexts that are not constraint
+ /// substitution.
+ unsigned NonConstraintSubstitutionEntries;
+
/// The depth of the context stack at the point when the most recent
/// error or warning was produced.
///
@@ -13342,7 +13358,7 @@ class Sema final : public SemaBase {
ExprResult
SubstConstraintExpr(Expr *E,
const MultiLevelTemplateArgumentList &TemplateArgs);
- // Unlike the above, this does not evaluates constraints.
+ // Unlike the above, this does not evaluate constraints.
ExprResult SubstConstraintExprWithoutSatisfaction(
Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs);
@@ -13491,6 +13507,11 @@ class Sema final : public SemaBase {
return CodeSynthesisContexts.size() > NonInstantiationEntries;
}
+ /// Determine whether we are currently performing constraint substitution.
+ bool inConstraintSubstitution() const {
+ return CodeSynthesisContexts.size() > NonConstraintSubstitutionEntries;
+ }
+
using EntityPrinter = llvm::function_ref<void(llvm::raw_ostream &)>;
/// \brief create a Requirement::SubstitutionDiagnostic with only a
@@ -14463,10 +14484,10 @@ class Sema final : public SemaBase {
const MultiLevelTemplateArgumentList &TemplateArgs,
SourceRange TemplateIDRange);
- bool CheckInstantiatedFunctionTemplateConstraints(
- SourceLocation PointOfInstantiation, FunctionDecl *Decl,
- ArrayRef<TemplateArgument> TemplateArgs,
- ConstraintSatisfaction &Satisfaction);
+ bool CheckFunctionTemplateConstraints(SourceLocation PointOfInstantiation,
+ FunctionDecl *Decl,
+ ArrayRef<TemplateArgument> TemplateArgs,
+ ConstraintSatisfaction &Satisfaction);
/// \brief Emit diagnostics explaining why a constraint expression was deemed
/// unsatisfied.
diff --git a/clang/include/clang/Sema/Template.h b/clang/include/clang/Sema/Template.h
index 9800f75f676aaf..59a0575ca98036 100644
--- a/clang/include/clang/Sema/Template.h
+++ b/clang/include/clang/Sema/Template.h
@@ -522,6 +522,12 @@ enum class TemplateSubstitutionKind : char {
llvm::PointerUnion<Decl *, DeclArgumentPack *> *
findInstantiationOf(const Decl *D);
+ /// Similar to \p findInstantiationOf(), but it wouldn't assert if the
+ /// instantiation was not found within the current instantiation scope. This
+ /// is helpful for on-demand declaration instantiation.
+ llvm::PointerUnion<Decl *, DeclArgumentPack *> *
+ findInstantiationUnsafe(const Decl *D);
+
void InstantiatedLocal(const Decl *D, Decl *Inst);
void InstantiatedLocalPackArg(const Decl *D, VarDecl *Inst);
void MakeInstantiatedLocalArgPack(const Decl *D);
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index abb46d3a84e74e..b7b154e55b3db9 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -263,7 +263,8 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
TyposCorrected(0), IsBuildingRecoveryCallExpr(false), NumSFINAEErrors(0),
AccessCheckingSFINAE(false), CurrentInstantiationScope(nullptr),
InNonInstantiationSFINAEContext(false), NonInstantiationEntries(0),
- ArgumentPackSubstitutionIndex(-1), SatisfactionCache(Context) {
+ NonConstraintSubstitutionEntries(0), ArgumentPackSubstitutionIndex(-1),
+ SatisfactionCache(Context) {
assert(pp.TUKind == TUKind);
TUScope = nullptr;
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 539de00bd104f5..10f4920a761f3c 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -846,7 +846,7 @@ bool Sema::CheckFunctionConstraints(const FunctionDecl *FD,
bool ForOverloadResolution) {
// Don't check constraints if the function is dependent. Also don't check if
// this is a function template specialization, as the call to
- // CheckinstantiatedFunctionTemplateConstraints after this will check it
+ // CheckFunctionTemplateConstraints after this will check it
// better.
if (FD->isDependentContext() ||
FD->getTemplatedKind() ==
@@ -1111,12 +1111,55 @@ bool Sema::EnsureTemplateArgumentListConstraints(
return false;
}
-bool Sema::CheckInstantiatedFunctionTemplateConstraints(
+static bool CheckFunctionConstraintsWithoutInstantiation(
+ Sema &SemaRef, SourceLocation PointOfInstantiation,
+ FunctionTemplateDecl *Template, ArrayRef<TemplateArgument> TemplateArgs,
+ ConstraintSatisfaction &Satisfaction) {
+ SmallVector<const Expr *, 3> TemplateAC;
+ Template->getAssociatedConstraints(TemplateAC);
+ if (TemplateAC.empty()) {
+ Satisfaction.IsSatisfied = true;
+ return false;
+ }
+
+ LocalInstantiationScope Scope(SemaRef);
+
+ FunctionDecl *FD = Template->getTemplatedDecl();
+ // Collect the list of template arguments relative to the 'primary'
+ // template. We need the entire list, since the constraint is completely
+ // uninstantiated at this point.
+
+ // FIXME: Add TemplateArgs through the 'Innermost' parameter once
+ // the refactoring of getTemplateInstantiationArgs() relands.
+ MultiLevelTemplateArgumentList MLTAL;
+ MLTAL.addOuterTemplateArguments(Template, std::nullopt, /*Final=*/false);
+ SemaRef.getTemplateInstantiationArgs(
+ MLTAL, /*D=*/FD, FD,
+ /*Final=*/false, /*Innermost=*/std::nullopt, /*RelativeToPrimary=*/true,
+ /*Pattern=*/nullptr, /*ForConstraintInstantiation=*/true);
+ MLTAL.replaceInnermostTemplateArguments(Template, TemplateArgs);
+
+ Sema::ContextRAII SavedContext(SemaRef, FD);
+ std::optional<Sema::CXXThisScopeRAII> ThisScope;
+ if (auto *Method = dyn_cast<CXXMethodDecl>(FD))
+ ThisScope.emplace(SemaRef, /*Record=*/Method->getParent(),
+ /*ThisQuals=*/Method->getMethodQualifiers());
+ return SemaRef.CheckConstraintSatisfaction(
+ Template, TemplateAC, MLTAL, PointOfInstantiation, Satisfaction);
+}
+
+bool Sema::CheckFunctionTemplateConstraints(
SourceLocation PointOfInstantiation, FunctionDecl *Decl,
ArrayRef<TemplateArgument> TemplateArgs,
ConstraintSatisfaction &Satisfaction) {
// In most cases we're not going to have constraints, so check for that first.
FunctionTemplateDecl *Template = Decl->getPrimaryTemplate();
+
+ if (!Template)
+ return ::CheckFunctionConstraintsWithoutInstantiation(
+ *this, PointOfInstantiation, Decl->getDescribedFunctionTemplate(),
+ TemplateArgs, Satisfaction);
+
// Note - code synthesis context for the constraints check is created
// inside CheckConstraintsSatisfaction.
SmallVector<const Expr *, 3> TemplateAC;
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 1c1f6e30ab7b83..acd1151184e42f 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -3936,18 +3936,6 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
Result != TemplateDeductionResult::Success)
return Result;
- // C++ [temp.deduct.call]p10: [DR1391]
- // If deduction succeeds for all parameters that contain
- // template-parameters that participate in template argument deduction,
- // and all template arguments are explicitly specified, deduced, or
- // obtained from default template arguments, remaining parameters are then
- // compared with the corresponding arguments. For each remaining parameter
- // P with a type that was non-dependent before substitution of any
- // explicitly-specified template arguments, if the corresponding argument
- // A cannot be implicitly converted to P, deduction fails.
- if (CheckNonDependent())
- return TemplateDeductionResult::NonDependentConversionFailure;
-
// Form the template argument list from the deduced template arguments.
TemplateArgumentList *SugaredDeducedArgumentList =
TemplateArgumentList::CreateCopy(Context, SugaredBuilder);
@@ -3977,6 +3965,39 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
FD = const_cast<FunctionDecl *>(FDFriend);
Owner = FD->getLexicalDeclContext();
}
+ // C++20 [temp.deduct.general]p5: [CWG2369]
+ // If the function template has associated constraints, those constraints
+ // are checked for satisfaction. If the constraints are not satisfied, type
+ // deduction fails.
+ //
+ // FIXME: We haven't implemented CWG2369 for lambdas yet, because we need
+ // to figure out how to instantiate lambda captures to the scope without
+ // first instantiating the lambda.
+ bool IsLambda = isLambdaCallOperator(FD) || isLambdaConversionOperator(FD);
+ if (!IsLambda && !IsIncomplete) {
+ if (CheckFunctionTemplateConstraints(
+ Info.getLocation(),
+ FunctionTemplate->getCanonicalDecl()->getTemplatedDecl(),
+ CanonicalBuilder, Info.AssociatedConstraintsSatisfaction))
+ return TemplateDeductionResult::MiscellaneousDeductionFailure;
+ if (!Info.AssociatedConstraintsSatisfaction.IsSatisfied) {
+ Info.reset(Info.takeSugared(),
+ TemplateArgumentList::CreateCopy(Context, CanonicalBuilder));
+ return TemplateDeductionResult::ConstraintsNotSatisfied;
+ }
+ }
+ // C++ [temp.deduct.call]p10: [CWG1391]
+ // If deduction succeeds for all parameters that contain
+ // template-parameters that participate in template argument deduction,
+ // and all template arguments are explicitly specified, deduced, or
+ // obtained from default template arguments, remaining parameters are then
+ // compared with the corresponding arguments. For each remaining parameter
+ // P with a type that was non-dependent before substitution of any
+ // explicitly-specified template arguments, if the corresponding argument
+ // A cannot be implicitly converted to P, deduction fails.
+ if (CheckNonDependent())
+ return TemplateDeductionResult::NonDependentConversionFailure;
+
MultiLevelTemplateArgumentList SubstArgs(
FunctionTemplate, CanonicalDeducedArgumentList->asArray(),
/*Final=*/false);
@@ -4011,8 +4032,8 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
// ([temp.constr.decl]), those constraints are checked for satisfaction
// ([temp.constr.constr]). If the constraints are not satisfied, type
// deduction fails.
- if (!IsIncomplete) {
- if (CheckInstantiatedFunctionTemplateConstraints(
+ if (IsLambda && !IsIncomplete) {
+ if (CheckFunctionTemplateConstraints(
Info.getLocation(), Specialization, CanonicalBuilder,
Info.AssociatedConstraintsSatisfaction))
return TemplateDeductionResult::MiscellaneousDeductionFailure;
diff --git a/clang/lib/Sema/SemaTemplateDeductionGuide.cpp b/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
index d42c3765aa534f..5d6c11a75303df 100644
--- a/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
+++ b/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
@@ -902,10 +902,12 @@ Expr *buildIsDeducibleConstraint(Sema &SemaRef,
Context.getTrivialTypeSourceInfo(
Context.getDeducedTemplateSpecializationType(
TemplateName(AliasTemplate), /*DeducedType=*/QualType(),
- /*IsDependent=*/true)), // template specialization type whose
- // arguments will be deduced.
+ /*IsDependent=*/true),
+ AliasTemplate->getLocation()), // template specialization type whose
+ // arguments will be deduced.
Context.getTrivialTypeSourceInfo(
- ReturnType), // type from which template arguments are deduced.
+ ReturnType, AliasTemplate->getLocation()), // type from which template
+ // arguments are deduced.
};
return TypeTraitExpr::Create(
Context, Context.getLogicalOperationType(), AliasTemplate->getLocation(),
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index fb0f38df62a744..6db6d52e27d6dc 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -475,6 +475,21 @@ MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
assert((ND || DC) && "Can't find arguments for a decl if one isn't provided");
// Accumulate the set of template argument lists in this structure.
MultiLevelTemplateArgumentList Result;
+ getTemplateInstantiationArgs(
+ Result, ND, DC, Final, Innermost, RelativeToPrimary, Pattern,
+ ForConstraintInstantiation, SkipForSpecialization,
+ ForDefaultArgumentSubstitution);
+ return Result;
+}
+
+void Sema::getTemplateInstantiationArgs(
+ MultiLevelTemplateArgumentList &Result, const NamedDecl *ND,
+ const DeclContext *DC, bool Final,
+ std::optional<ArrayRef<TemplateArgument>> Innermost, bool RelativeToPrimary,
+ const FunctionDecl *Pattern, bool ForConstraintInstantiation,
+ bool SkipForSpecialization, bool ForDefaultArgumentSubstitution) {
+ assert((ND || DC) && "Can't find arguments for a decl if one isn't provided");
+ // Accumulate the set of template argument lists in this structure.
using namespace TemplateInstArgsHelpers;
const Decl *CurDecl = ND;
@@ -535,14 +550,12 @@ MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
}
if (R.IsDone)
- return Result;
+ return;
if (R.ClearRelativeToPrimary)
RelativeToPrimary = false;
assert(R.NextDecl);
CurDecl = R.NextDecl;
}
-
- return Result;
}
bool Sema::CodeSynthesisContext::isInstantiationRecord() const {
@@ -815,6 +828,9 @@ void Sema::pushCodeSynthesisContext(CodeSynthesisContext Ctx) {
if (!Ctx.isInstantiationRecord())
++NonInstantiationEntries;
+ if (Ctx.Kind != CodeSynthesisContext::ConstraintSubstitution)
+ ++NonConstraintSubstitutionEntries;
+
// Check to see if we're low on stack space. We can't do anything about this
// from here, but we can at least warn the user.
StackHandler.warnOnStackNearlyExhausted(Ctx.PointOfInstantiation);
@@ -827,6 +843,11 @@ void Sema::popCodeSynthesisContext() {
--NonInstantiationEntries;
}
+ if (Active.Kind != CodeSynthesisContext::ConstraintSubstitution) {
+ assert(NonConstraintSubstitutionEntries > 0);
+ --NonConstraintSubstitutionEntries;
+ }
+
InNonInstantiationSFINAEContext = Active.SavedInNonInstantiationSFINAEContext;
// Name lookup no longer looks in this template's defining module.
@@ -1349,6 +1370,12 @@ namespace {
// Whether an incomplete substituion should be treated as an error.
bool BailOutOnIncomplete;
+ private:
+ // CWG2770: Function parameters should be instantiated when they are
+ // needed by a satisfaction check of an atomic constraint or
+ // (recursively) by another function parameter.
+ bool maybeInstantiateFunctionParameterToScope(ParmVarDecl *OldParm);
+
public:
typedef TreeTransform<TemplateInstantiator> inherited;
@@ -1405,12 +1432,20 @@ namespace {
ArrayRef<UnexpandedParameterPack> Unexpanded,
bool &ShouldExpand, bool &RetainExpansion,
std::optional<unsigned> &NumExpansions) {
- return getSema().CheckParameterPacksForExpansion(EllipsisLoc,
- PatternRange, Unexpanded,
- TemplateArgs,
- ShouldExpand,
- RetainExpansion,
- NumExpansions);
+ if (SemaRef.CurrentInstantiationScope &&
+ SemaRef.inConstraintSubstitution()) {
+ for (UnexpandedParameterPack ParmPack : Unexpanded) {
+ NamedDecl *VD = ParmPack.first.dyn_cast<NamedDecl *>();
+ if (!isa_and_present<ParmVarDecl>(VD))
+ continue;
+ if (maybeInstantiateFunctionParameterToScope(cast<ParmVarDecl>(VD)))
+ return true;
+ }
+ }
+
+ return getSema().CheckParameterPacksForExpansion(
+ EllipsisLoc, PatternRange, Unexpanded, TemplateArgs, ShouldExpand,
+ RetainExpansion, NumExpansions);
}
void ExpandingFunctionParameterPack(ParmVarDecl *Pack) {
@@ -1911,9 +1946,62 @@ Decl *TemplateInstantiator::TransformDecl(SourceLocation Loc, Decl *D) {
// template parameter.
}
+ if (SemaRef.CurrentInstantiationScope) {
+ if (SemaRef.inConstraintSubstitution() && isa<ParmVarDecl>(D) &&
+ maybeInstantiateFunctionParameterToScope(cast<ParmVarDecl>(D)))
+ return nullptr;
+ }
+
return SemaRef.FindInstantiatedDecl(Loc, cast<NamedDecl>(D), TemplateArgs);
}
+bool TemplateInstantiator::maybeInstantiateFunctionParameterToScope(
+ ParmVarDecl *OldParm) {
+ if (SemaRef.CurrentInstantiationScope->findInstantiationUnsafe(OldParm))
+ return false;
+ // We're instantiating a function parameter whose associated function template
+ // has not been instantiated at this point for constraint evaluation, so make
+ // sure the instantiated parameters are owned by a function declaration such
+ // that they can be correctly 'captured' in tryCaptureVariable().
+ Sema::ContextRAII Context(SemaRef, OldParm->getDeclContext());
+
+ if (!OldParm->isParameterPack())
+ return !TransformFunctionTypeParam(OldParm, /*indexAdjustment=*/0,
+ /*NumExpansions=*/std::nullopt,
+ /*ExpectParameterPack=*/false);
+
+ SmallVector<UnexpandedParameterPack, 2> Unexpanded;
+
+ // Find the parameter packs that could be expanded.
+ TypeLoc TL = OldParm->getTypeSourceInfo()->getTypeLoc();
+ PackExpansionTypeLoc ExpansionTL = TL.castAs<PackExpansionTypeLoc>();
+ TypeLoc Pattern = ExpansionTL.getPatternLoc();
+ SemaRef.collectUnexpandedParameterPacks(Pattern, Unexpanded);
+ assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
+
+ bool ShouldExpand = false;
+ bool RetainExpansion = false;
+ std::optional<unsigned> OrigNumExpansions =
+ ExpansionTL.getTypePtr()->getNumExpansions();
+ std::optional<unsigned> NumExpansions = OrigNumExpansions;
+ if (TryExpandParameterPacks(ExpansionTL.getEllipsisLoc(),
+ Pattern.getSourceRange(), Unexpanded,
+ ShouldExpand, RetainExpansion, NumExpansions))
+ return true;
+
+ assert(ShouldExpand && !RetainExpansion &&
+ "Shouldn't preserve pack expansion when evaluating constraints");
+ ExpandingFunctionParameterPack(OldParm);
+ ...
[truncated]
|
zyn0217
force-pushed
the
cwg-2369-again
branch
4 times, most recently
from
65829ef to
1ed4033
Compare
|
I've confirmed with @zyn0217 that this iteration of the patch fixes the bug encountered by the chromium team wherein we tried to mangle lambdas with a deduced return type during constraint satisfaction - which caused failure on windows as mangling of lambda requires the enclosing context to be mangled. However if we merge this PR, the bug described in #121881 The issue is that users seem to rely on a GCC specific behavior where GCC does not check template candidates during overload resolution if an exact match exists. Given that GCC's behavior is strictly non-conforming but long standing, I have asked CWG for guidance That being said, I think our option are
|
|
@mysterymath @zmodem Can you both test this patch on Windows to confirm if it resolves issues you've reported? We'd greatly appreciate it! |
|
FYI, we've also seen problems compiling code that includes certain boost headers (version 1.84, as far as I can tell) after #102857: I'm building clang with this PR to see if it resolves this issue. |
|
Confirmed Chromium on Windows builds with this patch. |
|
@alexfh It wouldn't fix your issue, see this comment #122423 (comment) (there were 2 issues, a bug in the PR implementation, and an unfortunate breakage caused by some libraries relying on non-conforming behavior) |
Indeed, it doesn't fix this case. Neither does it fix another compilation error in the same version of boost: |
|
This was discussed during Clang C/C++ Language Working Group meeting today, and the consensus seems that this PR shouldn't land before Clang 20 branch, which is scheduled to happen on January 28th. |
|
Can we have this behavior guarded by a compiler option? |
That depends on the rationale:
For (2), we can simply delay enabling the resolution until the fallout is mitigated or accepted. |
| bool ShouldSkipCG = [&] { | ||
| auto *RD = dyn_cast<CXXRecordDecl>(Function->getParent()); | ||
| if (!RD || !RD->isLambda()) | ||
| return false; | ||
|
|
||
| return llvm::any_of(ExprEvalContexts, [](auto &Context) { | ||
| return Context.isUnevaluated() || Context.isImmediateFunctionContext(); | ||
| }); | ||
| }(); | ||
| if (!ShouldSkipCG) { | ||
| DeclGroupRef DG(Function); | ||
| Consumer.HandleTopLevelDecl(DG); | ||
| } |
There was a problem hiding this comment.
Thanks, I'll take a look
As suggested by Corentin, this is taken from llvm#122423 (comment)
…match exists This implements the same overload resolution behavior as GCC, as described in https://wg21.link/p3606 (section 1-2, not 3) If during overload resolution, there is a non-template candidate that would be always be picked - because each of the argument is a perfect match (ie the source and target types are the same), we do not perform deduction for any template candidate that might exists. The goal is to be able to merge llvm#122423 without being too disruptive. This change means that the selection of the best viable candidate and template argument deduction become interleaved. To avoid rewriting half of Clang we store in `OverloadCandidateSet` enough information to be able to deduce template candidates from `OverloadCandidateSet::BestViableFunction`. Which means the lifetime of any object used by template argument must outlive a call to `Add*Template*Candidate`. This two phase resolution is not performed for some initialization as there are cases where template candidate are better match in these cases per the standard. It's also bypassed for code completion. The change has a nice impact on compile times https://llvm-compile-time-tracker.com/compare.php?from=719b029c16eeb1035da522fd641dfcc4cee6be74&to=bf7041045c9408490c395230047c5461de72fc39&stat=instructions%3Au Fixes llvm#62096 Fixes llvm#74581
…match exists This implements the same overload resolution behavior as GCC, as described in https://wg21.link/p3606 (section 1-2, not 3) If during overload resolution, there is a non-template candidate that would be always be picked - because each of the argument is a perfect match (ie the source and target types are the same), we do not perform deduction for any template candidate that might exists. The goal is to be able to merge llvm#122423 without being too disruptive. This change means that the selection of the best viable candidate and template argument deduction become interleaved. To avoid rewriting half of Clang we store in `OverloadCandidateSet` enough information to be able to deduce template candidates from `OverloadCandidateSet::BestViableFunction`. Which means the lifetime of any object used by template argument must outlive a call to `Add*Template*Candidate`. This two phase resolution is not performed for some initialization as there are cases where template candidate are better match in these cases per the standard. It's also bypassed for code completion. The change has a nice impact on compile times https://llvm-compile-time-tracker.com/compare.php?from=719b029c16eeb1035da522fd641dfcc4cee6be74&to=bf7041045c9408490c395230047c5461de72fc39&stat=instructions%3Au Fixes llvm#62096 Fixes llvm#74581
…ts (llvm#136203) This implements the same overload resolution behavior as GCC, as described in https://wg21.link/p3606 (section 1-2, not 3) If during overload resolution, there is a non-template candidate that would be always be picked - because each of the argument is a perfect match (ie the source and target types are the same), we do not perform deduction for any template candidate that might exists. The goal is to be able to merge llvm#122423 without being too disruptive. This change means that the selection of the best viable candidate and template argument deduction become interleaved. To avoid rewriting half of Clang we store in OverloadCandidateSet enough information to be able to deduce template candidates from OverloadCandidateSet::BestViableFunction. Which means the lifetime of any object used by template argument must outlive a call to Add*Template*Candidate. This two phase resolution is not performed for some initialization as there are cases where template candidate are better match in these cases per the standard. It's also bypassed for code completion. The change has a nice impact on compile times https://llvm-compile-time-tracker.com/compare.php?from=719b029c16eeb1035da522fd641dfcc4cee6be74&to=bf7041045c9408490c395230047c5461de72fc39&stat=instructions%3Au Fixes llvm#62096 Fixes llvm#74581 Reapplies llvm#133426
…ts (llvm#136203) This implements the same overload resolution behavior as GCC, as described in https://wg21.link/p3606 (section 1-2, not 3) If during overload resolution, there is a non-template candidate that would be always be picked - because each of the argument is a perfect match (ie the source and target types are the same), we do not perform deduction for any template candidate that might exists. The goal is to be able to merge llvm#122423 without being too disruptive. This change means that the selection of the best viable candidate and template argument deduction become interleaved. To avoid rewriting half of Clang we store in OverloadCandidateSet enough information to be able to deduce template candidates from OverloadCandidateSet::BestViableFunction. Which means the lifetime of any object used by template argument must outlive a call to Add*Template*Candidate. This two phase resolution is not performed for some initialization as there are cases where template candidate are better match in these cases per the standard. It's also bypassed for code completion. The change has a nice impact on compile times https://llvm-compile-time-tracker.com/compare.php?from=719b029c16eeb1035da522fd641dfcc4cee6be74&to=bf7041045c9408490c395230047c5461de72fc39&stat=instructions%3Au Fixes llvm#62096 Fixes llvm#74581 Reapplies llvm#133426
|
@zyn0217 is that ready for review? |
This reverts commit 8423cef.
…tion For a dependent variable template specialization, we don't build a dependent Decl node or a DeclRefExpr to represent it. Instead, we preserve the UnresolvedLookupExpr until instantiation. However, this approach isn't ideal for constraint normalization. We consider the qualifier during profiling, but since that's based on the written code, it can introduce confusing differences, even when the expressions resolve to the same declaration. This change ensures that, if possible, we profile the resolved declaration instead of its qualifier. For expressions that resolve to more than one declarations, we still profile its qualifier, as otherwise it would make us depend on the order of lookup results.
… declaration" This reverts commit debef50.
|
@cor3ntin it's ready :) |
clang/include/clang/Sema/Template.h
Outdated
| llvm::PointerUnion<Decl *, DeclArgumentPack *> * | ||
| findInstantiationUnsafe(const Decl *D); |
There was a problem hiding this comment.
findInstantiationUnsafe (can return null) and getInstantiationOf (asserts) would probably be better names.
Move that declaration before the existing one.
There was a problem hiding this comment.
Instead of unsafe, I'd suggest something like getInstantiationOfIfExists and getInstantiationOf (or similar) rather than unsafe. unsafe means, to me, that there is a precondition that if you don't setup correctly, it fails. Not that it migth just return null.
Finally, as interface, is there a reason these are returning a pointer to a pointer union? Seems oen should return by value, the other in an optional.
There was a problem hiding this comment.
I renamed it to getInstantiationOfIfExists.
Finally, as interface, is there a reason these are returning a pointer to a pointer union?
It's pre-existing, we track the instantiated decls and packs together within a map.
Refactoring the interface (either the return type or the name) would bring a lot of churn, for which I think merits a separate PR.
There was a problem hiding this comment.
Yep, I'm ok cleaning that interface up in a follow up. Was just curious because it is very much a 'smell'.
clang/include/clang/Sema/Template.h
Outdated
| llvm::PointerUnion<Decl *, DeclArgumentPack *> * | ||
| findInstantiationUnsafe(const Decl *D); |
There was a problem hiding this comment.
Instead of unsafe, I'd suggest something like getInstantiationOfIfExists and getInstantiationOf (or similar) rather than unsafe. unsafe means, to me, that there is a precondition that if you don't setup correctly, it fails. Not that it migth just return null.
Finally, as interface, is there a reason these are returning a pointer to a pointer union? Seems oen should return by value, the other in an optional.
|
I bisected a build failure to this commit. I can't figure out if it's expected or not. I minimized it to this: #include <concepts>
enum class KindEnum {
Unknown = 0,
Foo = 1,
};
template <typename T>
concept KnownKind = T::kind() != KindEnum::Unknown;
template <KnownKind T>
struct KnownType;
struct Type {
KindEnum kind() const;
static Type f(Type t);
template <KnownKind T>
static KnownType<T> f(T t);
static void g() {
Type t;
f(t);
}
};
template <KnownKind T>
struct KnownType {
static constexpr KindEnum kind() { return KindEnum::Foo; }
};gcc accepts it. After this commit, clang rejects it: Live link: https://godbolt.org/z/oGh319a44 Is this sort of breakage expected? |
|
@rupprecht I sent out #143096 for the fix :) |
…n" (llvm#122423) The previous approach broke code generation for the MS ABI due to an unintended code path during constraint substitution. This time we address the issue by inspecting the evaluation contexts and thereby avoiding that code path. This reapplies 96eced6 (llvm#102857).
|
Building Clang with libc++ and using it, the following test asserts with this patch: (Ubuntu) @zyn0217 Could you please take a look? Thanks. |
|
@maryammo I managed to reduce it to https://godbolt.org/z/vjcoq1qaG .. will look into it tomorrow |
template <typename> class basic_ostream;
template <typename _Tp> constexpr bool is_trivial_v = __is_trivial(_Tp);
template <typename _CharT> struct basic_string_view {
static_assert(__is_trivial(_CharT));
};
template <typename _CharT, typename>
void operator<<(basic_ostream<_CharT>, basic_string_view<_CharT>); // #1
struct basic_ios {
operator bool();
};
template <typename> struct basic_ostream : virtual basic_ios {};
template <typename _Ostream, typename _Tp> void operator<<(_Ostream, _Tp); // #2
basic_ostream<char> g00_a0;
void g00() {
__is_same(int, decltype(operator<< <basic_ostream<char>, char>(g00_a0, 1)));
}
So this is a issue brought by the heuristic non-dependent check which happens before constraint checking, where we think (OTOH, if we check However, if we swapped the order of template parameters, GCC would suffer from the same issue too: https://godbolt.org/z/M6xfGMPMo template <typename> class basic_ostream;
template <typename _CharT> struct basic_string_view {
static_assert(__is_trivial(_CharT));
};
template <typename _CharT, typename>
void Foo(basic_string_view<_CharT>, basic_ostream<_CharT>);
struct basic_ios {
operator bool();
};
template <typename> struct basic_ostream : virtual basic_ios {};
template <typename _Tp, typename _Ostream> void Foo(_Tp, _Ostream);
basic_ostream<char> g00_a0;
void g00() {
static_assert(!__is_same(int, decltype(Foo<basic_ostream<char>, char>(g00_a0, 1))));
}
It's not quite clear to me if it is assured that no arbitrary template instantiation would occur during template argument deduction, so CC @zygoloid @cor3ntin @erichkeane for help :) |
|
We've found another issue caused by this patch. I'm not quite sure what the standard says about this, but Clang after this commit is the only recent mainstream compiler that rejects this code (https://gcc.godbolt.org/z/KW56orYca): The test case above is reduced from https://github.com/CGAL/cgal/blob/5add7e715031a190a87cf6cdc9ad324543a1621a/Triangulation_3/test/Triangulation_3/test_regular_remove_3.cpp, where the error looks like this: |
|
A slightly modified version of that testcase gives some suggestion of what's going on: https://gcc.godbolt.org/z/eWY6zKz6G -- note that all the compilers reject this, because they are indeed all forming the type If you reverse the order of the parameters in |
@zygoloid It's not quite true that Clang is carrying on to checking for remaining conversions. It's that we now have two non-dependent conversion checks, one happens before constraint check, and the other happens after it. The first conversion check, which only speculatively checks those non-dependent conversions that we believe they wouldn't cause unwanted instantiations. And that hinges on the user defined conversions, i.e. if there are any user-defined conversions or conversion constructors in ParamType or ArgType, we skip the speculative check and defer conversion until after constraint evaluation. (To not defeat the purpose of CWG2369 which aims to use constraints to avoid non-SFINAE errors) In the example you have, This workaround was invented to alleviate existing regressions, as in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99599 Technically, GCC should have suffered from the same issue, however, they seemed to only have this speculative non-dependent check for those calls without explicit template arguments. So we can still bypass their workaround by specifying some explicit template arguments. https://godbolt.org/z/KM9E8aKjb We could mimic GCC's behavior, by not allowing explicit template arguments to have the first non-dependent check. However, it's unclear whether that behavior is wg21's desire, so it's unclear to me if that's the direction we actually want to take. |
The previous approach broke code generation for the MS ABI due to an unintended code path during constraint substitution. This time we address the issue by inspecting the evaluation contexts and thereby avoiding that code path.
This reapplies 96eced6 (#102857).