[C++ Patch] PR 69793 ("ICE on invalid code in "cp_lexer_peek_nth_token"")

[C++ Patch] PR 69793 ("ICE on invalid code in "cp_lexer_peek_nth_token"")

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

Hi,

this ICE during error recovery exposes a rather more general weakness: 
we should never call cp_lexer_peek_nth_token (*, 2) when a previous 
cp_lexer_peek_token returns CPP_EOF.

The fix seems easy, just reshape a bit the condition and delay the 
latter. It should also be a net, albeit minor, performance win: in 
general we don't want to call cp_lexer_peek_nth_token (which is out of 
line) unnecessarily.

Tested x86_64-linux.

Thanks,
Paolo.

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

[-- Attachment #2: CL_69793 --]
[-- Type: text/plain, Size: 318 bytes --]

/cp
2016-05-17  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/69793
	* parser.c (cp_parser_template_id): Don't call cp_lexer_peek_nth_token
	when the previous cp_lexer_peek_token returns CPP_EOF.

/testsuite
2016-05-17  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/69793
	* g++.dg/template/crash123.C: New.

[-- Attachment #3: patch_69793 --]
[-- Type: text/plain, Size: 1303 bytes --]

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 236338)
+++ cp/parser.c	(working copy)
@@ -14835,11 +14835,11 @@ cp_parser_template_id (cp_parser *parser,
   /* If we find the sequence `[:' after a template-name, it's probably
      a digraph-typo for `< ::'. Substitute the tokens and check if we can
      parse correctly the argument list.  */
-  next_token = cp_lexer_peek_token (parser->lexer);
-  next_token_2 = cp_lexer_peek_nth_token (parser->lexer, 2);
-  if (next_token->type == CPP_OPEN_SQUARE
+  if (((next_token = cp_lexer_peek_token (parser->lexer))->type
+       == CPP_OPEN_SQUARE)
       && next_token->flags & DIGRAPH
-      && next_token_2->type == CPP_COLON
+      && ((next_token_2 = cp_lexer_peek_nth_token (parser->lexer, 2))->type
+	  == CPP_COLON)
       && !(next_token_2->flags & PREV_WHITE))
     {
       cp_parser_parse_tentatively (parser);
Index: testsuite/g++.dg/template/crash123.C
===================================================================
--- testsuite/g++.dg/template/crash123.C	(revision 0)
+++ testsuite/g++.dg/template/crash123.C	(working copy)
@@ -0,0 +1,4 @@
+// PR c++/69793
+
+class fpos;
+template < state > bool operator!= (fpos,; operator!= // { dg-error "declared|expected|type" }

Re: [C++ Patch] PR 69793 ("ICE on invalid code in "cp_lexer_peek_nth_token"")

[Jason Merrill]

On 05/17/2016 04:47 PM, Paolo Carlini wrote:
> this ICE during error recovery exposes a rather more general weakness:
> we should never call cp_lexer_peek_nth_token (*, 2) when a previous
> cp_lexer_peek_token returns CPP_EOF.

Hmm, that seems fragile, I would expect it to keep returning EOF.

But your patch is OK.

Jason

Re: [C++ Patch] PR 69793 ("ICE on invalid code in "cp_lexer_peek_nth_token"")

[Paolo Carlini]

Hi,

On 18/05/2016 16:39, Jason Merrill wrote:
> On 05/17/2016 04:47 PM, Paolo Carlini wrote:
>> this ICE during error recovery exposes a rather more general weakness:
>> we should never call cp_lexer_peek_nth_token (*, 2) when a previous
>> cp_lexer_peek_token returns CPP_EOF.
>
> Hmm, that seems fragile, I would expect it to keep returning EOF.
Indeed. I didn't explain myself well enough. I meant something along the 
lines: outside this specific and minor case of ICE during error 
recovery, we should audit our code and keep in mind that calling 
cp_lexer_peek_nth_token (*, anything > 1, the common case) right after 
cp_lexer_peek_token is, how shall I put it, "suspect", due to that 
assert at the beginning of cp_lexer_peek_nth_token.
> But your patch is OK.
Thanks. I'm going to commit it then.

Paolo.

Re: [C++ Patch] PR 69793 ("ICE on invalid code in "cp_lexer_peek_nth_token"")

[Jason Merrill]

On 05/18/2016 11:05 AM, Paolo Carlini wrote:
> On 18/05/2016 16:39, Jason Merrill wrote:
>> On 05/17/2016 04:47 PM, Paolo Carlini wrote:
>>> this ICE during error recovery exposes a rather more general weakness:
>>> we should never call cp_lexer_peek_nth_token (*, 2) when a previous
>>> cp_lexer_peek_token returns CPP_EOF.

>> Hmm, that seems fragile, I would expect it to keep returning EOF.

> Indeed. I didn't explain myself well enough. I meant something along the
> lines: outside this specific and minor case of ICE during error
> recovery, we should audit our code and keep in mind that calling
> cp_lexer_peek_nth_token (*, anything > 1, the common case) right after
> cp_lexer_peek_token is, how shall I put it, "suspect", due to that
> assert at the beginning of cp_lexer_peek_nth_token.

I understood that, but I think that assert should be replaced with code 
to properly handle that case.

Jason

Re: [C++ Patch] PR 69793 ("ICE on invalid code in "cp_lexer_peek_nth_token"")

[David Malcolm]

On Wed, 2016-05-18 at 17:05 +0200, Paolo Carlini wrote:
> Hi,
> 
> On 18/05/2016 16:39, Jason Merrill wrote:
> > On 05/17/2016 04:47 PM, Paolo Carlini wrote:
> > > this ICE during error recovery exposes a rather more general
> > > weakness:
> > > we should never call cp_lexer_peek_nth_token (*, 2) when a
> > > previous
> > > cp_lexer_peek_token returns CPP_EOF.
> > 
> > Hmm, that seems fragile, I would expect it to keep returning EOF.
> Indeed. I didn't explain myself well enough. I meant something along
> the 
> lines: outside this specific and minor case of ICE during error 
> recovery, we should audit our code and keep in mind that calling 
> cp_lexer_peek_nth_token (*, anything > 1, the common case) right
> after 
> cp_lexer_peek_token is, how shall I put it, "suspect", due to that 
> assert at the beginning of cp_lexer_peek_nth_token.

Thinking aloud, I wonder if a plugin could detect this kind of thing? -
verify that any call of cp_lexer_peek_nth_token for N > 1 happens after
the result of cp_lexer_peek_token has been checked for EOF.

(another idea might be a fault-injection option for the C++ frontend,
perhaps simulating EOF at a given token index, and then using this,
somehow, to torture-test the existing test cases, but that seems like a
*lot* of extra parsing; static checking seems more efficient).

Re: [C++ Patch] PR 69793 ("ICE on invalid code in "cp_lexer_peek_nth_token"")

[Paolo Carlini]

Hi,

On 18/05/2016 17:17, Jason Merrill wrote:
> On 05/18/2016 11:05 AM, Paolo Carlini wrote:
>> On 18/05/2016 16:39, Jason Merrill wrote:
>>> On 05/17/2016 04:47 PM, Paolo Carlini wrote:
>>>> this ICE during error recovery exposes a rather more general weakness:
>>>> we should never call cp_lexer_peek_nth_token (*, 2) when a previous
>>>> cp_lexer_peek_token returns CPP_EOF.
>
>>> Hmm, that seems fragile, I would expect it to keep returning EOF.
>
>> Indeed. I didn't explain myself well enough. I meant something along the
>> lines: outside this specific and minor case of ICE during error
>> recovery, we should audit our code and keep in mind that calling
>> cp_lexer_peek_nth_token (*, anything > 1, the common case) right after
>> cp_lexer_peek_token is, how shall I put it, "suspect", due to that
>> assert at the beginning of cp_lexer_peek_nth_token.
>
> I understood that, but I think that assert should be replaced with 
> code to properly handle that case.
Ah yes, that comment before cp_lexer_peek_nth_token. I read it 
yesterday, got interested, but afterward focused on the specific issue 
in the bug report, seemed easy to fix.

Paolo.