[Clang] add missing ClangABICompat check

[ychen]

Summary

In the same spirit of da6187f566b7881cb. This maintains compatibility
with GCC at the C++ language level. Nothing is changed in the traditional ABI sense, i.e. layout/mangling etc.

[ychen]

Address comments in https://reviews.llvm.org/D128745/new/#3809692.

[probinson]

Anytime there's a Clang ABI version check, I have to wonder whether PS4/PS5 need an exception. (And I'm sorry I missed the earlier patches in this series.) We are pretty much locked to however things worked for Clang 3.2.
Is this specific to C++20 (or later)? In that case the question gets more complicated because we don't yet support C++20. But I see the predecessors of this patch are referring to DRs, and as we all know the C++ committee loves to retroactively redefine earlier versions of the language...
So I don't know what to think at the moment. It's late and I don't have the mental energy to pursue this right at the moment.
I'll try to get back and look more closely tomorrow.

[probinson]

Hi @ychen I saw you updated the patch description, thanks.
IIUC, the change causes some difference in the interpretation of particular template stuff. Would this be clearer if the BEFORE-15/AFTER-15 test cases used the same template construct? Currently they are different so it's hard to work out the effect of the change. In particular, naively I see different mangled names which makes me concerned that there _is_ in fact a net change to mangling.

I'll observe that D128745 updated the release notes, but not in the most useful way; it cites DRs without any explanation of what they do, and with no links.

[ychen]

In D134507#3811488, @probinson wrote:

Would this be clearer if the BEFORE-15/AFTER-15 test cases used the same template construct? Currently they are different so it's hard to work out the effect of the change. In particular, naively I see different mangled names which makes me concerned that there _is_ in fact a net change to mangling.

Understood. It is hard because the BEFORE-15 and AFTER-15 cases have compilation failures when the ClangABICompat switch is different and checking the IR requires it to compile cleanly. The mangling actually does not change for the same function type. If the mangling changes, then the (instantiated) function type is different at the souce-code level. It is a little bit different from the real ABI that the same source-code construct has a different binary representation (memory/symbol) etc.

I'll observe that D128745 updated the release notes, but not in the most useful way; it cites DRs without any explanation of what they do, and with no links.

Indeed. I'll submit a follow-up patch to improve that.

[probinson]

It feels odd to use a ClangABI check for something that is affecting what source is accepted, but this is not my area of expertise.
@aaron.ballman or @rjmccall would probably be the right people to weigh in on this.

[ychen]

In D134507#3817928, @probinson wrote:

It feels odd to use a ClangABI check for something that is affecting what source is accepted, but this is not my area of expertise.
@aaron.ballman or @rjmccall would probably be the right people to weigh in on this.

This was discussed here https://reviews.llvm.org/D128745#inline-1244757. Yeah, it is somewhat confusing to key the legacy language behavior on ClangABI. I'm not sure there are better choices than inventing new flags.

[rjmccall]

I don't think I agree that changes to template partial ordering rules are properly understood as breaking changes to the ABI. They're breaking changes to the *language behavior*, and the ABI break is downstream of that, just it would be if, say, adding two floats suddenly produced a double. I think the right approach for Clang is to gate this by language version and add a note somewhere in the documentation that C++23 changed the overload resolution rules around function templates and that this means adopting C++23 can be ABI-breaking. The same has been true of many other language revisions in the past. Fortunately, it should only be ABI-breaking (or source-breaking) in rare circumstances, which is presumably why the C++ committee felt it could get away with this change.

[probinson]

In D134507#3818765, @ychen wrote:

In D134507#3817928, @probinson wrote:

It feels odd to use a ClangABI check for something that is affecting what source is accepted, but this is not my area of expertise.
@aaron.ballman or @rjmccall would probably be the right people to weigh in on this.

This was discussed here https://reviews.llvm.org/D128745#inline-1244757. Yeah, it is somewhat confusing to key the legacy language behavior on ClangABI. I'm not sure there are better choices than inventing new flags.

Right, which points to https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp#L9786 concerning the treatment of copy constructors. But those at least can feed into how one goes about conjuring up calling sequences involving parameters of types that might have to invoke those constructors, and so that case seems a lot more ABI-relevant than subtle details about template parameter packs.

Anyway, I'm going to step out of this now, I don't have the expertise to review this responsibly.

[ychen]

ping.

To avoid confusion, note that this and D128745 are not about C++20 or newer language versions. They are implementing DRs which should apply to all language versions >= C++11 since the variadic template is involved.

ClangABICompat is used here and in a few other places by D128745. If an alternative method is needed, I could switch them over all at once in another patch. How about we land this first and see how far this goes if it is bad, confusing, etc., feel free to leave a comment here or in a bug report, I could address it promptly.

