PR preprocessor/53229 - Fix diagnostics location when pasting tokens
authorDodji Seketeli <dodji@redhat.com>
Tue, 29 May 2012 09:36:29 +0000 (09:36 +0000)
committerDodji Seketeli <dodji@gcc.gnu.org>
Tue, 29 May 2012 09:36:29 +0000 (11:36 +0200)
As stated in the audit trail of this problem report, consider this
test case:

    $ cat test.c
 1 struct x {
 2   int i;
 3 };
 4 struct x x;
 5
 6 #define TEST(X) x.##X
 7
 8 void foo (void)
 9 {
10   TEST(i) = 0;
11 }
    $

    $ cc1 -quiet test.c
    test.c: In function 'foo':
    test.c:10:1: error: pasting "." and "i" does not give a valid preprocessing token
       TEST(i) = 0;
     ^
    $

So, when pasting tokens, the error diagnostic uses the global and
imprecise input_location variable, leading to an imprecise output.

To properly fix this, I think libcpp should keep the token of the
pasting operator '##', instead of representing it with flag on the LHS
operand's token.  That way, it could use its location.  Doing that
would be quite intrusive though.  So this patch just uses the location
of the LHS of the pasting operator, for now.  It's IMHO better than
the current situation.

The patch makes paste_tokens take a location parameter that is used in
the diagnostics.  This change can still be useful later when we can
use the location of the pasting operator, because paste_tokens will
just be passed the new, more precise location.

Incidentally, it appeared that when getting tokens from within
preprocessor directives (like what is done in gcc.dg/cpp/paste12.c),
with -ftrack-macro-expansion disabled, the location of the expansion
point of macros was being lost because
cpp_reader::set_invocation_location wasn't being properly set.  It's
because when cpp_get_token_1 calls enter_macro_context, there is a
little period of time between the beginning of that later function and
when the macro is really pushed (and thus when the macro is really
expanded) where we wrongly consider that we are not expanding the
macro because macro_of_context is still NULL.  In that period of time,
in the occurrences of indirect recursive calls to cpp_get_token_1,
this later function wrongly sets cpp_reader::invocation_location
because cpp_reader::set_invocation_location is not being properly set.

To avoid that confusion the patch does away with
cpp_reader::set_invocation_location and introduces a new flag
cpp_reader::about_to_expand_macro_p that is set in the small time
interval exposed earlier.  A new in_macro_expansion_p is introduced as
well, so that cpp_get_token_1 can now accurately detect when we are in
the process of expanding a macro, and thus correctly collect the
location of the expansion point.

People seem to like screenshots.

Thus, after the patch, we now have:

    $ cc1 -quiet test.c
    test.c: In function 'foo':
    test.c:6:18: error: pasting "." and "i" does not give a valid preprocessing token
     #define TEST(X) x.##X
      ^
    test.c:10:3: note: in expansion of macro 'TEST'
       TEST(i) = 0;
       ^
    $

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.

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.

From-SVN: r187945

14 files changed:
gcc/testsuite/ChangeLog
gcc/testsuite/gcc.dg/cpp/paste12-2.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/cpp/paste12.c
gcc/testsuite/gcc.dg/cpp/paste13.c
gcc/testsuite/gcc.dg/cpp/paste14-2.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/cpp/paste14.c
gcc/testsuite/gcc.dg/cpp/paste18.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/cpp/paste6.c
gcc/testsuite/gcc.dg/cpp/paste8-2.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/cpp/paste8.c
libcpp/ChangeLog
libcpp/directives.c
libcpp/internal.h
libcpp/macro.c

index b08556c348ac30742c8da261e61a31823c1f77d1..1c1b28eeab97bfd16f4f6019b391d39bf3d366fb 100644 (file)
@@ -1,3 +1,21 @@
+2012-05-29  Dodji Seketeli  <dodji@redhat.com>
+
+       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.
+
 2012-05-29  Hans-Peter Nilsson  <hp@axis.com>
 
        * gcc.target/cris/torture/trap-1.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 (file)
index 0000000..6e2e4f1
--- /dev/null
@@ -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)
index e61ec51433c067cbf5a2e7d7a59b6435db4aa61b..3e0f7b9592220986bafd7ec64c61cb108622680a 100644 (file)
@@ -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.  */
index 62c72d449935619b4bf9bb0d0a8a897d87fa1e35..f0f4fd894e2b3ab0eee8f09b62f9cfa649fd44af 100644 (file)
@@ -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 (file)
index 0000000..3b23ada
--- /dev/null
@@ -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
+
index ec243c2326f8b9b5f06096d9e71dd013086d6392..043d5e5804fffaf8ceab85c284c55f85b3001916 100644 (file)
@@ -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 (file)
index 0000000..2888144
--- /dev/null
@@ -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;
+}
index ac9ae3901b39ba7abf6eeef5544fcede63653d6d..a4e70e450255a8415ddf6f90e94ab6a0765b6af9 100644 (file)
@@ -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 (file)
index 0000000..c037e99
--- /dev/null
@@ -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)                           
index ab01779786880285963febe7ff570d565caca200..db1416c120e7fbf420617f31fa985d73c93c7706 100644 (file)
@@ -1,5 +1,5 @@
 /* { dg-do preprocess } */
-/* { dg-options "" } */
+/* { dg-options "-ftrack-macro-expansion=0" } */
 
 int foo(int, ...);
 
index 45ff6ca2d893cf3ff92f0921dbc05ad7aa375717..19936fe431dcf9e95845cdebfe98852c9618bc00 100644 (file)
@@ -1,3 +1,28 @@
+2012-05-29  Dodji Seketeli  <dodji@redhat.com>
+
+       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.
+
 2012-05-24  Dodji Seketeli  <dodji@redhat.com>
 
        PR bootstrap/53459
index e46280e46b0466534ceb4b754d20ecfdab0fab3e..66fa66daa643a31f84638277fe6e4f1707bad790 100644 (file)
@@ -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)
index 5b3731bba1d7e44531e96421bce8ee9254f934d6..37aac821d62abd5f8d53d75d78f3b22079b19439 100644 (file)
@@ -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;       /* "" */
index c4e2a23c56bd2d56aed6619f61abf6445c535293..89d70f1da9e454eb662a4e352078581f4f386104 100644 (file)
@@ -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;
            }
 
@@ -1146,12 +1169,15 @@ enter_macro_context (cpp_reader *pfile, cpp_hashnode *node,
 
            }
          while (pragma_buff != NULL);
+         pfile->about_to_expand_macro_p = false;
          return 2;
        }
 
+      pfile->about_to_expand_macro_p = false;
       return 1;
     }
 
+  pfile->about_to_expand_macro_p = false;
   /* Handle built-in macros and the _Pragma operator.  */
   return builtin_macro (pfile, node);
 }
@@ -2143,6 +2169,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 +2338,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 +2397,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 +2465,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 +2473,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 +2536,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