[C++ Patch] PR 70635 ([4.9/5/6 Regression] ICE on (and rejects) valid code...)

[C++ Patch] PR 70635 ([4.9/5/6 Regression] ICE on (and rejects) valid code...)

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

Hi,

in this regression we have an infinite recursion affecting the 
same_type_p call at parser.c:25125 which I added in the patch for 
c++/38313. The issue is that for, eg, the testcase at issue, we are 
passing a TYPENAME_TYPE to same_type_p. I think we can simply handle the 
problem by checking first that we have a CLASS_TYPE_P as TREE_TYPE 
(type_decl). Tested x86_64-linux.

Thanks,
Paolo.

/////////////////////////////

[-- Attachment #2: CL_70635 --]
[-- Type: text/plain, Size: 305 bytes --]

/cp
2016-04-12  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/70635
	* parser.c (cp_parser_constructor_declarator_p): Only use same_type_p
	for true CLASS_TYPE_P (TREE_TYPE (type_decl)).

/testsuite
2016-04-12  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/70635
	* g++.dg/parse/pr70635.C: New.

[-- Attachment #3: patch_70635 --]
[-- Type: text/plain, Size: 1301 bytes --]

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 234874)
+++ cp/parser.c	(working copy)
@@ -25118,8 +25122,9 @@ cp_parser_constructor_declarator_p (cp_parser *par
       constructor_p = (!cp_parser_error_occurred (parser)
 		       && (outside_class_specifier_p
 			   || type_decl == error_mark_node
-			   || same_type_p (current_class_type,
-					   TREE_TYPE (type_decl))));
+			   || (CLASS_TYPE_P (TREE_TYPE (type_decl))
+			       && same_type_p (current_class_type,
+					       TREE_TYPE (type_decl)))));
 
       /* If we're still considering a constructor, we have to see a `(',
 	 to begin the parameter-declaration-clause, followed by either a
Index: testsuite/g++.dg/parse/pr70635.C
===================================================================
--- testsuite/g++.dg/parse/pr70635.C	(revision 0)
+++ testsuite/g++.dg/parse/pr70635.C	(working copy)
@@ -0,0 +1,23 @@
+// PR c++/70635
+// { dg-options "-fpermissive -w" }
+
+template < typename T > 
+struct A
+{
+  struct B;
+  typedef typename B::type type;
+};
+
+template < typename T > 
+struct A < T >::B
+{
+  typedef typename A < type >::type type;
+  type Foo ();
+};
+
+template < typename T > 
+typename A < T >::B::type
+A < T >::B::Foo ()
+{
+  return 0;
+}

Re: [C++ Patch] PR 70635 ([4.9/5/6 Regression] ICE on (and rejects) valid code...)

[Jason Merrill]

On 04/12/2016 09:25 AM, Paolo Carlini wrote:
> in this regression we have an infinite recursion affecting the
> same_type_p call at parser.c:25125 which I added in the patch for
> c++/38313. The issue is that for, eg, the testcase at issue, we are
> passing a TYPENAME_TYPE to same_type_p. I think we can simply handle the
> problem by checking first that we have a CLASS_TYPE_P as TREE_TYPE
> (type_decl). Tested x86_64-linux.

I don't see how this can be considered valid code; the typenames are 
indeed mutually recursive, with no real underlying type anywhere.  clang 
and EDG both reject the testcase if the template is instantiated. 
Please add an instantiation to the testcase.  And we should fix the 
recursion issue in structural_comptypes.

Jason

Re: [C++ Patch] PR 70635 ([4.9/5/6 Regression] ICE on (and rejects) valid code...)

[Paolo Carlini]

Hi,

On 12/04/2016 15:50, Jason Merrill wrote:
> On 04/12/2016 09:25 AM, Paolo Carlini wrote:
>> in this regression we have an infinite recursion affecting the
>> same_type_p call at parser.c:25125 which I added in the patch for
>> c++/38313. The issue is that for, eg, the testcase at issue, we are
>> passing a TYPENAME_TYPE to same_type_p. I think we can simply handle the
>> problem by checking first that we have a CLASS_TYPE_P as TREE_TYPE
>> (type_decl). Tested x86_64-linux.
>
> I don't see how this can be considered valid code; the typenames are 
> indeed mutually recursive, with no real underlying type anywhere.  
> clang and EDG both reject the testcase if the template is 
> instantiated. Please add an instantiation to the testcase.  And we 
> should fix the recursion issue in structural_comptypes.
Ok. But then, if I understand correctly, our user-visible behavior 
should not change much: accept the testcase (modulo the -fpermissive 
detail) without an instantiation and reject it when the template is 
actually instantiated. In other terms, exactly what EDG and clang also 
do. Thus same_type_p should be fixed to simply return false in such 
recursive cases without ICEing and nothing more, right? I'm not sure I 
will be able to do that in time for 6.0 but I'm going to try...

Thanks,
Paolo.

Re: [C++ Patch] PR 70635 ([4.9/5/6 Regression] ICE on (and rejects) valid code...)

[Jason Merrill]

On 04/13/2016 03:36 AM, Paolo Carlini wrote:
> Hi,
>
> On 12/04/2016 15:50, Jason Merrill wrote:
>> On 04/12/2016 09:25 AM, Paolo Carlini wrote:
>>> in this regression we have an infinite recursion affecting the
>>> same_type_p call at parser.c:25125 which I added in the patch for
>>> c++/38313. The issue is that for, eg, the testcase at issue, we are
>>> passing a TYPENAME_TYPE to same_type_p. I think we can simply handle the
>>> problem by checking first that we have a CLASS_TYPE_P as TREE_TYPE
>>> (type_decl). Tested x86_64-linux.
>>
>> I don't see how this can be considered valid code; the typenames are
>> indeed mutually recursive, with no real underlying type anywhere.
>> clang and EDG both reject the testcase if the template is
>> instantiated. Please add an instantiation to the testcase.  And we
>> should fix the recursion issue in structural_comptypes.

...but your patch is OK; even if it doesn't fix the real problem, it 
isn't wrong.  Only the testcase should be adjusted.

> Ok. But then, if I understand correctly, our user-visible behavior
> should not change much: accept the testcase (modulo the -fpermissive
> detail) without an instantiation and reject it when the template is
> actually instantiated. In other terms, exactly what EDG and clang also
> do.

The testcase without an instantiation is ill-formed, no diagnostic 
required, so we shouldn't test for accepting it, we should only test for 
rejecting the variant with an instantiation.

> Thus same_type_p should be fixed to simply return false in such
> recursive cases without ICEing and nothing more, right? I'm not sure I
> will be able to do that in time for 6.0 but I'm going to try...

resolve_typename_type should give up in such cases.

Jason

Re: [C++ Patch] PR 70635 ([4.9/5/6 Regression] ICE on (and rejects) valid code...)

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]

Hi again,

On 13/04/2016 16:42, Jason Merrill wrote:
> On 04/13/2016 03:36 AM, Paolo Carlini wrote:
>> Hi,
>>
>> On 12/04/2016 15:50, Jason Merrill wrote:
>>> On 04/12/2016 09:25 AM, Paolo Carlini wrote:
>>>> in this regression we have an infinite recursion affecting the
>>>> same_type_p call at parser.c:25125 which I added in the patch for
>>>> c++/38313. The issue is that for, eg, the testcase at issue, we are
>>>> passing a TYPENAME_TYPE to same_type_p. I think we can simply 
>>>> handle the
>>>> problem by checking first that we have a CLASS_TYPE_P as TREE_TYPE
>>>> (type_decl). Tested x86_64-linux.
>>>
>>> I don't see how this can be considered valid code; the typenames are
>>> indeed mutually recursive, with no real underlying type anywhere.
>>> clang and EDG both reject the testcase if the template is
>>> instantiated. Please add an instantiation to the testcase. And we
>>> should fix the recursion issue in structural_comptypes.
>
> ...but your patch is OK; even if it doesn't fix the real problem, it 
> isn't wrong.  Only the testcase should be adjusted.
>
>> Ok. But then, if I understand correctly, our user-visible behavior
>> should not change much: accept the testcase (modulo the -fpermissive
>> detail) without an instantiation and reject it when the template is
>> actually instantiated. In other terms, exactly what EDG and clang also
>> do.
>
> The testcase without an instantiation is ill-formed, no diagnostic 
> required, so we shouldn't test for accepting it, we should only test 
> for rejecting the variant with an instantiation.
Agreed, now I really get your point: in the future certainly it would 
not be wrong to somehow provide diagnostic for a testcase without the 
instantiation too. A testcase with the instantiation is robust.

As it happens, anyway, the below exceedingly trivial patch avoids the 
ICE in resolve_typename_type and appears to pass testing (is already 
past g++.dg/tm/tm.exp...). Should I apply it instead together with the 
amended testcase? Weird that nobody noticed the pair of typos before!?!

Thanks,
Paolo.

//////////////////////

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 597 bytes --]

Index: pt.c
===================================================================
--- pt.c	(revision 234938)
+++ pt.c	(working copy)
@@ -23598,9 +23598,9 @@ resolve_typename_type (tree type, bool only_curren
     {
       /* Ill-formed programs can cause infinite recursion here, so we
 	 must catch that.  */
-      TYPENAME_IS_RESOLVING_P (type) = 1;
+      TYPENAME_IS_RESOLVING_P (result) = 1;
       result = resolve_typename_type (result, only_current_p);
-      TYPENAME_IS_RESOLVING_P (type) = 0;
+      TYPENAME_IS_RESOLVING_P (result) = 0;
     }
   
   /* Qualify the resulting type.  */

Re: [C++ Patch] PR 70635 ([4.9/5/6 Regression] ICE on (and rejects) valid code...)

[Jason Merrill]

On 04/13/2016 11:05 AM, Paolo Carlini wrote:
> As it happens, anyway, the below exceedingly trivial patch avoids the
> ICE in resolve_typename_type and appears to pass testing (is already
> past g++.dg/tm/tm.exp...). Should I apply it instead together with the
> amended testcase? Weird that nobody noticed the pair of typos before!?!

OK.

Jason

Re: [C++ Patch] PR 70635 ([4.9/5/6 Regression] ICE on (and rejects) valid code...)

[Paolo Carlini]

Hi,

On 13/04/2016 17:27, Jason Merrill wrote:
> On 04/13/2016 11:05 AM, Paolo Carlini wrote:
>> As it happens, anyway, the below exceedingly trivial patch avoids the
>> ICE in resolve_typename_type and appears to pass testing (is already
>> past g++.dg/tm/tm.exp...). Should I apply it instead together with the
>> amended testcase? Weird that nobody noticed the pair of typos before!?!
> OK.
Leaving alone gcc-4_9-branch, I was considering backporting to 
gcc-5-branch and closing the bug. What do you think?

Thanks,
Paolo.

Re: [C++ Patch] PR 70635 ([4.9/5/6 Regression] ICE on (and rejects) valid code...)

[Jason Merrill]

On 05/03/2016 01:15 PM, Paolo Carlini wrote:
> Hi,
>
> On 13/04/2016 17:27, Jason Merrill wrote:
>> On 04/13/2016 11:05 AM, Paolo Carlini wrote:
>>> As it happens, anyway, the below exceedingly trivial patch avoids the
>>> ICE in resolve_typename_type and appears to pass testing (is already
>>> past g++.dg/tm/tm.exp...). Should I apply it instead together with the
>>> amended testcase? Weird that nobody noticed the pair of typos before!?!
>> OK.
> Leaving alone gcc-4_9-branch, I was considering backporting to
> gcc-5-branch and closing the bug. What do you think?

Makes sense to me.

Jason