Re: [PATCH] PR preprocessor/61817 - Fix location of tokens in built-in macro expansion list

[Dodji Seketeli <dodji at redhat dot com>]

Hello,

I have initially sent a patch for this at
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01144.html.  But then a
patch for a similarly expressed issue
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01521.html (which I think
is the same underlying problem) was committed.  As the PR
preprocessor/61817 is still not fixed, I have updated my initial patch
over the one that was committed already and am submitted the result
below.  I have added more test cases in the process.


Consider this function-like macro definition in the inc.h file

    ------------------->inc.h<-------------------
    #define F() const int line = __LINE__
    ------------------->8<-----------------------

Now consider its use in a file test.c:

    ------------------>cat -n test.c<----------------
	 1  #include "inc.h"
	 2
	 3  void
	 4  foo()
	 5  {
	 6    F
	 7      (
	 8       )
	 9      ;
	10  }
    ------------------->8<---------------------

Running test.c through cc1 -quiet -E yields:

    --------------------->8<--------------------
    # 1 "test.c"
    # 1 "<built-in>"
    # 1 "<command-line>"
    # 1 "/usr/include/stdc-predef.h" 1 3 4
    # 1 "<command-line>" 2
    # 1 "test.c"
    # 1 "inc.h" 1
    # 2 "test.c" 2

    void
    foo()
    {
      const int line =

     8
	;
    }
    --------------------->8<----------------------

Note how tokens "const", "int", "line" and "=" are all on the same
line (line 6) as the expansion point of the function-like F() macro,
but how the token "8", resulting from the expansion of the built-in
macro __FILE__ is on the same line as the closing parenthesis of the
invocation of F().

This is the problem.  The result of the __LINE__ macro should be "6"
and should be on the same line (line 6) as the other tokens of the
expansion-list of F().

This issue actually holds for the expansion of all built-in macros.

So more generelly, I would describe the issue as such:

When expanded in a function-like macro, the location of resulting
tokens of a built-in macro is set to the closing parenthesis of the
enclosing function-like macro invocation, rather than being set to the
location of the expansion point of the invocation the enclosing
functin-like macro.

I think the problem is that enter_macro_context() is passed the
location of the expansion point of the macro that is about to be
expanded.  But when that macro is built-in, builtin_macro() that is
then called by enter_macro_context() doesn't use the macro expansion
point location when expanding the __LINE__ builtin (in, e.g,
_cpp_builtin_macro_text) .  Rather, what it used is the location of
the closing parenthesis of the enclosing function-like macro
invocation or, more generally, the location of the previous token in
the stream.

The patch proposes to make builtin_macro() use the expansion point
location that enter_macro_context() passed along.  That location is
then used by the different routines that need it (e.g, to expand
builtin macros for instance).

Also, for the particular case of __LINE__, make that built-in expand
to the line number of that expansion point.

Note that in the function builtin_macro, this patch avoids re-setting
token->src_loc because for ease of maintenance (and debugging), I
think it's preferable that the spelling location of tokens be set only
in place -- in the tokens lexer.  To control the value of that
location we use the function cpp_force_token_locations, as intended.

The patch bootstraps and passes tests on x86_64-unknown-linux-gnu
against trunk.

libcpp/
	* macro.c (_cpp_builtin_macro_text): Honor the
	cpp_reader::forced_token_location_p data member to force the value
	__LINE__ expands to accordlingly.
	(builtin_macro): Use cpp_force_token_locations() to set the
	location of the resulting token to that expansion point location.
	(enter_macro_context): Pass the expansion point locatoin to
	builtin_macro.

gcc/testsuite/
	* gcc.dg/cpp/builtin-macro-{0,1,2,3}.c: New test files.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c | 18 ++++++++++
 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c | 30 ++++++++++++++++
 gcc/testsuite/gcc.dg/cpp/builtin-macro-2.c | 28 +++++++++++++++
 gcc/testsuite/gcc.dg/cpp/builtin-macro-3.c | 28 +++++++++++++++
 libcpp/macro.c                             | 56 +++++++++++++++++++++++-------
 5 files changed, 148 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-2.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/builtin-macro-3.c

diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c b/gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c
new file mode 100644
index 0000000..f57dd5b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-0.c
@@ -0,0 +1,18 @@
+/* Origin: PR preprocessor/61817
+
+  In this test case, the diagnostic (happening for tokens resulting
+  from the expansion of the built-in token __FILE__) must point to the
+  expansion point of the enclosing function-like macro invocation.
+
+  { dg-do compile }
+  { dg-options "-no-integrated-cpp" }  */
+
+#define F() static const char* __FILE__
+
+void
+foo ()
+{
+  F /* { dg-error "expected identifier" } */
+    (
+     );
+}
diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
new file mode 100644
index 0000000..280797b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-1.c
@@ -0,0 +1,30 @@
+/* Origin: PR preprocessor/61817
+
+   This test detects that the __LINE__ built-in macro expands to the
+   line number of the expansion point of the function-like macro in
+   which it's exanded.  So if you change the content of this file,
+   please adjust the test to reflect the new lines layout.
+
+   { dg-do run }
+   { dg-options -no-integrated-cpp }  */
+
+#include <assert.h>
+
+#define F() static const int line = __LINE__;
+
+void
+foo ()
+{
+  F /* the__LINE__ macro should expand to this line number. */
+    (
+     );
+
+  assert (line == 18);
+}
+
+int
+main ()
+{
+  foo();
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-2.c b/gcc/testsuite/gcc.dg/cpp/builtin-macro-2.c
new file mode 100644
index 0000000..ffbeb75
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-2.c
@@ -0,0 +1,28 @@
+/* Origin: PR preprocessor/61817
+
+   This test detects that the __LINE__ built-in macro expands to the
+   line number of the expansion point of the function-like macro that
+   initially triggered the expansion stack.
+
+   { dg-do compile }
+   { dg-options -no-integrated-cpp }  */
+
+#define JOIN(a,b) DO_JOIN(a,b)
+#define DO_JOIN(a,b) DO_JOIN2(a,b)
+#define DO_JOIN2(a,b) a##b
+#define STATIC_ASSERT(expr) \
+  int JOIN(a,__LINE__)[expr? 1 : -1] /* __LINE__ should not expand to
+				      this line.  */
+
+void
+foo()
+{
+  STATIC_ASSERT(1);
+  STATIC_ASSERT(2);		/* If __LINE__ (wrongly) expands to
+				   the point where it's used in the
+				   definition of JOIN above, this
+				   statement would trigger an error
+				   about an already defined
+				   variable. named
+				   'a<some-line-number>'.  */
+}
diff --git a/gcc/testsuite/gcc.dg/cpp/builtin-macro-3.c b/gcc/testsuite/gcc.dg/cpp/builtin-macro-3.c
new file mode 100644
index 0000000..53b698c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/builtin-macro-3.c
@@ -0,0 +1,28 @@
+/* Origin: PR preprocessor/61817
+
+   This test detects that the __LINE__ built-in macro expands to the
+   line number of the expansion point of the function-like macro that
+   initially triggered the expansion stack.
+
+   { dg-do compile }
+   { dg-options -ftrack-macro-expansion=0 }  */
+
+#define JOIN(a,b) DO_JOIN(a,b)
+#define DO_JOIN(a,b) DO_JOIN2(a,b)
+#define DO_JOIN2(a,b) a##b
+#define STATIC_ASSERT(expr) \
+  int JOIN(a,__LINE__)[expr? 1 : -1] /* __LINE__ should not expand to
+				     this line.  */
+
+void
+foo()
+{
+  STATIC_ASSERT(1);
+  STATIC_ASSERT(2);		/* If line (wrongly) expands to the
+				   point where it's used in the
+				   definition of JOIN above, this
+				   statement would trigger an error
+				   about an already defined
+				   variable. named
+				   'a<some-line-number>'.  */
+}
diff --git a/libcpp/macro.c b/libcpp/macro.c
index ff6685c..20f092d 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -309,10 +309,26 @@ _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node)
       /* If __LINE__ is embedded in a macro, it must expand to the
 	 line of the macro's invocation, not its definition.
 	 Otherwise things like assert() will not work properly.  */
