diff mbox series

*PING* [PATCH] c++: fix cases of core1001/1322 by not dropping cv-qualifier of function parameter of type of typename or decltype[PR101402,PR102033,PR102034,PR102039,PR102044]

Message ID DM5PR0501MB3733B7CEBDE5E578EF5244ACB5A49@DM5PR0501MB3733.namprd05.prod.outlook.com
State New
Headers show
Series *PING* [PATCH] c++: fix cases of core1001/1322 by not dropping cv-qualifier of function parameter of type of typename or decltype[PR101402,PR102033,PR102034,PR102039,PR102044] | expand

Commit Message

nick huang Sept. 24, 2021, 11:14 a.m. UTC 
These bugs are considered duplicate cases of PR51851 which has been suspended 
since 2012, an issue known as "core1001/1322". Considering this background, 
it deserves a long comment to explain.

Many people believed the root cause of this family of bugs is related with 
the nature of how and when the array type is converted to pointer type during 
function signature is calculated. This is true, but we may need to go into details 
to understand the exact reason.

There is a pattern for these bugs(PR101402,PR102033,PR102034,PR102039). In the 
template function declaration, the function parameter is consisted of a "const" 
followed by a typename-type which is actually an array type. According to 
standard, function signature is calculated by dropping so-called 
"top-level-cv-qualifier". As a result, the templater specialization complains 
no matching to declaration can be found because specialization has const and 
template function declaration doesn't have const which is dropped as mentioned.
Obviously the template function declaration should NOT drop the const. But why?
Let's review the procedure of standard first.
(https://timsong-cpp.github.io/cppwp/dcl.fct#5.sentence-3)

"After determining the type of each parameter, any parameter of type “array of T” 
or of function type T is adjusted to be “pointer to T”. After producing the list 
of parameter types, any top-level cv-qualifiers modifying a parameter type are 
deleted when forming the function type."

Please note the action of deleting top-level cv-qualifiers happens at last stage 
after array type is converted to pointer type. More importantly, there are two 
conditions:
a) Each type must be able to be determined.
b) The cv-qualifier must be top-level.
Let's analysis if these two conditions can be met one by one.
1) Keyword "typename" indicates inside template it involves dependent name
 (https://timsong-cpp.github.io/cppwp/n4659/temp.res#2) for which the name lookup 
can be postponed until template instantiation. Clearly the type of dependent 
name cannot be determined without name lookup. Then we can NOT proceed to next 
step until concrete template argument type is determined during specialization. 
2) After “array of T” is converted to “pointer to T”, the cv-qualifiers are no 
longer top-level! Unfortunately in standard there is no definition 
of "top-level". Mr. Dan Saks's articals (https://www.dansaks.com/articles.shtml) 
are tremendous help! Especially this wonderful paper (https://www.dansaks.com/articles/2000-02%20Top-Level%20cv-Qualifiers%20in%20Function%20Parameters.pdf)  
discusses this topic in details. In one short sentence, the "const" before 
array type is NOT top-level-cv-qualifier and should NOT be dropped.

So, understanding the root cause makes the fix very clear: Let's NOT drop 
cv-qualifier for typename-type inside template. Leave this task for template
substitution later when template specialization locks template argument types.

Similarly inside template, "decltype" may also include dependent name and 
the best strategy for parser is to preserve all original declaration and 
postpone the task till template substitution.

Here is an interesting observation to share. Originally my fix is trying to 
use function "resolve_typename_type" to see if the "typename-type" is indeed
an array type so as to decide whether the const should be dropped. It works 
for cases of PR101402,PR102033(with a small fix of function), but cannot 
succeed on cases of PR102034,PR102039. Especially PR102039 is impossible 
because it depends on template argument. This helps me realize that parser 
should not do any work if it cannot be 100% successful. All can wait.

At last I want to acknowledge other efforts to tackle this core 1001/1322 from 
PR92010 which is an irreplaceable different approach from this fix by doing 
rebuilding template function signature during template substitution stage. 
After all, this fix can only deal with dependent type started with "typename"
or "decltype" which is not the case of pr92010.
 

gcc/cp/ChangeLog:

2021-08-30  qingzhe huang  <nickhuang99@hotmail.com>

        * decl.c (grokparms):

gcc/testsuite/ChangeLog:

2021-08-30  qingzhe huang  <nickhuang99@hotmail.com>

        * g++.dg/parse/pr101402.C: New test.
        * g++.dg/parse/pr102033.C: New test.
        * g++.dg/parse/pr102034.C: New test.
        * g++.dg/parse/pr102039.C: New test.
        * g++.dg/parse/pr102044.C: New test.

Comments

Jason Merrill Sept. 24, 2021, 7:11 p.m. UTC | #1 
I already responded to this patch:

https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579527.html
nick huang Sept. 25, 2021, 7:15 p.m. UTC | #2 
Hi Jason,

Sorry for not noticing your response.

>>Unfortunately, your patch breaks
>>
>>template <class T>
>>struct A
>>{
>>   void f(T);
>>};
>>template <class T>
>>void A<T>::f(const T)
>>{ }

1. Perhaps I misunderstand your comment, but my patch does NOT fix this issue. Neither does current GCC fix this code, if it is incorrect grammatically. In other words,  tested in www.godbolt.org and all major compilers accept it.  And my patch does NOT affects GCC current behavior, meaning my patched GCC still accepts it like now. So, I don't think my patch breaks it because this code is never fixed. (Whether this code is correct or not, I keep an open mind.)


>>which is certainly questionable code, but is currently also accepted by 
>>clang and EDG compilers.

2. Like I said above, I am not even sure if it is an issue since most compilers accept it. But my patch does NOT touch this issue at all. In essential, my patch ONLY deals with dependent type starting with keyword "typename" or "decltype". Just like my email subject, my patch only fixes one particular case of this "family" of bugs, that is "typename" or "decltype" cases.

>>Why doesn't the PR92010 fix address these testcases as well?

3. PR92010 creates new functions of "rebuild_function_or_method_type" and by using gdb to trace PR101402 code as following:

template<class T> struct A {
     typedef T arr[3];
};
template<class T> void f(const typename A<T>::arr) { }    // #1
template void f<int>(const A<int>::arr);                                   // #2

I added some print function declaration code before and after calling "maybe_rebuild_function_decl_type" to print out its parameter "r" which is function declaration inside "tsubst_function_decl". 
Here is the result:
a) Before calling, the function declaration is "void f(int*)" and after calling, it is adjusted to correct one as "void f(const int*)". However, after this line "SET_DECL_IMPLICIT_INSTANTIATION (r);",  it fallback to original dependent type as "void f(typename A<T>::arr) [with T = int; typename A<T>::arr = int [3]]" till end. This completely defeats the purpose of template substitution effort.  

