[PATCH] c++: Fix up cp_lexer_safe_previous_token [PR96328]

[PATCH] c++: Fix up cp_lexer_safe_previous_token [PR96328]

[Jakub Jelinek]

Hi!

The following testcase ICEs, because cp_lexer_safe_previous_token calls
cp_lexer_previous_token and that ICEs, because all tokens in the lexer
buffer before the current one (CPP_EOF) have been purged.

cp_lexer_safe_previous_token is used in the context where it is ok if it
punts, so the patch changes the function so that it doesn't assert there is
some previous token, but instead returns NULL like in other cases where it
punts.

In addition to this, in the last hunk it does a micro-optimization, don't
call the potentially expensive function if it will not need the result,
instead check the least expensive condition first.

And the middle hunk is a similar change from Mark's version of the patch,
to use the safe variant in there because it is again just about a hint
and it is better not to provide the hint than to ICE, though we don't have a
testcase that would ICE.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2020-07-28  Jakub Jelinek  <jakub@redhat.com>
	    Mark Wielaard  <mark@klomp.org>

	PR c++/96328
	* parser.c (cp_lexer_safe_previous_token): Don't call
	cp_lexer_previous_token, instead inline it by hand and return NULL
	instead of failing assertion if all previous tokens until the first
	one are purged.
	(cp_parser_error_1): Optimize - only call cp_lexer_safe_previous_token
	if token->type is CPP_NAME.  Use cp_lexer_safe_previous_token instead
	of cp_lexer_previous_token for the missing_token_desc != RT_NONE
	case too.

	* g++.dg/diagnostic/pr96328.C: New test.

--- gcc/cp/parser.c.jj	2020-07-27 10:38:19.667621826 +0200
+++ gcc/cp/parser.c	2020-07-27 11:47:56.766858524 +0200
@@ -775,14 +775,25 @@ cp_lexer_previous_token (cp_lexer *lexer
 
 /* Same as above, but return NULL when the lexer doesn't own the token
    buffer or if the next_token is at the start of the token
-   vector.  */
+   vector or if all previous tokens are purged.  */
 
 static cp_token *
 cp_lexer_safe_previous_token (cp_lexer *lexer)
 {
-  if (lexer->buffer)
-    if (lexer->next_token != lexer->buffer->address ())
-      return cp_lexer_previous_token (lexer);
+  if (lexer->buffer
+      && lexer->next_token != lexer->buffer->address ())
+    {
+      cp_token_position tp = cp_lexer_previous_token_position (lexer);
+
+      /* Skip past purged tokens.  */
+      while (tp->purged_p)
+	{
+	  if (tp == lexer->buffer->address ())
+	    return NULL;
+	  tp--;
+	}
+      return cp_lexer_token_at (lexer, tp);
+    }
 
   return NULL;
 }
@@ -2934,22 +2945,23 @@ cp_parser_error_1 (cp_parser* parser, co
   bool added_matching_location = false;
 
   if (missing_token_desc != RT_NONE)
-    {
-      /* Potentially supply a fix-it hint, suggesting to add the
-	 missing token immediately after the *previous* token.
-	 This may move the primary location within richloc.  */
-      enum cpp_ttype ttype = get_required_cpp_ttype (missing_token_desc);
-      location_t prev_token_loc
-	= cp_lexer_previous_token (parser->lexer)->location;
-      maybe_suggest_missing_token_insertion (&richloc, ttype, prev_token_loc);
-
-      /* If matching_location != UNKNOWN_LOCATION, highlight it.
-	 Attempt to consolidate diagnostics by printing it as a
-	secondary range within the main diagnostic.  */
-      if (matching_location != UNKNOWN_LOCATION)
-	added_matching_location
-	  = richloc.add_location_if_nearby (matching_location);
-    }
+    if (cp_token *prev_token = cp_lexer_safe_previous_token (parser->lexer))
+      {
+	/* Potentially supply a fix-it hint, suggesting to add the
+	   missing token immediately after the *previous* token.
+	   This may move the primary location within richloc.  */
+	enum cpp_ttype ttype = get_required_cpp_ttype (missing_token_desc);
+	location_t prev_token_loc = prev_token->location;
+	maybe_suggest_missing_token_insertion (&richloc, ttype,
+					       prev_token_loc);
+
+	/* If matching_location != UNKNOWN_LOCATION, highlight it.
+	   Attempt to consolidate diagnostics by printing it as a
+	   secondary range within the main diagnostic.  */
+	if (matching_location != UNKNOWN_LOCATION)
+	  added_matching_location
+	    = richloc.add_location_if_nearby (matching_location);
+      }
 
   /* If we were parsing a string-literal and there is an unknown name
      token right after, then check to see if that could also have been
@@ -2957,19 +2969,19 @@ cp_parser_error_1 (cp_parser* parser, co
      standard string literal constants defined in header files. If
      there is one, then add that as an hint to the error message. */
   name_hint h;
-  cp_token *prev_token = cp_lexer_safe_previous_token (parser->lexer);
-  if (prev_token && cp_parser_is_string_literal (prev_token)
-      && token->type == CPP_NAME)
-    {
-      tree name = token->u.value;
-      const char *token_name = IDENTIFIER_POINTER (name);
-      const char *header_hint
-	= get_cp_stdlib_header_for_string_macro_name (token_name);
-      if (header_hint != NULL)
-	h = name_hint (NULL, new suggest_missing_header (token->location,
-							 token_name,
-							 header_hint));
-    }
+  if (token->type == CPP_NAME)
+    if (cp_token *prev_token = cp_lexer_safe_previous_token (parser->lexer))
+      if (cp_parser_is_string_literal (prev_token))
+	{
+	  tree name = token->u.value;
+	  const char *token_name = IDENTIFIER_POINTER (name);
+	  const char *header_hint
+	    = get_cp_stdlib_header_for_string_macro_name (token_name);
+	  if (header_hint != NULL)
+	    h = name_hint (NULL, new suggest_missing_header (token->location,
+							     token_name,
+							     header_hint));
+	}
 
   /* Actually emit the error.  */
   c_parse_error (gmsgid,
--- gcc/testsuite/g++.dg/diagnostic/pr96328.C.jj	2020-07-27 10:57:14.467370406 +0200
+++ gcc/testsuite/g++.dg/diagnostic/pr96328.C	2020-07-27 11:04:53.575796340 +0200
@@ -0,0 +1,4 @@
+// PR c++/96328
+// { dg-do compile }
+friend // { dg-error "'friend' used outside of class" }
+// { dg-prune-output "expected unqualified-id at end of input" }

	Jakub

Re: [PATCH] c++: Fix up cp_lexer_safe_previous_token [PR96328]

[Nathan Sidwell]

On 7/28/20 4:32 AM, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase ICEs, because cp_lexer_safe_previous_token calls
> cp_lexer_previous_token and that ICEs, because all tokens in the lexer
> buffer before the current one (CPP_EOF) have been purged.
> 
> cp_lexer_safe_previous_token is used in the context where it is ok if it
> punts, so the patch changes the function so that it doesn't assert there is
> some previous token, but instead returns NULL like in other cases where it
> punts.
> 
> In addition to this, in the last hunk it does a micro-optimization, don't
> call the potentially expensive function if it will not need the result,
> instead check the least expensive condition first.
> 
> And the middle hunk is a similar change from Mark's version of the patch,
> to use the safe variant in there because it is again just about a hint
> and it is better not to provide the hint than to ICE, though we don't have a
> testcase that would ICE.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

ok, thanks!

nathan

-- 
Nathan Sidwell