[PATCH preprocessor, diagnostics] PR preprocessor/53229 - Fix diagnostics location when pasting tokens

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> On 05/22/2012 05:04 AM, Dodji Seketeli wrote:
>> The problem is that cpp_get_token_1 can be called when we are at the
>> beginning of a macro expansion (inside enter_macro_expansion, called
>> from cpp_get_token_1), *before* context->c.macro is set.  This happens
>> e.g, when we call funlike_invocation_p to know if the current macro is
>> function-like or not.
>
> OK, sounds like we need some additional code to handle that.  I guess
> we could do something in funlike_invocation_p to prevent
> cpp_get_token_1 from setting invocation_location, or change the check
> we use to decide whether or not we already have an invocation
> location, perhaps by looking at invocation_location itself (and
> clearing it when we finish a macro).

So.  After thinking about this a bit more, here is how I phrase the
issue.

There is a small interval of time between when we decide to start the
expansion of a macro (when we get into enter_macro_context), and when we
really start that expansion (when push the context of macro the macro)
where we can recursively call cpp_get_token_1.  In that time interval,
cpp_get_token_1 might wrongly think that no macro expansion is in
progress.

And I think that's the issue root issue over which the
cpp_reader::set_invocation_location flag was papering.

It seems to me that a small and maintainable option to tackle this is to
introduce a flag cpp_reader::about_to_expand_macro_p, that is 'on'
during that time interval.  A new predicate function
in_macro_expansion_p then can now accurately tell cpp_get_token_1 if we
are in the process of expanding a macro or not, doing away with the need
for casual users of cpp_get_token_1 to set a flag to deal with macro
expansion point handling.

Tested and bootstrapped and tested on x86_64-unknown-linux-gnu against
trunk.


libcpp/

	PR preprocessor/53229
	* internal.h (cpp_reader::set_invocation_location): Remove.
	(cpp_reader::about_to_expand_macro_p): New member flag.
	* directives.c (do_pragma):  Remove Kludge as
	pfile->set_invocation_location is no more.
	* macro.c (cpp_get_token_1): Do away with the use of
	cpp_reader::set_invocation_location.  Just collect the macro
	expansion point when we are about to expand the top-most macro.
	Do not override cpp_reader::about_to_expand_macro_p.
	This fixes gcc.dg/cpp/paste12.c by making get_token_no_padding
	properly handle locations of expansion points.
	(cpp_get_token_with_location): Adjust, as
	cpp_reader::set_invocation_location is no more.
	(paste_tokens): Take a virtual location parameter for
	the LHS of the pasting operator.  Use it in diagnostics.  Update
	comments.
	(paste_all_tokens): Tighten the assert.  Propagate the location of
	the expansion point when no virtual locations are available.
	Pass the virtual location to paste_tokens.
	(in_macro_expansion_p): New static function.
	(enter_macro_context): Set the cpp_reader::about_to_expand_macro_p
	flag until we really start expanding the macro.
	(_cpp_push_token_context, push_ptoken_context)
	(push_extended_tokens_context): Unset the
	cpp_reader::about_to_expand_macro_p flag when we push the macro
	context.

gcc/testsuite/

	PR preprocessor/53229
	* gcc.dg/cpp/paste6.c: Force to run without
	-ftrack-macro-expansion.
	* gcc.dg/cpp/paste8.c: Likewise.
	* gcc.dg/cpp/paste8-2.c: New test, like paste8.c but run with
	-ftrack-macro-expansion.
	* gcc.dg/cpp/paste12.c: Force to run without
	-ftrack-macro-expansion.
	* gcc.dg/cpp/paste12-2.c: New test, like paste12.c but run with
	-ftrack-macro-expansion.
	* gcc.dg/cpp/paste13.c: Likewise.
	* gcc.dg/cpp/paste14.c: Likewise.
	* gcc.dg/cpp/paste14-2.c: New test, like paste14.c but run with
	-ftrack-macro-expansion.
	* gcc.dg/cpp/paste18.c: New test.
