diff mbox

[C++,Patch/RFC] PR 59115

Message ID 5343D83E.3010700@oracle.com
State New
Headers show

Commit Message

Paolo Carlini April 8, 2014, 11:06 a.m. UTC
Hi,

I have been working on this issue, a minor regression, looking for the 
safest fix. What I have got so far seems neat enough to me, but maybe 
not totally safe (from the diagnostic quality point of view only, of 
course).

The ICE happens in unify, in:

       /* Check for mixed types and values.  */
       if ((TREE_CODE (parm) == TEMPLATE_TYPE_PARM
        && TREE_CODE (tparm) != TYPE_DECL)
       || (TREE_CODE (parm) == TEMPLATE_TEMPLATE_PARM
           && TREE_CODE (tparm) != TEMPLATE_DECL))
     gcc_unreachable ();

because, after the meaningful error message about the float non-type 
template parameter, unify wrongly handles, for parm == U, tparm == int. 
Obviously something is wrong with the indexing. This is because 
process_template_parm, upon error, early returns a chainon (list, 
err_parm_list), where TREE_VALUE (err_parm_list) == error_mark_node, 
thus no index information (build_template_parm_index is not used at 
all). This means that, afterward, when process_template_parm processes 
the next template parameters, the code at the beginning isn't really 
able to figure out the index of tree_last (list) and all the following 
template parameters have it off by one (*). Thus my idea of returning to 
the changes in r116473, which included the addition of the early 
returns, to not append simply an error_mark_node as parm, but the actual 
parm with its TREE_TYPE replaced by an error_mark_node. This way the 
indexes are computed normally and unify doesn't get confused. Besides 
that, I went through the rest of r116473 and made sure we use 
error_operand_p, to catch the TREE_TYPEs (I also added 3/4 more besides 
those in that commit, which seemed obviously correct). What do you think?

Tested x86_64-linux.

Thanks,
Paolo.

(*) Interestingly, replacing the block at beginning of 
process_template_parm with something simply using list_length would 
**almost** work: the only regressions are -std=c++1y, due to the calls 
by synthesize_implicit_template_parm, which tracks the end of the 
template parameters list for compile-time performance reasons, thus 
list_length doesn't match the actual length of the whole list as stored 
as an index in tree_last. Looking forward, we should probably consider 
handling consistently both real and synthesized parameters.

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

Comments

Jason Merrill April 8, 2014, 6:06 p.m. UTC | #1
OK.

Jason
diff mbox

