DR692 handles two cases: pack expansion (for class/var template) and function parameter pack. The former needs DR1432 as a fix, and the latter needs DR1395 as a fix. However, DR1432 has not yet made a wording change. so I made a tentative fix for DR1432 with the same spirit as DR1395.
Details
Details
- Reviewers
rsmith aaron.ballman erichkeane hubert.reinterpretcast - Group Reviewers
Restricted Project - Commits
- rG6afcc4a459ea: [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering…
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Thank you for working on this! Can you please add more details to the patch summary about the changes?
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 5184 | Does GCC still implement it this way? One concern I have is that this will be an ABI breaking change, and I'm not certain how disruptive it will be. If GCC already made that change, it may be reasonable for us to also break it without having to add ABI tags or something special. But if these changes diverge us from GCC, that may require some extra effort to mitigate. | |
| 5184–5191 | We tend not to use top-level const on locals in the project as a matter of style. | |
| clang/test/CXX/drs/dr6xx.cpp | ||
| 1084 | This DR is also about partial class specializations -- are changes not needed in Sema::getMoreSpecializedPartialSpecialization() as well? It'd be helpful to add the examples from the DR here to make sure they're handled properly. | |
Comment Actions
Thanks for taking a look.
This was intended to correctly implement https://eel.is/c++draft/temp.deduct.partial#11. The test case is from https://eel.is/c++draft/temp.func.order#example-4.
If, after considering the above, function template F is at least as specialized as function template G and vice-versa, and if G has a trailing function parameter pack for which F does not have a corresponding parameter, and if F does not have a trailing function parameter pack, then F is more specialized than G.
It looks like this is not too far from implementing DR692. I'll go ahead and include the necessary change for fixing the template parameter packs case.
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 5184 | Unfortunately, GCC is still using the old/non-conforming behavior. https://clang.godbolt.org/z/5K4916W71. What is the usual way to mitigate this? | |
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 5184 | You would use the ClangABI enumeration to alter behavior between ABI versions: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/LangOptions.h#L174 like done in: https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/DeclCXX.cpp#L929 | |
Comment Actions
- handle more cases to implement DR692, DR1395
- tentatively implement DR1432 which DR692 needs, otherwise there would be regressions
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 2481 | FYI: isSameTemplateArg is for class/variable partial ordering deduction. The corresponding check for function template deduction is skipped by intention. See https://reviews.llvm.org/rG86a1b135f0c67a022ef628f092c6691892752876 and https://github.com/llvm/llvm-project/blob/e6f1f062457c928c18a88c612f39d9e168f65a85/clang/lib/Sema/SemaTemplateDeduction.cpp#L5064-L5066. | |
| 5184 | Looking at how ClangABI is currently used, it looks like it is for low-level object layout ABI compatibility. For library/language ABI compatibility, maybe we should not use ClangABI? Looking at https://reviews.llvm.org/rG86a1b135f0c67a022ef628f092c6691892752876, I guess the practice is just committed and see? If it breaks important or many existing libraries, just revert or add compiler options? | |
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 5184 |
I'm fairly optimistic considering check-runtimes passes. | |
Comment Actions
I admit that I'm feeling a bit out of my element with all the template machinery involved, so I've reviewed as best I'm able (so if any of my questions seem odd to you, push back on them if you want).
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 1109 | Why are you checking ArgIdx + 1 == NumArgs here? It looks like you're only handling the case where the pack is the last argument, but packs can appear anywhere, right? | |
| 2481 | Same question here about looking at just the last element. | |
| 5184 |
I don't know that there's any specific guidance that we only use it for object layout ABI compatibility; we have used it for behavior beyond layout in the past: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp#L9786
Somewhat, yes. We aim for full ABI compatibility with GCC and consider ABI differences to be bugs (generally speaking). If we break some important existing library, that bug is critical to get fixed ASAP, but any ABI different can potentially bite users. I would say: if matching the old ABI requires convoluting the implementation too much, we can skip it; otherwise, we should probably match GCC's ABI just to avoid concerns. CC @rsmith in case he has different opinions as code owner. | |
| 5392 | You should end the anonymous namespace here per our coding style guidelines. | |
| 5416–5417 | Can't this be a local constexpr variable instead of an NTTP, or are there reasons you want to allow callers to be able to override that? | |
| 5419–5420 | Curiosity question -- can you make P1 and P2 be pointers to const without many other viral changes to existing code? | |
| 5437 | Again, should you be looking at all the packs here and not just trailing ones? | |
| clang/www/cxx_dr_status.html | ||
| 4197 ↗ | (On Diff #443771) | You should use unreleased as the class for these because Clang 15 hasn't been release yet (they'll switch over to full once we release). |
| 8187 ↗ | (On Diff #443771) | Same here |
Comment Actions
Thanks for the review. @aaron.ballman @erichkeane. About the non-trailing pack, I had the impression that DR692 is only about the trailing pack in practice but don't remember the exact wording, so I dig the standardese further.
For partial specialization:
https://eel.is/c++draft/temp.spec.partial#general-9.4
An argument shall not contain an unexpanded pack. If an argument is a pack expansion ([temp.variadic]), it shall be the last argument in the template argument list.
For the primary template, the rule is the same for the partial ordering purpose (See the second code example of DR1495).
DR692 does not need to consider a non-trailing function parameter pack since it is a non-deduced context.
https://eel.is/c++draft/temp.deduct#type-5.7
The non-deduced contexts are: ... - A function parameter pack that does not occur at the end of the parameter-declaration-list.
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 5437 | If it's not trailing, it's not deduced. and afaik, if it's not deduced it can't be involved spacial specialization ordering | |
Comment Actions
Thank you for the explanation on non-trailing packs, I think I'm convinced. Aside from some test coverage, I think this LGTM. Please hold off on landing until @erichkeane has had a chance to look again though.
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 1109 | Can you add CodeGen test coverage for both ABI modes (new RUN line with -fclang-abi-compat=14) | |
| 5419–5420 | Good to know, then no changes needed here, thanks! | |
| 5435 | Can you add CodeGen test coverage for both ABI modes (new RUN line with -fclang-abi-compat=14) | |
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 5435 | Some test cases could reach the CodeGen phase *with* -fclang-abi-compat=14(i.e without this patch), some test cases could reach the CodeGen phase *without* -fclang-abi-compat=14(i.e with this patch). Please let me know if the added CodeGen tests are what you expected. | |
Comment Actions
LGTM!
| clang/lib/Sema/SemaTemplateDeduction.cpp | ||
|---|---|---|
| 5435 | Yes, that test is what I was looking for, thank you! | |
Comment Actions
This is breaking us.
template <typename T, typename... Ts> struct S; // #1
template <typename T> struct S<T> {}; // #2The following compiled before but is now broken, (we use -fclang-abi-compat=13.0). We get a warning from -Winvalid-partial-specialization
This change _does_ make https://eel.is/c++draft/temp.func.order#example-5 work, ie for function overload resolution but regresses the type deduction example above.
There seem to be examples in the standard that suggest #2 is more specialized. @mcgrathr found https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1432 and https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1395 among others. WDYT?
Comment Actions
I tried your test case but was unable to reproduce the issue. Is this the complete test case? I'm asking because the provided test case does not involve deduction.
Comment Actions
Or do you mean the duction for partial ordering, then I did a simple S<int> a which seems fine?
Comment Actions
Just that input is enough to get the error. I did clang++ test.cpp -fsyntax-only -fclang-abi-compat=14.0
Comment Actions
I can see failures related to this change in a downstream version of clang where the default ClangABICompat value is not Latest (after the followup https://reviews.llvm.org/rGda6187f566b7881cb8350621aea9bd582de569b9).
My understanding is that with -fclang-abi-compat=14 we should get the behavior prior to this change. If my understanding is correct, then I'd think that such failure shouldn't occur.
These are the tests that I see failing with new diagnostic messages class template partial specialization is not more specialized than the primary template being raised:
Clang :: CXX/temp/temp.decls/temp.class.spec/p8-0x.cpp Clang :: CXX/temp/temp.decls/temp.variadic/example-bind.cpp Clang :: CXX/temp/temp.decls/temp.variadic/example-tuple.cpp Clang :: CXX/temp/temp.decls/temp.variadic/injected-class-name.cpp Clang :: CXX/temp/temp.decls/temp.variadic/metafunctions.cpp Clang :: CXX/temp/temp.fct.spec/temp.arg.explicit/p3-0x.cpp Clang :: CXX/temp/temp.fct.spec/temp.arg.explicit/p9-0x.cpp Clang :: CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p1-0x.cpp Clang :: CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.type/p21.cpp Clang :: CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.type/p9-0x.cpp Clang :: CXX/temp/temp.param/p11-0x.cpp Clang :: SemaCXX/coreturn-exp-namespace.cpp Clang :: SemaCXX/coreturn.cpp Clang :: SemaCXX/coroutine-unhandled_exception-warning-exp-namespace.cpp Clang :: SemaCXX/coroutine-unhandled_exception-warning.cpp Clang :: SemaCXX/coroutines-exp-namespace.cpp Clang :: SemaCXX/coroutines.cpp Clang :: SemaCXX/crash-lambda-12645424.cpp Clang :: SemaCXX/discrim-union.cpp Clang :: SemaTemplate/attributes.cpp Clang :: SemaTemplate/class-template-spec.cpp Clang :: SemaTemplate/deduction.cpp Clang :: SemaTemplate/pack-deduction.cpp Clang :: SemaTemplate/temp_arg_nontype_cxx1z.cpp Clang :: SemaTemplate/temp_arg_template_cxx1z.cpp
I've reproduced this locally with the following patch to inject -fclang-abi-compat=14 on the command line executed for those tests:
diff --git a/clang/test/CXX/temp/lit.local.cfg b/clang/test/CXX/temp/lit.local.cfg new file mode 100644 index 000000000000..23d23bb6c46e --- /dev/null +++ b/clang/test/CXX/temp/lit.local.cfg @@ -0,0 +1,3 @@ +config.substitutions.insert(0, + ('%clang_cc1', + '%clang_cc1 -fclang-abi-compat=14.0')) diff --git a/clang/test/SemaCXX/lit.local.cfg b/clang/test/SemaCXX/lit.local.cfg new file mode 100644 index 000000000000..23d23bb6c46e --- /dev/null +++ b/clang/test/SemaCXX/lit.local.cfg @@ -0,0 +1,3 @@ +config.substitutions.insert(0, + ('%clang_cc1', + '%clang_cc1 -fclang-abi-compat=14.0')) diff --git a/clang/test/SemaTemplate/lit.local.cfg b/clang/test/SemaTemplate/lit.local.cfg new file mode 100644 index 000000000000..23d23bb6c46e --- /dev/null +++ b/clang/test/SemaTemplate/lit.local.cfg @@ -0,0 +1,3 @@ +config.substitutions.insert(0, + ('%clang_cc1', + '%clang_cc1 -fclang-abi-compat=14.0'))
(Note that by adding the lit.local.cfg file abovementioned there will also be the failure of SemaCXX/class-layout.cpp, but that can be ignored as for some command lines the expected behavior is equivalent to pass -fclang-abi-compat=latest)
Comment Actions
Stopping by - I assume this targets Clang 16 now, so the abi compat level should be adjusted to 15.
Comment Actions
We have some compilation failures on our end because of function template parameter deduction when passing the function template to a function pointer.
typedef void (*f)(const int&);
template <typename T>
void F(T value) {}
template <typename T>
void F(const T& value){}
void q(f);
void w() {
q(&F);
}https://gcc.godbolt.org/z/faoq74q7G
Is this an intended outcome for this patch?
Comment Actions
Thanks for the report. No that's not the intent. This should only affect partial ordering but not which candidate is viable (error message candidate function not viable ...). I'll take a look.
Comment Actions
This new behavior is correct. It is due to the change on line 1758 in SemaTemplateDeduction.cpp. Basically, both the taking address of and regular function call to the overloaded function set using the same partial ordering rule (Relevant wording https://eel.is/c++draft/over.over#5). GCC is inconsistent in this regard (https://gcc.godbolt.org/z/WrKsaKrdz). MSVC agrees with this new/correct behavior.
Comment Actions
TBH, the Clang diagnosis could be better here. It kinda misleading about what is happening. MSVC's diagnosis is more clear (https://gcc.godbolt.org/z/WrKsaKrdz).
Comment Actions
This change appears to be responsible for the following test case now being rejected. That seems right and is consistent with the intent, but use of -fclang-abi-compat=14 doesn't seem to restore the prior behavior. https://godbolt.org/z/ad1TPjTza
struct map
{
template <typename Sequence>
map(Sequence&& seq , void* = 0 ); // g++ prefers this one
template <typename First, typename ...T_>
map(First&& first, T_&&... rest);
};
int main()
{
int i;
map m(i);
return 0;
}Comment Actions
Pinging this thread because it has more reviewers who probably have opinions about this.
https://reviews.llvm.org/D134507 is a patch that adds -fclang-abi-compat support around the breaking change here, which basically means that targets using old -fclang-abi-compat settings never get this change. I've argued on that patch that that isn't the right way of resolving this issue. Basically, the DR here is changing the language behavior in a way that you can imagine having ABI impact, but it's not primarily an ABI change, it's a language semantics change. The ABI impact is purely that we potentially select a different overload in some cases, which can have downstream ODR impact. I think the appropriate way to handle language semantics changes like this which potentially have compatibility impact is to condition them on language version. Changing the target language standard is already broadly understood to have source/semantic compatibility impact, which is why we allow different target language standards to be specified in the first place. This also makes it straightforward to document this potential break as a consequence of moving to -std=c++23, and it removes a potential rather bizarre portability issue where platforms that embrace stable ABIs are permanently stuck with a language dialect.
Comment Actions
This is very reasonable to me except that I hope, for this particular patch, to key on -std=c++20 instead of -std=c++23 since this is needed for P2113, a C++20 change.
Comment Actions
There were multiple DRs implemented in this patch.
DR692 was a fix against C++11
DR1395 was a fix against C++17
DR1432 is still open but was opened against C++11
Which DRs do we wish to implement in what language modes?
@rjmccall is correct about us not being required to apply DRs when they're disruptive, but at the same time, WG21 DRs are intended to be handled as if the original standard they were reported against had always been using the fixed form. So *not* applying a DR in order to avoid problems for existing code can cause problems for existing and future code in terms of portability between compilers (and ABI impacts that stem from the semantic changes). So I think we wish to apply the DRs as broadly as we can. The question is: do we think users with existing code should not have to change or do we think it's reasonable to give them a flag to opt into the old behavior? My personal feeling is -- the default compiler mode should be as conforming as we can make it be within reason, and since this has some impact but not a massive break (no major OS SDKs or third party libraries seem to be impacted as best I can tell), my feeling is that we should strongly consider adding a feature flag (other than ABI compat, as that does seem like a bit of an odd choice to key on) to opt into the older behavior, esp since the break is a loud one and not a silent one.
Adding @hubert.reinterpretcast as C++ conformance code owner in case he's got opinions as well.
Comment Actions
Is there any ability here to diagnose the difference? It would probably be helpful if at any point (in both directions!) we diagnose that our behavior changes. We did something similar for the reversible operators at one point (but WG21 might have fixed it since?).
IMO, if we decide that a DR is 'too breaking' to implement in all language modes it applies to, we shouldn't implement it at all, and instead should question the standards committee about their breaks. Flipping it based on language version increases the friction (in a silent and really breaking way!) to switch language modes.
Comment Actions
Is there consensus that applying this DR resolution is distruptive enough that it's actually an issue? If clang-abi-compat is not a good way to offer backwards compatibility in this case, maybe we should just leave it be.
FWIW, I worry that applying it according to standard version will make it harder to write code that works cross version and the benefit here is not clear.
Comment Actions
I happen to be on vacation this week ahead of Canadian Thanksgiving (or trying as much as I can anyway). I agree that broad application of DRs is generally what has been expected in the context of Clang and GCC (except where it is believed the DR resolution itself is defective, in which case the committee is consulted).
It seems there is a question of whether the DR resolutions are defective/too breaking to be advisable. Do we have a useful summary of what led to that opinion?
In considering a compatibility flag, I think some thought should be given to whether to time-limit it.
Comment Actions
There were multiple DRs implemented in this patch.
DR692 was a fix against C++11
DR1395 was a fix against C++17
DR1432 is still open but was opened against C++11Which DRs do we wish to implement in what language modes?
There are regressions if DR1395 is not applied on top of DR692. So I'd consider all three fixes on C++11.
FYI, I don't see users reporting that this is very disruptive. So I think these DRs should be the compiler default.
I think the consensus is some flag is needed to put it back to legacy behavior just in case. Keying on -fclang-abi-compat is confusing. Keying on language versions works but is less conforming. My feeling is that it is rare that we have a situation like this, so I'm hesitant to come up with a general solution like a language compatibility flag. The past experience is that we could simply introduce a specific language feature flag like -faligned-new. How about adding a flag -fcxx-dr692 / -fno-cxx-dr692, defaulted on? The naming convention could be used for similar purposes in the future.
Comment Actions
I am not sure there is strong consensus to add a flag "just in case". Historically, many DRs had effects on overload resolution (via SFINAE or otherwise). Once we start adding flags "just in case", we might end up having a lot of them. These flags can exacerbate fracturing of the ecosystem.
Comment Actions
New flag or not, as long as we allow users to use the old behavior, it is already fracturing the ecosystem. Do you mean (1) we shouldn't give the user the choices or (2) we allow users to use the old behavior but only for a few releases and then remove the flag? I think (1) is a little bit harsh but I would not say it is disruptive. (2) is more user-friendly.
Comment Actions
Is it known what gcc and Microsoft maintainers are going to do with these DRs? I don't see any recent changes in gcc, but the following is interesting...
For the test below, Clang has historically accepted it but now rejects it. MSVC has historically rejected it, but started accepting it for v19.34 (perhaps a regression?). Gcc continues to accept it. See https://godbolt.org/z/jax3GsYY5.
struct map {
template <typename Sequence>
map(Sequence&& seq , void* = 0 );
template <typename First, typename ...T_>
map(First&& first, T_&&... rest);
};
map m(0);Comment Actions
It's hard to tell if they didn't implement these DRs or if they're broken. But for P2113 (C++20) to work, DR692 is needed.
For the test below, Clang has historically accepted it but now rejects it. MSVC has historically rejected it, but started accepting it for v19.34 (perhaps a regression?). Gcc continues to accept it. See https://godbolt.org/z/jax3GsYY5.
struct map { template <typename Sequence> map(Sequence&& seq , void* = 0 ); template <typename First, typename ...T_> map(First&& first, T_&&... rest); }; map m(0);
This should be ambiguous. Only the first parameter should be considered for partial ordering. This is basically the g(42) example in https://eel.is/c++draft/temp.func.order#example-4.
Comment Actions
The same argument applies to limiting by target. Platforms that choose to maintain a stable ABI should not be condemned to permanent non-conformance, creating additional obstacles to C++ portability. And honestly, I think the target portability argument is much stronger than the standard-version portabillity argument. It's not by any means uncommon to have C++ headers that need to work consistently under multiple language standards, but they also generally need to work on multiple compilers, and so they cannot necessarily rely on having this DR in place. There's also an awful lot of C++ code that *doesn't* have to work under multiple standards because it's internal to a project that uses consistent build settings; but a lot of that code still needs to be portable to different platforms. While some portability differences are unavoidable, we certainly don't want to create new differences.
More generally, language users understand that new language releases sometimes change existing language rules. They don't always like it, but it is a standard part of the story of changing the target language standard.
It does seem like we don't have a good assessment of how likely a compatibility break is here in practice — whether this pattern comes up in common libraries and so on. If it's vanishingly unlikely, maybe we can change this behavior unconditionally. If it's likely enough for this to be a major problem, then I think the C++ committee really needs to think about whether this change is wise. Maybe we can get more data on that.
I know that people sometimes have a strong reaction to feeling like the language is constrained by ABI compatibility concerns, so let me reiterate that this is *not* primarily about the ABI; it is about C++ introducing a semantic break something like 15 years after variadic templates were introduced to the language (11 years after standard release, but C++11 features were in wide use well before they became officially standardized).
Comment Actions
(1) has been the default absent information to indicate that the new behaviour is not ready for deployment (because it would cause widespread disruption). To consider alternatives would require information about the nature of the deployment difficulties.
Comment Actions
Agreed.
To all:
As a next step, I'll remove the ClangABICompat checks for these DRs (make these DRs effective unconditionally). If we saw proof that these have deployment difficulties. We can (1) re-run the rules with the committee as suggested by @rjmccall; (2) consider alternatives (including reverting these DRs) based on the feedback. Please let me know if you object to this or have any other concerns.
Comment Actions
I think that's fair. I don't know how much concrete information we'll get about this in a short period of time, but it's worth trying to collect it.
Was this change in LLVM 15, or is it still unreleased?
Comment Actions
This sounds like a good plan to me. When you remove the ABI compat checks, please be sure to add the clang-vendors group to the review and a release note to the potentially breaking changes section. Once that lands, you should also make an announcement in Discourse.
Comment Actions
This is still unreleased. It will be released in Clang16.
Will do. Thanks for the heads-up.
Why are you checking ArgIdx + 1 == NumArgs here? It looks like you're only handling the case where the pack is the last argument, but packs can appear anywhere, right?