-      number = linemap_get_expansion_line (pfile->line_table,
-					   CPP_OPTION (pfile, traditional)
-					   ? pfile->line_table->highest_line
-					   : pfile->cur_token[-1].src_loc);
+      if (pfile->forced_token_location_p)
+	{
+	  /* We were asked to force the location of the resulting
+	     token to have a certain value.
+
+	    If we are tracking macro locations, this location might be
+	    virtual.  In that case, resolve it to the root expansion
+	    point of the macro that triggered the whole expansion
+	    sequence.  */
+	  number = linemap_resolve_location (pfile->line_table,
+					     *pfile->forced_token_location_p,
+					     LRK_MACRO_EXPANSION_POINT,
+					     NULL);
+	  number = linemap_get_expansion_line (pfile->line_table, number);
+	}
+      else
+	number = linemap_get_expansion_line (pfile->line_table,
+					     CPP_OPTION (pfile, traditional)
+					     ? pfile->line_table->highest_line
+					     : pfile->cur_token[-1].src_loc);
       break;
 
       /* __STDC__ has the value 1 under normal circumstances.
@@ -398,11 +414,14 @@ _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node)
 
 /* Convert builtin macros like __FILE__ to a token and push it on the
    context stack.  Also handles _Pragma, for which a new token may not
-   be created.  Returns 1 if it generates a new token context, 0 to
-   return the token to the caller.  LOC is the location of the expansion
-   point of the macro.  */
+   be created.  NODE is the built-in macro to consider.
+   EXPANSION_LOCATION is the location of the expansion of the macro
+   NODE.  Note that this location can be virtual if we are tracking
+   macro locations.  Returns 1 if it generates a new token context, 0
+   to return the token to the caller.  */
 static int
-builtin_macro (cpp_reader *pfile, cpp_hashnode *node, source_location loc)
+builtin_macro (cpp_reader *pfile, cpp_hashnode *node,
+	       source_location expansion_location)
 {
   const uchar *buf;
   size_t len;
@@ -418,6 +437,13 @@ builtin_macro (cpp_reader *pfile, cpp_hashnode *node, source_location loc)
       return _cpp_do__Pragma (pfile);
     }
 
+  /* Make sure the location of the token resulting from the expansion
+     of the built-in token is the location of the expansion point that
+     we are passed.  As a result, _cpp_builtin_macro_text() and
+     _cpp_lex_direct() that are called below will set the location of
+     their resulting token to this expansion point.  */
+  cpp_force_token_locations (pfile, &expansion_location);
+
   buf = _cpp_builtin_macro_text (pfile, node);
   len = ustrlen (buf);
   nbuf = (char *) alloca (len + 1);
@@ -430,8 +456,8 @@ builtin_macro (cpp_reader *pfile, cpp_hashnode *node, source_location loc)
   /* Set pfile->cur_token as required by _cpp_lex_direct.  */
   pfile->cur_token = _cpp_temp_token (pfile);
   cpp_token *token = _cpp_lex_direct (pfile);
-  /* We should point to the expansion point of the builtin macro.  */
-  token->src_loc = loc;
+  cpp_stop_forcing_token_locations (pfile);
+
   if (pfile->context->tokens_kind == TOKENS_KIND_EXTENDED)
     {
       /* We are tracking tokens resulting from macro expansion.
@@ -442,7 +468,7 @@ builtin_macro (cpp_reader *pfile, cpp_hashnode *node, source_location loc)
       _cpp_buff *token_buf = tokens_buff_new (pfile, 1, &virt_locs);
       const line_map * map =
 	linemap_enter_macro (pfile->line_table, node,
-					    token->src_loc, 1);
+			     expansion_location, 1);
       tokens_buff_add_token (token_buf, virt_locs, token,
 			     pfile->line_table->builtin_location,
 			     pfile->line_table->builtin_location,
@@ -1215,7 +1241,13 @@ enter_macro_context (cpp_reader *pfile, cpp_hashnode *node,
 
   pfile->about_to_expand_macro_p = false;
   /* Handle built-in macros and the _Pragma operator.  */
-  return builtin_macro (pfile, node, location);
+  {
+    source_location root_expansion_location =
+      CPP_OPTION (pfile, track_macro_expansion)
+      ? location
+      : pfile->invocation_location;
+    return builtin_macro (pfile, node, root_expansion_location);
+  }
 }
 
 /* De-allocate the memory used by BUFF which is an array of instances
-- 
		Dodji