b) On the other hand, by screening input parameter of "rebuild_function_or_method_type", I can see it is using an incorrect function type as  "void(typename A<T>::arr)" which already drops "const" prematurely.  This is due to previous #1 template function declaration parsing. So, This gives us the clear idea of the root cause of this kind of bugs. It happens during template function declaration stage #1 when producing template declarator.  Instead of doing a later-correction-effort in PR92010, my patch tries to at least avoid dropping "const" in case of "typename" and "decltype" during template function declaration stage in #1.   

Best regards,

Qingzhe Huang

From: Jason Merrill <jason@redhat.com>
Sent: September 24, 2021 3:11 PM
To: nick huang <nickhuang99@hotmail.com>; gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
Subject: Re: *PING* [PATCH] c++: fix cases of core1001/1322 by not dropping cv-qualifier of function parameter of type of typename or decltype[PR101402,PR102033,PR102034,PR102039,PR102044] 
 
I already responded to this patch:

https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579527.html
nick huang Sept. 27, 2021, 3:40 a.m. UTC | #3 
>>template <class T>
>>struct A
>>{
>>   void f(T);
>>};
>>
>>template <class T>
>>void A<T>::f(const T)
>>{ }
>>
>>which is certainly questionable code, but is currently also accepted by 
>>clang and EDG compilers.