Patch

Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 209212)
+++ cp/pt.c	(working copy)
@@ -414,7 +414,7 @@  push_inline_template_parms_recursive (tree parmlis
     {
       tree parm = TREE_VALUE (TREE_VEC_ELT (parms, i));
 
-      if (parm == error_mark_node)
+      if (error_operand_p (parm))
 	continue;
 
       gcc_assert (DECL_P (parm));
@@ -2829,7 +2829,7 @@  comp_template_parms (const_tree parms1, const_tree
 
           /* If either of the template parameters are invalid, assume
              they match for the sake of error recovery. */
-          if (parm1 == error_mark_node || parm2 == error_mark_node)
+          if (error_operand_p (parm1) || error_operand_p (parm2))
             return 1;
 
 	  if (TREE_CODE (parm1) != TREE_CODE (parm2))
@@ -3640,11 +3640,7 @@  reduce_template_parm_level (tree index, tree type,
    to the LIST being built.  This new parameter is a non-type
    parameter iff IS_NON_TYPE is true. This new parameter is a
    parameter pack iff IS_PARAMETER_PACK is true.  The location of PARM
-   is in PARM_LOC. NUM_TEMPLATE_PARMS is the size of the template
-   parameter list PARM belongs to. This is used used to create a
-   proper canonical type for the type of PARM that is to be created,
-   iff PARM is a type.  If the size is not known, this parameter shall
-   be set to 0.  */
+   is in PARM_LOC.  */
 
 tree
 process_template_parm (tree list, location_t parm_loc, tree parm,
@@ -3652,7 +3648,6 @@  process_template_parm (tree list, location_t parm_
 {
   tree decl = 0;
   tree defval;
-  tree err_parm_list;
   int idx = 0;
 
   gcc_assert (TREE_CODE (parm) == TREE_LIST);
@@ -3673,8 +3668,6 @@  process_template_parm (tree list, location_t parm_
 
       ++idx;
     }
-  else
-    idx = 0;
 
   if (is_non_type)
     {
@@ -3682,39 +3675,29 @@  process_template_parm (tree list, location_t parm_
 
       SET_DECL_TEMPLATE_PARM_P (parm);
 
-      if (TREE_TYPE (parm) == error_mark_node)
-        {
-          err_parm_list = build_tree_list (defval, parm);
-          TREE_VALUE (err_parm_list) = error_mark_node;
-	   return chainon (list, err_parm_list);
-        }
-      else
-      {
-	/* [temp.param]
+      if (TREE_TYPE (parm) != error_mark_node)
+	{
+	  /* [temp.param]
 
-	   The top-level cv-qualifiers on the template-parameter are
-	   ignored when determining its type.  */
-	TREE_TYPE (parm) = TYPE_MAIN_VARIANT (TREE_TYPE (parm));
-	if (invalid_nontype_parm_type_p (TREE_TYPE (parm), 1))
-          {
-            err_parm_list = build_tree_list (defval, parm);
-            TREE_VALUE (err_parm_list) = error_mark_node;
-	     return chainon (list, err_parm_list);
-          }
+	     The top-level cv-qualifiers on the template-parameter are
+	     ignored when determining its type.  */
+	  TREE_TYPE (parm) = TYPE_MAIN_VARIANT (TREE_TYPE (parm));
+	  if (invalid_nontype_parm_type_p (TREE_TYPE (parm), 1))
+	    TREE_TYPE (parm) = error_mark_node;
+	  else if (uses_parameter_packs (TREE_TYPE (parm))
+		   && !is_parameter_pack
+		   /* If we're in a nested template parameter list, the template
+		      template parameter could be a parameter pack.  */
+		   && processing_template_parmlist == 1)
+	    {
+	      /* This template parameter is not a parameter pack, but it
+		 should be. Complain about "bare" parameter packs.  */
+	      check_for_bare_parameter_packs (TREE_TYPE (parm));
 
-        if (uses_parameter_packs (TREE_TYPE (parm)) && !is_parameter_pack
-	    /* If we're in a nested template parameter list, the template
-	       template parameter could be a parameter pack.  */
-	    && processing_template_parmlist == 1)
-	  {
-	    /* This template parameter is not a parameter pack, but it
-	       should be. Complain about "bare" parameter packs.  */
-	    check_for_bare_parameter_packs (TREE_TYPE (parm));
-	    
-	    /* Recover by calling this a parameter pack.  */
-	    is_parameter_pack = true;
-	  }
-      }
+	      /* Recover by calling this a parameter pack.  */
+	      is_parameter_pack = true;
+	    }
+	}
 
       /* A template parameter is not modifiable.  */
       TREE_CONSTANT (parm) = 1;
@@ -5127,7 +5110,7 @@  redeclare_class_template (tree type, tree parms)
         continue;
 
       tmpl_parm = TREE_VALUE (TREE_VEC_ELT (tmpl_parms, i));
-      if (tmpl_parm == error_mark_node)
+      if (error_operand_p (tmpl_parm))
 	return false;
 
       parm = TREE_VALUE (TREE_VEC_ELT (parms, i));
@@ -6087,8 +6070,8 @@  coerce_template_template_parm (tree parm,
                               tree in_decl,
                               tree outer_args)
 {
-  if (arg == NULL_TREE || arg == error_mark_node
-      || parm == NULL_TREE || parm == error_mark_node)
+  if (arg == NULL_TREE || error_operand_p (arg)
+      || parm == NULL_TREE || error_operand_p (parm))
     return 0;
   
   if (TREE_CODE (arg) != TREE_CODE (parm))
@@ -6181,7 +6164,7 @@  coerce_template_template_parms (tree parm_parms,
     {
       parm = TREE_VALUE (TREE_VEC_ELT (parm_parms, nparms - 1));
       
-      if (parm == error_mark_node)
+      if (error_operand_p (parm))
 	return 0;
 
       switch (TREE_CODE (parm))
@@ -17517,7 +17500,7 @@  unify (tree tparms, tree targs, tree parm, tree ar
     case TEMPLATE_TEMPLATE_PARM:
     case BOUND_TEMPLATE_TEMPLATE_PARM:
       tparm = TREE_VALUE (TREE_VEC_ELT (tparms, 0));
-      if (tparm == error_mark_node)
+      if (error_operand_p (tparm))
 	return unify_invalid (explain_p);
 
       if (TEMPLATE_TYPE_LEVEL (parm)
@@ -17535,7 +17518,7 @@  unify (tree tparms, tree targs, tree parm, tree ar
       idx = TEMPLATE_TYPE_IDX (parm);
       targ = TREE_VEC_ELT (INNERMOST_TEMPLATE_ARGS (targs), idx);
       tparm = TREE_VALUE (TREE_VEC_ELT (tparms, idx));
-      if (tparm == error_mark_node)
+      if (error_operand_p (tparm))
 	return unify_invalid (explain_p);
 
       /* Check for mixed types and values.  */
@@ -17718,7 +17701,7 @@  unify (tree tparms, tree targs, tree parm, tree ar
 
     case TEMPLATE_PARM_INDEX:
       tparm = TREE_VALUE (TREE_VEC_ELT (tparms, 0));
-      if (tparm == error_mark_node)
+      if (error_operand_p (tparm))
 	return unify_invalid (explain_p);
 
       if (TEMPLATE_PARM_LEVEL (parm)
Index: testsuite/g++.dg/template/crash119.C
===================================================================
--- testsuite/g++.dg/template/crash119.C	(revision 0)
+++ testsuite/g++.dg/template/crash119.C	(working copy)
@@ -0,0 +1,8 @@ 
+// PR c++/59115
+
+template<typename T, float, int, typename U> void foo(T, U) {} // { dg-error "valid type" }
+
+void bar()
+{
+  foo(0, 0);  // { dg-error "matching" }
+}