Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
OK.
On Mon, May 21, 2018 at 8:41 AM, Paolo Carlini wrote:
> Hi again,
>
> On 19/05/2018 15:30, Jason Merrill wrote:
>>
>> How about doing cp_parser_commit_to_tentative_parse if we see
>> something that must be a declaration? cp_parser_simple_declaration
>> has
>>
>>/* If we have seen at least one decl-specifier, and the next token
>> is not a parenthesis, then we must be looking at a declaration.
>> (After "int (" we might be looking at a functional cast.) */
>>if (decl_specifiers.any_specifiers_p
>>&& cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN)
>>&& cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)
>>&& !cp_parser_error_occurred (parser))
>> cp_parser_commit_to_tentative_parse (parser);
>>
>> That seems useful here, as well. Maybe factored into a separate function.
>
> The below implements this new idea, which indeed appears to work well: I
> tested it and testsuite-wise seems essentially equivalent to what I posted
> yesterday, besides a slightly worse error-recovery for the first issue in
> cpp1z/decomp16.C: an additional 'no match for ‘operator=’' error.
>
> Thanks!
> Paolo.
>
> //
>
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi again,
On 19/05/2018 15:30, Jason Merrill wrote:
How about doing cp_parser_commit_to_tentative_parse if we see
something that must be a declaration? cp_parser_simple_declaration
has
/* If we have seen at least one decl-specifier, and the next token
is not a parenthesis, then we must be looking at a declaration.
(After "int (" we might be looking at a functional cast.) */
if (decl_specifiers.any_specifiers_p
&& cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN)
&& cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)
&& !cp_parser_error_occurred (parser))
cp_parser_commit_to_tentative_parse (parser);
That seems useful here, as well. Maybe factored into a separate function.
The below implements this new idea, which indeed appears to work well: I
tested it and testsuite-wise seems essentially equivalent to what I
posted yesterday, besides a slightly worse error-recovery for the first
issue in cpp1z/decomp16.C: an additional 'no match for ‘operator=’' error.
Thanks!
Paolo.
//
Index: cp/parser.c
===
--- cp/parser.c (revision 260402)
+++ cp/parser.c (working copy)
@@ -11527,6 +11527,53 @@ cp_parser_selection_statement (cp_parser* parser,
}
}
+/* Helper function for cp_parser_condition and cp_parser_simple_declaration.
+ If we have seen at least one decl-specifier, and the next token
+ is not a parenthesis, then we must be looking at a declaration.
+ (After "int (" we might be looking at a functional cast.) */
+
+static bool
+cp_parser_maybe_commit_to_declaration (cp_parser* parser,
+ bool any_specifiers_p)
+{
+ if (any_specifiers_p
+ && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN)
+ && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)
+ && !cp_parser_error_occurred (parser))
+{
+ cp_parser_commit_to_tentative_parse (parser);
+ return true;
+}
+ return false;
+}
+
+/* Helper function for cp_parser_condition. Enforces [stmt.stmt]/2:
+ The declarator shall not specify a function or an array. Returns
+ TRUE if the declarator is valid, FALSE otherwise. */
+
+static bool
+cp_parser_check_condition_declarator (cp_parser* parser,
+ cp_declarator *declarator,
+ location_t loc)
+{
+ if (function_declarator_p (declarator)
+ || declarator->kind == cdk_array)
+{
+ if (declarator->kind == cdk_array)
+ error_at (loc, "condition declares an array");
+ else
+ error_at (loc, "condition declares a function");
+ if (parser->fully_implicit_function_template_p)
+ abort_fully_implicit_template (parser);
+ cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true,
+/*or_comma=*/false,
+/*consume_paren=*/false);
+ return false;
+}
+ else
+return true;
+}
+
/* Parse a condition.
condition:
@@ -11563,6 +11610,10 @@ cp_parser_condition (cp_parser* parser)
&declares_class_or_enum);
/* Restore the saved message. */
parser->type_definition_forbidden_message = saved_message;
+
+ cp_parser_maybe_commit_to_declaration (parser,
+type_specifiers.any_specifiers_p);
+
/* If all is well, we might be looking at a declaration. */
if (!cp_parser_error_occurred (parser))
{
@@ -11571,6 +11622,7 @@ cp_parser_condition (cp_parser* parser)
tree attributes;
cp_declarator *declarator;
tree initializer = NULL_TREE;
+ location_t loc = cp_lexer_peek_token (parser->lexer)->location;
/* Parse the declarator. */
declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED,
@@ -11601,6 +11653,9 @@ cp_parser_condition (cp_parser* parser)
bool non_constant_p;
int flags = LOOKUP_ONLYCONVERTING;
+ if (!cp_parser_check_condition_declarator (parser, declarator, loc))
+ return error_mark_node;
+
/* Create the declaration. */
decl = start_decl (declarator, &type_specifiers,
/*initialized_p=*/true,
@@ -11614,12 +11669,18 @@ cp_parser_condition (cp_parser* parser)
CONSTRUCTOR_IS_DIRECT_INIT (initializer) = 1;
flags = 0;
}
- else
+ else if (cp_lexer_next_token_is (parser->lexer, CPP_EQ))
{
/* Consume the `='. */
- cp_parser_require (parser, CPP_EQ, RT_EQ);
- initializer = cp_parser_initializer_clause (parser,
&non_constant_p);
+ cp_lexer_consume_token (parser->lexer);
+ initializer = cp_parser_initializer_clause (parser,
+ &non_constant_p);
}
+
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi,
On 19/05/2018 15:30, Jason Merrill wrote:
I would expect it to cause different diagnostic issues, from
complaining about something not being a proper declaration when it's
really an expression. I also wonder about warning problems (either
missed or bogus) due to trying these in a different order.
Yes. It seems kind of science-fiction project ;) I'll give it more
thought, anyway...
How about doing cp_parser_commit_to_tentative_parse if we see
something that must be a declaration? cp_parser_simple_declaration
has
/* If we have seen at least one decl-specifier, and the next token
is not a parenthesis, then we must be looking at a declaration.
(After "int (" we might be looking at a functional cast.) */
if (decl_specifiers.any_specifiers_p
&& cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN)
&& cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)
&& !cp_parser_error_occurred (parser))
cp_parser_commit_to_tentative_parse (parser);
That seems useful here, as well. Maybe factored into a separate function.
Hmm, I see, thanks for the tip. To other day, eventually I figured out
the below and fully tested it, which seems quite good to me, frankly: it
simply adds a check of parenthesized_p (per your highly welcome previous
clarification) to the existing checks: having spent quite a bit of time
on [dcl.ambig.res] I think it's Ok - if the declarator isn't
parenthesized can't be in fact an expression - and alone that allows us
to make very good progress - well beyond the requirements of c++/84588 -
on a number of diagnostic-quality fronts, see the attached additional
testcases too. I can still construct a nasty condition of the form, say,
'a (a(int auto)JUNK' which we don't handle well in terms of error
recovery, but if you agree that the below is correct, it's probably
enough for the *regression* part...
Thanks!
Paolo.
/
Index: cp/parser.c
===
--- cp/parser.c (revision 260402)
+++ cp/parser.c (working copy)
@@ -11527,6 +11527,33 @@ cp_parser_selection_statement (cp_parser* parser,
}
}
+/* Helper function for cp_parser_condition. Enforces [stmt.stmt]/2:
+ The declarator shall not specify a function or an array. Returns
+ TRUE if the declarator is valid, FALSE otherwise. */
+
+static bool
+cp_parser_check_condition_declarator (cp_parser* parser,
+ cp_declarator *declarator,
+ location_t loc)
+{
+ if (function_declarator_p (declarator)
+ || declarator->kind == cdk_array)
+{
+ if (declarator->kind == cdk_array)
+ error_at (loc, "condition declares an array");
+ else
+ error_at (loc, "condition declares a function");
+ if (parser->fully_implicit_function_template_p)
+ abort_fully_implicit_template (parser);
+ cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true,
+/*or_comma=*/false,
+/*consume_paren=*/false);
+ return false;
+}
+ else
+return true;
+}
+
/* Parse a condition.
condition:
@@ -11571,11 +11598,13 @@ cp_parser_condition (cp_parser* parser)
tree attributes;
cp_declarator *declarator;
tree initializer = NULL_TREE;
+ bool parenthesized_p = false;
+ location_t loc = cp_lexer_peek_token (parser->lexer)->location;
/* Parse the declarator. */
declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED,
/*ctor_dtor_or_conv_p=*/NULL,
-/*parenthesized_p=*/NULL,
+&parenthesized_p,
/*member_p=*/false,
/*friend_p=*/false);
/* Parse the attributes. */
@@ -11582,8 +11611,9 @@ cp_parser_condition (cp_parser* parser)
attributes = cp_parser_attributes_opt (parser);
/* Parse the asm-specification. */
asm_specification = cp_parser_asm_specification_opt (parser);
- /* If the next token is not an `=' or '{', then we might still be
-looking at an expression. For example:
+ /* If the next token is not an `=' or '{' and the declarator is
+parenthesized, then we might still be looking at an expression.
+For example:
if (A(a).x)
@@ -11590,11 +11620,12 @@ cp_parser_condition (cp_parser* parser)
looks like a decl-specifier-seq and a declarator -- but then
there is no `=', so this is an expression. */
if (cp_lexer_next_token_is_not (parser->lexer, CPP_EQ)
- && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE))
+ && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)
+ && parenthesized_p)
cp_parser_simulate_error (p
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
On Fri, May 18, 2018 at 8:27 PM, Paolo Carlini wrote:
> On 19/05/2018 01:40, Jason Merrill wrote:
>> On Fri, May 18, 2018 at 1:40 PM, Paolo Carlini
>> wrote:
>>>
>>> Hi again,
>>>
>>> I'm playing with a wild, wild idea: would it make sense to try *first* an
>>> expression? Because, a quickly hacked draft appears to handle very
>>> elegantly
>>> all the possible cases of "junk" after the declarator, eg:
>>>
>>> void foo()
>>> {
>>> if (void bar()JUNK);
>>> }
>>>
>>> and the parenthesized case, etc, etc. Before trying to seriously work on
>>> that I wanted to ask...
>>
>> We'd need to try parsing as a declaration whether or not parsing as an
>> expression works, since any ambiguous cases are treated as
>> declarations [stmt.ambig].
>
> Yeah, that complicates the implementation of my idea. However, I'm thinking
> that at least *in the specific case of cp_parse_condition* from the
> computational complexity point of view probably we wouldn't regress much,
> because declarations are rare anyway, thus in most of the cases we end up
> doing both anyway. If we do expressions first and we save the result, then I
> believe when we can handle the declarator as something which cannot be a
> function or an array even if we don't see the initializer much more easily,
> we easily have a better diagnostic for things like
>
> if (int x);
>
> (another case we currently handle pretty badly, we don't talk about the
> missing initializer at all!), we cope elegantly with any junk following the
> wrong function/array declaration, etc. See that attached, if you are
> curious, which essentially passes the testsuite modulo a nit (*) which
> doesn't have anything to do with [stmt.ambig] per se (which of course is
> *not* correctly implemented in the wip).
>
> Can you give me your opinion about the more detailed idea, in particular
> whether we already have good infrastructure to implement [stmt.ambig] in
> this context, thus to keep the first parsing as an expression around,
> possibly returning to it if the parsing as a declaration fails??
I would expect it to cause different diagnostic issues, from
complaining about something not being a proper declaration when it's
really an expression. I also wonder about warning problems (either
missed or bogus) due to trying these in a different order.
How about doing cp_parser_commit_to_tentative_parse if we see
something that must be a declaration? cp_parser_simple_declaration
has
/* If we have seen at least one decl-specifier, and the next token
is not a parenthesis, then we must be looking at a declaration.
(After "int (" we might be looking at a functional cast.) */
if (decl_specifiers.any_specifiers_p
&& cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN)
&& cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)
&& !cp_parser_error_occurred (parser))
cp_parser_commit_to_tentative_parse (parser);
That seems useful here, as well. Maybe factored into a separate function.
Jason
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi,
On 19/05/2018 01:40, Jason Merrill wrote:
On Fri, May 18, 2018 at 1:40 PM, Paolo Carlini wrote:
Hi again,
I'm playing with a wild, wild idea: would it make sense to try *first* an
expression? Because, a quickly hacked draft appears to handle very elegantly
all the possible cases of "junk" after the declarator, eg:
void foo()
{
if (void bar()JUNK);
}
and the parenthesized case, etc, etc. Before trying to seriously work on
that I wanted to ask...
We'd need to try parsing as a declaration whether or not parsing as an
expression works, since any ambiguous cases are treated as
declarations [stmt.ambig].
Yeah, that complicates the implementation of my idea. However, I'm
thinking that at least *in the specific case of cp_parse_condition* from
the computational complexity point of view probably we wouldn't regress
much, because declarations are rare anyway, thus in most of the cases we
end up doing both anyway. If we do expressions first and we save the
result, then I believe when we can handle the declarator as something
which cannot be a function or an array even if we don't see the
initializer much more easily, we easily have a better diagnostic for
things like
if (int x);
(another case we currently handle pretty badly, we don't talk about the
missing initializer at all!), we cope elegantly with any junk following
the wrong function/array declaration, etc. See that attached, if you are
curious, which essentially passes the testsuite modulo a nit (*) which
doesn't have anything to do with [stmt.ambig] per se (which of course is
*not* correctly implemented in the wip).
Can you give me your opinion about the more detailed idea, in particular
whether we already have good infrastructure to implement [stmt.ambig] in
this context, thus to keep the first parsing as an expression around,
possibly returning to it if the parsing as a declaration fails??
I hope I'm making sense, it's again late here... in any case the wip
patch I did much earlier today ;)
Paolo.
(*) Has to do with David's access failure hint not handling deferred
access checks (accessor-fixits-6.C).
Index: parser.c
===
--- parser.c(revision 260347)
+++ parser.c(working copy)
@@ -11527,6 +11527,33 @@ cp_parser_selection_statement (cp_parser* parser,
}
}
+/* Helper function for cp_parser_condition. Enforces [stmt.stmt]/2:
+ The declarator shall not specify a function or an array. Returns
+ TRUE if the declarator is valid, FALSE otherwise. */
+
+static bool
+cp_parser_check_condition_declarator (cp_parser* parser,
+ cp_declarator *declarator,
+ location_t loc)
+{
+ if (function_declarator_p (declarator)
+ || declarator->kind == cdk_array)
+{
+ if (declarator->kind == cdk_array)
+ error_at (loc, "condition declares an array");
+ else
+ error_at (loc, "condition declares a function");
+ if (parser->fully_implicit_function_template_p)
+ abort_fully_implicit_template (parser);
+ cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true,
+/*or_comma=*/false,
+/*consume_paren=*/false);
+ return false;
+}
+ else
+return true;
+}
+
/* Parse a condition.
condition:
@@ -11545,12 +11572,20 @@ cp_parser_selection_statement (cp_parser* parser,
static tree
cp_parser_condition (cp_parser* parser)
{
+ cp_parser_parse_tentatively (parser);
+
+ /* Try the expression first. */
+ tree expr = cp_parser_expression (parser);
+
+ if (cp_parser_parse_definitely (parser))
+return expr;
+
+ cp_parser_parse_tentatively (parser);
+
cp_decl_specifier_seq type_specifiers;
const char *saved_message;
int declares_class_or_enum;
- /* Try the declaration first. */
- cp_parser_parse_tentatively (parser);
/* New types are not allowed in the type-specifier-seq for a
condition. */
saved_message = parser->type_definition_forbidden_message;
@@ -11563,6 +11598,7 @@ cp_parser_condition (cp_parser* parser)
&declares_class_or_enum);
/* Restore the saved message. */
parser->type_definition_forbidden_message = saved_message;
+
/* If all is well, we might be looking at a declaration. */
if (!cp_parser_error_occurred (parser))
{
@@ -11570,7 +11606,8 @@ cp_parser_condition (cp_parser* parser)
tree asm_specification;
tree attributes;
cp_declarator *declarator;
- tree initializer = NULL_TREE;
+ tree initializer;
+ location_t loc = cp_lexer_peek_token (parser->lexer)->location;
/* Parse the declarator. */
declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED,
@@ -11582,19 +11619,7 @@ cp_parser_condition (cp_parser* parser)
attributes = cp_parser_attributes_opt
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
On Fri, May 18, 2018 at 1:40 PM, Paolo Carlini wrote:
> Hi again,
>
> I'm playing with a wild, wild idea: would it make sense to try *first* an
> expression? Because, a quickly hacked draft appears to handle very elegantly
> all the possible cases of "junk" after the declarator, eg:
>
> void foo()
> {
> if (void bar()JUNK);
> }
>
> and the parenthesized case, etc, etc. Before trying to seriously work on
> that I wanted to ask...
We'd need to try parsing as a declaration whether or not parsing as an
expression works, since any ambiguous cases are treated as
declarations [stmt.ambig].
Jason
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi again,
I'm playing with a wild, wild idea: would it make sense to try *first*
an expression? Because, a quickly hacked draft appears to handle very
elegantly all the possible cases of "junk" after the declarator, eg:
void foo()
{
if (void bar()JUNK);
}
and the parenthesized case, etc, etc. Before trying to seriously work on
that I wanted to ask...
Paolo.
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi,
On 18/05/2018 16:45, Jason Merrill wrote:
I guess it's desirable to also give this error when the declarator is
followed by ) or ; rather than other tokens that could be more
expression (like in A(a).x in the comment).
I can certainly try to implement this, maybe just something minimal to begin
with, as you say ) or ; alone would be safe and would already take care of
all the tests I have around.
Certainly, unconditionally deferring at that point to cp_parser_expression
leads to *very* bad error-recovery. For fun, try with 8.1.0:
void foo()
{
for (;void bar(););
}
and it gets worse.
It would also need to be only for a non-parenthesized declarator,
which can't be an expression; a parenthesized declarator can be, as in
this well-formed testcase:
bool (bar()) { return 0; } // declaration
void foo()
{
for (;bool (bar());); // expression
}
Thanks for clarifying that. I'll make sure we have such testcases.
Interestingly, if we aren't careful about that in the new code,
libstdc++-v3 doesn't even build, eh!
Paolo.
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
On Fri, May 18, 2018 at 10:30 AM, Paolo Carlini
wrote:
> Hi,
>
> On 18/05/2018 16:19, Jason Merrill wrote:
>>
>> On Fri, May 18, 2018 at 10:05 AM, Paolo Carlini
>> wrote:
>>>
>>> Hi,
>>>
>>> On 18/05/2018 15:56, Jason Merrill wrote:
I had in mind moving the call to cp_parser_check_condition_declarator
into the block controlled by cp_parser_parse_definitely; this is a
semantic check that should follow the syntactic checks. If there's no
initializer, it doesn't parse as a condition declaration, so we don't
want to complain about it being a semantically invalid condition
declaration.
>>>
>>> If we do that we are back to something very, very, similar to what I
>>> posted
>>> at the beginning of the thread, right? Therefore, for all the testcases
>>> which don't have an initializer we end-up with *horrible*, literally
>>> *horrible* cascades of errors, plus we ICE on the c++/84588 variants
>>> without
>>> an initializer.
>>
>> Ah. Why is that?
>>
>> I guess it's desirable to also give this error when the declarator is
>> followed by ) or ; rather than other tokens that could be more
>> expression (like in A(a).x in the comment).
>
> I can certainly try to implement this, maybe just something minimal to begin
> with, as you say ) or ; alone would be safe and would already take care of
> all the tests I have around.
>
> Certainly, unconditionally deferring at that point to cp_parser_expression
> leads to *very* bad error-recovery. For fun, try with 8.1.0:
>
> void foo()
> {
> for (;void bar(););
> }
>
> and it gets worse.
It would also need to be only for a non-parenthesized declarator,
which can't be an expression; a parenthesized declarator can be, as in
this well-formed testcase:
bool (bar()) { return 0; } // declaration
void foo()
{
for (;bool (bar());); // expression
}
Jason
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi,
On 18/05/2018 16:19, Jason Merrill wrote:
On Fri, May 18, 2018 at 10:05 AM, Paolo Carlini
wrote:
Hi,
On 18/05/2018 15:56, Jason Merrill wrote:
I had in mind moving the call to cp_parser_check_condition_declarator
into the block controlled by cp_parser_parse_definitely; this is a
semantic check that should follow the syntactic checks. If there's no
initializer, it doesn't parse as a condition declaration, so we don't
want to complain about it being a semantically invalid condition
declaration.
If we do that we are back to something very, very, similar to what I posted
at the beginning of the thread, right? Therefore, for all the testcases
which don't have an initializer we end-up with *horrible*, literally
*horrible* cascades of errors, plus we ICE on the c++/84588 variants without
an initializer.
Ah. Why is that?
I guess it's desirable to also give this error when the declarator is
followed by ) or ; rather than other tokens that could be more
expression (like in A(a).x in the comment).
I can certainly try to implement this, maybe just something minimal to
begin with, as you say ) or ; alone would be safe and would already take
care of all the tests I have around.
Certainly, unconditionally deferring at that point to
cp_parser_expression leads to *very* bad error-recovery. For fun, try
with 8.1.0:
void foo()
{
for (;void bar(););
}
and it gets worse.
Paolo.
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
On Fri, May 18, 2018 at 10:05 AM, Paolo Carlini wrote: > Hi, > > On 18/05/2018 15:56, Jason Merrill wrote: >> >> I had in mind moving the call to cp_parser_check_condition_declarator >> into the block controlled by cp_parser_parse_definitely; this is a >> semantic check that should follow the syntactic checks. If there's no >> initializer, it doesn't parse as a condition declaration, so we don't >> want to complain about it being a semantically invalid condition >> declaration. > > If we do that we are back to something very, very, similar to what I posted > at the beginning of the thread, right? Therefore, for all the testcases > which don't have an initializer we end-up with *horrible*, literally > *horrible* cascades of errors, plus we ICE on the c++/84588 variants without > an initializer. Ah. Why is that? I guess it's desirable to also give this error when the declarator is followed by ) or ; rather than other tokens that could be more expression (like in A(a).x in the comment). Jason
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi, On 18/05/2018 15:56, Jason Merrill wrote: I had in mind moving the call to cp_parser_check_condition_declarator into the block controlled by cp_parser_parse_definitely; this is a semantic check that should follow the syntactic checks. If there's no initializer, it doesn't parse as a condition declaration, so we don't want to complain about it being a semantically invalid condition declaration. If we do that we are back to something very, very, similar to what I posted at the beginning of the thread, right? Therefore, for all the testcases which don't have an initializer we end-up with *horrible*, literally *horrible* cascades of errors, plus we ICE on the c++/84588 variants without an initializer. If you like we can do that and defer the other related issues - strictly speaking c++/84588 would be fixed - to another new bug. Paolo.
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
On Fri, May 18, 2018 at 4:41 AM, Paolo Carlini wrote: > Hi, > >>> On 18/05/2018 01:21, Jason Merrill wrote: On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini wrote: > > On 17/05/2018 16:58, Jason Merrill wrote: >> >> On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini >> wrote: >>> >>> PS: maybe better using function_declarator_p? >> >> I think so, yes. The relevant rule seems to be "The declarator shall >> not specify a function or an array.", so let's check for arrays, too. > > Agreed. I had the amended patch ready when I noticed (again) that it > wasn't > addressing another related class of issues which involves declarators > not > followed by initializers. Thus I tried to fix those too, and the below > which > moves the check up appears to work fine, passes testing, etc. Are there > any > risks that an erroneous function / array as declarator is in fact a > well > formed expression?!? That doesn't matter; if it parses as a declarator, it's a declarator, even if it's an ill-formed declarator. But... + bool decl_p = cp_parser_parse_definitely (parser); + if (!cp_parser_check_condition_declarator (parser, declarator, loc)) +return error_mark_node; ...if cp_parser_parse_definitely returns false, parsing as a declarator failed, so we shouldn't look at "declarator". > > What I'm attaching below isn't affected by this problem: I moved the check > further up - *before* maybe calling cp_parser_simulated_error because an > initializer isn't in sight - and is carried out only when > !cp_parser_error_occurred, thus cp_parser_declarator succeeded . > cp_parser_commit_to_tentative_parse is called when the check fails. > Bootstrapped and tested on x86_64-linux. I had in mind moving the call to cp_parser_check_condition_declarator into the block controlled by cp_parser_parse_definitely; this is a semantic check that should follow the syntactic checks. If there's no initializer, it doesn't parse as a condition declaration, so we don't want to complain about it being a semantically invalid condition declaration. Jason
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi,
On 18/05/2018 01:21, Jason Merrill wrote:
On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini
wrote:
On 17/05/2018 16:58, Jason Merrill wrote:
On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini
wrote:
PS: maybe better using function_declarator_p?
I think so, yes. The relevant rule seems to be "The declarator shall
not specify a function or an array.", so let's check for arrays, too.
Agreed. I had the amended patch ready when I noticed (again) that
it wasn't
addressing another related class of issues which involves
declarators not
followed by initializers. Thus I tried to fix those too, and the
below which
moves the check up appears to work fine, passes testing, etc. Are
there any
risks that an erroneous function / array as declarator is in fact a
well
formed expression?!?
That doesn't matter; if it parses as a declarator, it's a declarator,
even if it's an ill-formed declarator. But...
+ bool decl_p = cp_parser_parse_definitely (parser);
+ if (!cp_parser_check_condition_declarator (parser,
declarator, loc))
+ return error_mark_node;
...if cp_parser_parse_definitely returns false, parsing as a
declarator failed, so we shouldn't look at "declarator".
What I'm attaching below isn't affected by this problem: I moved the
check further up - *before* maybe calling cp_parser_simulated_error
because an initializer isn't in sight - and is carried out only when
!cp_parser_error_occurred, thus cp_parser_declarator succeeded .
cp_parser_commit_to_tentative_parse is called when the check fails.
Bootstrapped and tested on x86_64-linux.
Thanks!
Paolo.
//
Index: cp/parser.c
===
--- cp/parser.c (revision 260347)
+++ cp/parser.c (working copy)
@@ -11527,6 +11527,34 @@ cp_parser_selection_statement (cp_parser* parser,
}
}
+/* Helper function for cp_parser_condition. Enforces [stmt.stmt]/2:
+ The declarator shall not specify a function or an array. Returns
+ TRUE if the declarator is valid, FALSE otherwise. */
+
+static bool
+cp_parser_check_condition_declarator (cp_parser* parser,
+ cp_declarator *declarator,
+ location_t loc)
+{
+ if (function_declarator_p (declarator)
+ || declarator->kind == cdk_array)
+{
+ cp_parser_commit_to_tentative_parse (parser);
+ if (declarator->kind == cdk_array)
+ error_at (loc, "condition declares an array");
+ else
+ error_at (loc, "condition declares a function");
+ if (parser->fully_implicit_function_template_p)
+ abort_fully_implicit_template (parser);
+ cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true,
+/*or_comma=*/false,
+/*consume_paren=*/false);
+ return false;
+}
+ else
+return true;
+}
+
/* Parse a condition.
condition:
@@ -11571,6 +11599,7 @@ cp_parser_condition (cp_parser* parser)
tree attributes;
cp_declarator *declarator;
tree initializer = NULL_TREE;
+ location_t loc = cp_lexer_peek_token (parser->lexer)->location;
/* Parse the declarator. */
declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED,
@@ -11582,6 +11611,11 @@ cp_parser_condition (cp_parser* parser)
attributes = cp_parser_attributes_opt (parser);
/* Parse the asm-specification. */
asm_specification = cp_parser_asm_specification_opt (parser);
+
+ if (!cp_parser_error_occurred (parser)
+ && !cp_parser_check_condition_declarator (parser, declarator, loc))
+ return error_mark_node;
+
/* If the next token is not an `=' or '{', then we might still be
looking at an expression. For example:
Index: testsuite/g++.dg/cpp0x/cond1.C
===
--- testsuite/g++.dg/cpp0x/cond1.C (nonexistent)
+++ testsuite/g++.dg/cpp0x/cond1.C (working copy)
@@ -0,0 +1,23 @@
+// PR c++/84588
+// { dg-do compile { target c++11 } }
+
+void foo()
+{
+ if (int bar() {}) // { dg-error "condition declares a function" }
+;
+
+ for (;int bar() {};) // { dg-error "condition declares a function" }
+;
+
+ while (int bar() {}) // { dg-error "condition declares a function" }
+;
+
+ if (int a[] {}) // { dg-error "condition declares an array" }
+;
+
+ for (;int a[] {};) // { dg-error "condition declares an array" }
+;
+
+ while (int a[] {}) // { dg-error "condition declares an array" }
+;
+}
Index: testsuite/g++.dg/cpp1y/pr84588-1.C
===
--- testsuite/g++.dg/cpp1y/pr84588-1.C (nonexistent)
+++ testsuite/g++.dg/cpp1y/pr84588-1.C (working copy)
@@ -0,0 +1,25 @@
+// { dg-do compile { target c++14 } }
+
+struct a {
+ void b() {}
+ void c(void (*) () = [] {
+ if (a a(int auto) {}) // { dg-err
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi again,
On 18/05/2018 02:31, Paolo Carlini wrote:
Hi,
On 18/05/2018 01:21, Jason Merrill wrote:
On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini
wrote:
On 17/05/2018 16:58, Jason Merrill wrote:
On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini
wrote:
PS: maybe better using function_declarator_p?
I think so, yes. The relevant rule seems to be "The declarator shall
not specify a function or an array.", so let's check for arrays, too.
Agreed. I had the amended patch ready when I noticed (again) that it
wasn't
addressing another related class of issues which involves
declarators not
followed by initializers. Thus I tried to fix those too, and the
below which
moves the check up appears to work fine, passes testing, etc. Are
there any
risks that an erroneous function / array as declarator is in fact a
well
formed expression?!?
That doesn't matter; if it parses as a declarator, it's a declarator,
even if it's an ill-formed declarator. But...
+ bool decl_p = cp_parser_parse_definitely (parser);
+ if (!cp_parser_check_condition_declarator (parser, declarator,
loc))
+ return error_mark_node;
...if cp_parser_parse_definitely returns false, parsing as a
declarator failed, so we shouldn't look at "declarator".
Uhm, then you are saying that we should fix cp_parser_declarator
itself, right? Because we don't want cp_parser_parse_definitely
returning false after cp_parser_declarator parses, say, 'if (int
foo())' and therefore cp_parser_condition proceed with
cp_parser_expression, we want to emit our error and bail out.
Therefore the problem in the new patch seems that it tries to paper
over that cp_parser_declarator issue in the caller?!? Like, Ok,
cp_parser_declarator failed, but it was anyway trying to declare a
function / array and that can't possibly be an expression, see what I
mean? *Somehow*, the question you answered above.
Ok, now I finally see what's the exact issue you pointed out (I'm a bit
tired). Seems fixable.
If I understand correctly, the reason why the 3 lines you cited above
are wrong as they are is that my patch *assumes* that
cp_parser_declarator didn't really fail and cp_parser_condition has
forced the tentative parse to fail by calling cp_parser_simulate_error
immediately before when it didn't see an initializer immediately
following. That's actually true for 'if (int foo())', thus it makes
sense to check the declarator anyway for such cases *even* if
cp_parser_parse_definitely returns false. See what I mean?
Therefore, it seems to me that an amended patch would rearrange
cp_parser_condition to *not* call cp_parser_simulate_error for the cases
we care about ('if (int foo())') and instead check the declarator.
I'll work on that tomorrow...
Thanks,
Paolo.
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi, On 18/05/2018 01:21, Jason Merrill wrote: On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini wrote: On 17/05/2018 16:58, Jason Merrill wrote: On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini wrote: PS: maybe better using function_declarator_p? I think so, yes. The relevant rule seems to be "The declarator shall not specify a function or an array.", so let's check for arrays, too. Agreed. I had the amended patch ready when I noticed (again) that it wasn't addressing another related class of issues which involves declarators not followed by initializers. Thus I tried to fix those too, and the below which moves the check up appears to work fine, passes testing, etc. Are there any risks that an erroneous function / array as declarator is in fact a well formed expression?!? That doesn't matter; if it parses as a declarator, it's a declarator, even if it's an ill-formed declarator. But... + bool decl_p = cp_parser_parse_definitely (parser); + if (!cp_parser_check_condition_declarator (parser, declarator, loc)) +return error_mark_node; ...if cp_parser_parse_definitely returns false, parsing as a declarator failed, so we shouldn't look at "declarator". Uhm, then you are saying that we should fix cp_parser_declarator itself, right? Because we don't want cp_parser_parse_definitely returning false after cp_parser_declarator parses, say, 'if (int foo())' and therefore cp_parser_condition proceed with cp_parser_expression, we want to emit our error and bail out. Therefore the problem in the new patch seems that it tries to paper over that cp_parser_declarator issue in the caller?!? Like, Ok, cp_parser_declarator failed, but it was anyway trying to declare a function / array and that can't possibly be an expression, see what I mean? *Somehow*, the question you answered above. Paolo.
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
On Thu, May 17, 2018 at 5:54 PM, Paolo Carlini wrote: > On 17/05/2018 16:58, Jason Merrill wrote: >> >> On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini >> wrote: >>> >>> PS: maybe better using function_declarator_p? >> >> I think so, yes. The relevant rule seems to be "The declarator shall >> not specify a function or an array.", so let's check for arrays, too. > > Agreed. I had the amended patch ready when I noticed (again) that it wasn't > addressing another related class of issues which involves declarators not > followed by initializers. Thus I tried to fix those too, and the below which > moves the check up appears to work fine, passes testing, etc. Are there any > risks that an erroneous function / array as declarator is in fact a well > formed expression?!? That doesn't matter; if it parses as a declarator, it's a declarator, even if it's an ill-formed declarator. But... + bool decl_p = cp_parser_parse_definitely (parser); + if (!cp_parser_check_condition_declarator (parser, declarator, loc)) +return error_mark_node; ...if cp_parser_parse_definitely returns false, parsing as a declarator failed, so we shouldn't look at "declarator". Also, "here" in the diagnostic seems unnecessarily vague; we could be more specific. Maybe "condition declares a function/array"? Jason
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
Hi,
On 17/05/2018 16:58, Jason Merrill wrote:
On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini
wrote:
PS: maybe better using function_declarator_p?
I think so, yes. The relevant rule seems to be "The declarator shall
not specify a function or an array.", so let's check for arrays, too.
Agreed. I had the amended patch ready when I noticed (again) that it
wasn't addressing another related class of issues which involves
declarators not followed by initializers. Thus I tried to fix those too,
and the below which moves the check up appears to work fine, passes
testing, etc. Are there any risks that an erroneous function / array as
declarator is in fact a well formed expression?!? I haven't been able so
far to construct examples...
Thanks!
Paolo.
Index: cp/parser.c
===
--- cp/parser.c (revision 260331)
+++ cp/parser.c (working copy)
@@ -11527,6 +11527,33 @@ cp_parser_selection_statement (cp_parser* parser,
}
}
+/* Helper function for cp_parser_condition. Enforces [stmt.stmt]:
+ The declarator shall not specify a function or an array. Returns
+ TRUE is the declator is valid, FALSE otherwise. */
+
+static bool
+cp_parser_check_condition_declarator (cp_parser* parser,
+ cp_declarator *declarator,
+ location_t loc)
+{
+ if (function_declarator_p (declarator)
+ || declarator->kind == cdk_array)
+{
+ if (declarator->kind == cdk_array)
+ error_at (loc, "an array type is not allowed here");
+ else
+ error_at (loc, "a function type is not allowed here");
+ if (parser->fully_implicit_function_template_p)
+ abort_fully_implicit_template (parser);
+ cp_parser_skip_to_closing_parenthesis (parser, /*recovering=*/true,
+/*or_comma=*/false,
+/*consume_paren=*/false);
+ return false;
+}
+ else
+return true;
+}
+
/* Parse a condition.
condition:
@@ -11571,6 +11598,7 @@ cp_parser_condition (cp_parser* parser)
tree attributes;
cp_declarator *declarator;
tree initializer = NULL_TREE;
+ location_t loc = cp_lexer_peek_token (parser->lexer)->location;
/* Parse the declarator. */
declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED,
@@ -11592,10 +11620,15 @@ cp_parser_condition (cp_parser* parser)
if (cp_lexer_next_token_is_not (parser->lexer, CPP_EQ)
&& cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE))
cp_parser_simulate_error (parser);
-
+
/* If we did see an `=' or '{', then we are looking at a declaration
for sure. */
- if (cp_parser_parse_definitely (parser))
+ bool decl_p = cp_parser_parse_definitely (parser);
+
+ if (!cp_parser_check_condition_declarator (parser, declarator, loc))
+ return error_mark_node;
+
+ if (decl_p)
{
tree pushed_scope;
bool non_constant_p;
Index: testsuite/g++.dg/cpp0x/cond1.C
===
--- testsuite/g++.dg/cpp0x/cond1.C (nonexistent)
+++ testsuite/g++.dg/cpp0x/cond1.C (working copy)
@@ -0,0 +1,23 @@
+// PR c++/84588
+// { dg-do compile { target c++11 } }
+
+void foo()
+{
+ if (int bar() {}) // { dg-error "function type is not allowed" }
+;
+
+ for (;int bar() {};) // { dg-error "function type is not allowed" }
+;
+
+ while (int bar() {}) // { dg-error "function type is not allowed" }
+;
+
+ if (int a[] {}) // { dg-error "array type is not allowed" }
+;
+
+ for (;int a[] {};) // { dg-error "array type is not allowed" }
+;
+
+ while (int a[] {}) // { dg-error "array type is not allowed" }
+;
+}
Index: testsuite/g++.dg/cpp1y/pr84588-1.C
===
--- testsuite/g++.dg/cpp1y/pr84588-1.C (nonexistent)
+++ testsuite/g++.dg/cpp1y/pr84588-1.C (working copy)
@@ -0,0 +1,25 @@
+// { dg-do compile { target c++14 } }
+
+struct a {
+ void b() {}
+ void c(void (*) () = [] {
+ if (a a(int auto) {}) // { dg-error "two or more data types|function
type" }
+ ;
+ }) {}
+};
+
+struct d {
+ void e() {}
+ void f(void (*) () = [] {
+ for (;d d(int auto) {};) // { dg-error "two or more data types|function
type" }
+ ;
+ }) {}
+};
+
+struct g {
+ void h() {}
+ void i(void (*) () = [] {
+ while (g g(int auto) {}) // { dg-error "two or more data types|function
type" }
+ ;
+ }) {}
+};
Index: testsuite/g++.dg/cpp1y/pr84588-2.C
===
--- testsuite/g++.dg/cpp1y/pr84588-2.C (nonexistent)
+++ testsuite/g++.dg/cpp1y/pr84588-2.C (working copy)
@@ -0,0 +1,25 @@
+// { dg-do compile { target c++14 } }
+
+struct a {
+ void b() {}
+ void c(void (*) () = [] {
+ if
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
On Thu, May 17, 2018 at 10:27 AM, Paolo Carlini wrote: > PS: maybe better using function_declarator_p? I think so, yes. The relevant rule seems to be "The declarator shall not specify a function or an array.", so let's check for arrays, too. Jason
Re: [C++ Patch] PR 84588 ("[8 Regression] internal compiler error: Segmentation fault (contains_struct_check())") (Take 2)
PS: maybe better using function_declarator_p??? I think I regression tested that variant too, at some point. Paolo.
