diff mbox

[C++] PR 34892

Message ID 5088236A.4060408@oracle.com
State New
Headers show

Commit Message

Paolo Carlini Oct. 24, 2012, 5:20 p.m. UTC 
Hi,

a *very* old ICE on invalid, even a regression (from before variadic 
templates, I guess!). I tried various other tweaks, like catching the 
issue earlier but diagnostic quality decreases, too many cascading error 
messages. The below means I have to tweak only a couple of existing 
testcases, and I'm actually pretty happy with that, because we produce 
much less verbose diagnostic in both cases (1 error message less / 2 
respectively).

Tested x86_64-linux.

Thanks,
Paolo.

//////////////////////////
/cp
2012-10-24  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/34892
	* pt.c (coerce_template_parms): Check TREE_PURPOSE (parm)
	for error_mark_node.

/testsuite
2012-10-24  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/34892
	* g++.dg/template/crash114.C: New.
	* g++.dg/template/crash55.C: Tweak dg-error directive.
	* g++.dg/template/crash57.C: Likewise.

Comments

Jason Merrill Oct. 24, 2012, 5:30 p.m. UTC | #1 
On 10/24/2012 01:20 PM, Paolo Carlini wrote:
> +      if (parm == error_mark_node
> +	  || TREE_PURPOSE (parm) == error_mark_node)

It seems odd to bail out early if the default argument is bad even if we 
aren't trying to use it.  Doesn't it work to check this further down 
where we actually look at the default argument?

Jason
Paolo Carlini Oct. 24, 2012, 6:11 p.m. UTC | #2 
Hi,

On 10/24/2012 07:30 PM, Jason Merrill wrote:
> On 10/24/2012 01:20 PM, Paolo Carlini wrote:
>> +      if (parm == error_mark_node
>> +      || TREE_PURPOSE (parm) == error_mark_node)
>
> It seems odd to bail out early if the default argument is bad even if 
> we aren't trying to use it.  Doesn't it work to check this further 
> down where we actually look at the default argument?
The problem is that the first time we go through the loop, when parm_idx 
== 0 and TREE_PURPOSE is error_mark_node, the condition:

       if (template_parameter_pack_p (TREE_VALUE (parm))
       && !(arg && ARGUMENT_PACK_P (arg)))

near the beginning of the loop is true, thus we call 
coerce_template_parameter_pack, no error and we continue to the next 
iteration. The next iteration, arg == NULL_TREE and require_all_args is 
true, we actually use TREE_PURPOSE (parm) but for the next parm, when 
things are fine, then we get to the:

TREE_VEC_ELT (new_inner_args, arg_idx) = arg;

at the bottom of the loop and we are in big troubles because 
new_inner_args has only 1 element and arg_idx is 2.

Thus, in summary, it really seems to me that we should do *something* 
the *first* time through the loop, when currently we only use TREE_VALUE 
per the above and we don't notice that TREE_PURPOSE is error_mark_node. 
Did I explain myself well enough?

Thanks,
Paolo.
Jason Merrill Oct. 24, 2012, 7:53 p.m. UTC | #3 
On 10/24/2012 02:11 PM, Paolo Carlini wrote:
> The problem is that the first time we go through the loop, when parm_idx
> == 0 and TREE_PURPOSE is error_mark_node, the condition:
>
>        if (template_parameter_pack_p (TREE_VALUE (parm))
>        && !(arg && ARGUMENT_PACK_P (arg)))
>
> near the beginning of the loop is true

The parm shouldn't be a parameter pack in that case; the ... is part of 
the default argument, not the parameter.

Jason
Paolo Carlini Oct. 24, 2012, 8:20 p.m. UTC | #4 
On 10/24/2012 09:53 PM, Jason Merrill wrote:
> On 10/24/2012 02:11 PM, Paolo Carlini wrote:
>> The problem is that the first time we go through the loop, when parm_idx
>> == 0 and TREE_PURPOSE is error_mark_node, the condition:
>>
>>        if (template_parameter_pack_p (TREE_VALUE (parm))
>>        && !(arg && ARGUMENT_PACK_P (arg)))
>>
>> near the beginning of the loop is true
>
> The parm shouldn't be a parameter pack in that case; the ... is part 
> of the default argument, not the parameter.
Yes, I see that. Then it seems to me that we have to catch the problem 
(much) earlier...

Thanks,
Paolo.
diff mbox

Patch

Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 192762)
+++ cp/pt.c	(working copy)
@@ -6645,11 +6645,12 @@  coerce_template_parms (tree parms,
       /* Get the Ith template parameter.  */
       parm = TREE_VEC_ELT (parms, parm_idx);
  
-      if (parm == error_mark_node)
-      {
-        TREE_VEC_ELT (new_inner_args, arg_idx) = error_mark_node;
-        continue;
-      }
+      if (parm == error_mark_node
+	  || TREE_PURPOSE (parm) == error_mark_node)
+	{
+	  TREE_VEC_ELT (new_inner_args, arg_idx) = error_mark_node;
+	  continue;
+	}
 
       /* Calculate the next argument.  */
       if (arg_idx < nargs)
Index: testsuite/g++.dg/template/crash114.C
===================================================================
--- testsuite/g++.dg/template/crash114.C	(revision 0)
+++ testsuite/g++.dg/template/crash114.C	(working copy)
@@ -0,0 +1,5 @@ 
+// PR c++/34892
+
+template<int=..., int=0> struct A {}; // { dg-error "expected" }
+// { dg-message "variadic" "" { target c++98 } 3 }
+A<0> a;                               // { dg-error "type" }
Index: testsuite/g++.dg/template/crash55.C
===================================================================
--- testsuite/g++.dg/template/crash55.C	(revision 192762)
+++ testsuite/g++.dg/template/crash55.C	(working copy)
@@ -3,4 +3,4 @@ 
 template<typename class T, T = T()> // { dg-error "nested-name-specifier|two or more|valid type" }
 struct A {};
 
-template<int> void foo(A<int>);     // { dg-error "mismatch|constant|template argument" }
+template<int> void foo(A<int>);     // { dg-error "type|template" }
Index: testsuite/g++.dg/template/crash57.C
===================================================================
--- testsuite/g++.dg/template/crash57.C	(revision 192762)
+++ testsuite/g++.dg/template/crash57.C	(working copy)
@@ -7,4 +7,4 @@  template<typename> struct B
     template<int(> struct C;    // { dg-error "token" }
 };
 
-A<char> a;                      // { dg-error "type/value mismatch|constant|declaration" }
+A<char> a;                      // { dg-error "type" }