[C++,Patch/RFC] PR 89900 ("[9 Regression] ICE: Segmentation fault (in check_instantiated_arg)")

Message ID 71f91c87-d690-89d8-4275-798214c86df4@oracle.com
State New
Headers show
Series
  • [C++,Patch/RFC] PR 89900 ("[9 Regression] ICE: Segmentation fault (in check_instantiated_arg)")
Related show

Commit Message

Paolo Carlini April 11, 2019, 3:20 p.m.
Hi,

over the last few days I spent some time on this regression, which at 
first seemed just a minor error-recovery issue, but then I noticed that 
very slightly tweeking the original testcase uncovered a pretty serious 
ICE on valid:

template<typename SX, typename ...XE> void
fk (XE..., int/*SW*/);

void
w9 (void)
{
   fk<int> (0);
}

The regression has to do with the changes committed by Jason for 
c++/86932, in particular with the condition in coerce_template_parms:

    if (template_parameter_pack_p (TREE_VALUE (parm))
       && (arg || !(complain & tf_partial))
       && !(arg && ARGUMENT_PACK_P (arg)))

which has the additional (arg || !complain & tf_partial)) false for the 
present testcase, thus the null arg is not changed into an empty pack, 
thus later  instantiate_template calls check_instantiated_args which 
finds it still null and crashes. Now, likely some additional analysis is 
in order, but for sure there is an important difference between the 
testcase which came with c++/86932 and the above: non-type vs type 
template parameter pack. It seems to me that the kind of problem fixed 
in c++/86932 cannot occur with type packs, because it boils down to a 
reference to a previous parm (full disclosure: the comments and logic in 
fixed_parameter_pack_p helped me a lot here). Thus I had the idea of 
simply restricting the scope of the new condition above by adding an || 
TREE_CODE (TREE_VALUE (parm)) == TYPE_DECL, which definitely leads to a 
clean testsuite and a proper behavior on the new testcases, AFAICS. I'm 
attaching what I tested on x86_64-linux.

Thanks, Paolo.

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

Comments

Paolo Carlini April 12, 2019, 9:49 a.m. | #1
.. an additional observation. Over the last couple of days I wondered 
if the amended testcase was really valid, given the non-terminal 
parameter pack, beyond the evidence that all the compilers I have at 
hand accept it. Note anyway, that we - and all the compilers - already 
accept a version of the testcase without the explicit argument and 
deduce the pack as empty:

template<typename ...XE> void
fk (XE..., int);

void
w9 (void)
{
   fk (0);
}

Thus, it seems to me that at least we have a consistency issue, at some 
level. Are we already "inadvertently" implementing P0478R0 or I'm 
missing something else?!?

Paolo.
Jason Merrill April 12, 2019, 6:29 p.m. | #2
On 4/11/19 11:20 AM, Paolo Carlini wrote:
> Hi,

> 

> over the last few days I spent some time on this regression, which at 

> first seemed just a minor error-recovery issue, but then I noticed that 

> very slightly tweeking the original testcase uncovered a pretty serious 

> ICE on valid:

> 

> template<typename SX, typename ...XE> void

> fk (XE..., int/*SW*/);

> 

> void

> w9 (void)

> {

>    fk<int> (0);

> }

> 

> The regression has to do with the changes committed by Jason for 

> c++/86932, in particular with the condition in coerce_template_parms:

> 

>     if (template_parameter_pack_p (TREE_VALUE (parm))

>        && (arg || !(complain & tf_partial))

>        && !(arg && ARGUMENT_PACK_P (arg)))

> 

> which has the additional (arg || !complain & tf_partial)) false for the 

> present testcase, thus the null arg is not changed into an empty pack, 

> thus later  instantiate_template calls check_instantiated_args which 

> finds it still null and crashes. Now, likely some additional analysis is 

> in order, but for sure there is an important difference between the 

> testcase which came with c++/86932 and the above: non-type vs type 

> template parameter pack. It seems to me that the kind of problem fixed 

> in c++/86932 cannot occur with type packs, because it boils down to a 

> reference to a previous parm (full disclosure: the comments and logic in 

> fixed_parameter_pack_p helped me a lot here). Thus I had the idea of 

> simply restricting the scope of the new condition above by adding an || 

> TREE_CODE (TREE_VALUE (parm)) == TYPE_DECL, which definitely leads to a 

> clean testsuite and a proper behavior on the new testcases, AFAICS. I'm 

> attaching what I tested on x86_64-linux.


I think the important property here is that it's non-terminal, not that 
it's a type pack.  We can't deduce anything for a non-terminal pack, so 
we should go ahead and make an empty pack.

Jason
Paolo Carlini April 15, 2019, 8:08 p.m. | #3
Hi,

On 12/04/19 20:29, Jason Merrill wrote:
> On 4/11/19 11:20 AM, Paolo Carlini wrote:

