[PATCH, cpp] Fix pr61817 and 69391

[PATCH, cpp] Fix pr61817 and 69391

[Richard Henderson]

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

These two related PRs are all about remembering where a macro is expanded. 
Worse, we've got two competing goals -- the real location of the expansion, for 
__LINE__, and the virtual location of the expansion, for diagnostics.

There seems to be no way to unify the two competing goals.  If we simply "fix" 
the first, we break the second.  Therefore, I resort to passing down both 
locations.

Ok?


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 7199 bytes --]

	* internal.h (_cpp_builtin_macro_text): Update decl.
	* macro.c (_cpp_builtin_macro_text): Accept location for __LINE__.
	(builtin_macro): Accept a second location for __LINE__.
	(enter_macro_context): Compute both virtual and real expansion
	locations for the macro.

	* gcc.dg/pr61817.c: New test.
	* gcc.dg/pr69391-1.c: New test.
	* gcc.dg/pr69391-2.c: New test.


diff --git a/gcc/testsuite/gcc.dg/pr61817.c b/gcc/testsuite/gcc.dg/pr61817.c
new file mode 100644
index 0000000..4230485
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr61817.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-options "-std=c11 -ftrack-macro-expansion=0" } */
+
+#define A(x) _Static_assert(x, #x)
+#define F(x, y, z) a = __LINE__, b = x ## y, c = z
+
+enum {
+#line 10
+    F
+     (
+      __LI,
+      NE__,
+      __LINE__
+      )
+};
+
+A(a == 15);
+A(b == 15);
+A(c == 15);
diff --git a/gcc/testsuite/gcc.dg/pr69391-1.c b/gcc/testsuite/gcc.dg/pr69391-1.c
new file mode 100644
index 0000000..15e49dc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr69391-1.c
@@ -0,0 +1,12 @@
+/* { dg-do run } */
+/* { dg-additional-options "-ftrack-macro-expansion=0" } */
+#define STR_I(X) #X
+#define STR(X) STR_I(X)
+#define LINE STR(__LINE__) STR(__LINE__)
+int main()
+{
+  const char *s = LINE;
+  if (s[0] != '8' || s[1] != '8')
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/pr69391-2.c b/gcc/testsuite/gcc.dg/pr69391-2.c
new file mode 100644
index 0000000..7d2faae
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr69391-2.c
@@ -0,0 +1,12 @@
+/* { dg-do run } */
+/* { dg-additional-options "-ftrack-macro-expansion=1" } */
+#define STR_I(X) #X
+#define STR(X) STR_I(X)
+#define LINE STR(__LINE__) STR(__LINE__)
+int main()
+{
+  const char *s = LINE;
+  if (s[0] != '8' || s[1] != '8')
+    __builtin_abort ();
+  return 0;
+}
diff --git a/libcpp/internal.h b/libcpp/internal.h
index bafd480..9ce8707 100644
--- a/libcpp/internal.h
+++ b/libcpp/internal.h
@@ -626,7 +626,8 @@ extern bool _cpp_save_parameter (cpp_reader *, cpp_macro *, cpp_hashnode *,
 extern bool _cpp_arguments_ok (cpp_reader *, cpp_macro *, const cpp_hashnode *,
 			       unsigned int);
 extern const unsigned char *_cpp_builtin_macro_text (cpp_reader *,
-						     cpp_hashnode *);
+						     cpp_hashnode *,
+						     source_location = 0);
 extern int _cpp_warn_if_unused_macro (cpp_reader *, cpp_hashnode *, void *);
 extern void _cpp_push_token_context (cpp_reader *, cpp_hashnode *,
 				     const cpp_token *, unsigned int);
diff --git a/libcpp/macro.c b/libcpp/macro.c
index 759fbe7..c251553 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -93,7 +93,8 @@ struct macro_arg_saved_data {
 
 static int enter_macro_context (cpp_reader *, cpp_hashnode *,
 				const cpp_token *, source_location);
-static int builtin_macro (cpp_reader *, cpp_hashnode *, source_location);
+static int builtin_macro (cpp_reader *, cpp_hashnode *,
+			  source_location, source_location);
 static void push_ptoken_context (cpp_reader *, cpp_hashnode *, _cpp_buff *,
 				 const cpp_token **, unsigned int);
 static void push_extended_tokens_context (cpp_reader *, cpp_hashnode *,
@@ -229,7 +230,8 @@ static const char * const monthnames[] =
 /* Helper function for builtin_macro.  Returns the text generated by
    a builtin macro. */
 const uchar *
-_cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node)
+_cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node,
+			 source_location loc)
 {
   const uchar *result = NULL;
   linenum_type number = 1;
@@ -319,11 +321,14 @@ _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node)
     case BT_SPECLINE:
       /* 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);
+	 Otherwise things like assert() will not work properly.
+	 See WG14 N1911, WG21 N4220 sec 6.5, and PR 61861.  */
+      if (CPP_OPTION (pfile, traditional))
+	loc = pfile->line_table->highest_line;
+      else
+	loc = linemap_resolve_location (pfile->line_table, loc,
+					LRK_MACRO_EXPANSION_POINT, NULL);
+      number = linemap_get_expansion_line (pfile->line_table, loc);
       break;
 
       /* __STDC__ has the value 1 under normal circumstances.
@@ -417,7 +422,8 @@ _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node)
    return the token to the caller.  LOC is the location of the expansion
    point of the macro.  */
 static int
-builtin_macro (cpp_reader *pfile, cpp_hashnode *node, source_location loc)
+builtin_macro (cpp_reader *pfile, cpp_hashnode *node,
+	       source_location loc, source_location expand_loc)
 {
   const uchar *buf;
   size_t len;
@@ -433,7 +439,7 @@ builtin_macro (cpp_reader *pfile, cpp_hashnode *node, source_location loc)
       return _cpp_do__Pragma (pfile, loc);
     }
 
-  buf = _cpp_builtin_macro_text (pfile, node);
+  buf = _cpp_builtin_macro_text (pfile, node, expand_loc);
   len = ustrlen (buf);
   nbuf = (char *) alloca (len + 1);
   memcpy (nbuf, buf, len);
@@ -456,8 +462,7 @@ builtin_macro (cpp_reader *pfile, cpp_hashnode *node, source_location loc)
       source_location *virt_locs = NULL;
       _cpp_buff *token_buf = tokens_buff_new (pfile, 1, &virt_locs);
       const line_map_macro * map =
-	linemap_enter_macro (pfile->line_table, node,
-					    token->src_loc, 1);
+	linemap_enter_macro (pfile->line_table, node, loc, 1);
       tokens_buff_add_token (token_buf, virt_locs, token,
 			     pfile->line_table->builtin_location,
 			     pfile->line_table->builtin_location,
@@ -1231,22 +1236,29 @@ enter_macro_context (cpp_reader *pfile, cpp_hashnode *node,
   pfile->about_to_expand_macro_p = false;
   /* Handle built-in macros and the _Pragma operator.  */
   {
-    source_location loc;
+    source_location loc, expand_loc;
+
     if (/* The top-level macro invocation that triggered the expansion
 	   we are looking at is with a standard macro ...*/
 	!(pfile->top_most_macro_node->flags & NODE_BUILTIN)
 	/* ... and it's a function-like macro invocation.  */
 	&& pfile->top_most_macro_node->value.macro->fun_like)
-      /* Then the location of the end of the macro invocation is the
-	 location of the closing parenthesis.  */
-      loc = pfile->cur_token[-1].src_loc;
+      {
+	/* Then the location of the end of the macro invocation is the
+	   location of the closing parenthesis.  */
+	loc = pfile->cur_token[-1].src_loc;
+	expand_loc = loc;
+      }
     else
-      /* Otherwise, the location of the end of the macro invocation is
-	 the location of the expansion point of that top-level macro
-	 invocation.  */
-      loc = location;
+      {
+	/* Otherwise, the location of the end of the macro invocation is
+	   the location of the expansion point of that top-level macro
+	   invocation.  */
+	loc = location;
+	expand_loc = pfile->invocation_location;
+      }
 
-    return builtin_macro (pfile, node, loc);
+    return builtin_macro (pfile, node, loc, expand_loc);
   }
 }
 

Re: [PATCH, cpp] Fix pr61817 and 69391

[Manuel López-Ibáñez]

On 05/04/16 17:22, Richard Henderson wrote:
> These two related PRs are all about remembering where a macro is expanded.
> Worse, we've got two competing goals -- the real location of the expansion, for
> __LINE__, and the virtual location of the expansion, for diagnostics.
>
> There seems to be no way to unify the two competing goals.  If we simply "fix"
> the first, we break the second.  Therefore, I resort to passing down both
> locations.

> +++ b/gcc/testsuite/gcc.dg/pr61817.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=c11 -ftrack-macro-expansion=0" } */
> +

Why use -ftrack-macro-expansion=0? This should work with =1, which is also the 
default, no?

Cheers,

	Manuel.

Re: [PATCH, cpp] Fix pr61817 and 69391

[Richard Henderson]

On 04/05/2016 11:03 AM, Manuel López-Ibáñez wrote:
> Why use -ftrack-macro-expansion=0?

That's the point of the PR -- we were producing totally bogus results.


r~

Re: [PATCH, cpp] Fix pr61817 and 69391

[Jakub Jelinek]

On Tue, Apr 05, 2016 at 09:22:37AM -0700, Richard Henderson wrote:
> These two related PRs are all about remembering where a macro is expanded.
> Worse, we've got two competing goals -- the real location of the expansion,
> for __LINE__, and the virtual location of the expansion, for diagnostics.
> 
> There seems to be no way to unify the two competing goals.  If we simply
> "fix" the first, we break the second.  Therefore, I resort to passing down
> both locations.
> 
> Ok?
> 
> 
> r~


Missing PR numbers here.

> 	* internal.h (_cpp_builtin_macro_text): Update decl.
> 	* macro.c (_cpp_builtin_macro_text): Accept location for __LINE__.
> 	(builtin_macro): Accept a second location for __LINE__.
> 	(enter_macro_context): Compute both virtual and real expansion
> 	locations for the macro.
> 
> 	* gcc.dg/pr61817.c: New test.
> 	* gcc.dg/pr69391-1.c: New test.
> 	* gcc.dg/pr69391-2.c: New test.
> 
> 
> diff --git a/gcc/testsuite/gcc.dg/pr61817.c b/gcc/testsuite/gcc.dg/pr61817.c
> new file mode 100644
> index 0000000..4230485
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr61817.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=c11 -ftrack-macro-expansion=0" } */

Wouldn't it make sense to provide this test also as -1.c and -2.c, one
with -ftrack-macro-expansion=0 and one with -ftrack-macro-expansion=1?

Otherwise LGTM.

	Jakub