[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
https://github.com/jcsxky created https://github.com/llvm/llvm-project/pull/90646 attempt to fix https://github.com/llvm/llvm-project/issues/90349 Skip to add outer class template arguments to `MTAL` when the friend function has the same depth with its lexical context(`CXXRecordDecl`). >From e2785999ed38cd3b0addcf28d9122ecf8100d908 Mon Sep 17 00:00:00 2001 From: Qizhi Hu <836744...@qq.com> Date: Wed, 1 May 2024 02:25:04 +0800 Subject: [PATCH] [Clang][Sema] fix a bug on constraint check with template friend function --- clang/docs/ReleaseNotes.rst| 1 + clang/lib/Sema/SemaTemplateInstantiate.cpp | 14 +++ clang/test/SemaCXX/PR90349.cpp | 43 ++ 3 files changed, 58 insertions(+) create mode 100644 clang/test/SemaCXX/PR90349.cpp diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index c11f117cd6e8b4..ec10c82a3a5403 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -627,6 +627,7 @@ Bug Fixes to C++ Support - Fix a bug on template partial specialization with issue on deduction of nontype template parameter whose type is `decltype(auto)`. Fixes (#GH68885). - Clang now correctly treats the noexcept-specifier of a friend function to be a complete-class context. +- Fix a bug on constraint check with template friend function. Fixes (#GH90349). Bug Fixes to AST Handling ^ diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 3a9fd906b7af86..1805f8f6e5ad90 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -281,6 +281,20 @@ Response HandleFunction(Sema &SemaRef, const FunctionDecl *Function, if (Function->getPrimaryTemplate()->isMemberSpecialization()) return Response::Done(); +if (Function->getFriendObjectKind()) + if (const ClassTemplateSpecializationDecl *TD = + dyn_cast( + Function->getLexicalDeclContext())) { +const CXXRecordDecl *TemplatePattern = +TD->getTemplateInstantiationPattern(); +const FunctionDecl *FunctionPattern = +Function->getTemplateInstantiationPattern(); +if (TemplatePattern && FunctionPattern && +TemplatePattern->getTemplateDepth() == +FunctionPattern->getTemplateDepth()) + return Response::Done(); + } + // If this function is a generic lambda specialization, we are done. if (!ForConstraintInstantiation && isGenericLambdaCallOperatorOrStaticInvokerSpecialization(Function)) { diff --git a/clang/test/SemaCXX/PR90349.cpp b/clang/test/SemaCXX/PR90349.cpp new file mode 100644 index 00..6a4b5c21e88f3b --- /dev/null +++ b/clang/test/SemaCXX/PR90349.cpp @@ -0,0 +1,43 @@ +// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s + +// expected-no-diagnostics + +namespace std { +template +concept floating_point = __is_same(T,double) || __is_same(T,float); + +template +concept integral = __is_same(T,int); + +} + +template +class Blob; + +template +Blob MakeBlob(); + +template +class Blob { +private: +Blob() {} + +friend Blob MakeBlob(); +}; + +template +Blob MakeBlob() +{ +return Blob(); +} + +template +Blob FindBlobs() +{ +return MakeBlob(); +} + +int main(int argc, const char * argv[]) { +FindBlobs(); +return 0; +} ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
llvmbot wrote: @llvm/pr-subscribers-clang Author: Qizhi Hu (jcsxky) Changes attempt to fix https://github.com/llvm/llvm-project/issues/90349 Skip to add outer class template arguments to `MTAL` when the friend function has the same depth with its lexical context(`CXXRecordDecl`). --- Full diff: https://github.com/llvm/llvm-project/pull/90646.diff 3 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (+1) - (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+14) - (added) clang/test/SemaCXX/PR90349.cpp (+43) ``diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index c11f117cd6e8b4..ec10c82a3a5403 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -627,6 +627,7 @@ Bug Fixes to C++ Support - Fix a bug on template partial specialization with issue on deduction of nontype template parameter whose type is `decltype(auto)`. Fixes (#GH68885). - Clang now correctly treats the noexcept-specifier of a friend function to be a complete-class context. +- Fix a bug on constraint check with template friend function. Fixes (#GH90349). Bug Fixes to AST Handling ^ diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 3a9fd906b7af86..1805f8f6e5ad90 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -281,6 +281,20 @@ Response HandleFunction(Sema &SemaRef, const FunctionDecl *Function, if (Function->getPrimaryTemplate()->isMemberSpecialization()) return Response::Done(); +if (Function->getFriendObjectKind()) + if (const ClassTemplateSpecializationDecl *TD = + dyn_cast( + Function->getLexicalDeclContext())) { +const CXXRecordDecl *TemplatePattern = +TD->getTemplateInstantiationPattern(); +const FunctionDecl *FunctionPattern = +Function->getTemplateInstantiationPattern(); +if (TemplatePattern && FunctionPattern && +TemplatePattern->getTemplateDepth() == +FunctionPattern->getTemplateDepth()) + return Response::Done(); + } + // If this function is a generic lambda specialization, we are done. if (!ForConstraintInstantiation && isGenericLambdaCallOperatorOrStaticInvokerSpecialization(Function)) { diff --git a/clang/test/SemaCXX/PR90349.cpp b/clang/test/SemaCXX/PR90349.cpp new file mode 100644 index 00..6a4b5c21e88f3b --- /dev/null +++ b/clang/test/SemaCXX/PR90349.cpp @@ -0,0 +1,43 @@ +// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s + +// expected-no-diagnostics + +namespace std { +template +concept floating_point = __is_same(T,double) || __is_same(T,float); + +template +concept integral = __is_same(T,int); + +} + +template +class Blob; + +template +Blob MakeBlob(); + +template +class Blob { +private: +Blob() {} + +friend Blob MakeBlob(); +}; + +template +Blob MakeBlob() +{ +return Blob(); +} + +template +Blob FindBlobs() +{ +return MakeBlob(); +} + +int main(int argc, const char * argv[]) { +FindBlobs(); +return 0; +} `` https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
sdkrystian wrote: I'm actually working on constraint checking for function template specializations in #88963. I don't think this patch is quite right... this will cause a crash if the befriended function is a member of a class template specialization. Relative to the changes in #88963, I believe the correct fix would be to change line 278 to: ```cpp if (RelativeToPrimary && (Function->getTemplateSpecializationKind() == TSK_ExplicitSpecialization || (Function->getFriendObjectKind() && !Function->getPrimaryTemplate()->getFriendObjectKind( return Response::UseNextDecl(Function); ``` I added a commit to #88963 which makes this change (be79079507ffbd9b29683498f405dc2c32dd8ba7) https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
jcsxky wrote: > I'm actually working on constraint checking for function template > specializations in #88963. I don't think this patch is quite right... this > will cause a crash if the befriended function is a member of a class template > specialization. Relative to the changes in #88963, I believe the correct fix > would be to change line 278 to: > > ```c++ > if (RelativeToPrimary && > (Function->getTemplateSpecializationKind() == > TSK_ExplicitSpecialization || > (Function->getFriendObjectKind() && > !Function->getPrimaryTemplate()->getFriendObjectKind( > return Response::UseNextDecl(Function); > ``` > > I added a commit to #88963 which makes this change > ([be79079](https://github.com/llvm/llvm-project/commit/be79079507ffbd9b29683498f405dc2c32dd8ba7)) > > cc @erichkeane Only when the friend function doesn't use any other new template parameters, i.e. their depth is equal can we skip to add the outer arguments to `MTAL`. > this will cause a crash if the befriended function is a member of a class > template specialization Could you please provide a testcase? https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
jcsxky wrote: Windows CI failed with some unrelated files. https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
https://github.com/erichkeane commented: This seems reasonable, @mizvekov is doing more work in this area, so I'd like him to make sure this isn't something he's fixed elsewhere. https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
sdkrystian wrote: ```cpp template concept D = sizeof(T) == sizeof(U); template struct A { template requires D static void f(); }; template struct B { template struct C { friend void A::f(); }; }; template struct B::C; extern template void A::f(); // crash here ``` https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
jcsxky wrote: > ```c++ > template > concept D = sizeof(T) == sizeof(U); > > template > struct A > { > template requires D > static void f(); > }; > > template > struct B > { > template > struct C > { > friend void A::f(); > }; > }; > > template struct B::C; > > extern template void A::f(); // crash here > ``` > > @jcsxky causes crash with and without this patch applied. Thanks for your feedback! This may be another issue. clang trunk crashes with this case. https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
mizvekov wrote: I agree with @sdkrystian, even though the test crashes for maybe yet another reason, it demonstrates you can friend a function from a different template context, so comparing the depths from different branches is not helpful. https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
sdkrystian wrote: FWIW, the changes in be79079507ffbd9b29683498f405dc2c32dd8ba7 fix the crash https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
https://github.com/jcsxky updated https://github.com/llvm/llvm-project/pull/90646 >From 50aa5b64f6d2cf98706bfb4792f274bd071e3b9c Mon Sep 17 00:00:00 2001 From: Qizhi Hu <836744...@qq.com> Date: Wed, 1 May 2024 02:25:04 +0800 Subject: [PATCH 1/2] [Clang][Sema] fix a bug on constraint check with template friend function --- clang/docs/ReleaseNotes.rst| 1 + clang/lib/Sema/SemaTemplateInstantiate.cpp | 14 +++ clang/test/SemaCXX/PR90349.cpp | 43 ++ 3 files changed, 58 insertions(+) create mode 100644 clang/test/SemaCXX/PR90349.cpp diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 2c5308fbcb319a..ac90e3933798cb 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -630,6 +630,7 @@ Bug Fixes to C++ Support - Fix a bug on template partial specialization with issue on deduction of nontype template parameter whose type is `decltype(auto)`. Fixes (#GH68885). - Clang now correctly treats the noexcept-specifier of a friend function to be a complete-class context. +- Fix a bug on constraint check with template friend function. Fixes (#GH90349). Bug Fixes to AST Handling ^ diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 3a9fd906b7af86..1805f8f6e5ad90 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -281,6 +281,20 @@ Response HandleFunction(Sema &SemaRef, const FunctionDecl *Function, if (Function->getPrimaryTemplate()->isMemberSpecialization()) return Response::Done(); +if (Function->getFriendObjectKind()) + if (const ClassTemplateSpecializationDecl *TD = + dyn_cast( + Function->getLexicalDeclContext())) { +const CXXRecordDecl *TemplatePattern = +TD->getTemplateInstantiationPattern(); +const FunctionDecl *FunctionPattern = +Function->getTemplateInstantiationPattern(); +if (TemplatePattern && FunctionPattern && +TemplatePattern->getTemplateDepth() == +FunctionPattern->getTemplateDepth()) + return Response::Done(); + } + // If this function is a generic lambda specialization, we are done. if (!ForConstraintInstantiation && isGenericLambdaCallOperatorOrStaticInvokerSpecialization(Function)) { diff --git a/clang/test/SemaCXX/PR90349.cpp b/clang/test/SemaCXX/PR90349.cpp new file mode 100644 index 00..6a4b5c21e88f3b --- /dev/null +++ b/clang/test/SemaCXX/PR90349.cpp @@ -0,0 +1,43 @@ +// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s + +// expected-no-diagnostics + +namespace std { +template +concept floating_point = __is_same(T,double) || __is_same(T,float); + +template +concept integral = __is_same(T,int); + +} + +template +class Blob; + +template +Blob MakeBlob(); + +template +class Blob { +private: +Blob() {} + +friend Blob MakeBlob(); +}; + +template +Blob MakeBlob() +{ +return Blob(); +} + +template +Blob FindBlobs() +{ +return MakeBlob(); +} + +int main(int argc, const char * argv[]) { +FindBlobs(); +return 0; +} >From 3d27f11d79b00608bf9135bcc4b4cb859027933a Mon Sep 17 00:00:00 2001 From: Qizhi Hu <836744...@qq.com> Date: Thu, 2 May 2024 20:05:10 +0800 Subject: [PATCH 2/2] continue to handle context instead of finish --- clang/lib/Sema/SemaTemplateInstantiate.cpp | 2 +- clang/test/SemaCXX/PR90349.cpp | 24 ++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 1805f8f6e5ad90..22789c72b2c90f 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -292,7 +292,7 @@ Response HandleFunction(Sema &SemaRef, const FunctionDecl *Function, if (TemplatePattern && FunctionPattern && TemplatePattern->getTemplateDepth() == FunctionPattern->getTemplateDepth()) - return Response::Done(); + return Response::UseNextDecl(Function); } // If this function is a generic lambda specialization, we are done. diff --git a/clang/test/SemaCXX/PR90349.cpp b/clang/test/SemaCXX/PR90349.cpp index 6a4b5c21e88f3b..570a49fd2073bb 100644 --- a/clang/test/SemaCXX/PR90349.cpp +++ b/clang/test/SemaCXX/PR90349.cpp @@ -41,3 +41,27 @@ int main(int argc, const char * argv[]) { FindBlobs(); return 0; } + +template +concept D = sizeof(T) == sizeof(U); + +template +struct A +{ +template requires D +static void f(); +}; + +template +struct B +{ +template +struct C +{ +friend void A::f(); +}; +}; + +template struct B::C; + +extern template void A::f(); // crash here ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mai
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
jcsxky wrote: > I agree with @sdkrystian, even though the test crashes for maybe yet another > reason, it demonstrates you can friend a function from a different template > context, so comparing the depths from different branches is not helpful. @mizvekov I missed the case which friend function is declared in an inner class, and [3d27f11](https://github.com/llvm/llvm-project/pull/90646/commits/3d27f11d79b00608bf9135bcc4b4cb859027933a) demonstrates the comparison works fine. https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
sdkrystian wrote: This still doesn't handle cases such as: ```cpp template concept E = sizeof(T) == sizeof(U) && sizeof(V) == sizeof(W); template // T = char struct A { template // U = signed char struct B { template // V = int, W = int, X = short requires E // incorrectly substitutes T = int, U = short, V = int, V = int static void f(); }; }; template // T = int, U = int struct C { template // V = short struct D { friend void A::B::f(); }; }; template struct C::D; extern template void A::B::f(); ``` (which is handled by be79079507ffbd9b29683498f405dc2c32dd8ba7) https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] fix a bug on constraint check with template friend function (PR #90646)
jcsxky wrote: > This still doesn't handle cases such as: > > ```c++ > template > concept E = sizeof(T) == sizeof(U) && sizeof(V) == sizeof(W); > > template // T = char > struct A > { > template // U = signed char > struct B > { > template // V = int, W = int, X = > short > requires E // incorrectly substitutes T = int, U = short, > V = int, V = int > static void f(); > }; > }; > > > template // T = int, U = int > struct C > { > template // V = short > struct D > { > friend void A::B::f(); > }; > }; > > template struct C::D; > > extern template void A::B::f(); > ``` > > (which is handled by > [be79079](https://github.com/llvm/llvm-project/commit/be79079507ffbd9b29683498f405dc2c32dd8ba7)) Thanks for your patience and testcases! They really make me understand these related issues more clearly. https://github.com/llvm/llvm-project/pull/90646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits