[C++] PR 84092 ("[8 Regression] ICE on C++14 code with variable template: in build_qualified_name, at cp/tree.c:2043")

Message ID 308f9750-52ed-4cd3-2340-50d189b24349@oracle.com
State New
Headers show
Series
  • [C++] PR 84092 ("[8 Regression] ICE on C++14 code with variable template: in build_qualified_name, at cp/tree.c:2043")
Related show

Commit Message

Paolo Carlini Jan. 29, 2018, 3:45 p.m.
Hi,

the fix for c++/81236 removed some special code for dependent_p from 
finish_id_expression, and now finish_qualified_id_expr is used for this 
snippet too. Then special code at the beginning of the latter takes care 
of calling build_qualified_name to create the relevant SCOPE_REF. 
Therefore it seems to me that - unless we really want to return an 
OFFSET_REF - at that point we are done, we don't want to get to the end 
of the following long conditional and call again build_qualified_name on 
the SCOPE_REF and ICE. We don't need convert_from_reference either 
because build_qualified_name is passed a null type. Finishing testing 
(in the library) on x86_64-linux.

Thanks! Paolo.

////////////////////////
/cp
2018-01-29  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/84092
	* semantics.c (finish_qualified_id_expr): Maybe early return when handling
	an UNBOUND_CLASS_TEMPLATE.

/testsuite
2018-01-29  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/84092
	* g++.dg/cpp1y/var-templ57.C: New.

Comments

Jason Merrill Jan. 29, 2018, 8:41 p.m. | #1
On Mon, Jan 29, 2018 at 10:45 AM, Paolo Carlini
<paolo.carlini@oracle.com> wrote:
> the fix for c++/81236 removed some special code for dependent_p from

> finish_id_expression, and now finish_qualified_id_expr is used for this

> snippet too. Then special code at the beginning of the latter takes care of

> calling build_qualified_name to create the relevant SCOPE_REF. Therefore it

> seems to me that - unless we really want to return an OFFSET_REF - at that

> point we are done, we don't want to get to the end of the following long

> conditional and call again build_qualified_name on the SCOPE_REF and ICE. We

> don't need convert_from_reference either because build_qualified_name is

> passed a null type. Finishing testing (in the library) on x86_64-linux.


Hmm, it seems to me that the later code would handle this case fine
if, instead of calling build_qualified_name here, we do

qualifying_class = TYPE_CONTEXT (expr);
expr = TYPE_IDENTIFIER (expr);

Does that work?

Jason
Paolo Carlini Jan. 29, 2018, 10 p.m. | #2
Hi,

On 29/01/2018 21:41, Jason Merrill wrote:
> On Mon, Jan 29, 2018 at 10:45 AM, Paolo Carlini

> <paolo.carlini@oracle.com> wrote:

>> the fix for c++/81236 removed some special code for dependent_p from

>> finish_id_expression, and now finish_qualified_id_expr is used for this

>> snippet too. Then special code at the beginning of the latter takes care of

>> calling build_qualified_name to create the relevant SCOPE_REF. Therefore it

>> seems to me that - unless we really want to return an OFFSET_REF - at that

>> point we are done, we don't want to get to the end of the following long

>> conditional and call again build_qualified_name on the SCOPE_REF and ICE. We

>> don't need convert_from_reference either because build_qualified_name is

>> passed a null type. Finishing testing (in the library) on x86_64-linux.

> Hmm, it seems to me that the later code would handle this case fine

> if, instead of calling build_qualified_name here, we do

>

> qualifying_class = TYPE_CONTEXT (expr);

> expr = TYPE_IDENTIFIER (expr);

>

> Does that work?

Indeed it appears to work great and would make for a nice 
simplification, thanks! Testing the below is already past the C++ 
testsuite. Ok if it fully passes?

Thanks again,
Paolo.

