This is an archive of the discontinued LLVM Phabricator instance.

[c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template
ClosedPublic

Authored by ychen on Jun 28 2022, 11:11 AM.

Details

Summary

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.

Diff Detail

Event Timeline

ychen created this revision.Jun 28 2022, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2022, 11:11 AM
ychen requested review of this revision.Jun 28 2022, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2022, 11:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ychen updated this revision to Diff 440703.Jun 28 2022, 11:18 AM

fix typo

aaron.ballman added reviewers: erichkeane, Restricted Project.Jul 6 2022, 8:02 AM

Thank you for working on this! Can you please add more details to the patch summary about the changes?

clang/lib/Sema/SemaTemplateDeduction.cpp
5191

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.

5198–5199

We tend not to use top-level const on locals in the project as a matter of style.

clang/test/CXX/drs/dr6xx.cpp
1082–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.

ychen added a comment.Jul 6 2022, 4:11 PM

Thank you for working on this! Can you please add more details to the patch summary about the changes?

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.

ychen added inline comments.Jul 6 2022, 4:14 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
5191

Unfortunately, GCC is still using the old/non-conforming behavior. https://clang.godbolt.org/z/5K4916W71. What is the usual way to mitigate this?

aaron.ballman added inline comments.Jul 7 2022, 1:12 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
5191
ychen updated this revision to Diff 443756.Jul 11 2022, 2:17 PM
  • handle more cases to implement DR692, DR1395
  • tentatively implement DR1432 which DR692 needs, otherwise there would be regressions
ychen updated this revision to Diff 443771.Jul 11 2022, 3:21 PM
ychen marked an inline comment as done.
  • fix a bug
  • update comments
  • check-all, check-runtimes pass
ychen retitled this revision from [Sema] fix trailing parameter pack handling for function template partial ordering to [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template.Jul 11 2022, 3:44 PM
ychen edited the summary of this revision. (Show Details)
ychen added inline comments.Jul 11 2022, 3:50 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
2490

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.

5191

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?

ychen added inline comments.Jul 11 2022, 3:55 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
5191

I guess the practice is just committed and see? If it breaks important or many existing libraries, just revert or add compiler options?

I'm fairly optimistic considering check-runtimes passes.

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).

aaron.ballman added inline comments.Jul 12 2022, 6:58 AM
clang/lib/Sema/SemaTemplateDeduction.cpp
1110

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?

2490

Same question here about looking at just the last element.

5191

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?

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

I guess the practice is just committed and see? If it breaks important or many existing libraries, just revert or add compiler options?

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.

5408

You should end the anonymous namespace here per our coding style guidelines.

5432–5433

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?

5435–5436

Curiosity question -- can you make P1 and P2 be pointers to const without many other viral changes to existing code?

5453

Again, should you be looking at all the packs here and not just trailing ones?

clang/www/cxx_dr_status.html
4197

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).

8181

Same here

erichkeane added inline comments.Jul 12 2022, 7:24 AM
clang/lib/Sema/SemaTemplateDeduction.cpp
1110

This is definitely a concern to me as well, we probably have to check ALL packs and figure out what was deduced.

5207

This is still the same issue I think? This should probably be a 'parameters contains pack', not just checking the last.

ychen added a comment.Jul 12 2022, 1:05 PM

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.
cor3ntin added inline comments.
clang/lib/Sema/SemaTemplateDeduction.cpp
5453

If it's not trailing, it's not deduced. and afaik, if it's not deduced it can't be involved spacial specialization ordering

ychen updated this revision to Diff 444099.Jul 12 2022, 3:55 PM
ychen marked 4 inline comments as done.
  • add Clang ABI check
  • replace one NTTP with a local constexpr variable
  • style changes
ychen added inline comments.Jul 12 2022, 3:56 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
5191

That makes sense to me. Added the ABI compatibility check.

5432–5433

Ah, I didn't think of that. The caller has no need to override this.

5435–5436

isAtLeastAsSpecializedAs uses template instantiations which modify P1/P2.

ychen marked an inline comment as done.Jul 12 2022, 3:57 PM
aaron.ballman accepted this revision.Jul 13 2022, 6:34 AM

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
1110