---
 gcc/testsuite/gcc.dg/cpp/paste12-2.c |   11 +++++
 gcc/testsuite/gcc.dg/cpp/paste12.c   |    5 ++-
 gcc/testsuite/gcc.dg/cpp/paste13.c   |    5 ++-
 gcc/testsuite/gcc.dg/cpp/paste14-2.c |   11 +++++
 gcc/testsuite/gcc.dg/cpp/paste14.c   |    5 ++-
 gcc/testsuite/gcc.dg/cpp/paste18.c   |   16 +++++++
 gcc/testsuite/gcc.dg/cpp/paste6.c    |    5 ++-
 gcc/testsuite/gcc.dg/cpp/paste8-2.c  |   15 ++++++
 gcc/testsuite/gcc.dg/cpp/paste8.c    |    2 +-
 libcpp/directives.c                  |   30 +------------
 libcpp/internal.h                    |   10 +++-
 libcpp/macro.c                       |   82 ++++++++++++++++++++++++++-------
 12 files changed, 142 insertions(+), 55 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/paste12-2.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/paste14-2.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/paste18.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/paste8-2.c

diff --git a/gcc/testsuite/gcc.dg/cpp/paste12-2.c b/gcc/testsuite/gcc.dg/cpp/paste12-2.c
new file mode 100644
index 0000000..6e2e4f1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/paste12-2.c
@@ -0,0 +1,11 @@
+/* 
+   { dg-options "-ftrack-macro-expansion=2" }
+   { dg-do preprocess }
+ */
+
+/* Test correct diagnostics when pasting in #include.
+   Source: PR preprocessor/6780.  */
+
+#define inc2(a,b) <##a.b>  /* { dg-error "pasting \"<\" and \"stdio\" does not" } */
+#define INC(X) inc2(X,h)
+#include INC(stdio)
diff --git a/gcc/testsuite/gcc.dg/cpp/paste12.c b/gcc/testsuite/gcc.dg/cpp/paste12.c
index e61ec51..3e0f7b9 100644
--- a/gcc/testsuite/gcc.dg/cpp/paste12.c
+++ b/gcc/testsuite/gcc.dg/cpp/paste12.c
@@ -1,4 +1,7 @@
-/* { dg-do preprocess } */
+/*
+  { dg-options "-ftrack-macro-expansion=0" }
+  { dg-do preprocess }
+*/
 
 /* Test correct diagnostics when pasting in #include.
    Source: PR preprocessor/6780.  */
diff --git a/gcc/testsuite/gcc.dg/cpp/paste13.c b/gcc/testsuite/gcc.dg/cpp/paste13.c
index 62c72d4..f0f4fd8 100644
--- a/gcc/testsuite/gcc.dg/cpp/paste13.c
+++ b/gcc/testsuite/gcc.dg/cpp/paste13.c
@@ -1,6 +1,9 @@
 /* Copyright (C) 2000 Free Software Foundation, Inc.  */
 
-/* { dg-do preprocess } */
+/*
+  { dg-options "-ftrack-macro-expansion=0" }
+  { dg-do preprocess }
+*/
 
 /* This used to be recognized as a comment when lexing after pasting
    spellings.  Neil Booth, 9 Oct 2002.  */
diff --git a/gcc/testsuite/gcc.dg/cpp/paste14-2.c b/gcc/testsuite/gcc.dg/cpp/paste14-2.c
new file mode 100644
index 0000000..3b23ada
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/paste14-2.c
@@ -0,0 +1,11 @@
+/* PR preprocessor/28709 */
+/* 
+   { dg-options "-ftrack-macro-expansion=2" }
+   { dg-do preprocess }
+*/
+
+#define foo - ## >> /* { dg-error "pasting \"-\" and \">>\"" } */
+foo
+#define bar = ## == /* { dg-error "pasting \"=\" and \"==\"" } */
+bar
+
diff --git a/gcc/testsuite/gcc.dg/cpp/paste14.c b/gcc/testsuite/gcc.dg/cpp/paste14.c
index ec243c2..043d5e5 100644
--- a/gcc/testsuite/gcc.dg/cpp/paste14.c
+++ b/gcc/testsuite/gcc.dg/cpp/paste14.c
@@ -1,5 +1,8 @@
 /* PR preprocessor/28709 */