//////////////////
Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 257159)
+++ cp/semantics.c	(working copy)
@@ -2001,12 +2001,12 @@ finish_qualified_id_expr (tree qualifying_class,
   if (template_p)
     {
       if (TREE_CODE (expr) == UNBOUND_CLASS_TEMPLATE)
-	/* cp_parser_lookup_name thought we were looking for a type,
-	   but we're actually looking for a declaration.  */
-	expr = build_qualified_name (/*type*/NULL_TREE,
-				     TYPE_CONTEXT (expr),
-				     TYPE_IDENTIFIER (expr),
-				     /*template_p*/true);
+	{
+	  /* cp_parser_lookup_name thought we were looking for a type,
+	     but we're actually looking for a declaration.  */
+	  qualifying_class = TYPE_CONTEXT (expr);
+	  expr = TYPE_IDENTIFIER (expr);
+	}
       else
 	check_template_keyword (expr);
     }
Index: testsuite/g++.dg/cpp1y/var-templ57.C
===================================================================
--- testsuite/g++.dg/cpp1y/var-templ57.C	(nonexistent)
+++ testsuite/g++.dg/cpp1y/var-templ57.C	(working copy)
@@ -0,0 +1,4 @@
+// PR c++/84092
+// { dg-do compile { target c++14 } }
+
+template < typename T > int a (T::template b);
Jason Merrill Jan. 31, 2018, 3:40 p.m. | #3
OK.

On Mon, Jan 29, 2018 at 5:00 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,

>

> On 29/01/2018 21:41, Jason Merrill wrote:

>>

>> On Mon, Jan 29, 2018 at 10:45 AM, Paolo Carlini

>> <paolo.carlini@oracle.com> wrote:

>>>

>>> the fix for c++/81236 removed some special code for dependent_p from

>>> finish_id_expression, and now finish_qualified_id_expr is used for this

>>> snippet too. Then special code at the beginning of the latter takes care

>>> of

>>> calling build_qualified_name to create the relevant SCOPE_REF. Therefore

>>> it

>>> seems to me that - unless we really want to return an OFFSET_REF - at

>>> that

>>> point we are done, we don't want to get to the end of the following long

>>> conditional and call again build_qualified_name on the SCOPE_REF and ICE.

>>> We

>>> don't need convert_from_reference either because build_qualified_name is

>>> passed a null type. Finishing testing (in the library) on x86_64-linux.

>>

>> Hmm, it seems to me that the later code would handle this case fine

>> if, instead of calling build_qualified_name here, we do

>>

>> qualifying_class = TYPE_CONTEXT (expr);

>> expr = TYPE_IDENTIFIER (expr);

>>

>> Does that work?

>

> Indeed it appears to work great and would make for a nice simplification,

> thanks! Testing the below is already past the C++ testsuite. Ok if it fully

> passes?

>

> Thanks again,

> Paolo.

>

> //////////////////

Patch

Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 257139)
+++ cp/semantics.c	(working copy)
@@ -2001,12 +2001,16 @@  finish_qualified_id_expr (tree qualifying_class,
   if (template_p)
     {
       if (TREE_CODE (expr) == UNBOUND_CLASS_TEMPLATE)
-	/* cp_parser_lookup_name thought we were looking for a type,
-	   but we're actually looking for a declaration.  */
-	expr = build_qualified_name (/*type*/NULL_TREE,
-				     TYPE_CONTEXT (expr),
-				     TYPE_IDENTIFIER (expr),
-				     /*template_p*/true);
+	{
+	  /* cp_parser_lookup_name thought we were looking for a type,
+	     but we're actually looking for a declaration.  */
+	  expr = build_qualified_name (/*type*/NULL_TREE,
+				       TYPE_CONTEXT (expr),
+				       TYPE_IDENTIFIER (expr),
+				       /*template_p*/true);
+	  if (!(address_p && done))
+	    return expr;
+	}
       else
 	check_template_keyword (expr);
     }
Index: testsuite/g++.dg/cpp1y/var-templ57.C
===================================================================
--- testsuite/g++.dg/cpp1y/var-templ57.C	(nonexistent)
+++ testsuite/g++.dg/cpp1y/var-templ57.C	(working copy)
@@ -0,0 +1,4 @@ 
+// PR c++/84092
+// { dg-do compile { target c++14 } }
+
+template < typename T > int a (T::template b);