[rjmccall]

DRs do not modify existing language versions. We can decide as a vendor whether to apply DRs to existing language versions. The fact that this is a language semantics change makes me think that we should not in this case.

[ychen]

In D134507#3831951, @rjmccall wrote:

DRs do not modify existing language versions. We can decide as a vendor whether to apply DRs to existing language versions. The fact that this is a language semantics change makes me think that we should not in this case.

Sounds good to me. @aaron.ballman do you agree? If so, I'll make a follow-up patch to make the DRs conditional on C++20.

[rjmccall]

I left a comment on the old review thread; probably best to center the conversation there.

[ychen]

Per further discussions in D128745, this is not needed.

[tahonermann]

Per further discussions in D128745, this is not needed.

I agree that the code change is not needed. I think it is worth keeping the test though (modified to verify the expected ambiguity).

[ychen]

In D134507#3856956, @tahonermann wrote:

Per further discussions in D128745, this is not needed.

I agree that the code change is not needed. I think it is worth keeping the test though (modified to verify the expected ambiguity).

Agreed. It turns out the ambiguous case is already in place. (https://github.com/llvm/llvm-project/blob/d8af31ecede0c54ec667ab687784149e806c9e4c/clang/test/CXX/drs/dr6xx.cpp#L1104)

[tahonermann]

Agreed. It turns out the ambiguous case is already in place. (https://github.com/llvm/llvm-project/blob/d8af31ecede0c54ec667ab687784149e806c9e4c/clang/test/CXX/drs/dr6xx.cpp#L1104)

Ah, perfect!

clang/lib/Sema/SemaTemplateDeduction.cpp

FunctionTemplateDecl *Sema::getMoreSpecializedTemplate(
   //   function parameter pack, then F is more specialized than G.
   FunctionDecl *FD1 = FT1->getTemplatedDecl();
   FunctionDecl *FD2 = FT2->getTemplatedDecl();
   unsigned NumParams1 = FD1->getNumParams();
   unsigned NumParams2 = FD2->getNumParams();
   bool Variadic1 = NumParams1 && FD1->parameters().back()->isParameterPack();
   bool Variadic2 = NumParams2 && FD2->parameters().back()->isParameterPack();
   if (Variadic1 != Variadic2) {
+    bool ClangABICompat15 = Context.getLangOpts().getClangABICompat() <=
+                            LangOptions::ClangABI::Ver15;
+    if (ClangABICompat15)
+      return Variadic1 ? FT2 : FT1;
     if (Variadic1 && NumParams1 > NumParams2)
       return FT2;
     if (Variadic2 && NumParams2 > NumParams1)
       return FT1;
   }
 
   return JudgeByConstraints();
 }

clang/test/CodeGen/partial-order-variadic.cpp

-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15 %s -emit-llvm -disable-llvm-passes -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -emit-llvm -disable-llvm-passes -o - | FileCheck %s --check-prefixes=CHECK,AFTER-15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fclang-abi-compat=15 \
+// RUN:     -DCLANG_ABI_COMPAT=15 %s -emit-llvm -disable-llvm-passes -o - \
+// RUN:     | FileCheck %s --check-prefixes=CHECK,BEFORE-15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -emit-llvm \
+// RUN:     -disable-llvm-passes -o - \
+// RUN:     | FileCheck %s --check-prefixes=CHECK,AFTER-15
 
 // CHECK: %struct.S = type { i8 }
 // CHECK: @_Z2ggiRi
 // CHECK: @_Z1gIiJEERiPT_DpT0_
 template <typename T, typename... U> int &g(T *, U...);
 template <typename T> void g(T);
 template <typename T, typename... Ts> struct S;
 template <typename T> struct S<T> {};
 void gg(int i, int &r) {
   r = g(&i);
   S<int> a;
 }
 
 // CHECK: @_Z1hIJiEEvDpPT_
 template<class ...T> void h(T*...) {}
 template<class T>    void h(const T&) {}
 template void h(int*);
 
-#if !defined(CLANG_ABI_COMPAT)
+#if defined(CLANG_ABI_COMPAT)
+
+// BEFORE-15: _Z2g2IiJEEvT_DpT0_
+template <class T, class... U> void g2(T, U...) {};
+template <class T> void g2(T, int x = 0) {};
+template void g2(int);
+
+#else
 
 // AFTER-15: @_Z1fIiJEEvPT_DpT0_
 template<class T, class... U> void f(T*, U...){}
 template<class T> void f(T){}
 template void f(int*);
 
 template<class T, class... U> struct A;
 template<class T1, class T2, class... U> struct A<T1,T2*,U...> {};
 template<class T1, class T2> struct A<T1,T2>;
 template struct A<int, int*>;
 
 #endif