-/* { dg-do preprocess } */
+/* 
+   { dg-options "-ftrack-macro-expansion=0" }
+   { dg-do preprocess }
+*/
 
 #define foo - ## >>
 foo		/* { dg-error "pasting \"-\" and \">>\"" } */
diff --git a/gcc/testsuite/gcc.dg/cpp/paste18.c b/gcc/testsuite/gcc.dg/cpp/paste18.c
new file mode 100644
index 0000000..2888144
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/paste18.c
@@ -0,0 +1,16 @@
+/* 
+   { dg-options "-ftrack-macro-expansion=2" }
+   { dg-do compile }
+ */
+
+struct x {
+  int i;
+};
+struct x x;
+
+#define TEST(X) x.##X /* { dg-error "pasting\[^\n\r\]*does not give\[^\n\r\]*token" } */
+
+void foo (void)
+{
+  TEST(i) = 0;
+}
diff --git a/gcc/testsuite/gcc.dg/cpp/paste6.c b/gcc/testsuite/gcc.dg/cpp/paste6.c
index ac9ae39..a4e70e4 100644
--- a/gcc/testsuite/gcc.dg/cpp/paste6.c
+++ b/gcc/testsuite/gcc.dg/cpp/paste6.c
@@ -2,7 +2,10 @@
    actual arguments.  Original bug exposed by Linux kernel.  Problem
    reported by Jakub Jelinek <jakub@redhat.com>.  */
 
-/* { dg-do compile } */
+/*
+  { dg-options "-ftrack-macro-expansion=0" }
+  { dg-do compile }
+*/
 
 extern int foo(int x);
 
diff --git a/gcc/testsuite/gcc.dg/cpp/paste8-2.c b/gcc/testsuite/gcc.dg/cpp/paste8-2.c
new file mode 100644
index 0000000..c037e99
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/paste8-2.c
@@ -0,0 +1,15 @@
+/* { dg-do preprocess } */
+/* { dg-options "-ftrack-macro-expansion=2" } */
+
+int foo(int, ...);
+
+#define a(x, y...) foo(x, ##y)
+a(1)
+a(1, 2, 3)
+#define b(x, y, z...) foo(x, ##y) /* { dg-error "valid preprocessing token" } */
+b(1, 2, 3)			
+#define c(x, y, z...) foo(x, ##z)
+c(1, 2)
+c(1, 2, 3)
+#define d(x) fo(##x) /* { dg-error "valid preprocessing token" } */
+d(1)				
diff --git a/gcc/testsuite/gcc.dg/cpp/paste8.c b/gcc/testsuite/gcc.dg/cpp/paste8.c
index ab01779..db1416c 100644
--- a/gcc/testsuite/gcc.dg/cpp/paste8.c
+++ b/gcc/testsuite/gcc.dg/cpp/paste8.c
@@ -1,5 +1,5 @@
 /* { dg-do preprocess } */
-/* { dg-options "" } */
+/* { dg-options "-ftrack-macro-expansion=0" } */
 
 int foo(int, ...);
 