Can you add CodeGen test coverage for both ABI modes (new RUN line with -fclang-abi-compat=14)

5435–5436

Good to know, then no changes needed here, thanks!

5451

Can you add CodeGen test coverage for both ABI modes (new RUN line with -fclang-abi-compat=14)

This revision is now accepted and ready to land.Jul 13 2022, 6:34 AM
erichkeane accepted this revision.Jul 13 2022, 6:36 AM

Nothing sticks out to me, LGTM too.

ychen updated this revision to Diff 444363.Jul 13 2022, 11:48 AM
  • add CodeGen tests
ychen added inline comments.Jul 13 2022, 11:50 AM
clang/lib/Sema/SemaTemplateDeduction.cpp
5451

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.

aaron.ballman accepted this revision.Jul 15 2022, 7:30 AM

LGTM!

clang/lib/Sema/SemaTemplateDeduction.cpp
5451

Yes, that test is what I was looking for, thank you!

ychen updated this revision to Diff 452564.Aug 14 2022, 2:35 PM
  • rebase
This revision was landed with ongoing or failed builds.Aug 14 2022, 2:38 PM
This revision was automatically updated to reflect the committed changes.

This is breaking us.

template <typename T, typename... Ts> struct S; // #1

template <typename T> struct S<T> {}; // #2

The 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?

ychen added a comment.Aug 16 2022, 5:44 PM

This is breaking us.

template <typename T, typename... Ts> struct S; // #1

template <typename T> struct S<T> {}; // #2

The 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?

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.

ychen added a comment.Aug 16 2022, 5:46 PM

This is breaking us.

template <typename T, typename... Ts> struct S; // #1

template <typename T> struct S<T> {}; // #2

The 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?

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.

Or do you mean the duction for partial ordering, then I did a simple S<int> a which seems fine?

This is breaking us.

template <typename T, typename... Ts> struct S; // #1

template <typename T> struct S<T> {}; // #2

The 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?

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.

Just that input is enough to get the error. I did clang++ test.cpp -fsyntax-only -fclang-abi-compat=14.0

ychen added a comment.Aug 16 2022, 6:06 PM

This is breaking us.

template <typename T, typename... Ts> struct S; // #1

template <typename T> struct S<T> {}; // #2

The 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?

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.

Just that input is enough to get the error. I did clang++ test.cpp -fsyntax-only -fclang-abi-compat=14.0

Thanks, it works now. I was missing -fclang-abi-compat=14.0.

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)

Stopping by - I assume this targets Clang 16 now, so the abi compat level should be adjusted to 15.

ychen added a comment.Aug 17 2022, 1:29 AM

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).

Fixed in b296aed8ae239c20e

ychen added a comment.Aug 17 2022, 1:33 AM

Stopping by - I assume this targets Clang 16 now, so the abi compat level should be adjusted to 15.

Sure, I'll make a new patch for that.

joanahalili added a subscriber: joanahalili.EditedAug 26 2022, 4:03 AM

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?

ychen added a comment.Aug 26 2022, 9:45 AM

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?

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.

ychen added a comment.EditedAug 26 2022, 1:06 PM

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?

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.

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.

ychen added a comment.Aug 26 2022, 1:11 PM

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?

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.

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.

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).

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;
}

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.

ychen added a comment.Oct 3 2022, 11:10 PM

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.

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.

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.

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.

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.

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.

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.

@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.

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.

ychen added a comment.Oct 4 2022, 11:00 AM

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?

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.

I think the consensus is some flag is needed to put it back to legacy behavior just in case.

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.

ychen added a comment.EditedOct 4 2022, 11:33 AM

I think the consensus is some flag is needed to put it back to legacy behavior just in case.

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.

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.

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);
ychen added a comment.Oct 4 2022, 11:54 AM

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...

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.

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.

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).

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.

(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.

ychen added a comment.Oct 11 2022, 5:06 PM

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.

(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.

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.

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?

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.

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.

Was this change in LLVM 15, or is it still unreleased?

This is still unreleased. It will be released in Clang16.

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.

Will do. Thanks for the heads-up.