>> Hi,

>>

>> over the last few days I spent some time on this regression, which at 

>> first seemed just a minor error-recovery issue, but then I noticed 

>> that very slightly tweeking the original testcase uncovered a pretty 

>> serious ICE on valid:

>>

>> template<typename SX, typename ...XE> void

>> fk (XE..., int/*SW*/);

>>

>> void

>> w9 (void)

>> {

>>    fk<int> (0);

>> }

>>

>> The regression has to do with the changes committed by Jason for 

>> c++/86932, in particular with the condition in coerce_template_parms:

>>

>>     if (template_parameter_pack_p (TREE_VALUE (parm))

>>        && (arg || !(complain & tf_partial))

>>        && !(arg && ARGUMENT_PACK_P (arg)))

>>

>> which has the additional (arg || !complain & tf_partial)) false for 

>> the present testcase, thus the null arg is not changed into an empty 

>> pack, thus later  instantiate_template calls check_instantiated_args 

>> which finds it still null and crashes. Now, likely some additional 

>> analysis is in order, but for sure there is an important difference 

>> between the testcase which came with c++/86932 and the above: 

>> non-type vs type template parameter pack. It seems to me that the 

>> kind of problem fixed in c++/86932 cannot occur with type packs, 

>> because it boils down to a reference to a previous parm (full 

>> disclosure: the comments and logic in fixed_parameter_pack_p helped 

>> me a lot here). Thus I had the idea of simply restricting the scope 

>> of the new condition above by adding an || TREE_CODE (TREE_VALUE 

>> (parm)) == TYPE_DECL, which definitely leads to a clean testsuite and 

>> a proper behavior on the new testcases, AFAICS. I'm attaching what I 

>> tested on x86_64-linux.

>

> I think the important property here is that it's non-terminal, not 

> that it's a type pack.  We can't deduce anything for a non-terminal 

> pack, so we should go ahead and make an empty pack.


I see.

Then what about something bolder, like the below? Instead of fiddling 
with the details of coerce_template_parms - how it handles the explicit 
arguments - in fn_type_unification we deal with both parameter_pack == 
true and false in the same way when targ == NULL_TREE, thus we set 
incomplete. Then, for the new testcases, since incomplete is true, there 
is no jump to the deduced label and type_unification_real takes care of 
making the empty pack - the same happens already when there are no 
explicit arguments. Tested x86_64-linux. I also checked quite a few 
other variants of the tests but nothing new, nothing interesting, showed 
up...

Thanks, Paolo.

/////////////////////////
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 270364)
+++ cp/pt.c	(working copy)
@@ -20176,21 +20176,17 @@ fn_type_unification (tree fn,
               parameter_pack = TEMPLATE_PARM_PARAMETER_PACK (parm);
             }
 
-	  if (!parameter_pack && targ == NULL_TREE)
+	  if (targ == NULL_TREE)
 	    /* No explicit argument for this template parameter.  */
 	    incomplete = true;
-
-          if (parameter_pack && pack_deducible_p (parm, fn))
+	  else if (parameter_pack && pack_deducible_p (parm, fn))
             {
               /* Mark the argument pack as "incomplete". We could
                  still deduce more arguments during unification.
 	         We remove this mark in type_unification_real.  */
-              if (targ)
-                {
-                  ARGUMENT_PACK_INCOMPLETE_P(targ) = 1;
-                  ARGUMENT_PACK_EXPLICIT_ARGS (targ) 
-                    = ARGUMENT_PACK_ARGS (targ);
-                }
+	      ARGUMENT_PACK_INCOMPLETE_P(targ) = 1;
+	      ARGUMENT_PACK_EXPLICIT_ARGS (targ)
+		= ARGUMENT_PACK_ARGS (targ);
 
               /* We have some incomplete argument packs.  */
               incomplete = true;
Index: testsuite/g++.dg/cpp0x/pr89900-1.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr89900-1.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr89900-1.C	(working copy)
@@ -0,0 +1,10 @@
+// { dg-do compile { target c++11 } }
+
+template<typename SX, typename ...XE> void
+fk (XE..., SW);  // { dg-error "12:.SW. has not been declared" }
+
+void
+w9 (void)
+{
+  fk<int> (0);
+}
Index: testsuite/g++.dg/cpp0x/pr89900-2.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr89900-2.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr89900-2.C	(working copy)
@@ -0,0 +1,10 @@
+// { dg-do compile { target c++11 } }
+
+template<typename SX, typename ...XE> void
+fk (XE..., int);
+
+void
+w9 (void)
+{
+  fk<int> (0);
+}
Index: testsuite/g++.dg/cpp0x/pr89900-3.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr89900-3.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr89900-3.C	(working copy)
@@ -0,0 +1,10 @@
+// { dg-do compile { target c++11 } }
+
+template<typename ...XE> void
+fk (XE..., SW);  // { dg-error "12:.SW. has not been declared" }
+
+void
+w9 (void)
+{
+  fk (0);
+}
Index: testsuite/g++.dg/cpp0x/pr89900-4.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr89900-4.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr89900-4.C	(working copy)
@@ -0,0 +1,10 @@
+// { dg-do compile { target c++11 } }
+
+template<typename ...XE> void
+fk (XE..., int);
+
+void
+w9 (void)
+{
+  fk (0);
+}
Jason Merrill April 18, 2019, 5:44 p.m. | #4
On Mon, Apr 15, 2019 at 1:08 PM Paolo Carlini <paolo.carlini@oracle.com> wrote:
>

> Hi,

>

> On 12/04/19 20:29, Jason Merrill wrote:

> > On 4/11/19 11:20 AM, Paolo Carlini wrote:

> >> Hi,

> >>

> >> over the last few days I spent some time on this regression, which at

> >> first seemed just a minor error-recovery issue, but then I noticed

> >> that very slightly tweeking the original testcase uncovered a pretty

> >> serious ICE on valid:

> >>

> >> template<typename SX, typename ...XE> void

> >> fk (XE..., int/*SW*/);

> >>

> >> void

> >> w9 (void)

> >> {

> >>    fk<int> (0);

> >> }

> >>

> >> The regression has to do with the changes committed by Jason for

> >> c++/86932, in particular with the condition in coerce_template_parms:

> >>

> >>     if (template_parameter_pack_p (TREE_VALUE (parm))

> >>        && (arg || !(complain & tf_partial))

> >>        && !(arg && ARGUMENT_PACK_P (arg)))

> >>

> >> which has the additional (arg || !complain & tf_partial)) false for

