[Clang] Implement CWG2369 "Ordering between constraints and substitution" #102857
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cor3ntin
reviewed
Aug 17, 2024
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
mordante
added a commit
to mordante/llvm-project
that referenced
this pull request
Aug 18, 2024
The function
template<class Duration>
requires three_way_comparable_with<sys_seconds, sys_time<Duration>>
constexpr auto operator<=>(const leap_second& x, const sys_time<Duration>& y) noexcept;
Has a recursive constrained. This caused an infinite loop in GCC and is
now hit by llvm#102857.
A fix would be to make this function a hidden friend, this solution is
propsed in LWG4139.
For consistency all comparisons are made hidden friends.
Since the issue causes compilation failures no additional test are
needed.
Fixes: llvm#104700
The function
template<class Duration>
requires three_way_comparable_with<sys_seconds, sys_time<Duration>>
constexpr auto operator<=>(const leap_second& x, const sys_time<Duration>& y) noexcept;
Has a recursive constrained. This caused an infinite loop in GCC and is
now hit by llvm#102857.
A fix would be to make this function a hidden friend, this solution is
propsed in LWG4139.
For consistency all comparisons are made hidden friends.
Since the issue causes compilation failures no additional test are
needed.
|
@mordante |
mordante
added a commit
that referenced
this pull request
Aug 20, 2024
The function
template<class Duration>
requires three_way_comparable_with<sys_seconds, sys_time<Duration>>
constexpr auto operator<=>(const leap_second& x, const
sys_time<Duration>& y) noexcept;
Has a recursive constrained. This caused an infinite loop in GCC and is
now hit by #102857.
A fix would be to make this function a hidden friend, this solution is
propsed in LWG4139.
For consistency all comparisons are made hidden friends. Since the issue
causes compilation failures no additional test are needed.
Fixes: #104700
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/7054 Here is the relevant piece of the build log for the reference |
Endilll
reviewed
Jan 6, 2025
|
We've started seeing libc++ test failures in the Fuchsia toolchain buildbots for Windows: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8726615170786362641/overview This may be related to the discussion in #121881, but the error looks slightly different. This is definitely the only plausible PR in the blamelist: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8726615170786362641/blamelist |
We're hitting the same error in Chromium builds. Here is a stand-alone reproducer: https://crbug.com/388428503#comment4 |
Thanks. @Endilll is working on a reduction, so that we get a better sense of what the issue is |
|
@cor3ntin Do we need to revert it for now? |
I'd like us to figure out if it's a conformance issue first - but bearing resolution we should revert before end of week imo |
|
Here's what I got from creduce: |
|
I see the problem here: When substituting into the constraint |
|
@zyn0217 Ouch. Lets revert and try to figure that out... @erichkeane |
zyn0217
added a commit
to zyn0217/llvm-project
that referenced
this pull request
Jan 8, 2025
…ubstitution"" Unfortunately that breaks some code on Windows when lambdas come into play, as reported in llvm#102857 (comment) This reverts commit 96eced6.
|
Reverted in #122130 |
|
Oof, hrmph. It seems to me that we just have to be slightly more aggressive at making sure we instantiate stuff I think. This sentence: Is kinda wrong, it should be in the instantiation of that function... |
cor3ntin
pushed a commit
that referenced
this pull request
Jan 8, 2025
…ubstitution"" (#122130) Unfortunately that breaks some code on Windows when lambdas come into play, as reported in #102857 (comment) This reverts commit 96eced6.
@erichkeane I don’t quite understand: the CWG issue itself requires us to check the constraint before substituting into the function during template argument deduction. So it shouldn’t live in a specialization because we don’t really have one. This situation feels more like a chicken-and-egg problem. GCC doesn’t encounter this issue because it doesn’t need to support the MSVC ABI. The problem only arises with the MSVC ABI because it requires parent mangling when instantiating the lambda body. |
|
Do we want to add some workaround to libc++'s |
cor3ntin
reviewed
Jan 9, 2025
| /*Final=*/false, /*Innermost=*/std::nullopt, /*RelativeToPrimary=*/true, | ||
| /*Pattern=*/nullptr, /*ForConstraintInstantiation=*/true); | ||
| MLTAL.replaceInnermostTemplateArguments(Template, TemplateArgs); | ||
|
|
There was a problem hiding this comment.
I think we want to enter an Unevaluated ExpressionEvaluationContext
Given the windows bug, I think the SavedContext might not be correct. Can you play with that?
Actually, I perhaps misread the example (I think I inserted extra curleys there). It isn't clear to me why we are mangling the name of the lambda. It isn't in a context where it should be emitted? Corentin might be right about the starting an unevaluated context. It also isn't clear to me why compare_three_way_result_t is being instantiated with a lambda in it, but the example is one that perhaps needs more contemplating. |
|
I spent some time on this with @zyn0217 template <class, class _Up>
using compare_three_way_result_t = _Up ::type;
struct __sfinae_assign_base {};
template <class _Tp, class _Up>
requires ([]<class W>(W) { return true; }(_Up())) // #1
compare_three_way_result_t<_Tp, _Up> operator<=>(_Tp, _Up); //#2
struct RuntimeModeArgs {
auto operator<=>(const RuntimeModeArgs &) const = default;
__sfinae_assign_base needs_admin;
};In In turn This in turns requires the lambda to have a mangled name, causing its enclosing, dependent function template type to get mangled on the windows abi, which obviously isn't going to work. Fortunately, we do not need to mangle constraints, so we just need to find a reasonable way to skip the codegen. This patch does the trick by skipping diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 18fd95f77ec2..0e30f21ab68b 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13721,7 +13721,8 @@ public:
FunctionDecl *Function,
bool Recursive = false,
bool DefinitionRequired = false,
- bool AtEndOfTU = false);
+ bool AtEndOfTU = false,
+ bool DeducingReturnType = false);
VarTemplateSpecializationDecl *BuildVarTemplateInstantiation(
VarTemplateDecl *VarTemplate, VarDecl *FromVar,
const TemplateArgumentList *PartialSpecArgs,
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index acd1151184e4..b9918c556443 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -5459,7 +5459,7 @@ bool Sema::DeduceReturnType(FunctionDecl *FD, SourceLocation Loc,
if (FD->getTemplateInstantiationPattern()) {
runWithSufficientStackSpace(Loc, [&] {
- InstantiateFunctionDefinition(Loc, FD);
+ InstantiateFunctionDefinition(Loc, FD, false, false, false, /*DeducingReturnType=*/true);
});
}
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index e058afe81da5..e3d34c305429 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4955,7 +4955,8 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
FunctionDecl *Function,
bool Recursive,
bool DefinitionRequired,
- bool AtEndOfTU) {
+ bool AtEndOfTU,
+ bool DeducingReturnType) {
if (Function->isInvalidDecl() || isa<CXXDeductionGuideDecl>(Function))
return;
@@ -5284,8 +5285,11 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
savedContext.pop();
}
- DeclGroupRef DG(Function);
- Consumer.HandleTopLevelDecl(DG);
+ if(!DeducingReturnType || !(parentEvaluationContext().isUnevaluated()
+ || parentEvaluationContext().isImmediateFunctionContext())) {
+ DeclGroupRef DG(Function);
+ Consumer.HandleTopLevelDecl(DG);
+ }
// This class may have local implicit instantiations that need to be
// instantiation within this scope.The |
|
That seems like a heavy hand. Should we instead be checking the 'unevaluated context' state to see whether we should be emitting stuff? I can't imagine that anything from an unevaluated context should ever be emitted. |
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 10, 2025
…aints and substitution"" (#122130) Unfortunately that breaks some code on Windows when lambdas come into play, as reported in llvm/llvm-project#102857 (comment) This reverts commit 96eced6.
DhruvSrivastavaX
pushed a commit
to DhruvSrivastavaX/lldb-for-aix
that referenced
this pull request
Jun 12, 2025
…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).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch partially implements CWG 2369 for non-lambda-constrained functions.
Lambdas are left intact at this point because we need extra work to correctly instantiate captures before the function instantiation.
As a premise of CWG2369, this patch also implements CWG2770 to ensure the function parameters are instantiated on demand.
Closes #54440