Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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).

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