diff --git a/libcpp/directives.c b/libcpp/directives.c
index e46280e..66fa66d 100644
--- a/libcpp/directives.c
+++ b/libcpp/directives.c
@@ -1362,35 +1362,7 @@ do_pragma (cpp_reader *pfile)
 	{
 	  bool allow_name_expansion = p->allow_expansion;
 	  if (allow_name_expansion)
-	    {
-	      pfile->state.prevent_expansion--;
-	      /*
-		Kludge ahead.
-
-		Consider this code snippet:
-
-		#define P parallel
-		#pragma omp P for
-		... a for loop ...
-
-		Once we parsed the 'omp' namespace of the #pragma
-		directive, we then parse the 'P' token that represents the
-		pragma name.  P being a macro, it is expanded into the
-		resulting 'parallel' token.
-
-		At this point the 'p' variable contains the 'parallel'
-		pragma name.  And pfile->context->macro is non-null
-		because we are still right at the end of the macro
-		context of 'P'.  The problem is, if we are being
-		(indirectly) called by cpp_get_token_with_location,
-		that function might test pfile->context->macro to see
-		if we are in the context of a macro expansion, (and we
-		are) and then use pfile->invocation_location as the
-		location of the macro invocation.  So we must instruct
-		cpp_get_token below to set
-		pfile->invocation_location.  */
-	      pfile->set_invocation_location = true;
-	    }
+	    pfile->state.prevent_expansion--;
 
 	  token = cpp_get_token (pfile);
 	  if (token->type == CPP_NAME)
diff --git a/libcpp/internal.h b/libcpp/internal.h
index 5b3731b..37aac82 100644
--- a/libcpp/internal.h
+++ b/libcpp/internal.h
@@ -413,9 +413,13 @@ struct cpp_reader
      macro invocation.  */
   source_location invocation_location;
 
-  /* True if this call to cpp_get_token should consider setting
-     invocation_location.  */
-  bool set_invocation_location;
+  /* Nonzero if we are about to expand a macro.  Note that if we are
+     really expanding a macro, the function macro_of_context returns
+     the macro being expanded and this flag is set to false.  Client
+     code should use the function in_macro_expansion_p to know if we
+     are either about to expand a macro, or are actually expanding
+     one.  */
+  bool about_to_expand_macro_p;
 
   /* Search paths for include files.  */
   struct cpp_dir *quote_include;	/* "" */
diff --git a/libcpp/macro.c b/libcpp/macro.c
index c4e2a23..48f2473 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -100,7 +100,8 @@ static void expand_arg (cpp_reader *, macro_arg *);
 static const cpp_token *new_string_token (cpp_reader *, uchar *, unsigned int);
 static const cpp_token *stringify_arg (cpp_reader *, macro_arg *);
 static void paste_all_tokens (cpp_reader *, const cpp_token *);
-static bool paste_tokens (cpp_reader *, const cpp_token **, const cpp_token *);
+static bool paste_tokens (cpp_reader *, source_location,
+			  const cpp_token **, const cpp_token *);
 static void alloc_expanded_arg_mem (cpp_reader *, macro_arg *, size_t);
 static void ensure_expanded_arg_room (cpp_reader *, macro_arg *, size_t, size_t *);
 static void delete_macro_args (_cpp_buff*, unsigned num_args);
@@ -167,6 +168,8 @@ static const cpp_token* cpp_get_token_1 (cpp_reader *, source_location *);
 
 static cpp_hashnode* macro_of_context (cpp_context *context);
 
+static bool in_macro_expansion_p (cpp_reader *pfile);
+
 /* Statistical counter tracking the number of macros that got
    expanded.  */
 unsigned num_expanded_macros_counter = 0;
@@ -544,9 +547,11 @@ stringify_arg (cpp_reader *pfile, macro_arg *arg)
 
 /* Try to paste two tokens.  On success, return nonzero.  In any
    case, PLHS is updated to point to the pasted token, which is
-   guaranteed to not have the PASTE_LEFT flag set.  */
+   guaranteed to not have the PASTE_LEFT flag set.  LOCATION is
+   the virtual location used for error reporting.  */
 static bool
-paste_tokens (cpp_reader *pfile, const cpp_token **plhs, const cpp_token *rhs)
+paste_tokens (cpp_reader *pfile, source_location location,
+	      const cpp_token **plhs, const cpp_token *rhs)
 {
   unsigned char *buf, *end, *lhsend;
   cpp_token *lhs;
@@ -590,7 +595,7 @@ paste_tokens (cpp_reader *pfile, const cpp_token **plhs, const cpp_token *rhs)
 
       /* Mandatory error for all apart from assembler.  */
       if (CPP_OPTION (pfile, lang) != CLK_ASM)
-	cpp_error (pfile, CPP_DL_ERROR,
+	cpp_error_with_line (pfile, CPP_DL_ERROR, location, 0,
 	 "pasting \"%s\" and \"%s\" does not give a valid preprocessing token",
 		   buf, cpp_token_as_text (pfile, rhs));
       return false;
@@ -615,9 +620,10 @@ paste_all_tokens (cpp_reader *pfile, const cpp_token *lhs)
   cpp_context *context = pfile->context;
   source_location virt_loc = 0;
 
-  /* We must have been called on a token that appears at the left
-     hand side of a ## operator.  */
-  if (!(lhs->flags & PASTE_LEFT))
+  /* We are expanding a macro and we must have been called on a token
+     that appears at the left hand side of a ## operator.  */
+  if (macro_of_context (pfile->context) == NULL
+      || (!(lhs->flags & PASTE_LEFT)))
     abort ();
 
   if (context->tokens_kind == TOKENS_KIND_EXTENDED)
@@ -628,6 +634,11 @@ paste_all_tokens (cpp_reader *pfile, const cpp_token *lhs)
        resulting pasted token to have the location of the current
        *LHS, though.  */
     virt_loc = context->c.mc->cur_virt_loc[-1];
+  else
+    /* We are not tracking macro expansion.  So the best virtual
+       location we can get here is the expansion point of the macro we
+       are currently expanding.  */
+    virt_loc = pfile->invocation_location;
 
   do
     {
@@ -661,7 +672,7 @@ paste_all_tokens (cpp_reader *pfile, const cpp_token *lhs)
 	  if (rhs->flags & PASTE_LEFT)
 	    abort ();
 	}
-      if (!paste_tokens (pfile, &lhs, rhs))
+      if (!paste_tokens (pfile, virt_loc, &lhs, rhs))
 	break;
     }
   while (rhs->flags & PASTE_LEFT);
@@ -1018,6 +1029,17 @@ enter_macro_context (cpp_reader *pfile, cpp_hashnode *node,
 
   pfile->state.angled_headers = false;
 
+  /* From here to when we push the context for the macro later down
+     this function, we need to flag the fact that we are about to
+     expand a macro.  This is useful when -ftrack-macro-expansion is
+     turned off.  In that case, we need to record the location of the
+     expansion point of the top-most macro we are about to to expand,
+     into pfile->invocation_location.  But we must not record any such
+     location once the process of expanding the macro starts; that is,
+     we must not do that recording between now and later down this
+     function where set this flag to FALSE.  */
+  pfile->about_to_expand_macro_p = true;
+
   if ((node->flags & NODE_BUILTIN) && !(node->flags & NODE_USED))
     {
       node->flags |= NODE_USED;
@@ -1057,6 +1079,7 @@ enter_macro_context (cpp_reader *pfile, cpp_hashnode *node,
 	      if (pragma_buff)
 		_cpp_release_buff (pfile, pragma_buff);
 
+	      pfile->about_to_expand_macro_p = false;
 	      return 0;
 	    }
 
@@ -1804,6 +1827,10 @@ push_ptoken_context (cpp_reader *pfile, cpp_hashnode *macro, _cpp_buff *buff,
   cpp_context *context = next_context (pfile);
 
   context->tokens_kind = TOKENS_KIND_INDIRECT;
+
+  if (macro != NULL)
+    pfile->about_to_expand_macro_p = false;
+
   context->c.macro = macro;
   context->buff = buff;
   FIRST (context).ptoken = first;
@@ -1825,6 +1852,9 @@ _cpp_push_token_context (cpp_reader *pfile, cpp_hashnode *macro,
    if (macro == NULL)
      macro = macro_of_context (pfile->context);
 
+   if (macro != NULL)
+     pfile->about_to_expand_macro_p = false;
+
    context = next_context (pfile);
    context->tokens_kind = TOKENS_KIND_DIRECT;
    context->c.macro = macro;
@@ -1858,6 +1888,9 @@ push_extended_tokens_context (cpp_reader *pfile,
   if (macro == NULL)
     macro = macro_of_context (pfile->context);
 
+  if (macro != NULL)
+    pfile->about_to_expand_macro_p = false;
+
   context = next_context (pfile);
   context->tokens_kind = TOKENS_KIND_EXTENDED;
   context->buff = token_buff;
@@ -2143,6 +2176,20 @@ macro_of_context (cpp_context *context)
     : context->c.macro;
 }
 
+/* Return TRUE iff we are expanding a macro or are about to start
+   expanding one.  If we are effectively expanding a macro, the
+   function macro_of_context returns a pointer to the macro being
+   expanded.  */
+static bool
+in_macro_expansion_p (cpp_reader *pfile)
+{
+  if (pfile == NULL)
+    return false;
+
+  return (pfile->about_to_expand_macro_p 
+	  || macro_of_context (pfile->context));
+}
+
 /* Pop the current context off the stack, re-enabling the macro if the
    context represented a macro's replacement list.  Initially the
    context structure was not freed so that we can re-use it later, but
@@ -2298,11 +2345,13 @@ static const cpp_token*
 cpp_get_token_1 (cpp_reader *pfile, source_location *location)
 {
   const cpp_token *result;
-  bool can_set = pfile->set_invocation_location;
   /* This token is a virtual token that either encodes a location
      related to macro expansion or a spelling location.  */
   source_location virt_loc = 0;
-  pfile->set_invocation_location = false;
+  /* pfile->about_to_expand_macro_p can be overriden by indirect calls
+     to functions that push macro contexts.  So let's save it so that
+     we can restore it when we are about to leave this routine.  */
+  bool saved_about_to_expand_macro = pfile->about_to_expand_macro_p;
 
   for (;;)
     {
@@ -2355,7 +2404,7 @@ cpp_get_token_1 (cpp_reader *pfile, source_location *location)
 	  int ret = 0;
 	  /* If not in a macro context, and we're going to start an
 	     expansion, record the location.  */
-	  if (can_set && !context->c.macro)
+	  if (!in_macro_expansion_p (pfile))
 	    pfile->invocation_location = result->src_loc;
 	  if (pfile->state.prevent_expansion)
 	    break;
@@ -2423,8 +2472,7 @@ cpp_get_token_1 (cpp_reader *pfile, source_location *location)
       *location = virt_loc;
 
       if (!CPP_OPTION (pfile, track_macro_expansion)
-	  && can_set
-	  && pfile->context->c.macro != NULL)
+	  && macro_of_context (pfile->context) != NULL)
 	/* We are in a macro expansion context, are not tracking
 	   virtual location, but were asked to report the location
 	   of the expansion point of the macro being expanded.  */
@@ -2432,6 +2480,8 @@ cpp_get_token_1 (cpp_reader *pfile, source_location *location)
 
       *location = maybe_adjust_loc_for_trad_cpp (pfile, *location);
     }
+
+  pfile->about_to_expand_macro_p = saved_about_to_expand_macro;
   return result;
 }
 
@@ -2493,11 +2543,7 @@ cpp_get_token (cpp_reader *pfile)
 const cpp_token *
 cpp_get_token_with_location (cpp_reader *pfile, source_location *loc)
 {
-  const cpp_token *result;
-
-  pfile->set_invocation_location = true;
-  result = cpp_get_token_1 (pfile, loc);
-  return result;
+  return cpp_get_token_1 (pfile, loc);
 }
 
 /* Returns true if we're expanding an object-like macro that was
-- 
		Dodji