| Message ID | orwoya293a.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PR,c++/71251] out-of-range parms in tmpl arg substitution | expand |
On Sat, Mar 17, 2018 at 8:13 AM, Alexandre Oliva <aoliva@redhat.com> wrote: > As we go through each of the template parameters, substituting it with > corresponding template arguments, an incorrect argument list might > cause us to index argument vectors past their length (or fail in the > preceding tree checks). Avoid such dereferences and instead issue an > error (if requested) if we find the argument index to be past the > parameter vector length. Any time we hit this abort, it indicates a bug in earlier processing, so that we're looking up a template parameter in an argument list for a different template. Aborting in that situation is appropriate; it has revealed many bugs. Jason
On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote: > On Sat, Mar 17, 2018 at 8:13 AM, Alexandre Oliva <aoliva@redhat.com> wrote: >> As we go through each of the template parameters, substituting it with >> corresponding template arguments, an incorrect argument list might >> cause us to index argument vectors past their length (or fail in the >> preceding tree checks). Avoid such dereferences and instead issue an >> error (if requested) if we find the argument index to be past the >> parameter vector length. > Any time we hit this abort, it indicates a bug in earlier processing, > so that we're looking up a template parameter in an argument list for > a different template. That doesn't seem to be the case here. The argument list given for U is <T>, as in the testcase, the problem is that U is misdeclared as taking <int,int> <typename> (two template levels for a single template). Should we aim at rejecting the declaration of U? template<int,int> template<typename> using U=void; template<typename,typename> struct S1; template<typename T> struct S1<T,U<T>>{ template<typename> struct S2:S2<T>{}; };
On Tue, Mar 20, 2018 at 4:15 PM, Alexandre Oliva <aoliva@redhat.com> wrote: > On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote: > >> On Sat, Mar 17, 2018 at 8:13 AM, Alexandre Oliva <aoliva@redhat.com> wrote: >>> As we go through each of the template parameters, substituting it with >>> corresponding template arguments, an incorrect argument list might >>> cause us to index argument vectors past their length (or fail in the >>> preceding tree checks). Avoid such dereferences and instead issue an >>> error (if requested) if we find the argument index to be past the >>> parameter vector length. > >> Any time we hit this abort, it indicates a bug in earlier processing, >> so that we're looking up a template parameter in an argument list for >> a different template. > > That doesn't seem to be the case here. The argument list given for U is > <T>, as in the testcase, the problem is that U is misdeclared as taking > <int,int> <typename> (two template levels for a single template). > > Should we aim at rejecting the declaration of U? Yes. Jason
On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote: > On Tue, Mar 20, 2018 at 4:15 PM, Alexandre Oliva <aoliva@redhat.com> wrote: >> Should we aim at rejecting the declaration of U? > Yes. Like this? [PR c++/71251] check tmpl parms in template using decl Check that template using decls have the correct number of parm lists. Will regstrap shortly, ok to install if it passes? for gcc/cp/ChangeLog PR c++/71251 * parser.c (cp_parser_alias_declaration): Call parser_check_template_parameters. for gcc/testsuite/ChangeLog PR c++/71251 * g++.dg/cpp0x/pr71251.C: New. --- gcc/cp/parser.c | 10 ++++++++++ gcc/testsuite/g++.dg/cpp0x/pr71251.C | 15 +++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr71251.C diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index ce05615adfba..8fa6a37c82f0 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -18910,6 +18910,13 @@ cp_parser_alias_declaration (cp_parser* parser) if (id == error_mark_node) return error_mark_node; + if (parser->num_template_parameter_lists + && !cp_parser_check_template_parameters (parser, + /*num_templates=*/0, + id_location, + /*declarator=*/NULL)) + id = error_mark_node; + cp_token *attrs_token = cp_lexer_peek_token (parser->lexer); attributes = cp_parser_attributes_opt (parser); if (attributes == error_mark_node) @@ -18980,6 +18987,9 @@ cp_parser_alias_declaration (cp_parser* parser) ds_alias, using_token); + if (id == error_mark_node) + return error_mark_node; + declarator = make_id_declarator (NULL_TREE, id, sfk_none); declarator->id_loc = id_location; diff --git a/gcc/testsuite/g++.dg/cpp0x/pr71251.C b/gcc/testsuite/g++.dg/cpp0x/pr71251.C new file mode 100644 index 000000000000..3df831bb581d --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/pr71251.C @@ -0,0 +1,15 @@ +// { dg-do compile { target c++11 } } + +template<int,int> template<typename> +using U = void; // { dg-error "too many" } + +template<typename> +using V = void; + +template<typename> struct X { + template<typename> template<typename> + using U = void; // { dg-error "too many" } + + template<typename> + using V = void; +};
On Tue, Mar 20, 2018 at 6:51 PM, Alexandre Oliva <aoliva@redhat.com> wrote: > On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote: > >> On Tue, Mar 20, 2018 at 4:15 PM, Alexandre Oliva <aoliva@redhat.com> wrote: >>> Should we aim at rejecting the declaration of U? > >> Yes. > > Like this? > > [PR c++/71251] check tmpl parms in template using decl > > Check that template using decls have the correct number of parm lists. > > Will regstrap shortly, ok to install if it passes? > > for gcc/cp/ChangeLog > > PR c++/71251 > * parser.c (cp_parser_alias_declaration): Call > parser_check_template_parameters. > > for gcc/testsuite/ChangeLog > > PR c++/71251 > * g++.dg/cpp0x/pr71251.C: New. > --- > gcc/cp/parser.c | 10 ++++++++++ > gcc/testsuite/g++.dg/cpp0x/pr71251.C | 15 +++++++++++++++ > 2 files changed, 25 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr71251.C > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index ce05615adfba..8fa6a37c82f0 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -18910,6 +18910,13 @@ cp_parser_alias_declaration (cp_parser* parser) > if (id == error_mark_node) > return error_mark_node; > > + if (parser->num_template_parameter_lists > + && !cp_parser_check_template_parameters (parser, > + /*num_templates=*/0, > + id_location, > + /*declarator=*/NULL)) > + id = error_mark_node; > cp_token *attrs_token = cp_lexer_peek_token (parser->lexer); > attributes = cp_parser_attributes_opt (parser); > if (attributes == error_mark_node) > @@ -18980,6 +18987,9 @@ cp_parser_alias_declaration (cp_parser* parser) > ds_alias, > using_token); > > + if (id == error_mark_node) > + return error_mark_node; Why wait until here to return? There are error returns immediately above and below your first hunk. Jason
On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote: >> + if (id == error_mark_node) >> + return error_mark_node; > Why wait until here to return? There are error returns immediately > above and below your first hunk. QOI. Returning immediately, we then get other errors. We could consume tokens till the end of the declaration, but I figured we might as well try to parse them and see whether there were any other legitimate errors to report.
On Tue, Mar 20, 2018 at 10:29 PM, Alexandre Oliva <aoliva@redhat.com> wrote: > On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote: > >>> + if (id == error_mark_node) >>> + return error_mark_node; > >> Why wait until here to return? There are error returns immediately >> above and below your first hunk. > > QOI. Returning immediately, we then get other errors. We could consume > tokens till the end of the declaration, but I figured we might as well > try to parse them and see whether there were any other legitimate errors > to report. It just seems a bit odd to have the check and the return so far apart. Do they need to be separate at all? I think we definitely want to move the check down below the cp_parser_commit_to_tentative_parse. Jason
On Mar 22, 2018, Jason Merrill <jason@redhat.com> wrote: > On Tue, Mar 20, 2018 at 10:29 PM, Alexandre Oliva <aoliva@redhat.com> wrote: >> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote: >> >>>> + if (id == error_mark_node) >>>> + return error_mark_node; >> >>> Why wait until here to return? There are error returns immediately >>> above and below your first hunk. >> >> QOI. Returning immediately, we then get other errors. We could consume >> tokens till the end of the declaration, but I figured we might as well >> try to parse them and see whether there were any other legitimate errors >> to report. > It just seems a bit odd to have the check and the return so far apart. > Do they need to be separate at all? That depends on how much cp_parser_check_template_parameters depends on the parser state; I worried moving it down might cause the template state to have changed enough that the test wouldn't be testing what we wanted any more, so I kept it right after parsing the identifier. > I think we definitely want to move the check down below the > cp_parser_commit_to_tentative_parse. If you say that won't get it the wrong context for the test, sure. I'll run a test cycle with that change.
On Mar 22, 2018, Jason Merrill <jason@redhat.com> wrote: > I think we definitely want to move the check down below the > cp_parser_commit_to_tentative_parse. This worked. Regstrapped on i686- and x86_64-linux-gnu. Ok to install? [PR c++/71251] check tmpl parms in template using decl Check that template using decls have the correct number of parm lists. for gcc/cp/ChangeLog PR c++/71251 * parser.c (cp_parser_alias_declaration): Call parser_check_template_parameters. for gcc/testsuite/ChangeLog PR c++/71251 * g++.dg/cpp0x/pr71251.C: New. --- gcc/cp/parser.c | 7 +++++++ gcc/testsuite/g++.dg/cpp0x/pr71251.C | 15 +++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr71251.C diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index fd817024eacf..602cc991ff6e 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -18965,6 +18965,13 @@ cp_parser_alias_declaration (cp_parser* parser) ds_alias, using_token); + if (parser->num_template_parameter_lists + && !cp_parser_check_template_parameters (parser, + /*num_templates=*/0, + id_location, + /*declarator=*/NULL)) + return error_mark_node; + declarator = make_id_declarator (NULL_TREE, id, sfk_none); declarator->id_loc = id_location; diff --git a/gcc/testsuite/g++.dg/cpp0x/pr71251.C b/gcc/testsuite/g++.dg/cpp0x/pr71251.C new file mode 100644 index 000000000000..3df831bb581d --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/pr71251.C @@ -0,0 +1,15 @@ +// { dg-do compile { target c++11 } } + +template<int,int> template<typename> +using U = void; // { dg-error "too many" } + +template<typename> +using V = void; + +template<typename> struct X { + template<typename> template<typename> + using U = void; // { dg-error "too many" } + + template<typename> + using V = void; +};
OK. On Fri, Mar 23, 2018 at 12:04 AM, Alexandre Oliva <aoliva@redhat.com> wrote: > On Mar 22, 2018, Jason Merrill <jason@redhat.com> wrote: > >> I think we definitely want to move the check down below the >> cp_parser_commit_to_tentative_parse. > > This worked. Regstrapped on i686- and x86_64-linux-gnu. Ok to install? > > [PR c++/71251] check tmpl parms in template using decl > > Check that template using decls have the correct number of parm lists. > > for gcc/cp/ChangeLog > > PR c++/71251 > * parser.c (cp_parser_alias_declaration): Call > parser_check_template_parameters. > > for gcc/testsuite/ChangeLog > > PR c++/71251 > * g++.dg/cpp0x/pr71251.C: New. > --- > gcc/cp/parser.c | 7 +++++++ > gcc/testsuite/g++.dg/cpp0x/pr71251.C | 15 +++++++++++++++ > 2 files changed, 22 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr71251.C > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index fd817024eacf..602cc991ff6e 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -18965,6 +18965,13 @@ cp_parser_alias_declaration (cp_parser* parser) > ds_alias, > using_token); > > + if (parser->num_template_parameter_lists > + && !cp_parser_check_template_parameters (parser, > + /*num_templates=*/0, > + id_location, > + /*declarator=*/NULL)) > + return error_mark_node; > + > declarator = make_id_declarator (NULL_TREE, id, sfk_none); > declarator->id_loc = id_location; > > diff --git a/gcc/testsuite/g++.dg/cpp0x/pr71251.C b/gcc/testsuite/g++.dg/cpp0x/pr71251.C > new file mode 100644 > index 000000000000..3df831bb581d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/pr71251.C > @@ -0,0 +1,15 @@ > +// { dg-do compile { target c++11 } } > + > +template<int,int> template<typename> > +using U = void; // { dg-error "too many" } > + > +template<typename> > +using V = void; > + > +template<typename> struct X { > + template<typename> template<typename> > + using U = void; // { dg-error "too many" } > + > + template<typename> > + using V = void; > +}; > > > -- > Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ > You must be the change you wish to see in the world. -- Gandhi > Be Free! -- http://FSFLA.org/ FSF Latin America board member > Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index fa9bfb12c297..4cc18d0abe78 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -13976,11 +13976,30 @@ tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl) if (level <= levels && TREE_VEC_LENGTH (TMPL_ARGS_LEVEL (args, level)) > 0) { - arg = TMPL_ARG (args, level, idx); + if (TREE_VEC_LENGTH (TMPL_ARGS_LEVEL (args, level)) > idx) + { + arg = TMPL_ARG (args, level, idx); - /* See through ARGUMENT_PACK_SELECT arguments. */ - if (arg && TREE_CODE (arg) == ARGUMENT_PACK_SELECT) - arg = argument_pack_select_arg (arg); + /* See through ARGUMENT_PACK_SELECT arguments. */ + if (arg && TREE_CODE (arg) == ARGUMENT_PACK_SELECT) + arg = argument_pack_select_arg (arg); + } + else + { + if (complain & tf_error) + { + /* ??? We could use a better location for this + message. It takes the context of the closing + bracket of the innermost template we're + substituting, but the error may very well be + related with some enclosing context. */ + error ("missing template argument at this" + " or enclosing context"); + error_at (DECL_SOURCE_LOCATION (TEMPLATE_PARM_DECL (t)), + "for substitution of template parameter"); + } + arg = error_mark_node; + } } if (arg == error_mark_node) diff --git a/gcc/testsuite/g++.dg/cpp0x/pr71251.C b/gcc/testsuite/g++.dg/cpp0x/pr71251.C new file mode 100644 index 000000000000..f16fb13cccc0 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/pr71251.C @@ -0,0 +1,9 @@ +// { dg-do compile { target c++11 } } + +template<int,int> template<typename> using U=void; // { dg-error "parameter" } + +template<typename,typename> struct S1; + +template<typename T> struct S1<T,U< + T>>{ template<typename> struct S2:S2<T>{}; }; // { dg-error "missing" } +//^ the error should be here, but it is^here, thus the weird line break