I just found out that clang actually correctly reject this code during specialization. 
(https://www.godbolt.org/z/evjvhqqoo)
It all depends on explicit argument which may turn "const" into top-level-cv-qualifier
or not.
For example, when argument is "int[3]", the function signature of specialization 
will have to keep the const because it is no longer top-level cv qualifier.

template<>
void A<int[3]>::f(const int*){} // not matching declaration "void(int*)"

Obviously when explicit argument is "int", the const would be dropped and specialization matches
declaration. i.e.
template<>
void A<int>::f(int){} // this matches declaration "void(int)"

So, clang is correct to NOT reject template definition when there is no specialization yet.

As a comparison, GCC is not able to reject  incorrect specialization. Should we file this bug? Or just add this 
as test case to original 1001 core issue?

Again, my patch cannot deal this case as it is not "typename". We may have to fix "tsubst_function_decl" to see any workaround of line "SET_DECL_IMPLICIT_INSTANTIATION (r);" 
(See my previous email https://gcc.gnu.org/pipermail/gcc-patches/2021-September/580260.html)
This macro "DECL_USE_TEMPLATE" is set to 1. However, the comment says "1" is for "implicit specialization", but we are actually dealing with "partial or explicit specialization" which is "2".

Here I quote comment from cp-tree.h

/* Nonzero iff NODE is a specialization of a template.  The value
   indicates the type of specializations:

     1=implicit instantiation

     2=partial or explicit specialization, e.g.:

        template <> int min<int> (int, int),

     3=explicit instantiation, e.g.:

        template int min<int> (int, int);

   Note that NODE will be marked as a specialization even if the
   template it is instantiating is not a primary template.  For
   example, given:

     template <typename T> struct O {
       void f();
       struct I {};
     };

   both O<int>::f and O<int>::I will be marked as instantiations.

   If DECL_USE_TEMPLATE is nonzero, then DECL_TEMPLATE_INFO will also
   be non-NULL.  */
Jason Merrill Sept. 28, 2021, 2 p.m. UTC | #4 
On 9/25/21 15:15, nick huang wrote:
>>> Why doesn't the PR92010 fix address these testcases as well?
> 
> 3. PR92010 creates new functions of "rebuild_function_or_method_type" and by using gdb to trace PR101402 code as following:
> 
> template<class T> struct A {
>       typedef T arr[3];
> };
> template<class T> void f(const typename A<T>::arr) { }    // #1
> template void f<int>(const A<int>::arr);                                   // #2
> 
> I added some print function declaration code before and after calling "maybe_rebuild_function_decl_type" to print out its parameter "r" which is function declaration inside "tsubst_function_decl".
> Here is the result:
> a) Before calling, the function declaration is "void f(int*)" and after calling, it is adjusted to correct one as "void f(const int*)". However, after this line "SET_DECL_IMPLICIT_INSTANTIATION (r);",  it fallback to original dependent type as "void f(typename A<T>::arr) [with T = int; typename A<T>::arr = int [3]]" till end. This completely defeats the purpose of template substitution effort.

That's just an artifact of (bug in) how we print it as template+args 
once it's marked as an instantiation; the actual type of the function 
returned from tsubst_function_decl is still void (const int*).

The problem seems to come when we get back to determine_specialization, 
where we have

>           // Then, try to form the new function type.                                                          
> =>        insttype = tsubst (TREE_TYPE (fn), targs, tf_fndecl_type, NULL_TREE);

which does the wrong substitution again, and not the correct one from 
maybe_rebuild_function_decl_type.

Both this substitution check and the constraint check just before it 
seem redundant with the checks we already did in fn_type_unification, so 
the right fix may be to just remove the broken ones here in 
determine_specialization.

Jason
diff mbox series

Patch

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index e0c603aaab6..940c43ce707 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -14384,7 +14384,16 @@  grokparms (tree parmlist, tree *parms)
 
           /* Top-level qualifiers on the parameters are
              ignored for function types.  */
-         type = cp_build_qualified_type (type, 0);
+
+         int type_quals = 0;
+         /* Inside template declaration, typename and decltype indicating
+            dependent name and cv-qualifier are preserved until
+            template instantiation.
+            PR101402/PR102033/PR102034/PR102039/PR102044 */
+         if (processing_template_decl
+               && (TREE_CODE (type) == TYPENAME_TYPE || TREE_CODE (type) == DECLTYPE_TYPE))
+                 type_quals = CP_TYPE_CONST_P(type);
+         type = cp_build_qualified_type (type, type_quals);
           if (TREE_CODE (type) == METHOD_TYPE)
             {
               error ("parameter %qD invalidly declared method type", decl);
diff --git a/gcc/testsuite/g++.dg/parse/pr101402.C b/gcc/testsuite/g++.dg/parse/pr101402.C
new file mode 100644
index 00000000000..58d9c4f8542
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/pr101402.C
@@ -0,0 +1,5 @@ 
+template<class T> struct A {
+     typedef T arr[3];
+};
+template<class T> void f(const typename A<T>::arr) { } // #1
+template void f<int>(const A<int>::arr);
diff --git a/gcc/testsuite/g++.dg/parse/pr102033.C b/gcc/testsuite/g++.dg/parse/pr102033.C
new file mode 100644
index 00000000000..0d5cc17620f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/pr102033.C
@@ -0,0 +1,34 @@ 
+/* {dg-do compile } */
+/* {dg-options "-std=c++11" } */
+
+namespace test1
+{
+template<class TA>
+struct A{
+        template<class TB>
+        using Type=TB[3];
+};
+template<class TA, class TB>
+void f(const typename A<TA>::template Type<TB>){}
+template <>
+void f<int, char>(const typename A<int>::template Type<char>){}
+}
+namespace test2
+{
+template<class TA>
+struct A{
+        template<class TB>
+        struct B{
+                using TB_Alias=TB;
+                template<class TC=TB_Alias>
+                struct C{
+                        typedef TC Arr3[3];
+                };
+        };
+};
+template<class TA, class TB>
+void f(const typename A<TA>::template B<TB>::template C<>::Arr3){}
+template <>
+void f<int, char>(const typename A<int>::template B<char>::template C<>::Arr3){}
+}
+
diff --git a/gcc/testsuite/g++.dg/parse/pr102034.C b/gcc/testsuite/g++.dg/parse/pr102034.C
new file mode 100644
index 00000000000..37fdce52912
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/pr102034.C
@@ -0,0 +1,13 @@ 
+/*{dg-do compile} */
+template<class TA>
+struct A{
+        template<class TB>
+        struct B{
+        typedef TB Arr3[3];
+        };
+};
+template<class TA, class TB>
+void f(const typename A<TA>::template B<TB>::Arr3){}
+template <>
+void f<int, char>(const typename A<int>::B<char>::Arr3){}
+
diff --git a/gcc/testsuite/g++.dg/parse/pr102039.C b/gcc/testsuite/g++.dg/parse/pr102039.C
new file mode 100644
index 00000000000..25d3e77fd74
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/pr102039.C
@@ -0,0 +1,24 @@ 
+namespace test1
+{
+struct A{
+        typedef int Arr3[3];
+};
+
+template<class T>
+void f(const typename T::Arr3){}
+
+template<>
+void f<A>(const int[3]){}
+}
+
+namespace test2
+{
+struct A{
+        typedef int Arr3[3];
+};
+template<class T>
+void f(const typename T::Arr3){}
+template<>
+void f<A>(const int*){}
+}
+
diff --git a/gcc/testsuite/g++.dg/parse/pr102044.C b/gcc/testsuite/g++.dg/parse/pr102044.C
new file mode 100644
index 00000000000..bef6a920b47
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/pr102044.C
@@ -0,0 +1,32 @@ 
+/* {dg-do compile } */
+/* {dg-options "-std=c++11" } */
+namespace test1
+{
+template<unsigned int N, class T>
+void f(const T[N]){}
+
+template<unsigned int N, class T>
+using fPtr=decltype(f<N,T>)*;
+
+template<unsigned int N, class T>
+fPtr<N,T> af[N]={&f<N,T>};
+
+template<unsigned int N, class T>
+void g(const decltype(af<N,T>)){}
+
+template<>
+void g<1,int>(const fPtr<1,int>[1]){}
+}
+
+namespace test2
+{
+template <class T>
+struct A{
+T arr3[3];
+};
+template <class T>
+void f(const decltype(A<T>::arr3)){}
+template <>
+void f<int>(const int[3]){}
+}
+