> >> the present testcase, thus the null arg is not changed into an empty

> >> pack, thus later  instantiate_template calls check_instantiated_args

> >> which finds it still null and crashes. Now, likely some additional

> >> analysis is in order, but for sure there is an important difference

> >> between the testcase which came with c++/86932 and the above:

> >> non-type vs type template parameter pack. It seems to me that the

> >> kind of problem fixed in c++/86932 cannot occur with type packs,

> >> because it boils down to a reference to a previous parm (full

> >> disclosure: the comments and logic in fixed_parameter_pack_p helped

> >> me a lot here). Thus I had the idea of simply restricting the scope

> >> of the new condition above by adding an || TREE_CODE (TREE_VALUE

> >> (parm)) == TYPE_DECL, which definitely leads to a clean testsuite and

> >> a proper behavior on the new testcases, AFAICS. I'm attaching what I

> >> tested on x86_64-linux.

> >

> > I think the important property here is that it's non-terminal, not

> > that it's a type pack.  We can't deduce anything for a non-terminal

> > pack, so we should go ahead and make an empty pack.

>

> I see.

>

> Then what about something bolder, like the below? Instead of fiddling

> with the details of coerce_template_parms - how it handles the explicit

> arguments - in fn_type_unification we deal with both parameter_pack ==

> true and false in the same way when targ == NULL_TREE, thus we set

> incomplete. Then, for the new testcases, since incomplete is true, there

> is no jump to the deduced label and type_unification_real takes care of

> making the empty pack - the same happens already when there are no

> explicit arguments. Tested x86_64-linux. I also checked quite a few

> other variants of the tests but nothing new, nothing interesting, showed

> up...


OK.

Jason

Patch

Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 270279)
+++ cp/pt.c	(working copy)
@@ -8475,7 +8475,8 @@  coerce_template_parms (tree parms,
 	arg = NULL_TREE;
 
       if (template_parameter_pack_p (TREE_VALUE (parm))
-	  && (arg || !(complain & tf_partial))
+	  && (arg || !(complain & tf_partial)
+	      || TREE_CODE (TREE_VALUE (parm)) == TYPE_DECL)
 	  && !(arg && ARGUMENT_PACK_P (arg)))
         {
 	  /* Some arguments will be placed in the
Index: testsuite/g++.dg/cpp0x/pr89900-1.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr89900-1.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr89900-1.C	(working copy)
@@ -0,0 +1,10 @@ 
+// { dg-do compile { target c++11 } }
+
+template<typename SX, typename ...XE> void
+fk (XE..., SW);  // { dg-error "12:.SW. has not been declared" }
+
+void
+w9 (void)
+{
+  fk<int> (0);
+}
Index: testsuite/g++.dg/cpp0x/pr89900-2.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr89900-2.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr89900-2.C	(working copy)
@@ -0,0 +1,10 @@ 
+// { dg-do compile { target c++11 } }
+
+template<typename SX, typename ...XE> void
+fk (XE..., int);
+
+void
+w9 (void)
+{
+  fk<int> (0);
+}