| Message ID | cb7f7c0f-95e1-ddfe-4a2c-6db049560b6f@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++,Patch/RFC] PR 71169 ("[7/8 Regression] ICE on invalid C++ code in pop_nested_class"), PR 71832 and more | expand |
.. grrrr, consider the patch withdrawn in its detailed form, I have a new testcase which add some random code after that ill-formed class and it's mishandled, it ICEs again. Well, I would still appreciate some feedback about the general idea... Paolo.
Hi again, On 06/03/2018 19:58, Paolo Carlini wrote: > .. grrrr, consider the patch withdrawn in its detailed form, I have a > new testcase which add some random code after that ill-formed class > and it's mishandled, it ICEs again. The below avoids the idiotic thinko (I added a testcase for that) and is likewise passing testing. Thanks, Paolo. ///////////////////////// Index: cp/parser.c =================================================================== --- cp/parser.c (revision 258264) +++ cp/parser.c (working copy) @@ -565,6 +565,8 @@ cp_debug_parser (FILE *file, cp_parser *parser) "local class", parser->in_function_body); cp_debug_print_flag (file, "Auto correct a colon to a scope operator", parser->colon_corrects_to_scope_p); + cp_debug_print_flag (file, "Error in template parameter list", + parser->error_in_template_parameter_list_p); cp_debug_print_flag (file, "Colon doesn't start a class definition", parser->colon_doesnt_start_class_def_p); if (parser->type_definition_forbidden_message) @@ -3910,6 +3912,9 @@ cp_parser_new (void) /* We can correct until told otherwise. */ parser->colon_corrects_to_scope_p = true; + /* No error so far. */ + parser->error_in_template_parameter_list_p = false; + /* The unparsed function queue is empty. */ push_unparsed_function_queues (parser); @@ -10151,6 +10156,8 @@ cp_parser_lambda_expression (cp_parser* parser) /* Inside the class, surrounding template-parameter-lists do not apply. */ unsigned int saved_num_template_parameter_lists = parser->num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; unsigned char in_statement = parser->in_statement; bool in_switch_statement_p = parser->in_switch_statement_p; bool fully_implicit_function_template_p @@ -10161,6 +10168,7 @@ cp_parser_lambda_expression (cp_parser* parser) = parser->auto_is_implicit_function_template_parm_p; parser->num_template_parameter_lists = 0; + parser->error_in_template_parameter_list_p = false; parser->in_statement = 0; parser->in_switch_statement_p = false; parser->fully_implicit_function_template_p = false; @@ -10199,6 +10207,8 @@ cp_parser_lambda_expression (cp_parser* parser) type = finish_struct (type, /*attributes=*/NULL_TREE); parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; parser->in_statement = in_statement; parser->in_switch_statement_p = in_switch_statement_p; parser->fully_implicit_function_template_p @@ -10624,7 +10634,8 @@ cp_parser_lambda_declarator_opt (cp_parser* parser { fco = finish_member_template_decl (fco); finish_template_decl (template_param_list); - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; } else if (parser->fully_implicit_function_template_p) fco = finish_fully_implicit_template (parser, fco); @@ -15065,6 +15076,11 @@ cp_parser_template_parameter_list (cp_parser* pars parameter_list = chainon (parameter_list, err_parm); } + if (parameter == error_mark_node + || error_operand_p (TREE_VALUE (parameter)) + || error_operand_p (TREE_PURPOSE (parameter))) + parser->error_in_template_parameter_list_p = true; + /* If the next token is not a `,', we're done. */ if (cp_lexer_next_token_is_not (parser->lexer, CPP_COMMA)) break; @@ -16673,7 +16689,8 @@ cp_parser_explicit_specialization (cp_parser* pars if (need_lang_pop) pop_lang_context (); /* We're done with this parameter list. */ - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; } /* Parse a type-specifier. @@ -22438,6 +22455,7 @@ cp_parser_class_specifier_1 (cp_parser* parser) tree attributes = NULL_TREE; bool nested_name_specifier_p; unsigned saved_num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p; bool saved_in_function_body; unsigned char in_statement; bool in_switch_statement_p; @@ -22480,6 +22498,9 @@ cp_parser_class_specifier_1 (cp_parser* parser) saved_num_template_parameter_lists = parser->num_template_parameter_lists; parser->num_template_parameter_lists = 0; + saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; + parser->error_in_template_parameter_list_p = false; /* We are not in a function body. */ saved_in_function_body = parser->in_function_body; parser->in_function_body = false; @@ -22658,8 +22679,13 @@ cp_parser_class_specifier_1 (cp_parser* parser) struct A::B { void f() { } }; there is no need to delay the parsing of `A::B::f'. */ - if (--parser->num_classes_being_defined == 0) + if (saved_error_in_template_parameter_list_p) { + /* Do nothing, we skip the bodies in order to improve error + recovery (c++/71169, c++/71832, etc). */; + } + else if (--parser->num_classes_being_defined == 0) + { tree decl; tree class_type = NULL_TREE; tree pushed_scope = NULL_TREE; @@ -22752,6 +22778,8 @@ cp_parser_class_specifier_1 (cp_parser* parser) parser->in_function_body = saved_in_function_body; parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; parser->in_unbraced_linkage_specification_p = saved_in_unbraced_linkage_specification_p; @@ -23253,7 +23281,8 @@ cp_parser_class_head (cp_parser* parser, if (invalid_explicit_specialization_p) { end_specialization (); - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; } if (type) @@ -26616,6 +26645,7 @@ cp_parser_constructor_declarator_p (cp_parser *par tree type; tree pushed_scope = NULL_TREE; unsigned saved_num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p; /* Names appearing in the type-specifier should be looked up in the scope of the class. */ @@ -26642,6 +26672,9 @@ cp_parser_constructor_declarator_p (cp_parser *par saved_num_template_parameter_lists = parser->num_template_parameter_lists; parser->num_template_parameter_lists = 0; + saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; + parser->error_in_template_parameter_list_p = false; /* Look for the type-specifier. */ cp_parser_type_specifier (parser, @@ -26653,6 +26686,8 @@ cp_parser_constructor_declarator_p (cp_parser *par parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; /* Leave the scope of the class. */ if (pushed_scope) @@ -26751,6 +26786,7 @@ cp_parser_function_definition_after_declarator (cp bool saved_in_unbraced_linkage_specification_p; bool saved_in_function_body; unsigned saved_num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p; cp_token *token; bool fully_implicit_function_template_p = parser->fully_implicit_function_template_p; @@ -26799,6 +26835,9 @@ cp_parser_function_definition_after_declarator (cp saved_num_template_parameter_lists = parser->num_template_parameter_lists; parser->num_template_parameter_lists = 0; + saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; + parser->error_in_template_parameter_list_p = false; /* If the next token is `try', `__transaction_atomic', or `__transaction_relaxed`, then we are looking at either function-try-block @@ -26824,6 +26863,8 @@ cp_parser_function_definition_after_declarator (cp = saved_in_unbraced_linkage_specification_p; parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; parser->in_function_body = saved_in_function_body; parser->fully_implicit_function_template_p @@ -26893,7 +26934,8 @@ cp_parser_template_declaration_after_parameters (c /*complain=*/true); } /* We are done with the current parameter list. */ - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; pop_deferring_access_checks (); @@ -39249,7 +39291,8 @@ finish_fully_implicit_template (cp_parser *parser, end_template_decl (); parser->fully_implicit_function_template_p = false; - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; return member_decl_opt; } Index: cp/parser.h =================================================================== --- cp/parser.h (revision 258264) +++ cp/parser.h (working copy) @@ -346,6 +346,9 @@ struct GTY(()) cp_parser { is terminated by colon. */ bool colon_doesnt_start_class_def_p; + /* TRUE if there was an error in a template parameter list. */ + bool error_in_template_parameter_list_p; + /* If non-NULL, then we are parsing a construct where new type definitions are not permitted. The string stored here will be issued as an error message if a type is defined. */ Index: testsuite/g++.dg/cpp0x/pr71169-2.C =================================================================== --- testsuite/g++.dg/cpp0x/pr71169-2.C (nonexistent) +++ testsuite/g++.dg/cpp0x/pr71169-2.C (working copy) @@ -0,0 +1,19 @@ +// { dg-do compile { target c++11 } } + +template <Preconditioner> class A { // { dg-error "declared" } + template <class = int> void m_fn1() { + m_fn1(); + } +}; + +template<typename> +struct B +{ + int f() { return 0; } +}; + +int main() +{ + B<int> b; + return b.f(); +} Index: testsuite/g++.dg/cpp0x/pr71169.C =================================================================== --- testsuite/g++.dg/cpp0x/pr71169.C (nonexistent) +++ testsuite/g++.dg/cpp0x/pr71169.C (working copy) @@ -0,0 +1,7 @@ +// { dg-do compile { target c++11 } } + +template <Preconditioner> class A { // { dg-error "declared" } + template <class = int> void m_fn1() { + m_fn1(); + } +}; Index: testsuite/g++.dg/cpp0x/pr71832.C =================================================================== --- testsuite/g++.dg/cpp0x/pr71832.C (nonexistent) +++ testsuite/g++.dg/cpp0x/pr71832.C (working copy) @@ -0,0 +1,7 @@ +// { dg-do compile { target c++11 } } + +template < typename decltype (0) > struct A // { dg-error "expected|two or more" } +{ + void foo () { baz (); } + template < typename ... S > void baz () {} +};
Interesting, that seems like a promising idea. I'm not sure we need to do this based on an error in a default template arg, though; can we drop + || error_operand_p (TREE_PURPOSE (parameter))) ? Jason
Hi, On 06/03/2018 21:33, Jason Merrill wrote: > Interesting, that seems like a promising idea. I'm not sure we need > to do this based on an error in a default template arg, though; can we > drop > > + || error_operand_p (TREE_PURPOSE (parameter))) > > ? Good point, yes, I believe we can, isn't necessary for all the snippets I have around neither apparently to pass the testsuite (but this is rather straightforward, right?). As I said, I tried many different things, some also fiddled with TREE_PURPOSE, in pt.c too, but in what I sent I only added the check out of a reflex. Anyway, the below is in libstdc++, so far so good. Thanks, Paolo. ////////////////// Index: cp/parser.c =================================================================== --- cp/parser.c (revision 258264) +++ cp/parser.c (working copy) @@ -565,6 +565,8 @@ cp_debug_parser (FILE *file, cp_parser *parser) "local class", parser->in_function_body); cp_debug_print_flag (file, "Auto correct a colon to a scope operator", parser->colon_corrects_to_scope_p); + cp_debug_print_flag (file, "Error in template parameter list", + parser->error_in_template_parameter_list_p); cp_debug_print_flag (file, "Colon doesn't start a class definition", parser->colon_doesnt_start_class_def_p); if (parser->type_definition_forbidden_message) @@ -3910,6 +3912,9 @@ cp_parser_new (void) /* We can correct until told otherwise. */ parser->colon_corrects_to_scope_p = true; + /* No error so far. */ + parser->error_in_template_parameter_list_p = false; + /* The unparsed function queue is empty. */ push_unparsed_function_queues (parser); @@ -10151,6 +10156,8 @@ cp_parser_lambda_expression (cp_parser* parser) /* Inside the class, surrounding template-parameter-lists do not apply. */ unsigned int saved_num_template_parameter_lists = parser->num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; unsigned char in_statement = parser->in_statement; bool in_switch_statement_p = parser->in_switch_statement_p; bool fully_implicit_function_template_p @@ -10161,6 +10168,7 @@ cp_parser_lambda_expression (cp_parser* parser) = parser->auto_is_implicit_function_template_parm_p; parser->num_template_parameter_lists = 0; + parser->error_in_template_parameter_list_p = false; parser->in_statement = 0; parser->in_switch_statement_p = false; parser->fully_implicit_function_template_p = false; @@ -10199,6 +10207,8 @@ cp_parser_lambda_expression (cp_parser* parser) type = finish_struct (type, /*attributes=*/NULL_TREE); parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; parser->in_statement = in_statement; parser->in_switch_statement_p = in_switch_statement_p; parser->fully_implicit_function_template_p @@ -10624,7 +10634,8 @@ cp_parser_lambda_declarator_opt (cp_parser* parser { fco = finish_member_template_decl (fco); finish_template_decl (template_param_list); - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; } else if (parser->fully_implicit_function_template_p) fco = finish_fully_implicit_template (parser, fco); @@ -15065,6 +15076,10 @@ cp_parser_template_parameter_list (cp_parser* pars parameter_list = chainon (parameter_list, err_parm); } + if (parameter == error_mark_node + || error_operand_p (TREE_VALUE (parameter))) + parser->error_in_template_parameter_list_p = true; + /* If the next token is not a `,', we're done. */ if (cp_lexer_next_token_is_not (parser->lexer, CPP_COMMA)) break; @@ -16673,7 +16688,8 @@ cp_parser_explicit_specialization (cp_parser* pars if (need_lang_pop) pop_lang_context (); /* We're done with this parameter list. */ - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; } /* Parse a type-specifier. @@ -22438,6 +22454,7 @@ cp_parser_class_specifier_1 (cp_parser* parser) tree attributes = NULL_TREE; bool nested_name_specifier_p; unsigned saved_num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p; bool saved_in_function_body; unsigned char in_statement; bool in_switch_statement_p; @@ -22480,6 +22497,9 @@ cp_parser_class_specifier_1 (cp_parser* parser) saved_num_template_parameter_lists = parser->num_template_parameter_lists; parser->num_template_parameter_lists = 0; + saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; + parser->error_in_template_parameter_list_p = false; /* We are not in a function body. */ saved_in_function_body = parser->in_function_body; parser->in_function_body = false; @@ -22658,8 +22678,13 @@ cp_parser_class_specifier_1 (cp_parser* parser) struct A::B { void f() { } }; there is no need to delay the parsing of `A::B::f'. */ - if (--parser->num_classes_being_defined == 0) + if (saved_error_in_template_parameter_list_p) { + /* Do nothing, we skip the bodies in order to improve error + recovery (c++/71169, c++/71832, etc). */; + } + else if (--parser->num_classes_being_defined == 0) + { tree decl; tree class_type = NULL_TREE; tree pushed_scope = NULL_TREE; @@ -22752,6 +22777,8 @@ cp_parser_class_specifier_1 (cp_parser* parser) parser->in_function_body = saved_in_function_body; parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; parser->in_unbraced_linkage_specification_p = saved_in_unbraced_linkage_specification_p; @@ -23253,7 +23280,8 @@ cp_parser_class_head (cp_parser* parser, if (invalid_explicit_specialization_p) { end_specialization (); - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; } if (type) @@ -26616,6 +26644,7 @@ cp_parser_constructor_declarator_p (cp_parser *par tree type; tree pushed_scope = NULL_TREE; unsigned saved_num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p; /* Names appearing in the type-specifier should be looked up in the scope of the class. */ @@ -26642,6 +26671,9 @@ cp_parser_constructor_declarator_p (cp_parser *par saved_num_template_parameter_lists = parser->num_template_parameter_lists; parser->num_template_parameter_lists = 0; + saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; + parser->error_in_template_parameter_list_p = false; /* Look for the type-specifier. */ cp_parser_type_specifier (parser, @@ -26653,6 +26685,8 @@ cp_parser_constructor_declarator_p (cp_parser *par parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; /* Leave the scope of the class. */ if (pushed_scope) @@ -26751,6 +26785,7 @@ cp_parser_function_definition_after_declarator (cp bool saved_in_unbraced_linkage_specification_p; bool saved_in_function_body; unsigned saved_num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p; cp_token *token; bool fully_implicit_function_template_p = parser->fully_implicit_function_template_p; @@ -26799,6 +26834,9 @@ cp_parser_function_definition_after_declarator (cp saved_num_template_parameter_lists = parser->num_template_parameter_lists; parser->num_template_parameter_lists = 0; + saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; + parser->error_in_template_parameter_list_p = false; /* If the next token is `try', `__transaction_atomic', or `__transaction_relaxed`, then we are looking at either function-try-block @@ -26824,6 +26862,8 @@ cp_parser_function_definition_after_declarator (cp = saved_in_unbraced_linkage_specification_p; parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; parser->in_function_body = saved_in_function_body; parser->fully_implicit_function_template_p @@ -26893,7 +26933,8 @@ cp_parser_template_declaration_after_parameters (c /*complain=*/true); } /* We are done with the current parameter list. */ - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; pop_deferring_access_checks (); @@ -39249,7 +39290,8 @@ finish_fully_implicit_template (cp_parser *parser, end_template_decl (); parser->fully_implicit_function_template_p = false; - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; return member_decl_opt; } Index: cp/parser.h =================================================================== --- cp/parser.h (revision 258264) +++ cp/parser.h (working copy) @@ -346,6 +346,9 @@ struct GTY(()) cp_parser { is terminated by colon. */ bool colon_doesnt_start_class_def_p; + /* TRUE if there was an error in a template parameter list. */ + bool error_in_template_parameter_list_p; + /* If non-NULL, then we are parsing a construct where new type definitions are not permitted. The string stored here will be issued as an error message if a type is defined. */ Index: testsuite/g++.dg/cpp0x/pr71169-2.C =================================================================== --- testsuite/g++.dg/cpp0x/pr71169-2.C (nonexistent) +++ testsuite/g++.dg/cpp0x/pr71169-2.C (working copy) @@ -0,0 +1,19 @@ +// { dg-do compile { target c++11 } } + +template <Preconditioner> class A { // { dg-error "declared" } + template <class = int> void m_fn1() { + m_fn1(); + } +}; + +template<typename> +struct B +{ + int f() { return 0; } +}; + +int main() +{ + B<int> b; + return b.f(); +} Index: testsuite/g++.dg/cpp0x/pr71169.C =================================================================== --- testsuite/g++.dg/cpp0x/pr71169.C (nonexistent) +++ testsuite/g++.dg/cpp0x/pr71169.C (working copy) @@ -0,0 +1,7 @@ +// { dg-do compile { target c++11 } } + +template <Preconditioner> class A { // { dg-error "declared" } + template <class = int> void m_fn1() { + m_fn1(); + } +}; Index: testsuite/g++.dg/cpp0x/pr71832.C =================================================================== --- testsuite/g++.dg/cpp0x/pr71832.C (nonexistent) +++ testsuite/g++.dg/cpp0x/pr71832.C (working copy) @@ -0,0 +1,7 @@ +// { dg-do compile { target c++11 } } + +template < typename decltype (0) > struct A // { dg-error "expected|two or more" } +{ + void foo () { baz (); } + template < typename ... S > void baz () {} +};
On Tue, Mar 6, 2018 at 5:00 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > On 06/03/2018 21:33, Jason Merrill wrote: >> >> Interesting, that seems like a promising idea. I'm not sure we need >> to do this based on an error in a default template arg, though; can we >> drop >> >> + || error_operand_p (TREE_PURPOSE (parameter))) >> >> ? > > Good point, yes, I believe we can, isn't necessary for all the snippets I > have around neither apparently to pass the testsuite (but this is rather > straightforward, right?). As I said, I tried many different things, some > also fiddled with TREE_PURPOSE, in pt.c too, but in what I sent I only added > the check out of a reflex. Anyway, the below is in libstdc++, so far so > good. What if, instead of adding another flag to cp_parser, we look for errors in the template parms for a particular member before we do any late parsing for it? Jason
Hi, On 07/03/2018 16:38, Jason Merrill wrote: > On Tue, Mar 6, 2018 at 5:00 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> On 06/03/2018 21:33, Jason Merrill wrote: >>> Interesting, that seems like a promising idea. I'm not sure we need >>> to do this based on an error in a default template arg, though; can we >>> drop >>> >>> + || error_operand_p (TREE_PURPOSE (parameter))) >>> >>> ? >> Good point, yes, I believe we can, isn't necessary for all the snippets I >> have around neither apparently to pass the testsuite (but this is rather >> straightforward, right?). As I said, I tried many different things, some >> also fiddled with TREE_PURPOSE, in pt.c too, but in what I sent I only added >> the check out of a reflex. Anyway, the below is in libstdc++, so far so >> good. > What if, instead of adding another flag to cp_parser, we look for > errors in the template parms for a particular member before we do any > late parsing for it? Thus do the check before: FOR_EACH_VEC_SAFE_ELT (unparsed_funs_with_definitions, ix, decl) cp_parser_late_parsing_for_member (parser, decl); and therefore skip the whole set of unparsed_funs? Because the template parameters to be considered is the same for all of them, right? (otherwise something is seriously wrong with my very idea!) And what about the other entities in the 'else if (--parser->num_classes_being_defined == 0)' body? On one hand I'm certainly concerned that we may be too heavy handed, on the other hand we may risk inconsistencies, with some definitions available during error recovery other not, where all of them have broken outer template parameters anyway. Well, in general, I must say that - assuming the possibly erroneous template parameters are shared by all the entities in the 'else if (--parser->num_classes_being_define == 0)' body - it would be too bad going through again all the template parameters where with just a bool we could save the work that we already did anyway, if see what I mean... Paolo.
On Wed, Mar 7, 2018 at 10:55 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi, > > On 07/03/2018 16:38, Jason Merrill wrote: >> >> On Tue, Mar 6, 2018 at 5:00 PM, Paolo Carlini <paolo.carlini@oracle.com> >> wrote: >>> >>> On 06/03/2018 21:33, Jason Merrill wrote: >>>> >>>> Interesting, that seems like a promising idea. I'm not sure we need >>>> to do this based on an error in a default template arg, though; can we >>>> drop >>>> >>>> + || error_operand_p (TREE_PURPOSE (parameter))) >>>> >>>> ? >>> >>> Good point, yes, I believe we can, isn't necessary for all the snippets I >>> have around neither apparently to pass the testsuite (but this is rather >>> straightforward, right?). As I said, I tried many different things, some >>> also fiddled with TREE_PURPOSE, in pt.c too, but in what I sent I only >>> added >>> the check out of a reflex. Anyway, the below is in libstdc++, so far so >>> good. >> >> What if, instead of adding another flag to cp_parser, we look for >> errors in the template parms for a particular member before we do any >> late parsing for it? > > Thus do the check before: > > FOR_EACH_VEC_SAFE_ELT (unparsed_funs_with_definitions, ix, decl) > cp_parser_late_parsing_for_member (parser, decl); > > and therefore skip the whole set of unparsed_funs? Because the template > parameters to be considered is the same for all of them, right? No; member templates have more template parameters; the error might be in a nested class, and not affect functions from an enclosing class. > (otherwise something is seriously wrong with my very idea!) Not that seriously; once we've seen an error all bets are off, and we're free to decide that enclosing class functions aren't worth parsing in that case, either. > And what about the other > entities in the 'else if (--parser->num_classes_being_defined == 0)' body? They also have associated decls where we can look at template parameters. > On one hand I'm certainly concerned that we may be too heavy handed, on the > other hand we may risk inconsistencies, with some definitions available > during error recovery other not, where all of them have broken outer > template parameters anyway. Well, in general, I must say that - assuming the > possibly erroneous template parameters are shared by all the entities in the > 'else if (--parser->num_classes_being_define == 0)' body - it would be too > bad going through again all the template parameters where with just a bool > we could save the work that we already did anyway, if see what I mean... My thought was that this patch adds a lot of managing of the flag in different places in the parser, whereas looking for error_mark_node in the template parms here would be just in one place. But if you prefer the current approach, that's fine, it's straightforward enough. Jason
Hi, [snip the various clarifications] Il 7 Marzo 2018 17:57:07 CET, Jason Merrill <jason@redhat.com> ha scritto: >My thought was that this patch adds a lot of managing of the flag in >different places in the parser, whereas looking for error_mark_node in >the template parms here would be just in one place. But if you prefer >the current approach, that's fine, it's straightforward enough. Thanks a lot for the various clarifications above, where essentially turns out that some details of my patch are correct essentially by chance ;) Seriously, I'm thinking the following: since 8 is getting real close, what if, for 8, for the known mild regressions, we go ahead with my super safe Plan B which I mentioned at beginning of the thread, then as soon as trunk branches we immediately apply my patch and we give it a serious spin, say we rebuild distros with it, and see what happens? Thanks, Paolo
On Wed, Mar 7, 2018 at 12:18 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi, > > [snip the various clarifications] > > Il 7 Marzo 2018 17:57:07 CET, Jason Merrill <jason@redhat.com> ha scritto: >>My thought was that this patch adds a lot of managing of the flag in >>different places in the parser, whereas looking for error_mark_node in >>the template parms here would be just in one place. But if you prefer >>the current approach, that's fine, it's straightforward enough. > > Thanks a lot for the various clarifications above, where essentially turns out that some details of my patch are correct essentially by chance ;) Seriously, I'm thinking the following: since 8 is getting real close, what if, for 8, for the known mild regressions, we go ahead with my super safe Plan B which I mentioned at beginning of the thread, then as soon as trunk branches we immediately apply my patch and we give it a serious spin, say we rebuild distros with it, and see what happens? This is what I was suggesting, what do you think? commit 706f372b52e65694161b9dff0272481d23fa898a Author: Jason Merrill <jason@redhat.com> Date: Wed Mar 7 14:45:19 2018 -0500 PR c++/71832 - ICE with ill-formed template parameter. * pt.c (any_erroneous_template_args_p): New. * parser.c (cp_parser_class_specifier_1): Use it. diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 268be0fd543..8f3ec86e8ce 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -6558,6 +6558,7 @@ extern int processing_template_parmlist; extern bool dependent_type_p (tree); extern bool dependent_scope_p (tree); extern bool any_dependent_template_arguments_p (const_tree); +extern bool any_erroneous_template_args_p (const_tree); extern bool dependent_template_p (tree); extern bool dependent_template_id_p (tree, tree); extern bool type_dependent_expression_p (tree); diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index a19bbe1e1d0..f7a8be50b79 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -22682,6 +22682,8 @@ cp_parser_class_specifier_1 (cp_parser* parser) FOR_EACH_VEC_SAFE_ELT (unparsed_funs_with_default_args, ix, e) { decl = e->decl; + if (any_erroneous_template_args_p (decl)) + continue; /* If there are default arguments that have not yet been processed, take care of them now. */ if (class_type != e->class_type) @@ -22704,6 +22706,8 @@ cp_parser_class_specifier_1 (cp_parser* parser) save_ccr = current_class_ref; FOR_EACH_VEC_SAFE_ELT (unparsed_nsdmis, ix, decl) { + if (any_erroneous_template_args_p (decl)) + continue; if (class_type != DECL_CONTEXT (decl)) { if (pushed_scope) @@ -27642,6 +27646,9 @@ cp_parser_late_parsing_for_member (cp_parser* parser, tree member_function) if (DECL_FUNCTION_TEMPLATE_P (member_function)) member_function = DECL_TEMPLATE_RESULT (member_function); + if (any_erroneous_template_args_p (member_function)) + return; + /* There should not be any class definitions in progress at this point; the bodies of members are only parsed outside of all class definitions. */ diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index 89024c10fe2..1ce04aaabc7 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -25048,6 +25048,38 @@ any_dependent_template_arguments_p (const_tree args) return false; } +/* Returns true if ARGS contains any errors. */ + +bool +any_erroneous_template_args_p (const_tree args) +{ + int i; + int j; + + if (args && TREE_CODE (args) != TREE_VEC) + { + if (tree ti = get_template_info (args)) + args = TI_ARGS (ti); + else + args = NULL_TREE; + } + + if (!args) + return false; + if (args == error_mark_node) + return true; + + for (i = 0; i < TMPL_ARGS_DEPTH (args); ++i) + { + const_tree level = TMPL_ARGS_LEVEL (args, i + 1); + for (j = 0; j < TREE_VEC_LENGTH (level); ++j) + if (error_operand_p (TREE_VEC_ELT (level, j))) + return true; + } + + return false; +} + /* Returns TRUE if the template TMPL is type-dependent. */ bool
Hi, On 07/03/2018 21:23, Jason Merrill wrote: > On Wed, Mar 7, 2018 at 12:18 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> Hi, >> >> [snip the various clarifications] >> >> Il 7 Marzo 2018 17:57:07 CET, Jason Merrill <jason@redhat.com> ha scritto: >>> My thought was that this patch adds a lot of managing of the flag in >>> different places in the parser, whereas looking for error_mark_node in >>> the template parms here would be just in one place. But if you prefer >>> the current approach, that's fine, it's straightforward enough. >> Thanks a lot for the various clarifications above, where essentially turns out that some details of my patch are correct essentially by chance ;) Seriously, I'm thinking the following: since 8 is getting real close, what if, for 8, for the known mild regressions, we go ahead with my super safe Plan B which I mentioned at beginning of the thread, then as soon as trunk branches we immediately apply my patch and we give it a serious spin, say we rebuild distros with it, and see what happens? > This is what I was suggesting, what do you think? Eh, eh, certainly I don't have anything to say from the correctness point of view. As I already tried to explain, what I find annoying in this kind of approach, no matter how well is implemented, is that at parsing time we have to go anyway over all the parameters of all the template lists and then we know once and for all, for the corresponding class, whether there are erroneous parameters or not. In this kind of approach we go again through all the parameters, and, for example, multiple times when there are nested classes for example, I'm pretty sure in some (other) cases too (I should think more about that to be more specific). If you ask my opinion, at the moment I still believe that the best solution would be doing something at parsing time, save the information, in a more elegant way, maybe adding a special "erroneous TREE_VEC" flag to the TREE_VECs. I don't know exactly. Even better a unique flag for all the template parameter lists of each class. That said, if you don't foresee immediate performance-related issues, it's of course your call ;) If/when I will have a more concrete alternate proposal I will speak up ;) Nice anyway that we agree about the basic idea :-) Paolo.
Hi again, On 07/03/2018 21:49, Paolo Carlini wrote: > ... If you ask my opinion, at the moment I still believe that the best > solution would be doing something at parsing time, save the > information, in a more elegant way, maybe adding a special "erroneous > TREE_VEC" flag to the TREE_VECs. I don't know exactly. Even better a > unique flag for all the template parameter lists of each class. Just to explain more *concretely* what I had in mind, I quickly hacked the below, which would be a *minimal* implementation of my idea. It should be also possible to consolidate the flags of the various TMPL_ARGS_LEVELs to the flag of the TREE_VEC returned by TI_ARGS (get_template_info). That would be fantastic: ideally I would like to see an O(0) any_erroneous_template_args_p. Maybe we can do that when we use SET_TMPL_ARGS_LEVEL? Probably it would take you 5 mins to implement all of that. Or, I was thinking, something completely different, instead of fiddling with TREE_VECs, have something in DECL_TEMPLATE_INFO? (see again my remark about the 5 mins ;) Paolo. ////////////////// Index: cp-tree.h =================================================================== --- cp-tree.h (revision 258355) +++ cp-tree.h (working copy) @@ -592,6 +592,8 @@ struct GTY(()) ptrmem_cst { }; typedef struct ptrmem_cst * ptrmem_cst_t; +#define ERRONEOUS_TREE_VEC(NODE) ((NODE)->base.u.bits.lang_flag_0) + #define CLEANUP_P(NODE) TREE_LANG_FLAG_0 (TRY_BLOCK_CHECK (NODE)) #define BIND_EXPR_TRY_BLOCK(NODE) \ @@ -6558,6 +6560,7 @@ extern int processing_template_parmlist; extern bool dependent_type_p (tree); extern bool dependent_scope_p (tree); extern bool any_dependent_template_arguments_p (const_tree); +extern bool any_erroneous_template_args_p (const_tree); extern bool dependent_template_p (tree); extern bool dependent_template_id_p (tree, tree); extern bool type_dependent_expression_p (tree); Index: parser.c =================================================================== --- parser.c (revision 258355) +++ parser.c (working copy) @@ -22682,6 +22682,8 @@ cp_parser_class_specifier_1 (cp_parser* parser) FOR_EACH_VEC_SAFE_ELT (unparsed_funs_with_default_args, ix, e) { decl = e->decl; + if (any_erroneous_template_args_p (decl)) + continue; /* If there are default arguments that have not yet been processed, take care of them now. */ if (class_type != e->class_type) @@ -22704,6 +22706,8 @@ cp_parser_class_specifier_1 (cp_parser* parser) save_ccr = current_class_ref; FOR_EACH_VEC_SAFE_ELT (unparsed_nsdmis, ix, decl) { + if (any_erroneous_template_args_p (decl)) + continue; if (class_type != DECL_CONTEXT (decl)) { if (pushed_scope) @@ -27642,6 +27646,9 @@ cp_parser_late_parsing_for_member (cp_parser* pars if (DECL_FUNCTION_TEMPLATE_P (member_function)) member_function = DECL_TEMPLATE_RESULT (member_function); + if (any_erroneous_template_args_p (member_function)) + return; + /* There should not be any class definitions in progress at this point; the bodies of members are only parsed outside of all class definitions. */ Index: pt.c =================================================================== --- pt.c (revision 258355) +++ pt.c (working copy) @@ -4459,6 +4459,8 @@ end_template_parm_list (tree parms) { next = TREE_CHAIN (parm); TREE_VEC_ELT (saved_parmlist, nparms) = parm; + if (error_operand_p (parm)) + ERRONEOUS_TREE_VEC (saved_parmlist) = true; TREE_CHAIN (parm) = NULL_TREE; } @@ -25048,6 +25050,42 @@ any_dependent_template_arguments_p (const_tree arg return false; } +/* Returns true if ARGS contains any errors. */ + +bool +any_erroneous_template_args_p (const_tree args) +{ + int i; + int j; + + if (args && TREE_CODE (args) != TREE_VEC) + { + if (tree ti = get_template_info (args)) + args = TI_ARGS (ti); + else + args = NULL_TREE; + } + + if (!args) + return false; + if (args == error_mark_node) + return true; + + for (i = 0; i < TMPL_ARGS_DEPTH (args); ++i) + { + const_tree level = TMPL_ARGS_LEVEL (args, i + 1); + if (ERRONEOUS_TREE_VEC (level)) + return true; + /* + for (j = 0; j < TREE_VEC_LENGTH (level); ++j) + if (error_operand_p (TREE_VEC_ELT (level, j))) + return true; + */ + } + + return false; +} + /* Returns TRUE if the template TMPL is type-dependent. */ bool
Index: cp/parser.c =================================================================== --- cp/parser.c (revision 258264) +++ cp/parser.c (working copy) @@ -565,6 +565,8 @@ cp_debug_parser (FILE *file, cp_parser *parser) "local class", parser->in_function_body); cp_debug_print_flag (file, "Auto correct a colon to a scope operator", parser->colon_corrects_to_scope_p); + cp_debug_print_flag (file, "Error in template parameter list", + parser->error_in_template_parameter_list_p); cp_debug_print_flag (file, "Colon doesn't start a class definition", parser->colon_doesnt_start_class_def_p); if (parser->type_definition_forbidden_message) @@ -3910,6 +3912,9 @@ cp_parser_new (void) /* We can correct until told otherwise. */ parser->colon_corrects_to_scope_p = true; + /* No error so far. */ + parser->error_in_template_parameter_list_p = false; + /* The unparsed function queue is empty. */ push_unparsed_function_queues (parser); @@ -10151,6 +10156,8 @@ cp_parser_lambda_expression (cp_parser* parser) /* Inside the class, surrounding template-parameter-lists do not apply. */ unsigned int saved_num_template_parameter_lists = parser->num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; unsigned char in_statement = parser->in_statement; bool in_switch_statement_p = parser->in_switch_statement_p; bool fully_implicit_function_template_p @@ -10161,6 +10168,7 @@ cp_parser_lambda_expression (cp_parser* parser) = parser->auto_is_implicit_function_template_parm_p; parser->num_template_parameter_lists = 0; + parser->error_in_template_parameter_list_p = false; parser->in_statement = 0; parser->in_switch_statement_p = false; parser->fully_implicit_function_template_p = false; @@ -10199,6 +10207,8 @@ cp_parser_lambda_expression (cp_parser* parser) type = finish_struct (type, /*attributes=*/NULL_TREE); parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; parser->in_statement = in_statement; parser->in_switch_statement_p = in_switch_statement_p; parser->fully_implicit_function_template_p @@ -10624,7 +10634,8 @@ cp_parser_lambda_declarator_opt (cp_parser* parser { fco = finish_member_template_decl (fco); finish_template_decl (template_param_list); - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; } else if (parser->fully_implicit_function_template_p) fco = finish_fully_implicit_template (parser, fco); @@ -15065,6 +15076,11 @@ cp_parser_template_parameter_list (cp_parser* pars parameter_list = chainon (parameter_list, err_parm); } + if (parameter == error_mark_node + || error_operand_p (TREE_VALUE (parameter)) + || error_operand_p (TREE_PURPOSE (parameter))) + parser->error_in_template_parameter_list_p = true; + /* If the next token is not a `,', we're done. */ if (cp_lexer_next_token_is_not (parser->lexer, CPP_COMMA)) break; @@ -16673,7 +16689,8 @@ cp_parser_explicit_specialization (cp_parser* pars if (need_lang_pop) pop_lang_context (); /* We're done with this parameter list. */ - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; } /* Parse a type-specifier. @@ -22438,6 +22455,7 @@ cp_parser_class_specifier_1 (cp_parser* parser) tree attributes = NULL_TREE; bool nested_name_specifier_p; unsigned saved_num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p; bool saved_in_function_body; unsigned char in_statement; bool in_switch_statement_p; @@ -22480,6 +22498,9 @@ cp_parser_class_specifier_1 (cp_parser* parser) saved_num_template_parameter_lists = parser->num_template_parameter_lists; parser->num_template_parameter_lists = 0; + saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; + parser->error_in_template_parameter_list_p = false; /* We are not in a function body. */ saved_in_function_body = parser->in_function_body; parser->in_function_body = false; @@ -22658,7 +22679,8 @@ cp_parser_class_specifier_1 (cp_parser* parser) struct A::B { void f() { } }; there is no need to delay the parsing of `A::B::f'. */ - if (--parser->num_classes_being_defined == 0) + if (--parser->num_classes_being_defined == 0 + && !saved_error_in_template_parameter_list_p) { tree decl; tree class_type = NULL_TREE; @@ -22752,6 +22774,8 @@ cp_parser_class_specifier_1 (cp_parser* parser) parser->in_function_body = saved_in_function_body; parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; parser->in_unbraced_linkage_specification_p = saved_in_unbraced_linkage_specification_p; @@ -23253,7 +23277,8 @@ cp_parser_class_head (cp_parser* parser, if (invalid_explicit_specialization_p) { end_specialization (); - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; } if (type) @@ -26616,6 +26641,7 @@ cp_parser_constructor_declarator_p (cp_parser *par tree type; tree pushed_scope = NULL_TREE; unsigned saved_num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p; /* Names appearing in the type-specifier should be looked up in the scope of the class. */ @@ -26642,6 +26668,9 @@ cp_parser_constructor_declarator_p (cp_parser *par saved_num_template_parameter_lists = parser->num_template_parameter_lists; parser->num_template_parameter_lists = 0; + saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; + parser->error_in_template_parameter_list_p = false; /* Look for the type-specifier. */ cp_parser_type_specifier (parser, @@ -26653,6 +26682,8 @@ cp_parser_constructor_declarator_p (cp_parser *par parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; /* Leave the scope of the class. */ if (pushed_scope) @@ -26751,6 +26782,7 @@ cp_parser_function_definition_after_declarator (cp bool saved_in_unbraced_linkage_specification_p; bool saved_in_function_body; unsigned saved_num_template_parameter_lists; + bool saved_error_in_template_parameter_list_p; cp_token *token; bool fully_implicit_function_template_p = parser->fully_implicit_function_template_p; @@ -26799,6 +26831,9 @@ cp_parser_function_definition_after_declarator (cp saved_num_template_parameter_lists = parser->num_template_parameter_lists; parser->num_template_parameter_lists = 0; + saved_error_in_template_parameter_list_p + = parser->error_in_template_parameter_list_p; + parser->error_in_template_parameter_list_p = false; /* If the next token is `try', `__transaction_atomic', or `__transaction_relaxed`, then we are looking at either function-try-block @@ -26824,6 +26859,8 @@ cp_parser_function_definition_after_declarator (cp = saved_in_unbraced_linkage_specification_p; parser->num_template_parameter_lists = saved_num_template_parameter_lists; + parser->error_in_template_parameter_list_p + = saved_error_in_template_parameter_list_p; parser->in_function_body = saved_in_function_body; parser->fully_implicit_function_template_p @@ -26893,7 +26930,8 @@ cp_parser_template_declaration_after_parameters (c /*complain=*/true); } /* We are done with the current parameter list. */ - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; pop_deferring_access_checks (); @@ -39249,7 +39287,8 @@ finish_fully_implicit_template (cp_parser *parser, end_template_decl (); parser->fully_implicit_function_template_p = false; - --parser->num_template_parameter_lists; + if (--parser->num_template_parameter_lists == 0) + parser->error_in_template_parameter_list_p = false; return member_decl_opt; } Index: cp/parser.h =================================================================== --- cp/parser.h (revision 258264) +++ cp/parser.h (working copy) @@ -346,6 +346,9 @@ struct GTY(()) cp_parser { is terminated by colon. */ bool colon_doesnt_start_class_def_p; + /* TRUE if there was an error in a template parameter list. */ + bool error_in_template_parameter_list_p; + /* If non-NULL, then we are parsing a construct where new type definitions are not permitted. The string stored here will be issued as an error message if a type is defined. */ Index: testsuite/g++.dg/cpp0x/pr71169.C =================================================================== --- testsuite/g++.dg/cpp0x/pr71169.C (nonexistent) +++ testsuite/g++.dg/cpp0x/pr71169.C (working copy) @@ -0,0 +1,7 @@ +// { dg-do compile { target c++11 } } + +template <Preconditioner> class A { // { dg-error "declared" } + template <class = int> void m_fn1() { + m_fn1(); + } +}; Index: testsuite/g++.dg/cpp0x/pr71832.C =================================================================== --- testsuite/g++.dg/cpp0x/pr71832.C (nonexistent) +++ testsuite/g++.dg/cpp0x/pr71832.C (working copy) @@ -0,0 +1,7 @@ +// { dg-do compile { target c++11 } } + +template < typename decltype (0) > struct A // { dg-error "expected|two or more" } +{ + void foo () { baz (); } + template < typename ... S > void baz () {} +};