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)") | expand |
.. 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.
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
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); +}
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
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); +}