Patches to enable -ftrack-macro-expansion by default

Patches to enable -ftrack-macro-expansion by default

[Dodji Seketeli]

Hello,

I am proposing a series of patches which is supposed to address the
remaining issues (I am aware of) preventing us from enabling the
-ftrack-macro-expansion by default.

The idea is to address each issue I notice in the course of trying to
bootstrap the compiler and running the tests with
-ftrack-macro-expansion enabled.

Beside the fixes, I ended up disabling the -ftrack-macro-expansion for
many test cases (sometimes globally in the dg-*.exp files, or on a
case by case basis), because that option changes the compiler output
and so requires that I either adapt the test case or disable the
option.  For other tests, I chose to adapt the test case.

You will find the patch series as a follow-up of this message.

-- 
		Dodji

[PATCH 01/11] Fix cpp_sys_macro_p with -ftrack-macro-expansion

[Dodji Seketeli]

Hello,

cpp_sys_macro_p crashes when -ftrack-macro-expansion is on.  The issue
can be reproduced by running the tests:

    runtest --tool gcc --tool_opts="-ftrack-macro-expansion" cpp.exp=sysmac1.c
    runtest --tool gcc --tool_opts="-ftrack-macro-expansion" cpp.exp=sysmac2.c

This is because it just doesn't support that mode.  Fixed thus.
Tested and bootstrapped on x86_64-unknown-linux-gnu against trunk.

Note that the bootstrap with -ftrack-macro-expansion turned on
exhibits other separate issues that are addressed in subsequent
patches.  This patch just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
though.

libcpp/

	* macro.c (cpp_sys_macro_p):  Support -ftrack-macro-expansion.
---
 libcpp/macro.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/libcpp/macro.c b/libcpp/macro.c
index 54de3e3..58a722c 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -2436,7 +2436,15 @@ cpp_get_token_with_location (cpp_reader *pfile, source_location *loc)
 int
 cpp_sys_macro_p (cpp_reader *pfile)
 {
-  cpp_hashnode *node = pfile->context->c.macro;
+  cpp_hashnode *node = NULL;
+
+  if (CPP_OPTION (pfile, track_macro_expansion))
+    {
+      if (pfile->context->c.mc)
+	node = pfile->context->c.mc->macro_node;
+    }
+  else
+    node = pfile->context->c.macro;
 
   return node && node->value.macro && node->value.macro->syshdr;
 }
-- 
		Dodji

Re: [PATCH 01/11] Fix cpp_sys_macro_p with -ftrack-macro-expansion

[Jason Merrill]

On 04/10/2012 10:55 AM, Dodji Seketeli wrote:
> +  if (CPP_OPTION (pfile, track_macro_expansion))

I think this should check context->tokens_kind rather than the compiler 
flag.

Jason

Re: [PATCH 01/11] Fix cpp_sys_macro_p with -ftrack-macro-expansion

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> On 04/10/2012 10:55 AM, Dodji Seketeli wrote:
>> +  if (CPP_OPTION (pfile, track_macro_expansion))
>
> I think this should check context->tokens_kind rather than the
> compiler flag.

OK.

Below is the updated patch that does that.

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

libcpp/

	* macro.c (cpp_sys_macro_p):  Support -ftrack-macro-expansion.
---
 libcpp/macro.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/libcpp/macro.c b/libcpp/macro.c
index 54de3e3..4f8e52f 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -2436,7 +2436,12 @@ cpp_get_token_with_location (cpp_reader *pfile, source_location *loc)
 int
 cpp_sys_macro_p (cpp_reader *pfile)
 {
-  cpp_hashnode *node = pfile->context->c.macro;
+  cpp_hashnode *node = NULL;
+
+  if (pfile->context->tokens_kind == TOKENS_KIND_EXTENDED)
+    node = pfile->context->c.mc->macro_node;
+  else
+    node = pfile->context->c.macro;
 
   return node && node->value.macro && node->value.macro->syshdr;
 }
-- 
		Dodji

Re: [PATCH 01/11] Fix cpp_sys_macro_p with -ftrack-macro-expansion

[Jason Merrill]

OK.

Jason

[PATCH 02/11] Fix token pasting with -ftrack-macro-expansion

[Dodji Seketeli]

This patch makes token pasting work with -ftrack-macro-expansion
turned on.  It improves some pasting related tests of the gcc.dg/cpp
sub-directory.

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

Note that the bootstrap with -ftrack-macro-expansion exhibits other
separate issues that are addressed in subsequent patches.  This patch
just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
though.

libcpp/

	* macro.c (paste_all_tokens): Put the token resulting from pasting
	into an extended token context with -ftrack-macro-location is in
	effect.

gcc/testsuite/

	* gcc.dg/cpp/paste17.c: New test case for
	-ftrack-macro-expansion=2 mode only.
	* gcc.dg/cpp/macro-exp-tracking-5.c: Likewise.
---
 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c |   18 ++++++++++++++
 gcc/testsuite/gcc.dg/cpp/paste17.c              |    8 ++++++
 libcpp/macro.c                                  |   28 ++++++++++++++++++++++-
 3 files changed, 53 insertions(+), 1 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/paste17.c

diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c
new file mode 100644
index 0000000..7933660
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c
@@ -0,0 +1,18 @@
+/*
+  { dg-options "-fshow-column -ftrack-macro-expansion" }
+  { dg-do compile }
+ */
+
+#define PASTED var ## iable /* { dg-error "undeclared" } */
+#define call_foo(p1, p2) \
+  foo (p1,		 \
+       p2);  /*  { dg-message "in expansion of macro" } */
+
+void foo(int, char);
+
+void
+bar()
+{
+  call_foo(1,PASTED); /* { dg-message "expanded from here" } */
+}
+
diff --git a/gcc/testsuite/gcc.dg/cpp/paste17.c b/gcc/testsuite/gcc.dg/cpp/paste17.c
new file mode 100644
index 0000000..9c6506f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/paste17.c
@@ -0,0 +1,8 @@
+ /* { dg-options "-ftrack-macro-expansion=2" } */
+/* { dg-do preprocess } */
+
+#define do_paste 1.0e ## -1
+
+do_paste
+
+/* { dg-final {scan-file paste17.i "1.0e- 1" } }*/
diff --git a/libcpp/macro.c b/libcpp/macro.c
index 58a722c..e0bfc31 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -611,6 +611,21 @@ paste_all_tokens (cpp_reader *pfile, const cpp_token *lhs)
 {
   const cpp_token *rhs = NULL;
   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))
+    abort ();
+
+  if (context->tokens_kind == TOKENS_KIND_EXTENDED)
+    /* The caller must have called consume_next_token_from_context
+       right before calling us.  That has incremented the pointer to
+       the current virtual location.  So it now points to the location
+       of the token that comes right after *LHS.  We want the
+       resulting pasted token to have the location of the current
+       *LHS, though.  */
+    virt_loc = *(context->c.mc->cur_virt_loc - 1);
 
   do
     {
@@ -650,7 +665,18 @@ paste_all_tokens (cpp_reader *pfile, const cpp_token *lhs)
   while (rhs->flags & PASTE_LEFT);
 
   /* Put the resulting token in its own context.  */
-  _cpp_push_token_context (pfile, NULL, lhs, 1);
+  if (context->tokens_kind == TOKENS_KIND_EXTENDED)
+    {
+      source_location *virt_locs = NULL;
+      _cpp_buff *token_buf = tokens_buff_new (pfile, 1, &virt_locs);
+      tokens_buff_add_token (token_buf, virt_locs, lhs,
+			     virt_loc, 0, NULL, 0);
+      push_extended_tokens_context (pfile, context->c.mc->macro_node,
+				    token_buf, virt_locs,
+				    (const cpp_token **)token_buf->base, 1);
+    }
+  else
+    _cpp_push_token_context (pfile, NULL, lhs, 1);
 }
 
 /* Returns TRUE if the number of arguments ARGC supplied in an
-- 
1.7.6.5



-- 
		Dodji

Re: [PATCH 02/11] Fix token pasting with -ftrack-macro-expansion

[Jason Merrill]

On 04/10/2012 10:57 AM, Dodji Seketeli wrote:
> +    virt_loc = *(context->c.mc->cur_virt_loc - 1);

Style nit: I'd use [-1] here.  OK with that change.

Jason

Re: [PATCH 02/11] Fix token pasting with -ftrack-macro-expansion

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> On 04/10/2012 10:57 AM, Dodji Seketeli wrote:
>> +    virt_loc = *(context->c.mc->cur_virt_loc - 1);
>
> Style nit: I'd use [-1] here.  OK with that change.

Thanks, here is the updated patch that I'll commit in a few days.

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

libcpp/

	* macro.c (paste_all_tokens): Put the token resulting from pasting
	into an extended token context with -ftrack-macro-location is in
	effect.

gcc/testsuite/

	* gcc.dg/cpp/paste17.c: New test case for
	-ftrack-macro-expansion=2 mode only.
	* gcc.dg/cpp/macro-exp-tracking-5.c: Likewise.
---
 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c |   18 ++++++++++++++
 gcc/testsuite/gcc.dg/cpp/paste17.c              |    8 ++++++
 libcpp/macro.c                                  |   28 ++++++++++++++++++++++-
 3 files changed, 53 insertions(+), 1 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/paste17.c

diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c
new file mode 100644
index 0000000..7933660
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c
@@ -0,0 +1,18 @@
+/*
+  { dg-options "-fshow-column -ftrack-macro-expansion" }
+  { dg-do compile }
+ */
+
+#define PASTED var ## iable /* { dg-error "undeclared" } */
+#define call_foo(p1, p2) \
+  foo (p1,		 \
+       p2);  /*  { dg-message "in expansion of macro" } */
+
+void foo(int, char);
+
+void
+bar()
+{
+  call_foo(1,PASTED); /* { dg-message "expanded from here" } */
+}
+
diff --git a/gcc/testsuite/gcc.dg/cpp/paste17.c b/gcc/testsuite/gcc.dg/cpp/paste17.c
new file mode 100644
index 0000000..9c6506f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/paste17.c
@@ -0,0 +1,8 @@
+ /* { dg-options "-ftrack-macro-expansion=2" } */
+/* { dg-do preprocess } */
+
+#define do_paste 1.0e ## -1
+
+do_paste
+
+/* { dg-final {scan-file paste17.i "1.0e- 1" } }*/
diff --git a/libcpp/macro.c b/libcpp/macro.c
index 4f8e52f..f4638c4 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -611,6 +611,21 @@ paste_all_tokens (cpp_reader *pfile, const cpp_token *lhs)
 {
   const cpp_token *rhs = NULL;
   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))
+    abort ();
+
+  if (context->tokens_kind == TOKENS_KIND_EXTENDED)
+    /* The caller must have called consume_next_token_from_context
+       right before calling us.  That has incremented the pointer to
+       the current virtual location.  So it now points to the location
+       of the token that comes right after *LHS.  We want the
+       resulting pasted token to have the location of the current
+       *LHS, though.  */
+    virt_loc = context->c.mc->cur_virt_loc[-1];
 
   do
     {
@@ -650,7 +665,18 @@ paste_all_tokens (cpp_reader *pfile, const cpp_token *lhs)
   while (rhs->flags & PASTE_LEFT);
 
   /* Put the resulting token in its own context.  */
-  _cpp_push_token_context (pfile, NULL, lhs, 1);
+  if (context->tokens_kind == TOKENS_KIND_EXTENDED)
+    {
+      source_location *virt_locs = NULL;
+      _cpp_buff *token_buf = tokens_buff_new (pfile, 1, &virt_locs);
+      tokens_buff_add_token (token_buf, virt_locs, lhs,
+			     virt_loc, 0, NULL, 0);
+      push_extended_tokens_context (pfile, context->c.mc->macro_node,
+				    token_buf, virt_locs,
+				    (const cpp_token **)token_buf->base, 1);
+    }
+  else
+    _cpp_push_token_context (pfile, NULL, lhs, 1);
 }
 
 /* Returns TRUE if the number of arguments ARGC supplied in an
-- 
1.7.6.5


-- 
		Dodji

[PATCH 03/11] Fix PCH crash on GTYed pointer-to-scalar field of a

[Dodji Seketeli]

Disclaimer: I am sorry for the length of this message.

In essence, it's just a one liner fix in gengtype.c.  I felt the need to
explain extensively what I think I understood because it didn't seem
obvious to me.

So, when -ftrack-macro-expansion is activated, the PCH generation
machinery can crash in gt_pch_save when it's about to relocate the
pointer for the line_maps::info_macro::maps[i]::d.macro.macro_locations
member.

The call that crashes (in ggc-common.c) is:

    state.ptrs[i]->note_ptr_fn (state.ptrs[i]->obj,
				state.ptrs[i]->note_ptr_cookie,
				relocate_ptrs, &state);

The ->note_ptr_fn called in this case is the gengtype-generated
gt_pch_p_9line_maps function.  It crashes because the second argument
passed to it is a pointer to struct line_map, instead of being a
pointer to struct line_maps (extra 's') like what the function
expects.

You can see the crash for the test case:

    runtest --tool g++ --tool_opts="-ftrack-macro-expansion" pch.exp=system-1.C

I believe it's because a part of the code of gt_pch_nx_line_maps
(generated as part of gtype-desc.c by gengtype) is not correct.  Note
that this gt_pch_nx_line_maps function is called from gt_pch_save in
the snippet:

  for (rt = gt_ggc_rtab; *rt; rt++)
    for (rti = *rt; rti->base != NULL; rti++)
      for (i = 0; i < rti->nelt; i++)
	(*rti->pchw)(*(void **)((char *)rti->base + rti->stride * i));

So, in that gt_pch_nx_line_maps, in the branch that starts with the
code:

      if ((*x).info_macro.maps != NULL) {
        size_t i3;
        for (i3 = 0; i3 != (size_t)(((*x).info_macro).used); i3++) {
          switch (((*x).info_macro.maps[i3]).reason == LC_ENTER_MACRO)

we have the code:

    gt_pch_note_object ((*x).info_macro.maps[i3].d.macro.macro_locations,
			(*x).info_macro.maps,
			 gt_pch_p_9line_maps,
			  gt_types_enum_last);

This last snippet registers gt_pch_p_9line_maps to be called on the
object pointed by (*x).info_macro.maps[i3].d.macro.macro_locations (as
a first argument), with (*x).info_macro.maps as its second argument.

Note that (*x).info_macro.maps is of type struct line_map*, while 'x'
is of type struct line_maps* - beware, there is an 's' at the end of
the latter.

The problem is that gt_pch_p_9line_maps requires that its second
argument be an instance of _struct line_maps_, not struct line_map.
So later when gt_pch_p_9line_maps is called, it just crashes.

More generally, these gt_pch_p_xxx functions seem to require that
their second argument be an instance of the xxx in question.  And that
invariant is violated by the snippet of code above.

The invariant seems to be violated only for the case where a GTYed
structure (possibly embedded in another GTYed structure) contains a
pointer to a scalar (that is not a string) which memory is ggc/GTY
managed, like the line_map_macro::macro_locations field.  And this
only happens for PCH generation.

Looking at gengtype.c, it seems like write_types_process_field can be
fooled in that case.  It expects that the expression d->prev_val[3]
contains the name of the second argument of the gt_pch_p_xxx (which is
generically referenced by wtd->subfield_marker_routine there).  That
expression can resolve to either "x", as we would like it to be, but
can also resolve to another arbitrary name for e.g, the case of a
pointer-to-struct used as a root).

This patch simply forces the second argument of gt_pch_p_xxx to be 'x'
even in the case of a member that is a pointer to a scalar.

As a result, here is the the diff the new generated gtype-desc.c file:

@@ -5234,7 +5234,7 @@ gt_pch_nx_line_maps (void *x_p)
                 size_t i2;
                 for (i2 = 0; i2 != (size_t)(2 * ((*x).info_ordinary.maps[i0].d.macro).n_tokens); i2++) {
                 }
-                gt_pch_note_object ((*x).info_ordinary.maps[i0].d.macro.macro_locations, (*x).info_ordinary.maps, gt_pch_p_9line_maps, gt_types_enum_last);
+                gt_pch_note_object ((*x).info_ordinary.maps[i0].d.macro.macro_locations, x, gt_pch_p_9line_maps, gt_types_enum_last);
               }
               break;
             default:
@@ -5261,7 +5261,7 @@ gt_pch_nx_line_maps (void *x_p)
                 size_t i5;
                 for (i5 = 0; i5 != (size_t)(2 * ((*x).info_macro.maps[i3].d.macro).n_tokens); i5++) {
                 }
-                gt_pch_note_object ((*x).info_macro.maps[i3].d.macro.macro_locations, (*x).info_macro.maps, gt_pch_p_9line_maps, gt_types_enum_last);
+                gt_pch_note_object ((*x).info_macro.maps[i3].d.macro.macro_locations, x, gt_pch_p_9line_maps, gt_types_enum_last);
               }
               break;
             default:
@@ -9366,7 +9366,7 @@ gt_pch_na_regno_reg_rtx (ATTRIBUTE_UNUSED void *x_p)
     for (i1 = 0; i1 != (size_t)(crtl->emit.x_reg_rtx_no); i1++) {
       gt_pch_n_7rtx_def (regno_reg_rtx[i1]);
     }
-    gt_pch_note_object (regno_reg_rtx, &regno_reg_rtx, gt_pch_pa_regno_reg_rtx, gt_types_enum_last);
+    gt_pch_note_object (regno_reg_rtx, x, gt_pch_pa_regno_reg_rtx, gt_types_enum_last);
   }
 }

I think it's pretty much what I was willing to have.

Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.

Note that the bootstrap with -ftrack-macro-expansion exhibits
other separate issues that are addressed in subsequent patches.
This patch just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned
off, though.

gcc/

	* gengtype.c (write_types_process_field):  Force second argument
	of the call to the PCH object hierarchy walker to be 'x'.
---
 gcc/gengtype.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/gcc/gengtype.c b/gcc/gengtype.c
index 7450eeb..63a26bf 100644
--- a/gcc/gengtype.c
+++ b/gcc/gengtype.c
@@ -2858,7 +2858,26 @@ write_types_process_field (type_p f, const struct walk_type_data *d)
 	       wtd->subfield_marker_routine, cast, d->val);
       if (wtd->param_prefix)
 	{
-	  oprintf (d->of, ", %s", d->prev_val[3]);
+	  if (f->u.p->kind == TYPE_SCALAR)
+	    /* The current type is a pointer to a scalar (so not
+	       considered like a pointer to instances of user defined
+	       types) and we are seeing it; it means we must be even
+	       more careful about the second argument of the
+	       SUBFIELD_MARKER_ROUTINE call.  That argument must
+	       always be the instance of the type for which
+	       write_func_for_structure was called - this really is
+	       what the function SUBFIELD_MARKER_ROUTINE expects.
+	       That is, it must be an instance of the ORIG_S type
+	       parameter of write_func_for_structure.  The convention
+	       is that that argument must be "x" in that case (as set
+	       by write_func_for_structure).  The problem is, we can't
+	       count on d->prev_val[3] to be always set to "x" in that
+	       case.  Sometimes walk_type can set it to something else
+	       (to e.g cooperate with write_array when called from
+	       write_roots).  So let's set it to "x" here then.  */
+	    oprintf (d->of, ", x");
+	  else
+	    oprintf (d->of, ", %s", d->prev_val[3]);
 	  if (d->orig_s)
 	    {
 	      oprintf (d->of, ", gt_%s_", wtd->param_prefix);
-- 
		Dodji

Re: [PATCH 03/11] Fix PCH crash on GTYed pointer-to-scalar field of a

[Laurynas Biveinis]

2012 m. balandis 10 d. 18:08, Dodji Seketeli <dodji@redhat.com> rašė:
> gcc/
>
>        * gengtype.c (write_types_process_field):  Force second argument
>        of the call to the PCH object hierarchy walker to be 'x'.


I guess it'd be better if we separated prev_val[3] and constant "x"
uses to separate walk_type_data fields, and I'm not sure if the fix is
complete (well it is for the current usage, isn't it), but the patch
is certainly OK. Thank you.

-- 
Laurynas

[PATCH 04/11] Fix expansion point loc for macro-like tokens

[Dodji Seketeli]

Consider the test case gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c.
Its interesting part is:

    #define A(x) vari x /* line 7.  */
    #define vari(x)
    #define B , varj
    int A(B) ;  /* line 10.  */

In its initial version, this test was being pre-processed as:

    # 1 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    # 1 "build/gcc//"
    # 1 "<command-line>"
    # 1 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    # 10 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    int
    # 7 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
		 vari

	, varj ;

Note how "int" and "vari" are on separate lines, whereas "int" and
", varj" are on the same line.

This looks like a bug to me, even independantly from the macro
location tracking work.

With macro location tracking turned on, the preprocessed output
becomes:

    # 1 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    # 1 "<command-line>"
    # 1 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    # 10 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    int vari , varj ;

Which, IMO, is what we'd expect.

This is due to an unexpected side effect of enter_macro_context when
passed a token that might look like a function-like macro at first
sight, but that it eventually considers to not be a macro after all.

This is the case for the "vari" token which looks like a macro when it
is first lexed, but is eventually considered to be a normal token by
enter_macro_context because it's not used as a function-like macro
invocation.

In that case, besides returning NULL, enter_macro_context sets
pfile->context->c.macro to NULL, making cpp_get_token_1 forget to set
the location of the "vari" to the expansion point of A.

Please look at the extensive comments I have added to cpp_get_token_1
to understand the details.

The easy fix here is to speculatively set the location of the returned
token before calling enter_macro_context.

Tested on x86_64-unknown-linux-gnu against trunk.  Now this test has
the same output with and without tracking locations accross macro
expansions.

Note that the bootstrap with -ftrack-macro-expansion exhibits other
separate issues that are addressed in subsequent patches.  This patch
just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
though.

libcpp/
	* macro.c (enter_macro_context): Update comment.
	(cpp_get_token_1): Set virtual location before calling
	enter_macro_context.

gcc/testsuite/

	* gcc.dg/debug/dwarf2/pr41445-5.c: Adjust.
	* gcc.dg/debug/dwarf2/pr41445-6.c: Likewise.

---
 gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c |    5 ++-
 gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c |    5 ++-
 libcpp/macro.c                                |   43 +++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c
index 03af604..d21acd5 100644
--- a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c
@@ -9,6 +9,9 @@
 #define B , varj
 int A(B) ;
 
-/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0x)?7\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
+/*  We want to check that both vari and varj have the same line
+    number.  */
+
+/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
 /* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"varj\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
 /* { dg-final { cleanup-saved-temps } } */
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c
index 8aa37d1..d6d79cc 100644
--- a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c
@@ -4,5 +4,8 @@
 
 #include "pr41445-5.c"
 
-/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0x)?7\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
+/*  We want to check that both vari and varj have the same line
+    number.  */
+
+/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)?\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
 /* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"varj\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
diff --git a/libcpp/macro.c b/libcpp/macro.c
index e0bfc31..e8ced2e 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -1005,8 +1005,9 @@ macro_real_token_count (const cpp_macro *macro)
    directives among macro arguments, push another context containing
    the pragma tokens before the yet-to-be-rescanned replacement list
    and return two.  Otherwise, we don't push a context and return
-   zero. LOCATION is the location of the expansion point of the
-   macro.  */
+   zero.  LOCATION is the location of the expansion point of the
+   macro.  Note that callers might want to save pfile->context as this
+   function modifies it.  */
 static int
 enter_macro_context (cpp_reader *pfile, cpp_hashnode *node,
 		     const cpp_token *result, source_location location)
@@ -2316,6 +2317,44 @@ cpp_get_token_1 (cpp_reader *pfile, source_location *location)
 	  if (pfile->state.prevent_expansion)
 	    break;
 
+	  /*  We are currently expanding the macro
+	      pfile->context->c.macro and we see that RESULT might be
+	      a macro as well.
+
+	      If we are not tracking locations accross macro
+	      expansions, then set VIRT_LOC to the expansion point of
+	      pfile->context->c.macro.
+
+	      It's useful to do this in case RESULT is finally
+	      considered to *NOT* be a macro by enter_macro_context
+	      below.  This can happen in cases like:
+
+		  #define foo(x)  // line 1
+		  foo;  // line 2
+
+	      Preprocessing the above yields "foo;" because "foo" at
+	      line 2 is not considered to be a macro; it is different
+	      from the "function-like macro foo" defined at line 1.
+
+	      So enter_macro_context below will return 0 on "foo" in
+	      this case *and* will (surprisingly!) set
+	      pfile->context->c.macro to NULL[1].  Meaning that "foo"
+	      is just a normal token, rather than a macro to be
+	      expanded.
+
+	      [1]: Note that enter_macro_context sets
+	      pfile->context->c.macro to NULL in that case because
+	      funlike_invocation_p reads one token pass "foo", sees
+	      that there is no '(' token, so we are not invoking the
+	      function-like parameter.  It then puts the tokens (which
+	      it has read after "foo") back into the tokens stream by
+	      calling _cpp_push_token_context on it, which sets
+	      pfile->context->c.macro to NULL.  */
+	  if (!CPP_OPTION (pfile, track_macro_expansion)
+	      && can_set
+	      && pfile->context->c.macro != NULL)
+	    virt_loc = pfile->invocation_location;
+
 	  /* Conditional macros require that a predicate be evaluated
 	     first.  */
 	  if ((node->flags & NODE_CONDITIONAL) != 0)
-- 
		Dodji

Re: [PATCH 04/11] Fix expansion point loc for macro-like tokens

[Jason Merrill]

On 04/10/2012 03:42 PM, Dodji Seketeli wrote:
> In that case, besides returning NULL, enter_macro_context sets
> pfile->context->c.macro to NULL, making cpp_get_token_1 forget to set
> the location of the "vari" to the expansion point of A.

This seems like a bug that should be fixed rather than worked around; we 
are still expanding A.

Jason

Re: [PATCH 04/11] Fix expansion point loc for macro-like tokens

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> On 04/10/2012 03:42 PM, Dodji Seketeli wrote:
>> In that case, besides returning NULL, enter_macro_context sets
>> pfile->context->c.macro to NULL, making cpp_get_token_1 forget to set
>> the location of the "vari" to the expansion point of A.
>
> This seems like a bug that should be fixed rather than worked around;
> we are still expanding A.

Right.  Below is an updated patch (with an updated introductory text)
that addresses the core of the issue.

Consider the test case gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c.
Its interesting part is:

    #define A(x) vari x /* line 7.  */
    #define vari(x)
    #define B , varj
    int A(B) ;  /* line 10.  */

In its initial version, this test was being pre-processed as:

    # 1 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    # 1 "build/gcc//"
    # 1 "<command-line>"
    # 1 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    # 10 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    int
    # 7 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
		 vari

	, varj ;

Note how "int" and "vari" are on separate lines, whereas "int" and
", varj" are on the same line.

This looks like a bug to me, even independantly from the macro
location tracking work.

With macro location tracking turned on, the preprocessed output
becomes:

    # 1 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    # 1 "<command-line>"
    # 1 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    # 10 "gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c"
    int vari , varj ;

Which, IMO, is what we'd expect.

This is due to an unexpected side effect of enter_macro_context when
passed a token that might look like a function-like macro at first
sight, but that it eventually considers to not be a macro after all.

This is the case for the "vari" token which looks like a macro when it
is first lexed, but is eventually considered to be a normal token by
enter_macro_context because it's not used as a function-like macro
invocation.

In that case, besides returning NULL, enter_macro_context sets
pfile->context->c.macro to NULL, making cpp_get_token_1 forget to set
the location of the "vari" to the expansion point of A.

enter_macro_context sets pfile->context->c.macro to NULL in that case
because funlike_invocation_p reads one token pass "foo", sees that
there is no '(' token, so we are not invoking the function-like
parameter.  It then puts the tokens (which it has read after "foo")
back into the tokens stream by calling _cpp_push_token_context on it,
which sets pfile->context->c.macro to NULL.

The fix here is to prevent funlike_invocation_p from forgetting the
current "macro-ness".

Tested on x86_64-unknown-linux-gnu against trunk.  Now this test has
the same output with and without tracking locations accross macro
expansions.

Note that the bootstrap with -ftrack-macro-expansion exhibits other
separate issues that are addressed in subsequent patches.  This patch
just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
though.

libcpp/
	* macro.c (funlike_invocation_p): Don't forget macro-ness.

gcc/testsuite/

	* gcc.dg/debug/dwarf2/pr41445-5.c: Adjust.
	* gcc.dg/debug/dwarf2/pr41445-6.c: Likewise.
---
 gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c |    5 ++++-
 gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c |    5 ++++-
 libcpp/macro.c                                |   12 +++++++++++-
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c
index 03af604..d21acd5 100644
--- a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c
@@ -9,6 +9,9 @@
 #define B , varj
 int A(B) ;
 
-/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0x)?7\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
+/*  We want to check that both vari and varj have the same line
+    number.  */
+
+/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
 /* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"varj\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
 /* { dg-final { cleanup-saved-temps } } */
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c
index 8aa37d1..d6d79cc 100644
--- a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c
@@ -4,5 +4,8 @@
 
 #include "pr41445-5.c"
 
-/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0x)?7\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
+/*  We want to check that both vari and varj have the same line
+    number.  */
+
+/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)?\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
 /* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"varj\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
diff --git a/libcpp/macro.c b/libcpp/macro.c
index f4638c4..672020a 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -979,7 +979,17 @@ funlike_invocation_p (cpp_reader *pfile, cpp_hashnode *node,
 	 too difficult.  We re-insert it in its own context.  */
       _cpp_backup_tokens (pfile, 1);
       if (padding)
-	_cpp_push_token_context (pfile, NULL, padding, 1);
+	{
+	  /* If the first token we got was a padding token, let's put
+	     it back into the stream so that cpp_get_token will get it
+	     first; and if we are currently expanding a macro, don't
+	     forget that information.  */
+	  cpp_hashnode *macro =
+	    (pfile->context->tokens_kind == TOKENS_KIND_EXTENDED)
+	    ? pfile->context->c.mc->macro_node
+	    : pfile->context->c.macro;
+	  _cpp_push_token_context (pfile, macro, padding, 1);
+	}
     }
 
   return NULL;
-- 
1.7.6.5


-- 
		Dodji

Re: [PATCH 04/11] Fix expansion point loc for macro-like tokens

[Jason Merrill]

On 04/25/2012 05:07 AM, Dodji Seketeli wrote:
> +	  /* If the first token we got was a padding token, let's put
> +	     it back into the stream so that cpp_get_token will get it
> +	     first; and if we are currently expanding a macro, don't
> +	     forget that information.  */
> +	  cpp_hashnode *macro =
> +	    (pfile->context->tokens_kind == TOKENS_KIND_EXTENDED)
> +	    ? pfile->context->c.mc->macro_node
> +	    : pfile->context->c.macro;
> +	  _cpp_push_token_context (pfile, macro, padding, 1);

What about the other places that call _cpp_push_token_context with a 
NULL macro argument?  Don't we want to continue the current macro 
context in that case, too?  Perhaps we should move this new code inside 
_cpp_push_token_context for the case when the macro parameter is NULL.

Jason

Re: [PATCH 04/11] Fix expansion point loc for macro-like tokens

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> On 04/25/2012 05:07 AM, Dodji Seketeli wrote:
>> +	  /* If the first token we got was a padding token, let's put
>> +	     it back into the stream so that cpp_get_token will get it
>> +	     first; and if we are currently expanding a macro, don't
>> +	     forget that information.  */
>> +	  cpp_hashnode *macro =
>> +	    (pfile->context->tokens_kind == TOKENS_KIND_EXTENDED)
>> +	    ? pfile->context->c.mc->macro_node
>> +	    : pfile->context->c.macro;
>> +	  _cpp_push_token_context (pfile, macro, padding, 1);
>
> What about the other places that call _cpp_push_token_context with a
> NULL macro argument?  Don't we want to continue the current macro
> context in that case, too?  Perhaps we should move this new code
> inside _cpp_push_token_context for the case when the macro parameter
> is NULL.

Right.  I did that.

But then, now that there can be some contiguous contexts representing
the same macro, I had to adjust how _cpp_pop_context was re-enabling the
'expandability' of a given macro M.

For the background, When M is being expanded, it's flagged by
enter_macro_context as being non-expandable, to prevent its possible
recursive expansions.  And it's flagged back to being expandable when we
get out of its expansion context.

Now, getting out of the expansion context means to test that have
actually popped all the possibly contiguous contexts that are related to
M.  Otherwise, we get into situations of recursive expansion of M.

Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.

libcpp/
	* macro.c (macro_of_context): New static function.
	(_cpp_push_token_context, push_extended_tokens_context): If the
	macro argument is NULL, it means we are continuing the expansion
	of the current macro, if any.  Update comments.
	(_cpp_pop_context): Re-enable expansion of the macro only when we
	are really out of the context of the current expansion.

gcc/testsuite/

	* gcc.dg/debug/dwarf2/pr41445-5.c: Adjust.
	* gcc.dg/debug/dwarf2/pr41445-6.c: Likewise.
---
 gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c |    5 ++-
 gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c |    5 ++-
 libcpp/macro.c                                |   56 +++++++++++++++++++++----
 3 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c
index 03af604..d21acd5 100644
--- a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-5.c
@@ -9,6 +9,9 @@
 #define B , varj
 int A(B) ;
 
-/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0x)?7\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
+/*  We want to check that both vari and varj have the same line
+    number.  */
+
+/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
 /* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"varj\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
 /* { dg-final { cleanup-saved-temps } } */
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c
index 8aa37d1..d6d79cc 100644
--- a/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr41445-6.c
@@ -4,5 +4,8 @@
 
 #include "pr41445-5.c"
 
-/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0x)?7\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
+/*  We want to check that both vari and varj have the same line
+    number.  */
+
+/* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"vari\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)?\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
 /* { dg-final { scan-assembler "DW_TAG_variable\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\"varj\[^\\r\\n\]*DW_AT_name(\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*DW_AT_)*\[^\\r\\n\]*\[\\r\\n\]+\[^\\r\\n\]*\[^0-9a-fA-FxX](0xa|10)\[^0-9a-fA-FxX]\[^\\r\\n\]*DW_AT_decl_line" } } */
diff --git a/libcpp/macro.c b/libcpp/macro.c
index f4638c4..ab3e8f6 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -165,6 +165,8 @@ static void consume_next_token_from_context (cpp_reader *pfile,
 					     source_location *);
 static const cpp_token* cpp_get_token_1 (cpp_reader *, source_location *);
 
+static cpp_hashnode* macro_of_context (cpp_context *context);
+
 /* Statistical counter tracking the number of macros that got
    expanded.  */
 unsigned num_expanded_macros_counter = 0;
@@ -1808,18 +1810,27 @@ push_ptoken_context (cpp_reader *pfile, cpp_hashnode *macro, _cpp_buff *buff,
   LAST (context).ptoken = first + count;
 }
 
-/* Push a list of tokens.  */
+/* Push a list of tokens.
+
+   A NULL macro means that we should continue the current macro
+   expansion, in essence.  That means that if we are currently in a
+   macro expansion context, we'll make the new pfile->context refer to
+   the current macro.  */
 void
 _cpp_push_token_context (cpp_reader *pfile, cpp_hashnode *macro,
 			 const cpp_token *first, unsigned int count)
 {
-   cpp_context *context = next_context (pfile);
- 
+  cpp_context *context;
+
+   if (macro == NULL)
+     macro = macro_of_context (pfile->context);
+
+   context = next_context (pfile);
    context->tokens_kind = TOKENS_KIND_DIRECT;
    context->c.macro = macro;
    context->buff = NULL;
-  FIRST (context).token = first;
-  LAST (context).token = first + count;
+   FIRST (context).token = first;
+   LAST (context).token = first + count;
 }
 
 /* Build a context containing a list of tokens as well as their
@@ -1827,7 +1838,12 @@ _cpp_push_token_context (cpp_reader *pfile, cpp_hashnode *macro,
    contains the tokens pointed to by FIRST.  If TOKENS_BUFF is
    non-NULL, it means that the context owns it, meaning that
    _cpp_pop_context will free it as well as VIRT_LOCS_BUFF that
-   contains the virtual locations.  */
+   contains the virtual locations.
+
+   A NULL macro means that we should continue the current macro
+   expansion, in essence.  That means that if we are currently in a
+   macro expansion context, we'll make the new pfile->context refer to
+   the current macro.  */
 static void
 push_extended_tokens_context (cpp_reader *pfile,
 			      cpp_hashnode *macro,
@@ -1836,9 +1852,13 @@ push_extended_tokens_context (cpp_reader *pfile,
 			      const cpp_token **first,
 			      unsigned int count)
 {
-  cpp_context *context = next_context (pfile);
+  cpp_context *context;
   macro_context *m;
 
+  if (macro == NULL)
+    macro = macro_of_context (pfile->context);
+
+  context = next_context (pfile);
   context->tokens_kind = TOKENS_KIND_EXTENDED;
   context->buff = token_buff;
 
@@ -2110,6 +2130,19 @@ expand_arg (cpp_reader *pfile, macro_arg *arg)
   CPP_WTRADITIONAL (pfile) = saved_warn_trad;
 }
 
+/* Returns the macro associated to the current context if we are in
+   the context a macro expansion, NULL otherwise.  */
+static cpp_hashnode*
+macro_of_context (cpp_context *context)
+{
+  if (context == NULL)
+    return NULL;
+
+  return (context->tokens_kind == TOKENS_KIND_EXTENDED)
+    ? context->c.mc->macro_node
+    : context->c.macro;
+}
+
 /* 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
@@ -2146,7 +2179,14 @@ _cpp_pop_context (cpp_reader *pfile)
 	 tokens is pushed just for the purpose of walking them using
 	 cpp_get_token_1.  In that case, no 'macro' field is set into
 	 the dummy context.  */
-      if (macro != NULL)
+      if (macro != NULL
+	  /* Several contiguous macro expansion contexts can be
+	     associated to the same macro; that means it's the same
+	     macro expansion that spans accross all these (sub)
+	     contexts.  So we should re-enable an expansion-disabled
+	     macro only when we are sure we are really out of that
+	     macro expansion.  */
+	  && macro_of_context (context->prev) != macro)
 	macro->flags &= ~NODE_DISABLED;
     }
 
-- 
		Dodji

Re: [PATCH 04/11] Fix expansion point loc for macro-like tokens

[Jason Merrill]

OK.

Jason

[PATCH 05/11] Make expand_location resolve to locus in main source file

[Dodji Seketeli]

Apparently, quite some places in the compiler (like the C/C++
preprocessor, the debug info machinery) expect expand_location to
resolve to locations that are in the main source file, even if the
token at stake comes from a macro that was defined in a header
somewhere.  Turning on -ftrack-macro-expansion by default was
triggering a lot of failures (not necessarily related to diagnostics)
because expand_location resolves to spelling locations instead.

So I have changed expand_location to honour the initial expectation.

In addition, I came up with the new expand_location_to_spelling_point
used in diagnostic_build_prefix because the diagnostic system, on the
other hand, wants to point to the location of the token where it was
spelled, and then display the error context involving all the macro
whose expansion led to that spelling point - if we are in the context
of a macro expansion there.

This seems to me like a reasonable balance.

Tested and bootstrapped on x86_64-unknown-linux-gnu against trunk and
whitnessed that a lot more tests were PASSing.

Note that the bootstrap with -ftrack-macro-expansion exhibits other
separate issues that are addressed in subsequent patches.  This patch
just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
though.

gcc/

	* input.c (expand_location_1): New.  Takes a parameter to choose
	whether to resolve the location to spelling or expansion point.
	Was factorized from ...
	(expand_location): ... here.
	(expand_location_to_spelling_point): New.  Implemented in terms of
	expand_location_1.
	* diagnostic.c (diagnostic_build_prefix): Use the new
	expand_location_to_spelling_point instead of expand_location.
---
 gcc/diagnostic.c |    2 +-
 gcc/input.c      |   39 ++++++++++++++++++++++++++++++++++-----
 gcc/input.h      |    9 +++++++++
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index a68d6ce..337328c 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -183,7 +183,7 @@ diagnostic_build_prefix (diagnostic_context *context,
     "must-not-happen"
   };
   const char *text = _(diagnostic_kind_text[diagnostic->kind]);
-  expanded_location s = expand_location (diagnostic->location);
+  expanded_location s = expand_location_to_spelling_point (diagnostic->location);
   if (diagnostic->override_column)
     s.column = diagnostic->override_column;
   gcc_assert (diagnostic->kind < DK_LAST_DIAGNOSTIC_KIND);
diff --git a/gcc/input.c b/gcc/input.c
index 4077f9e..dcd348b 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -32,16 +32,22 @@ struct line_maps *line_table;
 
 /* Expand the source location LOC into a human readable location.  If
    LOC resolves to a builtin location, the file name of the readable
-   location is set to the string "<built-in>".  */
-
-expanded_location
-expand_location (source_location loc)
+   location is set to the string "<built-in>". If EXPANSION_POINT_P is
+   TRUE and LOC is virtual, then it is resolved to the expansion
+   point of the involved macro.  Otherwise, it is resolved to the
+   spelling location of the token.  */
+
+static expanded_location
+expand_location_1 (source_location loc,
+		   bool expansion_point_p)
 {
   expanded_location xloc;
   const struct line_map *map;
 
   loc = linemap_resolve_location (line_table, loc,
-				  LRK_SPELLING_LOCATION, &map);
+				  expansion_point_p
+				  ? LRK_MACRO_EXPANSION_POINT
+				  : LRK_SPELLING_LOCATION, &map);
   xloc = linemap_expand_location (line_table, map, loc);
 
   if (loc <= BUILTINS_LOCATION)
@@ -50,6 +56,29 @@ expand_location (source_location loc)
   return xloc;
 }
 
+/* Expand the source location LOC into a human readable location.  If
+   LOC is virtual, it resolves to the expansion point of the involved
+   macro.  If LOC resolves to a builtin location, the file name of the
+   readable location is set to the string "<built-in>".  */
+
+expanded_location
+expand_location (source_location loc)
+{
+  return expand_location_1 (loc, /*expansion_point_p=*/true);
+}
+
+/* Expand the source location LOC into a human readable location.  If
+   LOC is virtual, it resolves to the expansion location of the
+   relevant macro.  If LOC resolves to a builtin location, the file
+   name of the readable location is set to the string
+   "<built-in>".  */
+
+expanded_location
+expand_location_to_spelling_point (source_location loc)
+{
+  return expand_location_1 (loc, /*expansion_piont_p=*/false);
+}
+
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
 
diff --git a/gcc/input.h b/gcc/input.h
index f2f3513..5838153 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -38,6 +38,7 @@ extern char builtins_location_check[(BUILTINS_LOCATION
 				     < RESERVED_LOCATION_COUNT) ? 1 : -1];
 
 extern expanded_location expand_location (source_location);
+extern expanded_location expand_location_to_spelling_point (source_location);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
@@ -49,6 +50,14 @@ extern location_t input_location;
 #define LOCATION_LINE(LOC) ((expand_location (LOC)).line)
 #define LOCATION_COLUMN(LOC)((expand_location (LOC)).column)
 
+#define EXPANSION_POINT_LOCATION_FILE(LOC)		\
+  ((expand_location_to_expansion_point (LOC)).file)
+#define EXPANSION_POINT_LOCATION_LINE(LOC)		\
+  ((expand_location_to_expansion_point (LOC)).line)
+#define EXPANSION_POINT_LOCATION_COLUMN(LOC)		\
+  ((expand_location_to_expansion_point (LOC)).column)
+
+
 #define input_line LOCATION_LINE (input_location)
 #define input_filename LOCATION_FILE (input_location)
 #define in_system_header_at(LOC) \
-- 
		Dodji

Re: [PATCH 05/11] Make expand_location resolve to locus in main source file

[Jason Merrill]

On 04/10/2012 03:49 PM, Dodji Seketeli wrote:
> Apparently, quite some places in the compiler (like the C/C++
> preprocessor, the debug info machinery) expect expand_location to
> resolve to locations that are in the main source file, even if the
> token at stake comes from a macro that was defined in a header
> somewhere.  Turning on -ftrack-macro-expansion by default was
> triggering a lot of failures (not necessarily related to diagnostics)
> because expand_location resolves to spelling locations instead.

Can you elaborate on these failures?  For debug info I would think that 
the spelling location would be useful information, though I suppose 
without any way to "unwind" to the expansion context it could be a bit 
confusing.  What is the problem for the preprocessor?

Jason

Re: [PATCH 05/11] Make expand_location resolve to locus in main source file

[Dodji Seketeli]

> On 04/10/2012 03:49 PM, Dodji Seketeli wrote:
>> Apparently, quite some places in the compiler (like the C/C++
>> preprocessor, the debug info machinery) expect expand_location to
>> resolve to locations that are in the main source file, even if the
>> token at stake comes from a macro that was defined in a header
>> somewhere.  Turning on -ftrack-macro-expansion by default was
>> triggering a lot of failures (not necessarily related to diagnostics)
>> because expand_location resolves to spelling locations instead.
>
> Can you elaborate on these failures?  For debug info I would think
> that the spelling location would be useful information, though I
> suppose without any way to "unwind" to the expansion context it could
> be a bit confusing.  What is the problem for the preprocessor?

For the preprocessor, consider a short excerpt of the the test
gcc/testsuite/gcc.dg/cpp/avoidpaste1.c:

    #define f(x) x
    #define g
    #define tricky 1.0e ## -1

    :: :g: :f(): :f(^): tricky

As the comment in the text says, it should preprocess as:

    :: : : : : :^: 1.0e- 1

but it actually preprocess as:

    # 1 "/home/dodji/devel/git/gcc/tmp/gcc/testsuite/gcc.dg/cpp/avoidpaste1.c"
    # 1 "<command-line>"
    # 1 "/home/dodji/devel/git/gcc/tmp/gcc/testsuite/gcc.dg/cpp/avoidpaste1.c"
    # 25 "/home/dodji/devel/git/gcc/tmp/gcc/testsuite/gcc.dg/cpp/avoidpaste1.c"
    :: : : : : :^:
    # 14 "/home/dodji/devel/git/gcc/tmp/gcc/testsuite/gcc.dg/cpp/avoidpaste1.c"
		   1.0e- 1

Note how the "1.0e- 1" is not on the same line as its preceding
":: : : : : :^:".

The problem is that the pre-processor code indirectly uses
expand_location in many spots.  For that kind of existing code that is
not related to diagnostics, I am really not confident in changing the
underlying implicit assumption of the that function which is, to
return the expansion point location.  I don't even know before hand
where all these spots are in the code base, so auditing it is hard for
me.

I forgot to say that are also other weird failure like:

    FAIL: g++.dg/cdce3.C -std=gnu++98 scan-tree-dump cdce "cdce3.C:92: note: function call is shrink-wrapped into error conditions."

due to that change.

That's why I prefer erring on the safe side by not changing the
assumption of existing code, and provide a way to expand locations to
their spelling point, just for diagnostics.  I think this is already
an improvement over what we previously had.

And for the debug info cases, I'd propose that if we find specific
examples where the unwinding to the spelling locations is better, then
we'll consider using it at that moment.

-- 
		Dodji

Re: [PATCH 05/11] Make expand_location resolve to locus in main source file

[Dodji Seketeli]

I have re-based my tree on top of a recent tree that incorporates the
changes for caret diagnostics and I had to update this patch
accordingly.  Here is its new version on trunk of today.  It basically
updates the new diagnostic_show_locus function to point to spelling
locations.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
and passes bootstrap with the full series with -ftrack-macro-expansion
turned on.

gcc/

	* input.c (expand_location_1): New.  Takes a parameter to choose
	whether to resolve the location to spelling or expansion point.
	Was factorized from ...
	(expand_location): ... here.
	(expand_location_to_spelling_point): New.  Implemented in terms of
	expand_location_1.
	* diagnostic.c (diagnostic_build_prefix): Use the new
	expand_location_to_spelling_point instead of expand_location.
---
 gcc/diagnostic.c |    4 ++--
 gcc/input.c      |   40 +++++++++++++++++++++++++++++++++++-----
 gcc/input.h      |    9 +++++++++
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 4496803..729e865 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -214,7 +214,7 @@ diagnostic_build_prefix (diagnostic_context *context,
     "must-not-happen"
   };
   const char *text = _(diagnostic_kind_text[diagnostic->kind]);
-  expanded_location s = expand_location (diagnostic->location);
+  expanded_location s = expand_location_to_spelling_point (diagnostic->location);
   if (diagnostic->override_column)
     s.column = diagnostic->override_column;
   gcc_assert (diagnostic->kind < DK_LAST_DIAGNOSTIC_KIND);
@@ -266,7 +266,7 @@ diagnostic_show_locus (diagnostic_context * context,
       || diagnostic->location <= BUILTINS_LOCATION)
     return;
 
-  s = expand_location(diagnostic->location);
+  s = expand_location_to_spelling_point (diagnostic->location);
   line = location_get_source_line (s);
   if (line == NULL)
     return;
diff --git a/gcc/input.c b/gcc/input.c
index bf5fe48..e9ba301 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -32,16 +32,22 @@ struct line_maps *line_table;
 
 /* Expand the source location LOC into a human readable location.  If
    LOC resolves to a builtin location, the file name of the readable
-   location is set to the string "<built-in>".  */
-
-expanded_location
-expand_location (source_location loc)
+   location is set to the string "<built-in>". If EXPANSION_POINT_P is
+   TRUE and LOC is virtual, then it is resolved to the expansion
+   point of the involved macro.  Otherwise, it is resolved to the
+   spelling location of the token.  */
+
+static expanded_location
+expand_location_1 (source_location loc,
+		   bool expansion_point_p)
 {
   expanded_location xloc;
   const struct line_map *map;
 
   loc = linemap_resolve_location (line_table, loc,
-				  LRK_SPELLING_LOCATION, &map);
+				  expansion_point_p
+				  ? LRK_MACRO_EXPANSION_POINT
+				  : LRK_SPELLING_LOCATION, &map);
   xloc = linemap_expand_location (line_table, map, loc);
 
   if (loc <= BUILTINS_LOCATION)
@@ -109,6 +115,30 @@ location_get_source_line(expanded_location xloc)
   return buffer;
 }
 
+/* Expand the source location LOC into a human readable location.  If
+   LOC is virtual, it resolves to the expansion point of the involved
+   macro.  If LOC resolves to a builtin location, the file name of the
+   readable location is set to the string "<built-in>".  */
+
+expanded_location
+expand_location (source_location loc)
+{
+  return expand_location_1 (loc, /*expansion_point_p=*/true);
+}
+
+/* Expand the source location LOC into a human readable location.  If
+   LOC is virtual, it resolves to the expansion location of the
+   relevant macro.  If LOC resolves to a builtin location, the file
+   name of the readable location is set to the string
+   "<built-in>".  */
+
+expanded_location
+expand_location_to_spelling_point (source_location loc)
+{
+  return expand_location_1 (loc, /*expansion_piont_p=*/false);
+}
+
+
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
 
diff --git a/gcc/input.h b/gcc/input.h
index 4b15222..f755cdf 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -39,6 +39,7 @@ extern char builtins_location_check[(BUILTINS_LOCATION
 
 extern expanded_location expand_location (source_location);
 extern const char * location_get_source_line(expanded_location xloc);
+extern expanded_location expand_location_to_spelling_point (source_location);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
@@ -50,6 +51,14 @@ extern location_t input_location;
 #define LOCATION_LINE(LOC) ((expand_location (LOC)).line)
 #define LOCATION_COLUMN(LOC)((expand_location (LOC)).column)
 
+#define EXPANSION_POINT_LOCATION_FILE(LOC)		\
+  ((expand_location_to_expansion_point (LOC)).file)
+#define EXPANSION_POINT_LOCATION_LINE(LOC)		\
+  ((expand_location_to_expansion_point (LOC)).line)
+#define EXPANSION_POINT_LOCATION_COLUMN(LOC)		\
+  ((expand_location_to_expansion_point (LOC)).column)
+
+
 #define input_line LOCATION_LINE (input_location)
 #define input_filename LOCATION_FILE (input_location)
 #define in_system_header_at(LOC) \
-- 
		Dodji

Re: [PATCH 05/11] Make expand_location resolve to locus in main source file

[Jason Merrill]

On 04/25/2012 11:31 AM, Dodji Seketeli wrote:
> +#define EXPANSION_POINT_LOCATION_FILE(LOC)		\
> +  ((expand_location_to_expansion_point (LOC)).file)
> +#define EXPANSION_POINT_LOCATION_LINE(LOC)		\
> +  ((expand_location_to_expansion_point (LOC)).line)
> +#define EXPANSION_POINT_LOCATION_COLUMN(LOC)		\
> +  ((expand_location_to_expansion_point (LOC)).column)

These macros don't seem to be used anywhere.  The rest of the patch is OK.

Jason

Re: [PATCH 05/11] Make expand_location resolve to locus in main source file

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> On 04/25/2012 11:31 AM, Dodji Seketeli wrote:
>> +#define EXPANSION_POINT_LOCATION_FILE(LOC)		\
>> +  ((expand_location_to_expansion_point (LOC)).file)
>> +#define EXPANSION_POINT_LOCATION_LINE(LOC)		\
>> +  ((expand_location_to_expansion_point (LOC)).line)
>> +#define EXPANSION_POINT_LOCATION_COLUMN(LOC)		\
>> +  ((expand_location_to_expansion_point (LOC)).column)
>
> These macros don't seem to be used anywhere.  The rest of the patch is
> OK.

Thanks.  Here is the updated patch that I will commit with the rest when
all the patches are ACKed.

gcc/

	* input.c (expand_location_1): New.  Takes a parameter to choose
	whether to resolve the location to spelling or expansion point.
	Was factorized from ...
	(expand_location): ... here.
	(expand_location_to_spelling_point): New.  Implemented in terms of
	expand_location_1.
	* diagnostic.c (diagnostic_build_prefix): Use the new
	expand_location_to_spelling_point instead of expand_location.
---
 gcc/diagnostic.c |    4 ++--
 gcc/input.c      |   40 +++++++++++++++++++++++++++++++++++-----
 gcc/input.h      |    1 +
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 4496803..729e865 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -214,7 +214,7 @@ diagnostic_build_prefix (diagnostic_context *context,
     "must-not-happen"
   };
   const char *text = _(diagnostic_kind_text[diagnostic->kind]);
-  expanded_location s = expand_location (diagnostic->location);
+  expanded_location s = expand_location_to_spelling_point (diagnostic->location);
   if (diagnostic->override_column)
     s.column = diagnostic->override_column;
   gcc_assert (diagnostic->kind < DK_LAST_DIAGNOSTIC_KIND);
@@ -266,7 +266,7 @@ diagnostic_show_locus (diagnostic_context * context,
       || diagnostic->location <= BUILTINS_LOCATION)
     return;
 
-  s = expand_location(diagnostic->location);
+  s = expand_location_to_spelling_point (diagnostic->location);
   line = location_get_source_line (s);
   if (line == NULL)
     return;
diff --git a/gcc/input.c b/gcc/input.c
index bf5fe48..e9ba301 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -32,16 +32,22 @@ struct line_maps *line_table;
 
 /* Expand the source location LOC into a human readable location.  If
    LOC resolves to a builtin location, the file name of the readable
-   location is set to the string "<built-in>".  */
-
-expanded_location
-expand_location (source_location loc)
+   location is set to the string "<built-in>". If EXPANSION_POINT_P is
+   TRUE and LOC is virtual, then it is resolved to the expansion
+   point of the involved macro.  Otherwise, it is resolved to the
+   spelling location of the token.  */
+
+static expanded_location
+expand_location_1 (source_location loc,
+		   bool expansion_point_p)
 {
   expanded_location xloc;
   const struct line_map *map;
 
   loc = linemap_resolve_location (line_table, loc,
-				  LRK_SPELLING_LOCATION, &map);
+				  expansion_point_p
+				  ? LRK_MACRO_EXPANSION_POINT
+				  : LRK_SPELLING_LOCATION, &map);
   xloc = linemap_expand_location (line_table, map, loc);
 
   if (loc <= BUILTINS_LOCATION)
@@ -109,6 +115,30 @@ location_get_source_line(expanded_location xloc)
   return buffer;
 }
 
+/* Expand the source location LOC into a human readable location.  If
+   LOC is virtual, it resolves to the expansion point of the involved
+   macro.  If LOC resolves to a builtin location, the file name of the
+   readable location is set to the string "<built-in>".  */
+
+expanded_location
+expand_location (source_location loc)
+{
+  return expand_location_1 (loc, /*expansion_point_p=*/true);
+}
+
+/* Expand the source location LOC into a human readable location.  If
+   LOC is virtual, it resolves to the expansion location of the
+   relevant macro.  If LOC resolves to a builtin location, the file
+   name of the readable location is set to the string
+   "<built-in>".  */
+
+expanded_location
+expand_location_to_spelling_point (source_location loc)
+{
+  return expand_location_1 (loc, /*expansion_piont_p=*/false);
+}
+
+
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
 
diff --git a/gcc/input.h b/gcc/input.h
index 4b15222..ea19e07 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -39,6 +39,7 @@ extern char builtins_location_check[(BUILTINS_LOCATION
 
 extern expanded_location expand_location (source_location);
 extern const char * location_get_source_line(expanded_location xloc);
+extern expanded_location expand_location_to_spelling_point (source_location);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
-- 
		Dodji

[PATCH 06/11] Strip "<built-in>" loc from displayed expansion context

[Dodji Seketeli]

Now that diagnostics for tokens coming from macro expansions point to
the spelling location of the relevant token (and then displays the
context of the expansion), some ugly (not so seldom) corner cases can
happen.

When the relevant token is a built-in token (which means the location
of that token is BUILTINS_LOCATION) the location prefix displayed to
the user in the diagnostic line is the "<built-in>:0:0" string.  For
instance:

    <built-in>:0:0: warning: conversion to 'float' alters 'int' constant value

For the user, I think this is surprising and useless.

A more user-friendly approach would be to refer to the first location
that (in the reported macro expansion context) is for a location in
real source code, like what is shown in the new test case
gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C accompanying
this patch.

To do this, I am making the line-map module provide a new
linemap_unwind_to_first_non_reserved_loc function that resolves a
virtual location to the first spelling location that is in real source
code.

I am then using that facility in the diagnostics printing module and
in the macro unwinder to avoid printing diagnostics lines that refer
to the locations for built-ins or more generally for reserved
locations.  Note that when I start the dance of skipping a built-in
location I also skip locations that are in system headers, because it
turned out that a lot of those built-ins are actually used in system
headers (e.g, "#define INT_MAX __INT_MAX__" where __INT_MAX__ is a
built-in).

Besides the user-friendliness gain, this patch allows a number of
regression tests to PASS unchanged with and without
-ftrack-macro-expansion.

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

Note that the bootstrap with -ftrack-macro-expansion exhibits other
separate issues that are addressed in subsequent patches.  This patch
just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
though.

libcpp/

	* include/line-map.h (linemap_unwind_toward_expansion): Fix typo
	in comment.
	(linemap_unwind_to_first_non_reserved_loc): Declare new function.
	* line-map.c (linemap_unwind_to_first_non_reserved_loc): Define
	new function.

gcc/

	* input.c (expand_location_1): When expanding to spelling location
	in a context of a macro expansion, skip reserved system header
	locations.  Update comments.  * tree-diagnostic.c
	(maybe_unwind_expanded_macro_loc): Likewise.

gcc/testsuite/

	* g++.dg/warn/Wconversion-real-integer2.C: New test.
	* g++.dg/warn/Wconversion-real-integer-3.C: Likewise.
	* g++.dg/warn/conversion-real-integer-3.h: New header used by the
	new test above.
---
 gcc/input.c                                        |   38 ++++++++++++---
 .../g++.dg/warn/Wconversion-real-integer-3.C       |   20 ++++++++
 .../g++.dg/warn/Wconversion-real-integer2.C        |   33 +++++++++++++
 .../g++.dg/warn/conversion-real-integer-3.h        |    3 +
 gcc/tree-diagnostic.c                              |   12 +++++
 libcpp/include/line-map.h                          |   20 ++++++++-
 libcpp/line-map.c                                  |   49 ++++++++++++++++++++
 7 files changed, 167 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wconversion-real-integer-3.C
 create mode 100644 gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
 create mode 100644 gcc/testsuite/g++.dg/warn/conversion-real-integer-3.h

diff --git a/gcc/input.c b/gcc/input.c
index dcd348b..8edf05b 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -35,7 +35,14 @@ struct line_maps *line_table;
    location is set to the string "<built-in>". If EXPANSION_POINT_P is
    TRUE and LOC is virtual, then it is resolved to the expansion
    point of the involved macro.  Otherwise, it is resolved to the
-   spelling location of the token.  */
+   spelling location of the token.
+
+   When resolving to the spelling location of the token, if the
+   resulting location is for a built-in location (that is, it has no
+   associated line/column) in the context of a macro expansion, the
+   returned location is the first one (while unwinding the macro
+   location towards its expansion point) that is in real source
+   code.  */
 
 static expanded_location
 expand_location_1 (source_location loc,
@@ -43,12 +50,29 @@ expand_location_1 (source_location loc,
 {
   expanded_location xloc;
   const struct line_map *map;
-
-  loc = linemap_resolve_location (line_table, loc,
-				  expansion_point_p
-				  ? LRK_MACRO_EXPANSION_POINT
-				  : LRK_SPELLING_LOCATION, &map);
-  xloc = linemap_expand_location (line_table, map, loc);
+  enum location_resolution_kind lrk = LRK_MACRO_EXPANSION_POINT;
+
+  memset (&xloc, 0, sizeof (xloc));
+
+  if (loc >= RESERVED_LOCATION_COUNT)
+    {
+      if (!expansion_point_p)
+	{
+	  /* We want to resolve LOC to its spelling location.
+
+	     But if that spelling location is a reserved location that
+	     appears in the context of a macro expansion (like for a
+	     location for a built-in token), let's consider the first
+	     location (toward the expansion point) that is not reserved;
+	     that is, the first location that is in real source code.  */
+	  loc = linemap_unwind_to_first_non_reserved_loc (line_table,
+							  loc, &map);
+	  lrk = LRK_SPELLING_LOCATION;
+	}
+      loc = linemap_resolve_location (line_table, loc,
+				      lrk, &map);
+      xloc = linemap_expand_location (line_table, map, loc);
+    }
 
   if (loc <= BUILTINS_LOCATION)
     xloc.file = loc == UNKNOWN_LOCATION ? NULL : _("<built-in>");
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer-3.C b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer-3.C
new file mode 100644
index 0000000..a4df010
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer-3.C
@@ -0,0 +1,20 @@
+// { dg-do compile }
+// { dg-options "-Wconversion -ftrack-macro-expansion=2" }
+// { dg-require-effective-target int32plus }
+
+#include "conversion-real-integer-3.h"
+
+float  vfloat;
+
+void h (void)
+{
+    // We want to trigger an error on the token INT_MAX below, that is
+    // a macro that expands to the built-in __INT_MAX__.  Furthermore,
+    // INT_MAX is defined inside a system header.
+    //
+    // The behaviour we want is that the diagnostic should point to
+    // the locus that inside the source code here, at the relevant
+    // line below, even with -ftrack-macro-expansion.  We don't want
+    // it to point to the any locus that is inside the system header.
+    vfloat = INT_MAX; // { dg-warning "conversion to .float. alters .int. constant value" }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
new file mode 100644
index 0000000..29130f1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
@@ -0,0 +1,33 @@
+/* { dg-do compile }
+/* { dg-options "-Wconversion -ftrack-macro-expansion=2" } */
+/* { dg-require-effective-target int32plus } */
+
+// Before the fix that came with this test, we'd output an error for
+// the __INT_MAX__ token.  That token has a BUILTINS_LOCATION
+// location, so the the location prefix in the warning message would
+// be:
+//     <built-in>:0:0: warning: conversion to 'float' alters 'int' constant value
+//
+// Note the useless and confusing <built-in>:0:0 prefix.  This is
+// because '__INT_MAX__' being an internal macro token, it has a
+// BUILTINS_LOCATION location.
+//
+// In this case, we want the error message to refer to the first
+// location (in the macro expansion context) that is not a location
+// for a built-in token.  That location would be the one for where (in
+// real source code) the __INT_MAX__ macro has been expanded.
+//
+// That would be something like:
+//
+//     gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C:21:17: warning: conversion to 'float' alters 'int' constant value
+//
+// That is more useful.
+
+#define INT_MAX __INT_MAX__ // { dg-warning "conversion to .float. alters .int. constant value" }
+
+float  vfloat;
+
+void h (void)
+{
+    vfloat = INT_MAX; // { dg-message "expanded from here" }
+}
diff --git a/gcc/testsuite/g++.dg/warn/conversion-real-integer-3.h b/gcc/testsuite/g++.dg/warn/conversion-real-integer-3.h
new file mode 100644
index 0000000..6ed5b2c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/conversion-real-integer-3.h
@@ -0,0 +1,3 @@
+#pragma GCC system_header
+
+#define INT_MAX __INT_MAX__
diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c
index b4b60dc..064fcec 100644
--- a/gcc/tree-diagnostic.c
+++ b/gcc/tree-diagnostic.c
@@ -168,6 +168,18 @@ maybe_unwind_expanded_macro_loc (diagnostic_context *context,
           linemap_resolve_location (line_table, iter->where,
                                     LRK_MACRO_DEFINITION_LOCATION, NULL);
 
+	/* Don't print trace for locations that are reserved or from
+	   within a system header.  */
+	{
+	  const struct line_map *m = NULL;
+	  source_location l = linemap_resolve_location (line_table, resolved_def_loc,
+							LRK_SPELLING_LOCATION,
+							&m);
+	  if (l < RESERVED_LOCATION_COUNT
+	      || LINEMAP_SYSP (m))
+	    continue;
+	}
+
         /* Resolve the location of the expansion point of the macro
            which expansion gave the token represented by def_loc.
            This is the locus 2/ of the earlier comment.  */
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 4e30742..8496aa1 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -666,12 +666,30 @@ source_location linemap_resolve_location (struct line_maps *,
    location L of the point where M got expanded.  If L is a spelling
    location inside a macro expansion M', then this function returns
    the point where M' was expanded.  LOC_MAP is an output parameter.
-   When non-NULL, *LOC_MAP is set the the map of the returned
+   When non-NULL, *LOC_MAP is set to the map of the returned
    location.  */
 source_location linemap_unwind_toward_expansion (struct line_maps *,
 						 source_location loc,
 						 const struct line_map **loc_map);
 
+/* If LOC is the virtual location of a token coming from the expansion
+   of a macro M and if its spelling location is reserved (e.g, a
+   location for a built-in token), then this function unwinds (using
+   linemap_unwind_toward_expansion) the location until a location that
+   is not reserved and is not in a system header is reached.  In other
+   words, this unwinds the reserved location until a location that is
+   in real source code is reached.
+
+   Otherwise, if the spelling location for LOC is not reserved or if
+   LOC doesn't come from the expansion of a macro, the function
+   returns LOC as is and *MAP is not touched.
+
+   *MAP is set to the map of the returned location if the later is
+   different from LOC.  */
+source_location linemap_unwind_to_first_non_reserved_loc (struct line_maps *,
+							  source_location loc,
+							  const struct line_map **map);
+
 /* Expand source code location LOC and return a user readable source
    code location.  LOC must be a spelling (non-virtual) location.  If
    it's a location < RESERVED_LOCATION_COUNT a zeroed expanded source
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index d7752bb..171aa0f 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -1111,6 +1111,55 @@ linemap_unwind_toward_expansion (struct line_maps *set,
   return resolved_location;
 }
 
+/* If LOC is the virtual location of a token coming from the expansion
+   of a macro M and if its spelling location is reserved (e.g, a
+   location for a built-in token), then this function unwinds (using
+   linemap_unwind_toward_expansion) the location until a location that
+   is not reserved and is not in a sytem header is reached.  In other
+   words, this unwinds the reserved location until a location that is
+   in real source code is reached.
+
+   Otherwise, if the spelling location for LOC is not reserved or if
+   LOC doesn't come from the expansion of a macro, the function
+   returns LOC as is and *MAP is not touched.
+
+   *MAP is set to the map of the returned location if the later is
+   different from LOC.  */
+source_location
+linemap_unwind_to_first_non_reserved_loc (struct line_maps *set,
+					  source_location loc,
+					  const struct line_map **map)
+{
+  source_location resolved_loc;
+  const struct line_map *map0 = NULL, *map1 = NULL;
+
+  map0 = linemap_lookup (set, loc);
+  if (!linemap_macro_expansion_map_p (map0))
+    return loc;
+
+  resolved_loc = linemap_resolve_location (set, loc,
+					   LRK_SPELLING_LOCATION,
+					   &map1);
+
+  if (resolved_loc >= RESERVED_LOCATION_COUNT
+      && !LINEMAP_SYSP (map1))
+    return loc;
+
+  while (linemap_macro_expansion_map_p (map0)
+	 && (resolved_loc < RESERVED_LOCATION_COUNT
+	     || LINEMAP_SYSP (map1)))
+    {
+      loc = linemap_unwind_toward_expansion (set, loc, &map0);
+      resolved_loc = linemap_resolve_location (set, loc,
+					       LRK_SPELLING_LOCATION,
+					       &map1);
+    }
+
+  if (map != NULL)
+    *map = map0;
+  return loc;
+}
+
 /* Expand source code location LOC and return a user readable source
    code location.  LOC must be a spelling (non-virtual) location.  If
    it's a location < RESERVED_LOCATION_COUNT a zeroed expanded source
-- 
		Dodji

Re: [PATCH 06/11] Strip "<built-in>" loc from displayed expansion context

[Jason Merrill]

Good thought.  OK.

Jason

[PATCH 07/11] Fix -Wuninitialized for -ftrack-macro-expansion

[Dodji Seketeli]

Besides the warning emitted by warn_uninit, this function wants
to hint the user at where the uninitialized variable was declared, for
cases where the declaration location is outside the current function.

Now that expand_location expands to the location that is in the main
source file (even for -ftrack-macro-expansion) the hinting part was
not working well for cases where the variable is declared in a macro
(outside the function), which is then expanded in the function.

So I had to adjust warn_uninit a little bit to make it consider the
spelling location of the variable declaration.

I have fixed the test gcc.dg/cpp/pragma-diagnostic-2.c on which I
believe gcc shouldn't emit any error.

Here is the new output on that test:

=~=
gcc.dg/cpp/pragma-diagnostic-2.c: In function 'g':
gcc.dg/cpp/pragma-diagnostic-2.c:10:5: warning: 'a' is used uninitialized in this function [-Wuninitialized]
gcc.dg/cpp/pragma-diagnostic-2.c:9:7: note: 'a' was declared here
gcc.dg/cpp/pragma-diagnostic-2.c:9:7: note: in expansion of macro 'CODE_WITH_WARNING'
gcc.dg/cpp/pragma-diagnostic-2.c:17:3: note: expanded from here
gcc.dg/cpp/pragma-diagnostic-2.c: In function 'h':
gcc.dg/cpp/pragma-diagnostic-2.c:10:5: warning: 'a' is used uninitialized in this function [-Wuninitialized]
gcc.dg/cpp/pragma-diagnostic-2.c:9:7: note: 'a' was declared here
gcc.dg/cpp/pragma-diagnostic-2.c:9:7: note: in expansion of macro 'CODE_WITH_WARNING'
gcc.dg/cpp/pragma-diagnostic-2.c:27:3: note: expanded from here
=~=

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

Note that the bootstrap with -ftrack-macro-expansion turned on
exhibits other separate issues that are addressed in subsequent
patches.  This patch just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
though.

gcc/
	* tree-ssa.c (warn_uninit): Use the spelling location of the
	variable declaration.  Use linemap_location_before_p for source
	locations.

gcc/testsuite/

	* gcc.dg/cpp/pragma-diagnostic-2.c:  Fix this.
---
 gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c |   10 ++--------
 gcc/tree-ssa.c                                 |   15 +++++++++++----
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c
index 7ab95b0..57f3f01 100644
--- a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c
+++ b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c
@@ -6,8 +6,8 @@
 void f (unsigned);
 
 #define CODE_WITH_WARNING \
-  int a; /* { dg-message "expansion|declared here" } */  \
-  f (a)	 /* { dg-message "expansion" } */
+  int a; /* { dg-message "was declared here" } */	 \
+  f (a)	 /* { dg-warning "used uninitialized" } */
 
 #pragma GCC diagnostic ignored "-Wuninitialized"
 
@@ -26,9 +26,3 @@ h (void)
 {
   CODE_WITH_WARNING;		/* { dg-message "expanded" } */
 }
-
-/*
-  { dg-message "some warnings being treated as errors" "" {target *-*-*} 0 }
-*/
-
-/* { dg-error "uninitialized" "" { target *-*-* } { 10 } } */
diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c
index 08f908f..977f0f7 100644
--- a/gcc/tree-ssa.c
+++ b/gcc/tree-ssa.c
@@ -1584,7 +1584,7 @@ warn_uninit (enum opt_code wc, tree t,
 	     tree expr, tree var, const char *gmsgid, void *data)
 {
   gimple context = (gimple) data;
-  location_t location;
+  location_t location, cfun_loc;
   expanded_location xloc, floc;
 
   if (!ssa_undefined_value_p (t))
@@ -1602,8 +1602,12 @@ warn_uninit (enum opt_code wc, tree t,
   location = (context != NULL && gimple_has_location (context))
 	     ? gimple_location (context)
 	     : DECL_SOURCE_LOCATION (var);
+  location = linemap_resolve_location (line_table, location,
+				       LRK_SPELLING_LOCATION,
+				       NULL);
+  cfun_loc = DECL_SOURCE_LOCATION (cfun->decl);
   xloc = expand_location (location);
-  floc = expand_location (DECL_SOURCE_LOCATION (cfun->decl));
+  floc = expand_location (cfun_loc);
   if (warning_at (location, wc, gmsgid, expr))
     {
       TREE_NO_WARNING (expr) = 1;
@@ -1611,8 +1615,11 @@ warn_uninit (enum opt_code wc, tree t,
       if (location == DECL_SOURCE_LOCATION (var))
 	return;
       if (xloc.file != floc.file
-	  || xloc.line < floc.line
-	  || xloc.line > LOCATION_LINE (cfun->function_end_locus))
+	  || linemap_location_before_p (line_table,
+					location, cfun_loc)
+	  || linemap_location_before_p (line_table,
+					cfun->function_end_locus,
+					location))
 	inform (DECL_SOURCE_LOCATION (var), "%qD was declared here", var);
     }
 }
-- 
		Dodji

Re: [PATCH 07/11] Fix -Wuninitialized for -ftrack-macro-expansion

[Jason Merrill]

OK.

Jason

[PATCH 09/11] Fix va_arg type location

[Dodji Seketeli]

Now that diagnostics first point to the spelling location of tokens
coming from macro expansion, the test case
gcc/testsuite/g++.old-deja/g++.other/vaarg3.C shows that when I write
va_args (args, some_type), the location that is recorded for
"some_type" is not correct.  We wrongly record a location that is in
the system header where the va_args macro is defined.

This patch changes that to correctly record the location for the type
operand of the va_arg expression.

With this patch applied, the
gcc/testsuite/g++.old-deja/g++.other/vaarg3.C test PASSes with and
without -ftrack-macro-expansion.

Tested on x86_64-unknown-linux-gnu against trunk.

Note that the bootstrap with -ftrack-macro-expansion exhibits other
separate issues that are addressed in subsequent patches.  This patch
just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
though.

gcc/cp/

	* cp-tree.h (build_x_va_arg): Take an additional location
	parameter.
	* call.c (build_x_va_arg): Take a loc parameter for the location
	of the type of the va_arg expression.
	* parser.c (cp_parser_primary_expression): Pass the type of the
	type in the va_arg expression to build_x_va_arg.
	* pt.c (tsubst_copy): Adjust calls to build_x_va_arg.
---
 gcc/cp/call.c    |    4 ++--
 gcc/cp/cp-tree.h |    2 +-
 gcc/cp/parser.c  |    4 +++-
 gcc/cp/pt.c      |    6 ++++--
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f2ea19b..6a26d6d 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6073,7 +6073,7 @@ convert_arg_to_ellipsis (tree arg)
 /* va_arg (EXPR, TYPE) is a builtin. Make sure it is not abused.  */
 
 tree
-build_x_va_arg (tree expr, tree type)
+build_x_va_arg (source_location loc, tree expr, tree type)
 {
   if (processing_template_decl)
     return build_min (VA_ARG_EXPR, type, expr);
@@ -6099,7 +6099,7 @@ build_x_va_arg (tree expr, tree type)
       return expr;
     }
 
-  return build_va_arg (input_location, expr, type);
+  return build_va_arg (loc, expr, type);
 }
 
 /* TYPE has been given to va_arg.  Apply the default conversions which
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index db5e8a5..9dd7510 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -4886,7 +4886,7 @@ extern void push_defarg_context			(tree);
 extern void pop_defarg_context			(void);
 extern tree convert_default_arg			(tree, tree, tree, int);
 extern tree convert_arg_to_ellipsis		(tree);
-extern tree build_x_va_arg			(tree, tree);
+extern tree build_x_va_arg			(source_location, tree, tree);
 extern tree cxx_type_promotes_to		(tree);
 extern tree type_passed_as			(tree);
 extern tree convert_for_arg_passing		(tree, tree);
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index eac60f1..12cfdc1 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -4168,6 +4168,7 @@ cp_parser_primary_expression (cp_parser *parser,
 	  {
 	    tree expression;
 	    tree type;
+	    source_location type_location;
 
 	    /* The `__builtin_va_arg' construct is used to handle
 	       `va_arg'.  Consume the `__builtin_va_arg' token.  */
@@ -4179,6 +4180,7 @@ cp_parser_primary_expression (cp_parser *parser,
 							  /*cast_p=*/false, NULL);
 	    /* Look for the `,'.  */
 	    cp_parser_require (parser, CPP_COMMA, RT_COMMA);
+	    type_location = cp_lexer_peek_token (parser->lexer)->location;
 	    /* Parse the type-id.  */
 	    type = cp_parser_type_id (parser);
 	    /* Look for the closing `)'.  */
@@ -4188,7 +4190,7 @@ cp_parser_primary_expression (cp_parser *parser,
 	    if (cp_parser_non_integral_constant_expression (parser,
 							    NIC_VA_ARG))
 	      return error_mark_node;
-	    return build_x_va_arg (expression, type);
+	    return build_x_va_arg (type_location, expression, type);
 	  }
 
 	case RID_OFFSETOF:
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index ee38254..0c3f7c0 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -12440,7 +12440,8 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
       gcc_unreachable ();
 
     case VA_ARG_EXPR:
-      return build_x_va_arg (tsubst_copy (TREE_OPERAND (t, 0), args, complain,
+      return build_x_va_arg (EXPR_LOCATION (t),
+			     tsubst_copy (TREE_OPERAND (t, 0), args, complain,
 					  in_decl),
 			     tsubst (TREE_TYPE (t), args, complain, in_decl));
 
@@ -14273,7 +14274,8 @@ tsubst_copy_and_build (tree t,
       }
 
     case VA_ARG_EXPR:
-      return build_x_va_arg (RECUR (TREE_OPERAND (t, 0)),
+      return build_x_va_arg (EXPR_LOCATION (t),
+			     RECUR (TREE_OPERAND (t, 0)),
 			     tsubst (TREE_TYPE (t), args, complain, in_decl));
 
     case OFFSETOF_EXPR:
-- 
		Dodji

Re: [PATCH 09/11] Fix va_arg type location

[Gabriel Dos Reis]

On Wed, Apr 11, 2012 at 3:59 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Now that diagnostics first point to the spelling location of tokens
> coming from macro expansion, the test case
> gcc/testsuite/g++.old-deja/g++.other/vaarg3.C shows that when I write
> va_args (args, some_type), the location that is recorded for
> "some_type" is not correct.  We wrongly record a location that is in
> the system header where the va_args macro is defined.
>
> This patch changes that to correctly record the location for the type
> operand of the va_arg expression.
>
> With this patch applied, the
> gcc/testsuite/g++.old-deja/g++.other/vaarg3.C test PASSes with and
> without -ftrack-macro-expansion.
>
> Tested on x86_64-unknown-linux-gnu against trunk.
>
> Note that the bootstrap with -ftrack-macro-expansion exhibits other
> separate issues that are addressed in subsequent patches.  This patch
> just fixes one class of problems.
>
> The patch does pass bootstrap with -ftrack-macro-expansion turned off,
> though.

OK.

[PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion

[Dodji Seketeli]

There are various conversion related warnings that trigger on
potentially dangerous uses of NULL (or __null).  NULL is defined as a
macro in a system header, so calling warning or warning_at on a virtual
location of NULL yields no diagnostic.  So the test accompanying this
patch (as well as others), was failling when run with
-ftrack-macro-expansion.

I think it's necessary to use the location of NULL that is in the main
source code (instead of, e.g, the spelling location that is in the
system header where the macro is defined) in those cases.  Note that for
__null, we don't have the issue.

I have augmented the test of this patch to check that we don't regress
when handling __null.

Tested on x86_64-unknown-linux-gnu against trunk.

Note that the bootstrap with -ftrack-macro-expansion exhibits other
separate issues that are addressed in subsequent patches.  This patch
just fixes one class of problems.

The patch does pass bootstrap with -ftrack-macro-expansion turned off,
though.

gcc/cp/

	* call.c (conversion_null_warnings): When on NULL, resolve
	location to the point in the main source code, rather than to the
	spelling location.
	* cvt.c (build_expr_type_conversion): Likewise.
	* typeck.c (cp_build_binary_op): Likewise.

gcc/testsuite/

	* g++.dg/warn/Wconversion-null-2.C: Add testing for __null,
	alongside the previous testing for NULL.
---
 gcc/cp/call.c                                  |   18 ++++++++++++-
 gcc/cp/cvt.c                                   |   18 ++++++++++++-
 gcc/cp/typeck.c                                |   20 +++++++++++++--
 gcc/testsuite/g++.dg/warn/Wconversion-null-2.C |   31 +++++++++++++++++++++++-
 4 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 3c3dabb..f2ea19b 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5538,12 +5538,26 @@ conversion_null_warnings (tree totype, tree expr, tree fn, int argnum)
   if (expr == null_node && TREE_CODE (totype) != BOOLEAN_TYPE
       && ARITHMETIC_TYPE_P (totype))
     {
+      source_location loc = input_location;
+      /* The location of the warning here is most certainly the one for
+	 the token that represented null_node in the source code.
+	 That is either NULL or __null.  If it is NULL, then that
+	 macro is defined in a system header and, as a consequence,
+	 warning_at won't emit any diagnostic for it.  In this case,
+	 we are going to resolve that location to the point in the
+	 source code where the expression that triggered this error
+	 message is, rather than the point where the NULL macro is
+	 defined.  */
+      if (in_system_header_at (input_location))
+	loc = linemap_resolve_location (line_table, loc,
+					LRK_MACRO_EXPANSION_POINT,
+					NULL);
       if (fn)
-	warning_at (input_location, OPT_Wconversion_null,
+	warning_at (loc, OPT_Wconversion_null,
 		    "passing NULL to non-pointer argument %P of %qD",
 		    argnum, fn);
       else
-	warning_at (input_location, OPT_Wconversion_null,
+	warning_at (loc, OPT_Wconversion_null,
 		    "converting to non-pointer type %qT from NULL", totype);
     }
 
diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
index c411a47..73bdf71 100644
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -1468,8 +1468,22 @@ build_expr_type_conversion (int desires, tree expr, bool complain)
   if (expr == null_node
       && (desires & WANT_INT)
       && !(desires & WANT_NULL))
-    warning_at (input_location, OPT_Wconversion_null,
-		"converting NULL to non-pointer type");
+    {
+      source_location loc = input_location;
+      /* The location of the warning here is the one for the
+	 (expansion of the) NULL macro, or for __null.  If it is for
+	 NULL, then, as that that macro is defined in a system header,
+	 warning_at won't emit any diagnostic.  In this case, we are
+	 going to resolve that location to the point in the source
+	 code where the expression that triggered this warning is,
+	 rather than the point where the NULL macro is defined.  */
+      if (in_system_header_at (loc))
+	loc = linemap_resolve_location (line_table, loc,
+					LRK_MACRO_EXPANSION_POINT,
+					NULL);
+      warning_at (loc, OPT_Wconversion_null,
+		  "converting NULL to non-pointer type");
+    }
 
   basetype = TREE_TYPE (expr);
 
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index d2ed940..d096f1e 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -3798,9 +3798,23 @@ cp_build_binary_op (location_t location,
 	  || (!null_ptr_cst_p (orig_op1) 
 	      && !TYPE_PTR_P (type1) && !TYPE_PTR_TO_MEMBER_P (type1)))
       && (complain & tf_warning))
-    /* Some sort of arithmetic operation involving NULL was
-       performed.  */
-    warning (OPT_Wpointer_arith, "NULL used in arithmetic");
+    {
+      source_location loc = input_location;
+      /* Some sort of arithmetic operation involving NULL was
+	 performed.  The location of the warning here is the one for
+	 the (expansion of the) NULL macro, or for __null.  If it is
+	 for NULL, then, as that that macro is defined in a system
+	 header, warning_at won't emit any diagnostic.  In this case,
+	 we are going to resolve that location to the point in the
+	 source code where the expression that triggered this warning
+	 is, rather than the point where the NULL macro is
+	 defined.  */
+      if (in_system_header_at (loc))
+	loc = linemap_resolve_location (line_table, loc,
+					LRK_MACRO_EXPANSION_POINT,
+					NULL);
+      warning_at (loc, OPT_Wpointer_arith, "NULL used in arithmetic");
+    }
 
   switch (code)
     {
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C b/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
index dd498c1..a71551f 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
@@ -25,7 +25,7 @@ void l(long) {}
 template <>
 void l(long long) {}
 
-int main()
+void warn_for_NULL()
 {
   int i = NULL; // { dg-warning "" } converting NULL to non-pointer type
   float z = NULL; // { dg-warning "" } converting NULL to non-pointer type
@@ -47,3 +47,32 @@ int main()
   l(NULL);   // No warning: NULL is used to implicitly instantiate the template
   NULL && NULL; // No warning: converting NULL to bool is OK
 }
+
+int warn_for___null()
+{
+  int i = __null; // { dg-warning "" } converting __null to non-pointer type
+  float z = __null; // { dg-warning "" } converting __null to non-pointer type
+  int a[2];
+
+  i != __null; // { dg-warning "" } __null used in arithmetic
+  __null != z; // { dg-warning "" } __null used in arithmetic
+  k != __null; // No warning: decay conversion
+  __null != a; // Likewise.
+  -__null;     // { dg-warning "" } converting __null to non-pointer type
+  +__null;     // { dg-warning "" } converting __null to non-pointer type
+  ~__null;     // { dg-warning "" } converting __null to non-pointer type
+  a[__null] = 3; // { dg-warning "" } converting __null to non-pointer-type
+  i = __null;  // { dg-warning "" } converting __null to non-pointer type
+  z = __null;  // { dg-warning "" } converting __null to non-pointer type
+  k(__null);   // { dg-warning "" } converting __null to int
+  g(__null);   // { dg-warning "" } converting __null to int
+  h<__null>(); // No warning: __null bound to integer template parameter
+  l(__null);   // No warning: __null is used to implicitly instantiate the template
+  __null && __null; // No warning: converting NULL to bool is OK
+}
+
+int main()
+{
+  warn_for_NULL();
+  warn_for___null();
+}
-- 
		Dodji

Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion

[Gabriel Dos Reis]

On Wed, Apr 11, 2012 at 3:46 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> There are various conversion related warnings that trigger on
> potentially dangerous uses of NULL (or __null).  NULL is defined as a
> macro in a system header, so calling warning or warning_at on a virtual
> location of NULL yields no diagnostic.  So the test accompanying this
> patch (as well as others), was failling when run with
> -ftrack-macro-expansion.
>
> I think it's necessary to use the location of NULL that is in the main
> source code (instead of, e.g, the spelling location that is in the
> system header where the macro is defined) in those cases.  Note that for
> __null, we don't have the issue.

I like the idea.  However, you should write a separate function
(get_null_ptr_cst_location?) that computes the location of the null
pointer constant token and call it from where you need the location.

> @@ -5538,12 +5538,26 @@ conversion_null_warnings (tree totype, tree expr, tree fn, int argnum)
>   if (expr == null_node && TREE_CODE (totype) != BOOLEAN_TYPE
>       && ARITHMETIC_TYPE_P (totype))
>     {
> +      source_location loc = input_location;
> +      /* The location of the warning here is most certainly the one for
> +        the token that represented null_node in the source code.
> +        That is either NULL or __null.  If it is NULL, then that
> +        macro is defined in a system header and, as a consequence,
> +        warning_at won't emit any diagnostic for it.  In this case,
> +        we are going to resolve that location to the point in the
> +        source code where the expression that triggered this error
> +        message is, rather than the point where the NULL macro is
> +        defined.  */
> +      if (in_system_header_at (input_location))
> +       loc = linemap_resolve_location (line_table, loc,
> +                                       LRK_MACRO_EXPANSION_POINT,
> +                                       NULL);
>       if (fn)
> -       warning_at (input_location, OPT_Wconversion_null,
> +       warning_at (loc, OPT_Wconversion_null,
>                    "passing NULL to non-pointer argument %P of %qD",
>                    argnum, fn);
>       else
> -       warning_at (input_location, OPT_Wconversion_null,
> +       warning_at (loc, OPT_Wconversion_null,
>                    "converting to non-pointer type %qT from NULL", totype);
>     }
>
> diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
> index c411a47..73bdf71 100644
> --- a/gcc/cp/cvt.c
> +++ b/gcc/cp/cvt.c
> @@ -1468,8 +1468,22 @@ build_expr_type_conversion (int desires, tree expr, bool complain)
>   if (expr == null_node
>       && (desires & WANT_INT)
>       && !(desires & WANT_NULL))
> -    warning_at (input_location, OPT_Wconversion_null,
> -               "converting NULL to non-pointer type");
> +    {
> +      source_location loc = input_location;
> +      /* The location of the warning here is the one for the
> +        (expansion of the) NULL macro, or for __null.  If it is for
> +        NULL, then, as that that macro is defined in a system header,
> +        warning_at won't emit any diagnostic.  In this case, we are
> +        going to resolve that location to the point in the source
> +        code where the expression that triggered this warning is,
> +        rather than the point where the NULL macro is defined.  */
> +      if (in_system_header_at (loc))
> +       loc = linemap_resolve_location (line_table, loc,
> +                                       LRK_MACRO_EXPANSION_POINT,
> +                                       NULL);
> +      warning_at (loc, OPT_Wconversion_null,
> +                 "converting NULL to non-pointer type");
> +    }
>
>   basetype = TREE_TYPE (expr);
>
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index d2ed940..d096f1e 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -3798,9 +3798,23 @@ cp_build_binary_op (location_t location,
>          || (!null_ptr_cst_p (orig_op1)
>              && !TYPE_PTR_P (type1) && !TYPE_PTR_TO_MEMBER_P (type1)))
>       && (complain & tf_warning))
> -    /* Some sort of arithmetic operation involving NULL was
> -       performed.  */
> -    warning (OPT_Wpointer_arith, "NULL used in arithmetic");
> +    {
> +      source_location loc = input_location;
> +      /* Some sort of arithmetic operation involving NULL was
> +        performed.  The location of the warning here is the one for
> +        the (expansion of the) NULL macro, or for __null.  If it is
> +        for NULL, then, as that that macro is defined in a system
> +        header, warning_at won't emit any diagnostic.  In this case,
> +        we are going to resolve that location to the point in the
> +        source code where the expression that triggered this warning
> +        is, rather than the point where the NULL macro is
> +        defined.  */
> +      if (in_system_header_at (loc))
> +       loc = linemap_resolve_location (line_table, loc,
> +                                       LRK_MACRO_EXPANSION_POINT,
> +                                       NULL);
> +      warning_at (loc, OPT_Wpointer_arith, "NULL used in arithmetic");
> +    }

Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion

[Dodji Seketeli]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> On Wed, Apr 11, 2012 at 3:46 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>> There are various conversion related warnings that trigger on
>> potentially dangerous uses of NULL (or __null).  NULL is defined as a
>> macro in a system header, so calling warning or warning_at on a virtual
>> location of NULL yields no diagnostic.  So the test accompanying this
>> patch (as well as others), was failling when run with
>> -ftrack-macro-expansion.
>>
>> I think it's necessary to use the location of NULL that is in the main
>> source code (instead of, e.g, the spelling location that is in the
>> system header where the macro is defined) in those cases.  Note that for
>> __null, we don't have the issue.
>
> I like the idea.  However, you should write a separate function
> (get_null_ptr_cst_location?) that computes the location of the null
> pointer constant token and call it from where you need the location.

OK.  I have introduced such a new function and I gave it the slightly
more generic name expansion_point_location_if_in_system_header as I
suspect it can be useful for macros that are similar to NULL.  I haven't
spotted such macros (from test regressions) yet, though.

Here is the updated patch that passes bootstrap with
-ftrack-macro-expansion turned off; it also passes bootstrap with
-ftrack-macro-expansion turned on with the whole series of patches I
have locally on my tree.

gcc/
	* input.h (expansion_point_location_if_in_system_header): Declare
	new function.
	* input.c (expansion_point_location_if_in_system_header): Define it.
gcc/cp/

	* call.c (conversion_null_warnings): Use the new
	expansion_point_location_if_in_system_header.
	* cvt.c (build_expr_type_conversion): Likewise.
	* typeck.c (cp_build_binary_op): Likewise.

gcc/testsuite/

	* g++.dg/warn/Wconversion-null-2.C: Add testing for __null,
	alongside the previous testing for NULL.
---
 gcc/cp/call.c                                  |   16 ++++++++++-
 gcc/cp/cvt.c                                   |   16 ++++++++++-
 gcc/cp/typeck.c                                |   18 +++++++++++--
 gcc/input.c                                    |   14 ++++++++++
 gcc/input.h                                    |    1 +
 gcc/testsuite/g++.dg/warn/Wconversion-null-2.C |   31 +++++++++++++++++++++++-
 6 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f9a7f08..1dc57fc 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5598,12 +5598,24 @@ conversion_null_warnings (tree totype, tree expr, tree fn, int argnum)
   if (expr == null_node && TREE_CODE (totype) != BOOLEAN_TYPE
       && ARITHMETIC_TYPE_P (totype))
     {
+      /* The location of the warning here is most certainly the one for
+	 the token that represented null_node in the source code.
+	 That is either NULL or __null.  If it is NULL, then that
+	 macro is defined in a system header and, as a consequence,
+	 warning_at won't emit any diagnostic for it.  In this case,
+	 we are going to resolve that location to the point in the
+	 source code where the expression that triggered this error
+	 message is, rather than the point where the NULL macro is
+	 defined.  */
+      source_location loc =
+	expansion_point_location_if_in_system_header (input_location);
+
       if (fn)
-	warning_at (input_location, OPT_Wconversion_null,
+	warning_at (loc, OPT_Wconversion_null,
 		    "passing NULL to non-pointer argument %P of %qD",
 		    argnum, fn);
       else
-	warning_at (input_location, OPT_Wconversion_null,
+	warning_at (loc, OPT_Wconversion_null,
 		    "converting to non-pointer type %qT from NULL", totype);
     }
 
diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
index 3dab372..8defc61 100644
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -1472,8 +1472,20 @@ build_expr_type_conversion (int desires, tree expr, bool complain)
   if (expr == null_node
       && (desires & WANT_INT)
       && !(desires & WANT_NULL))
-    warning_at (input_location, OPT_Wconversion_null,
-		"converting NULL to non-pointer type");
+    {
+      /* The location of the warning here is the one for the
+	 (expansion of the) NULL macro, or for __null.  If it is for
+	 NULL, then, as that that macro is defined in a system header,
+	 warning_at won't emit any diagnostic.  In this case, we are
+	 going to resolve that location to the point in the source
+	 code where the expression that triggered this warning is,
+	 rather than the point where the NULL macro is defined.  */
+      source_location loc =
+	expansion_point_location_if_in_system_header (input_location);
+
+      warning_at (loc, OPT_Wconversion_null,
+		  "converting NULL to non-pointer type");
+    }
 
   basetype = TREE_TYPE (expr);
 
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index fb2f1bc..f453536 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -3838,9 +3838,21 @@ cp_build_binary_op (location_t location,
 	  || (!null_ptr_cst_p (orig_op1) 
 	      && !TYPE_PTR_P (type1) && !TYPE_PTR_TO_MEMBER_P (type1)))
       && (complain & tf_warning))
-    /* Some sort of arithmetic operation involving NULL was
-       performed.  */
-    warning (OPT_Wpointer_arith, "NULL used in arithmetic");
+    {
+      /* Some sort of arithmetic operation involving NULL was
+	 performed.  The location of the warning here is the one for
+	 the (expansion of the) NULL macro, or for __null.  If it is
+	 for NULL, then, as that that macro is defined in a system
+	 header, warning_at won't emit any diagnostic.  In this case,
+	 we are going to resolve that location to the point in the
+	 source code where the expression that triggered this warning
+	 is, rather than the point where the NULL macro is
+	 defined.  */
+      source_location loc =
+	expansion_point_location_if_in_system_header (input_location);
+
+      warning_at (loc, OPT_Wpointer_arith, "NULL used in arithmetic");
+    }
 
   switch (code)
     {
diff --git a/gcc/input.c b/gcc/input.c
index 260be7e..75457a3 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -162,6 +162,20 @@ expand_location_to_spelling_point (source_location loc)
   return expand_location_1 (loc, /*expansion_piont_p=*/false);
 }
 
+/* If LOCATION is in a sytem header and if it's a virtual location for
+   a token coming from the expansion of a macro M, unwind it to the
+   location of the expansion point of M.  Otherwise, just return
+   LOCATION.  */
+
+source_location
+expansion_point_location_if_in_system_header (source_location location)
+{
+  if (in_system_header_at (location))
+    location = linemap_resolve_location (line_table, location,
+					 LRK_MACRO_EXPANSION_POINT,
+					 NULL);
+  return location;
+}
 
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
diff --git a/gcc/input.h b/gcc/input.h
index f755cdf..f588838 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -40,6 +40,7 @@ extern char builtins_location_check[(BUILTINS_LOCATION
 extern expanded_location expand_location (source_location);
 extern const char * location_get_source_line(expanded_location xloc);
 extern expanded_location expand_location_to_spelling_point (source_location);
+extern source_location expansion_point_location_if_in_system_header (source_location);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C b/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
index dd498c1..a71551f 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
@@ -25,7 +25,7 @@ void l(long) {}
 template <>
 void l(long long) {}
 
-int main()
+void warn_for_NULL()
 {
   int i = NULL; // { dg-warning "" } converting NULL to non-pointer type
   float z = NULL; // { dg-warning "" } converting NULL to non-pointer type
@@ -47,3 +47,32 @@ int main()
   l(NULL);   // No warning: NULL is used to implicitly instantiate the template
   NULL && NULL; // No warning: converting NULL to bool is OK
 }
+
+int warn_for___null()
+{
+  int i = __null; // { dg-warning "" } converting __null to non-pointer type
+  float z = __null; // { dg-warning "" } converting __null to non-pointer type
+  int a[2];
+
+  i != __null; // { dg-warning "" } __null used in arithmetic
+  __null != z; // { dg-warning "" } __null used in arithmetic
+  k != __null; // No warning: decay conversion
+  __null != a; // Likewise.
+  -__null;     // { dg-warning "" } converting __null to non-pointer type
+  +__null;     // { dg-warning "" } converting __null to non-pointer type
+  ~__null;     // { dg-warning "" } converting __null to non-pointer type
+  a[__null] = 3; // { dg-warning "" } converting __null to non-pointer-type
+  i = __null;  // { dg-warning "" } converting __null to non-pointer type
+  z = __null;  // { dg-warning "" } converting __null to non-pointer type
+  k(__null);   // { dg-warning "" } converting __null to int
+  g(__null);   // { dg-warning "" } converting __null to int
+  h<__null>(); // No warning: __null bound to integer template parameter
+  l(__null);   // No warning: __null is used to implicitly instantiate the template
+  __null && __null; // No warning: converting NULL to bool is OK
+}
+
+int main()
+{
+  warn_for_NULL();
+  warn_for___null();
+}
-- 
1.7.6.5


-- 
		Dodji

Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion

[Gabriel Dos Reis]

On Wed, Apr 25, 2012 at 8:42 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
>
>> On Wed, Apr 11, 2012 at 3:46 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>> There are various conversion related warnings that trigger on
>>> potentially dangerous uses of NULL (or __null).  NULL is defined as a
>>> macro in a system header, so calling warning or warning_at on a virtual
>>> location of NULL yields no diagnostic.  So the test accompanying this
>>> patch (as well as others), was failling when run with
>>> -ftrack-macro-expansion.
>>>
>>> I think it's necessary to use the location of NULL that is in the main
>>> source code (instead of, e.g, the spelling location that is in the
>>> system header where the macro is defined) in those cases.  Note that for
>>> __null, we don't have the issue.
>>
>> I like the idea.  However, you should write a separate function
>> (get_null_ptr_cst_location?) that computes the location of the null
>> pointer constant token and call it from where you need the location.
>
> OK.  I have introduced such a new function and I gave it the slightly
> more generic name expansion_point_location_if_in_system_header as I
> suspect it can be useful for macros that are similar to NULL.  I haven't
> spotted such macros (from test regressions) yet, though.

Thanks.  But a point of the suggestion was that we won't need the
same comment/explanation duplicated over at least 3 places.  Just
one.  All those three places deal exactly with one instance: null
pointer constant.  That deserves a function in and of itself, which
is documented by the duplicated comments.
Please make that change.  Everything else is OK.  thanks.

Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion

[Dodji Seketeli]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> Thanks.  But a point of the suggestion was that we won't need the
> same comment/explanation duplicated over at least 3 places.  Just
> one.  All those three places deal exactly with one instance: null
> pointer constant.  That deserves a function in and of itself, which
> is documented by the duplicated comments.
> Please make that change.  Everything else is OK.  thanks.

I am sorry for the round trips.  Please find below a patch udpated
accordingly.

I am bootstrapping the whole patch set, but the impacted files of this
patch have built fine so far.

Thanks.

gcc/
	* input.h (expansion_point_location_if_in_system_header): Declare
	new function.
	* input.c (expansion_point_location_if_in_system_header): Define it.
gcc/cp/

	* call.c (conversion_null_warnings): Use the new
	expansion_point_location_if_in_system_header.
	* cvt.c (build_expr_type_conversion): Likewise.
	* typeck.c (cp_build_binary_op): Likewise.

gcc/testsuite/

	* g++.dg/warn/Wconversion-null-2.C: Add testing for __null,
	alongside the previous testing for NULL.
---
 gcc/cp/call.c                                  |    7 ++++-
 gcc/cp/cvt.c                                   |    9 +++++-
 gcc/cp/typeck.c                                |    9 ++++--
 gcc/input.c                                    |   20 +++++++++++++++
 gcc/input.h                                    |    1 +
 gcc/testsuite/g++.dg/warn/Wconversion-null-2.C |   31 +++++++++++++++++++++++-
 6 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f9a7f08..85e45c2 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5598,12 +5598,15 @@ conversion_null_warnings (tree totype, tree expr, tree fn, int argnum)
   if (expr == null_node && TREE_CODE (totype) != BOOLEAN_TYPE
       && ARITHMETIC_TYPE_P (totype))
     {
+      source_location loc =
+	expansion_point_location_if_in_system_header (input_location);
+
       if (fn)
-	warning_at (input_location, OPT_Wconversion_null,
+	warning_at (loc, OPT_Wconversion_null,
 		    "passing NULL to non-pointer argument %P of %qD",
 		    argnum, fn);
       else
-	warning_at (input_location, OPT_Wconversion_null,
+	warning_at (loc, OPT_Wconversion_null,
 		    "converting to non-pointer type %qT from NULL", totype);
     }
 
diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
index 3dab372..49ba38a 100644
--- a/gcc/cp/cvt.c
+++ b/gcc/cp/cvt.c
@@ -1472,8 +1472,13 @@ build_expr_type_conversion (int desires, tree expr, bool complain)
   if (expr == null_node
       && (desires & WANT_INT)
       && !(desires & WANT_NULL))
-    warning_at (input_location, OPT_Wconversion_null,
-		"converting NULL to non-pointer type");
+    {
+      source_location loc =
+	expansion_point_location_if_in_system_header (input_location);
+
+      warning_at (loc, OPT_Wconversion_null,
+		  "converting NULL to non-pointer type");
+    }
 
   basetype = TREE_TYPE (expr);
 
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index fb2f1bc..52d264b 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -3838,9 +3838,12 @@ cp_build_binary_op (location_t location,
 	  || (!null_ptr_cst_p (orig_op1) 
 	      && !TYPE_PTR_P (type1) && !TYPE_PTR_TO_MEMBER_P (type1)))
       && (complain & tf_warning))
-    /* Some sort of arithmetic operation involving NULL was
-       performed.  */
-    warning (OPT_Wpointer_arith, "NULL used in arithmetic");
+    {
+      source_location loc =
+	expansion_point_location_if_in_system_header (input_location);
+
+      warning_at (loc, OPT_Wpointer_arith, "NULL used in arithmetic");
+    }
 
   switch (code)
     {
diff --git a/gcc/input.c b/gcc/input.c
index 260be7e..5f14489 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -162,6 +162,26 @@ expand_location_to_spelling_point (source_location loc)
   return expand_location_1 (loc, /*expansion_piont_p=*/false);
 }
 
+/* If LOCATION is in a sytem header and if it's a virtual location for
+   a token coming from the expansion of a macro M, unwind it to the
+   location of the expansion point of M.  Otherwise, just return
+   LOCATION.
+
+   This is used for instance when we want to emit diagnostics about a
+   token that is located in a macro that is itself defined in a system
+   header -- e.g for the NULL macro.  In that case, if LOCATION is
+   passed to diagnostics emitting functions like warning_at as is, no
+   diagnostic won't be emitted.  */
+
+source_location
+expansion_point_location_if_in_system_header (source_location location)
+{
+  if (in_system_header_at (location))
+    location = linemap_resolve_location (line_table, location,
+					 LRK_MACRO_EXPANSION_POINT,
+					 NULL);
+  return location;
+}
 
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
diff --git a/gcc/input.h b/gcc/input.h
index f755cdf..f588838 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -40,6 +40,7 @@ extern char builtins_location_check[(BUILTINS_LOCATION
 extern expanded_location expand_location (source_location);
 extern const char * location_get_source_line(expanded_location xloc);
 extern expanded_location expand_location_to_spelling_point (source_location);
+extern source_location expansion_point_location_if_in_system_header (source_location);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C b/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
index dd498c1..a71551f 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-null-2.C
@@ -25,7 +25,7 @@ void l(long) {}
 template <>
 void l(long long) {}
 
-int main()
+void warn_for_NULL()
 {
   int i = NULL; // { dg-warning "" } converting NULL to non-pointer type
   float z = NULL; // { dg-warning "" } converting NULL to non-pointer type
@@ -47,3 +47,32 @@ int main()
   l(NULL);   // No warning: NULL is used to implicitly instantiate the template
   NULL && NULL; // No warning: converting NULL to bool is OK
 }
+
+int warn_for___null()
+{
+  int i = __null; // { dg-warning "" } converting __null to non-pointer type
+  float z = __null; // { dg-warning "" } converting __null to non-pointer type
+  int a[2];
+
+  i != __null; // { dg-warning "" } __null used in arithmetic
+  __null != z; // { dg-warning "" } __null used in arithmetic
+  k != __null; // No warning: decay conversion
+  __null != a; // Likewise.
+  -__null;     // { dg-warning "" } converting __null to non-pointer type
+  +__null;     // { dg-warning "" } converting __null to non-pointer type
+  ~__null;     // { dg-warning "" } converting __null to non-pointer type
+  a[__null] = 3; // { dg-warning "" } converting __null to non-pointer-type
+  i = __null;  // { dg-warning "" } converting __null to non-pointer type
+  z = __null;  // { dg-warning "" } converting __null to non-pointer type
+  k(__null);   // { dg-warning "" } converting __null to int
+  g(__null);   // { dg-warning "" } converting __null to int
+  h<__null>(); // No warning: __null bound to integer template parameter
+  l(__null);   // No warning: __null is used to implicitly instantiate the template
+  __null && __null; // No warning: converting NULL to bool is OK
+}
+
+int main()
+{
+  warn_for_NULL();
+  warn_for___null();
+}
-- 
		Dodji

Re: [PATCH 08/11] Make conversion warnings work on NULL with -ftrack-macro-expansion

[Gabriel Dos Reis]

OK, thanks!

-- Gaby

[PATCH 10/13] Fix location for static class members

[Dodji Seketeli]

Consider the test case g++.dg/other/offsetof5.C:

    #include <stddef.h>

    struct A
    {
      char c;
      int &i;
    };

    int j = offsetof (A, i);		// { dg-warning "invalid access|offsetof" }

    template <typename T>
    struct S
    {
      T h;
      T &i;
      static const int j = offsetof (S, i);	// { dg-warning "invalid access|offsetof" }
    };

    int k = S<int>::j;			// { dg-message "required from here" }

The second warning (that involves the instantiation of the S template)
is not emitted when -ftrack-macro-expansion is on.

This is because during the instantiation of the member j of S
template, the location that is used for the warning is the one for the
DECL j (set by instantiate_decl).  And that location is inaccurately
set to the locus of 'offsetof', which is a macro defined in a system
header, so it's discarded by the diagnostics machinery.

Note that when we reach the point where we emit the warning in
build_class_member_access_expr offsetof expression has long been
folded, so we cannot use e.g, the location of the ')' token that would
have been in the source code.  So I believe the location of 'j' is the
best we can get at this point.

The patch below sets the location of the DECL for 'j' to what I
believe is its precise location; with that, the test case passes with
and without -ftrack-macro-expansion.  But I had to adjust
g++.dg/template/sfinae6_neg.C for that.

Tested on x86_64-unknown-linux-gnu against trunk.

gcc/cp

	* decl.c (grokdeclarator): Use the location carried by the
	declarator for the DECL of the static class member.

gcc/testsuite/

	* g++.dg/template/sfinae6_neg.C: Adjust.
---
 gcc/cp/decl.c                               |    3 ++-
 gcc/testsuite/g++.dg/template/sfinae6_neg.C |    4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 28c7cee..40818a3 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -10267,7 +10267,8 @@ grokdeclarator (const cp_declarator *declarator,
 	      {
 		/* C++ allows static class members.  All other work
 		   for this is done by grokfield.  */
-		decl = build_lang_decl (VAR_DECL, unqualified_id, type);
+		decl = build_lang_decl_loc (declarator->id_loc,
+					    VAR_DECL, unqualified_id, type);
 		set_linkage_for_static_data_member (decl);
 		/* Even if there is an in-class initialization, DECL
 		   is considered undefined until an out-of-class
diff --git a/gcc/testsuite/g++.dg/template/sfinae6_neg.C b/gcc/testsuite/g++.dg/template/sfinae6_neg.C
index d4be5dd..9b7bdfd1 100644
--- a/gcc/testsuite/g++.dg/template/sfinae6_neg.C
+++ b/gcc/testsuite/g++.dg/template/sfinae6_neg.C
@@ -21,9 +21,9 @@ no_type check_is_callable2(...);
 template<typename F, typename T1, typename T2 = T1>
 struct is_callable2
 {
-  static const bool value = 
+  static const bool value = // { dg-error "within this context" }
     (sizeof(check_is_callable2(type<F>(), type<T1>(), type<T2>()))
-     == sizeof(yes_type)); // { dg-error "within this context" }
+     == sizeof(yes_type));
 };
 
 #define JOIN( X, Y ) DO_JOIN( X, Y )
-- 
		Dodji

Re: [PATCH 10/13] Fix location for static class members

[Gabriel Dos Reis]

On Wed, Apr 25, 2012 at 8:55 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Consider the test case g++.dg/other/offsetof5.C:
>
>    #include <stddef.h>
>
>    struct A
>    {
>      char c;
>      int &i;
>    };
>
>    int j = offsetof (A, i);            // { dg-warning "invalid access|offsetof" }
>
>    template <typename T>
>    struct S
>    {
>      T h;
>      T &i;
>      static const int j = offsetof (S, i);     // { dg-warning "invalid access|offsetof" }
>    };
>
>    int k = S<int>::j;                  // { dg-message "required from here" }
>
> The second warning (that involves the instantiation of the S template)
> is not emitted when -ftrack-macro-expansion is on.
>
> This is because during the instantiation of the member j of S
> template, the location that is used for the warning is the one for the
> DECL j (set by instantiate_decl).  And that location is inaccurately
> set to the locus of 'offsetof', which is a macro defined in a system
> header, so it's discarded by the diagnostics machinery.
>
> Note that when we reach the point where we emit the warning in
> build_class_member_access_expr offsetof expression has long been
> folded, so we cannot use e.g, the location of the ')' token that would
> have been in the source code.  So I believe the location of 'j' is the
> best we can get at this point.
>
> The patch below sets the location of the DECL for 'j' to what I
> believe is its precise location; with that, the test case passes with
> and without -ftrack-macro-expansion.  But I had to adjust
> g++.dg/template/sfinae6_neg.C for that.
>
> Tested on x86_64-unknown-linux-gnu against trunk.

OK.

>
> gcc/cp
>
>        * decl.c (grokdeclarator): Use the location carried by the
>        declarator for the DECL of the static class member.
>
> gcc/testsuite/
>
>        * g++.dg/template/sfinae6_neg.C: Adjust.
> ---
>  gcc/cp/decl.c                               |    3 ++-
>  gcc/testsuite/g++.dg/template/sfinae6_neg.C |    4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 28c7cee..40818a3 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -10267,7 +10267,8 @@ grokdeclarator (const cp_declarator *declarator,
>              {
>                /* C++ allows static class members.  All other work
>                   for this is done by grokfield.  */
> -               decl = build_lang_decl (VAR_DECL, unqualified_id, type);
> +               decl = build_lang_decl_loc (declarator->id_loc,
> +                                           VAR_DECL, unqualified_id, type);
>                set_linkage_for_static_data_member (decl);
>                /* Even if there is an in-class initialization, DECL
>                   is considered undefined until an out-of-class
> diff --git a/gcc/testsuite/g++.dg/template/sfinae6_neg.C b/gcc/testsuite/g++.dg/template/sfinae6_neg.C
> index d4be5dd..9b7bdfd1 100644
> --- a/gcc/testsuite/g++.dg/template/sfinae6_neg.C
> +++ b/gcc/testsuite/g++.dg/template/sfinae6_neg.C
> @@ -21,9 +21,9 @@ no_type check_is_callable2(...);
>  template<typename F, typename T1, typename T2 = T1>
>  struct is_callable2
>  {
> -  static const bool value =
> +  static const bool value = // { dg-error "within this context" }
>     (sizeof(check_is_callable2(type<F>(), type<T1>(), type<T2>()))
> -     == sizeof(yes_type)); // { dg-error "within this context" }
> +     == sizeof(yes_type));
>  };
>
>  #define JOIN( X, Y ) DO_JOIN( X, Y )
> --
>                Dodji

[PATCH 11/13] Fix va_start related location

[Dodji Seketeli]

In gcc/testsuite/gcc.dg/pr30457.c, the first warning was not being
emitted because the relevant location was inside the var_start macro
defined in a system header.  It can even point to a token for a
builtin macro there.  This patch unwinds to the first token in real
source code in that case.

Tested on x86_64-unknown-linux-gnu against trunk.

	* builtins.c (fold_builtin_next_arg): Unwinds to the first
	location in real source code.
---
 gcc/builtins.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/gcc/builtins.c b/gcc/builtins.c
index b47f218..ef90b25 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -12164,8 +12164,20 @@ fold_builtin_next_arg (tree exp, bool va_start_p)
          the default argument promotions, the behavior is undefined."
       */
       else if (DECL_REGISTER (arg))
-        warning (0, "undefined behaviour when second parameter of "
-                 "%<va_start%> is declared with %<register%> storage");
+	{
+	  /* There is good chance the current input_location points
+	     inside the definition of the va_start macro (perhaps on
+	     the token for builtin) in a system header, so the warning
+	     will not be emitted.  Use the location in real source
+	     code.  */
+	  source_location current_location =
+	    linemap_unwind_to_first_non_reserved_loc (line_table, input_location,
+						      NULL);
+	  warning_at (current_location,
+		      0,
+		      "undefined behaviour when second parameter of "
+		      "%<va_start%> is declared with %<register%> storage");
+	}
 
       /* We want to verify the second parameter just once before the tree
 	 optimizers are run and then avoid keeping it in the tree,
-- 
		Dodji

Re: [PATCH 11/13] Fix va_start related location

[Gabriel Dos Reis]

On Wed, Apr 25, 2012 at 9:04 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> In gcc/testsuite/gcc.dg/pr30457.c, the first warning was not being
> emitted because the relevant location was inside the var_start macro
> defined in a system header.  It can even point to a token for a
> builtin macro there.  This patch unwinds to the first token in real
> source code in that case.

While you are at it, could you also use a non-zero value for the second
argument argument to warning_at?

>
> Tested on x86_64-unknown-linux-gnu against trunk.
>
>        * builtins.c (fold_builtin_next_arg): Unwinds to the first
>        location in real source code.
> ---
>  gcc/builtins.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index b47f218..ef90b25 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -12164,8 +12164,20 @@ fold_builtin_next_arg (tree exp, bool va_start_p)
>          the default argument promotions, the behavior is undefined."
>       */
>       else if (DECL_REGISTER (arg))
> -        warning (0, "undefined behaviour when second parameter of "
> -                 "%<va_start%> is declared with %<register%> storage");
> +       {
> +         /* There is good chance the current input_location points
> +            inside the definition of the va_start macro (perhaps on
> +            the token for builtin) in a system header, so the warning
> +            will not be emitted.  Use the location in real source
> +            code.  */
> +         source_location current_location =
> +           linemap_unwind_to_first_non_reserved_loc (line_table, input_location,
> +                                                     NULL);
> +         warning_at (current_location,
> +                     0,
> +                     "undefined behaviour when second parameter of "
> +                     "%<va_start%> is declared with %<register%> storage");
> +       }
>
>       /* We want to verify the second parameter just once before the tree
>         optimizers are run and then avoid keeping it in the tree,
> --
>                Dodji

Re: [PATCH 11/13] Fix va_start related location

[Dodji Seketeli]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> On Wed, Apr 25, 2012 at 9:04 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>> In gcc/testsuite/gcc.dg/pr30457.c, the first warning was not being
>> emitted because the relevant location was inside the var_start macro
>> defined in a system header.  It can even point to a token for a
>> builtin macro there.  This patch unwinds to the first token in real
>> source code in that case.
>
> While you are at it, could you also use a non-zero value for the second
> argument argument to warning_at?

I couldn't find any obvious value for it.  I am thinking maybe it would
make sense to introduction a new -Wva_start to warn about possible
dangerous uses of the va_start macro and use that as the second argument
for the relevant warnings emitted by fold_builtin_next_arg.  What do you
think?

In any case, this topic seems logically unrelated to the purpose of
enable -ftrack-macro-expansion by default, so IMHO it would be better to
do this in a separate self contain patch.  Among other things, this
would ease possible future back-ports.  Would you agree?

Thanks.

-- 
		Dodji

Re: [PATCH 11/13] Fix va_start related location

[Gabriel Dos Reis]

On Wed, Apr 25, 2012 at 10:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
>
>> On Wed, Apr 25, 2012 at 9:04 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>> In gcc/testsuite/gcc.dg/pr30457.c, the first warning was not being
>>> emitted because the relevant location was inside the var_start macro
>>> defined in a system header.  It can even point to a token for a
>>> builtin macro there.  This patch unwinds to the first token in real
>>> source code in that case.
>>
>> While you are at it, could you also use a non-zero value for the second
>> argument argument to warning_at?
>
> I couldn't find any obvious value for it.  I am thinking maybe it would
> make sense to introduction a new -Wva_start to warn about possible
> dangerous uses of the va_start macro and use that as the second argument
> for the relevant warnings emitted by fold_builtin_next_arg.  What do you
> think?

or -Wvarargs?


>
> In any case, this topic seems logically unrelated to the purpose of
> enable -ftrack-macro-expansion by default, so IMHO it would be better to
> do this in a separate self contain patch.  Among other things, this
> would ease possible future back-ports.  Would you agree?

OK.

-- Gaby

Re: [PATCH 11/13] Fix va_start related location

[Dodji Seketeli]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> On Wed, Apr 25, 2012 at 10:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>> Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
>>
>>> On Wed, Apr 25, 2012 at 9:04 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>>> In gcc/testsuite/gcc.dg/pr30457.c, the first warning was not being
>>>> emitted because the relevant location was inside the var_start macro
>>>> defined in a system header.  It can even point to a token for a
>>>> builtin macro there.  This patch unwinds to the first token in real
>>>> source code in that case.
>>>
>>> While you are at it, could you also use a non-zero value for the second
>>> argument argument to warning_at?
>>
>> I couldn't find any obvious value for it.  I am thinking maybe it would
>> make sense to introduction a new -Wva_start to warn about possible
>> dangerous uses of the va_start macro and use that as the second argument
>> for the relevant warnings emitted by fold_builtin_next_arg.  What do you
>> think?
>
> or -Wvarargs?

OK, I have cooked up a patch for this that I will send in a separate
thread shortly.

>>
>> In any case, this topic seems logically unrelated to the purpose of
>> enable -ftrack-macro-expansion by default, so IMHO it would be better to
>> do this in a separate self contain patch.  Among other things, this
>> would ease possible future back-ports.  Would you agree?
>
> OK.

While testing the separate patch, I realized that this one was missing
adjusting the location in another spot.  So I have updated this patch
accordingly.  The patch that adds -Wvarargs will come on top of it, and
will add some needed regression tests for the whole.

Tested on x86_64-unknown-linux-gnu against trunk.  Bootstrap is still
running ...

	* builtins.c (fold_builtin_next_arg): Unwinds to the first
	location in real source code.
---
 gcc/builtins.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/gcc/builtins.c b/gcc/builtins.c
index b47f218..5ddc47b 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -12095,6 +12095,13 @@ fold_builtin_next_arg (tree exp, bool va_start_p)
   tree fntype = TREE_TYPE (current_function_decl);
   int nargs = call_expr_nargs (exp);
   tree arg;
+  /* There is good chance the current input_location points inside the
+     definition of the va_start macro (perhaps on the token for
+     builtin) in a system header, so warnings will not be emitted.
+     Use the location in real source code.  */
+  source_location current_location =
+    linemap_unwind_to_first_non_reserved_loc (line_table, input_location,
+					      NULL);
 
   if (!stdarg_p (fntype))
     {
@@ -12119,7 +12126,9 @@ fold_builtin_next_arg (tree exp, bool va_start_p)
 	{
 	  /* Evidently an out of date version of <stdarg.h>; can't validate
 	     va_start's second argument, but can still work as intended.  */
-	  warning (0, "%<__builtin_next_arg%> called without an argument");
+	  warning_at (current_location,
+		      0,
+		      "%<__builtin_next_arg%> called without an argument");
 	  return true;
 	}
       else if (nargs > 1)
@@ -12154,7 +12163,9 @@ fold_builtin_next_arg (tree exp, bool va_start_p)
 	     argument.  We just warn and set the arg to be the last
 	     argument so that we will get wrong-code because of
 	     it.  */
-	  warning (0, "second parameter of %<va_start%> not last named argument");
+	  warning_at (current_location,
+		      0,
+		      "second parameter of %<va_start%> not last named argument");
 	}
 
       /* Undefined by C99 7.15.1.4p4 (va_start):
@@ -12164,8 +12175,12 @@ fold_builtin_next_arg (tree exp, bool va_start_p)
          the default argument promotions, the behavior is undefined."
       */
       else if (DECL_REGISTER (arg))
-        warning (0, "undefined behaviour when second parameter of "
-                 "%<va_start%> is declared with %<register%> storage");
+	{
+	  warning_at (current_location,
+		      0,
+		      "undefined behaviour when second parameter of "
+		      "%<va_start%> is declared with %<register%> storage");
+	}
 
       /* We want to verify the second parameter just once before the tree
 	 optimizers are run and then avoid keeping it in the tree,
-- 
		Dodji

Re: [PATCH 11/13] Fix va_start related location

[Dodji Seketeli]

Dodji Seketeli <dodji@redhat.com> writes:


> Tested on x86_64-unknown-linux-gnu against trunk.  Bootstrap is still
> running ...
>
> 	* builtins.c (fold_builtin_next_arg): Unwinds to the first
> 	location in real source code.
> ---
>  gcc/builtins.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)

FWIW, this completed bootstrap and testing fine.

-- 
		Dodji

Re: [PATCH 11/13] Fix va_start related location

[Gabriel Dos Reis]

On Fri, Apr 27, 2012 at 4:45 PM, Dodji Seketeli <dodji@redhat.com> wrote:
> Dodji Seketeli <dodji@redhat.com> writes:
>
>
>> Tested on x86_64-unknown-linux-gnu against trunk.  Bootstrap is still
>> running ...
>>
>>       * builtins.c (fold_builtin_next_arg): Unwinds to the first
>>       location in real source code.
>> ---
>>  gcc/builtins.c |   23 +++++++++++++++++++----
>>  1 files changed, 19 insertions(+), 4 deletions(-)
>
> FWIW, this completed bootstrap and testing fine.

OK.

[PATCH 12/13] Adjust relevant test cases wrt -ftrack-macro-expansion=[0|2]

[Dodji Seketeli]

Even after all the patches I have already submitted for this
-ftrack-macro-expansion business, some test cases where errors happens
on tokens that are defined in macros see their output change in an
incompatible way, when you run them with or without
-ftrack-macro-expansion.

I think this is expected, because the (spelling) locus inside the
definition of the macro pointed to with -ftrack-macro-expansion is
different from the locus of the expansion point of the macro pointed
to without -ftrack-macro-expansion.

In those cases this patch either adjusts the test case and forces it
be run either with -ftrack-macro-expansion, or it just forces it to be
run without -ftrack-macro-expansion.

There are so many libstdc++ tests that were failing because of that
benign issue that I preferred to just run them with
-ftrack-macro-expansion diabled, after inspecting each of them to be
sure there was nothing more serious underneath.  I believe we can latter
turn on -ftrack-macro-expansion there on a case by case basis.

Boostrapped on x86_64-unknown-linux-gnu against trunk with and without
-ftrack-macro-expansion turned on.

gcc/testsuite/

	* c-c++-common/tm/attrib-1.c: Force the test case to run without
	-ftrack-macro-expansion.
	* c-c++-common/warn-ommitted-condop.c: Likewise.
	* gcc.dg/assign-warn-1.c: Likewise.
	* gcc.dg/assign-warn-2.c: Likewise.
	* gcc.dg/attr-alloc_size.c: Likewise.
	* gcc.dg/builtin-stringop-chk-1.c: Likewise.
	* gcc.dg/builtin-stringop-chk-2.c: Likewise.
	* gcc.dg/builtin-strncat-chk-1.c: Likewise.
	* gcc.dg/c90-const-expr-9.c: Likewise.
	* gcc.dg/c99-const-expr-9.c: Likewise.
	* gcc.dg/cpp/direct2.c: Likewise.  Adjust.
	* gcc.dg/cpp/direct2s.c: Likewise.
	* gcc/testsuite/gcc.dg/cpp/pr28709.c: Likewise.
	* gcc.dg/cpp/pragma-diagnostic-1.c: Likewise.
	* gcc.dg/dfp/composite-type.c: Likewise.
	* gcc.dg/uninit-6-O0.c: Adjust the test case and force it to run
	with -ftrack-macro-expansion
	* g++.dg/cpp0x/constexpr-ex3.C: Likewise.
	* g++.dg/cpp0x/constexpr-overflow.C: Likewise.
	* g++.dg/ext/cleanup-1.C: Likewise.
	* g++.dg/ext/gnu-inline-global-reject.C: Likewise.
	* g++.dg/template/sfinae10.C: Likewise.
	* g++.dg/tm/wrap-2.C: Likewise.
	* g++.dg/warn/Wconversion-real-integer.C: Likewise.
	* g++.dg/warn/Wsign-conversion.C: Likewise.
	* g++.dg/warn/multiple-overflow-warn-1.C: Likewise.
	* g++.old-deja/g++.mike/p10769b.C: Likewise.
	* g++.dg/warn/Wdouble-promotion.C: Adjust the test case and force
	it to run with -ftrack-macro-expansion.
	* libstdc++-v3/scripts/testsuite_flags.in: By default, run the
	test cases without -ftrack-macro-expansion.
---
 gcc/testsuite/c-c++-common/tm/attrib-1.c           |    2 +-
 gcc/testsuite/c-c++-common/warn-ommitted-condop.c  |    2 +-
 gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C         |    2 +-
 gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C    |    2 +-
 gcc/testsuite/g++.dg/ext/cleanup-1.C               |    2 +-
 .../g++.dg/ext/gnu-inline-global-reject.C          |    2 +-
 gcc/testsuite/g++.dg/template/sfinae10.C           |    2 +-
 gcc/testsuite/g++.dg/tm/wrap-2.C                   |    2 +-
 .../g++.dg/warn/Wconversion-real-integer.C         |    2 +-
 gcc/testsuite/g++.dg/warn/Wdouble-promotion.C      |    6 +++---
 gcc/testsuite/g++.dg/warn/Wsign-conversion.C       |    2 +-
 .../g++.dg/warn/multiple-overflow-warn-1.C         |    2 +-
 gcc/testsuite/g++.old-deja/g++.mike/p10769b.C      |    2 +-
 gcc/testsuite/gcc.dg/assign-warn-1.c               |    2 +-
 gcc/testsuite/gcc.dg/assign-warn-2.c               |    2 +-
 gcc/testsuite/gcc.dg/attr-alloc_size.c             |    2 +-
 gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c      |    2 +-
 gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c      |    2 +-
 gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c       |    2 +-
 gcc/testsuite/gcc.dg/c90-const-expr-9.c            |    2 +-
 gcc/testsuite/gcc.dg/c99-const-expr-9.c            |    2 +-
 gcc/testsuite/gcc.dg/cpp/direct2.c                 |   12 +++++++-----
 gcc/testsuite/gcc.dg/cpp/direct2s.c                |    2 +-
 gcc/testsuite/gcc.dg/cpp/pr28709.c                 |    8 +++++---
 gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c     |    2 +-
 gcc/testsuite/gcc.dg/dfp/composite-type.c          |    2 +-
 gcc/testsuite/gcc.dg/uninit-6-O0.c                 |    6 +++---
 libstdc++-v3/scripts/testsuite_flags.in            |    2 +-
 28 files changed, 42 insertions(+), 38 deletions(-)

diff --git a/gcc/testsuite/c-c++-common/tm/attrib-1.c b/gcc/testsuite/c-c++-common/tm/attrib-1.c
index 536aeb3..534fa0e 100644
--- a/gcc/testsuite/c-c++-common/tm/attrib-1.c
+++ b/gcc/testsuite/c-c++-common/tm/attrib-1.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-fgnu-tm" } */
+/* { dg-options "-fgnu-tm -ftrack-macro-expansion=0" } */
 
 #define TC	__attribute__((transaction_callable))
 #define TU	__attribute__((transaction_unsafe))
diff --git a/gcc/testsuite/c-c++-common/warn-ommitted-condop.c b/gcc/testsuite/c-c++-common/warn-ommitted-condop.c
index de92b8f..0726f04 100644
--- a/gcc/testsuite/c-c++-common/warn-ommitted-condop.c
+++ b/gcc/testsuite/c-c++-common/warn-ommitted-condop.c
@@ -1,4 +1,4 @@
-/* { dg-options "-Wparentheses" } */
+/* { dg-options "-Wparentheses -ftrack-macro-expansion=0" } */
 
 extern void f2 (int);
 
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C
index 08552cd..5c0b1e2 100644
--- a/gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C
@@ -1,4 +1,4 @@
-// { dg-options "-std=c++0x" }
+// { dg-options "-std=c++0x -ftrack-macro-expansion=0" }
 
 #define SA(X) static_assert (X, #X)
 
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C
index 9b3b1fa..3eb27aa 100644
--- a/gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C
@@ -1,4 +1,4 @@
-// { dg-options "-std=c++0x -w" }
+// { dg-options "-std=c++0x -w -ftrack-macro-expansion=0" }
 
 #include <limits.h>
 extern constexpr int max_s = INT_MAX + 1;  // { dg-error "" }
diff --git a/gcc/testsuite/g++.dg/ext/cleanup-1.C b/gcc/testsuite/g++.dg/ext/cleanup-1.C
index 8e83537..7cf14c9 100644
--- a/gcc/testsuite/g++.dg/ext/cleanup-1.C
+++ b/gcc/testsuite/g++.dg/ext/cleanup-1.C
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-Wall" } */
+/* { dg-options "-Wall -ftrack-macro-expansion=0" } */
 /* Validate expected warnings and errors.  */
 
 #define U	__attribute__((unused))
diff --git a/gcc/testsuite/g++.dg/ext/gnu-inline-global-reject.C b/gcc/testsuite/g++.dg/ext/gnu-inline-global-reject.C
index d9e2660..fc7385d 100644
--- a/gcc/testsuite/g++.dg/ext/gnu-inline-global-reject.C
+++ b/gcc/testsuite/g++.dg/ext/gnu-inline-global-reject.C
@@ -4,7 +4,7 @@
 */
 
 /* { dg-do compile } */
-/* { dg-options " -ansi -Wno-long-long" } */
+/* { dg-options " -ansi -Wno-long-long -ftrack-macro-expansion=0" } */
 
 #include "gnu-inline-common.h"
 
diff --git a/gcc/testsuite/g++.dg/template/sfinae10.C b/gcc/testsuite/g++.dg/template/sfinae10.C
index c6cb12f..3b1d26b 100644
--- a/gcc/testsuite/g++.dg/template/sfinae10.C
+++ b/gcc/testsuite/g++.dg/template/sfinae10.C
@@ -1,7 +1,7 @@
 // DR 339
 //
 // Test of the use of various unary operators with SFINAE
-
+// { dg-options "-fmessage-length=0 -pedantic-errors -Wno-long-long -ftrack-macro-expansion=0 " }
 // Boilerplate helpers
 typedef char yes_type;
 struct no_type { char data[2]; };
diff --git a/gcc/testsuite/g++.dg/tm/wrap-2.C b/gcc/testsuite/g++.dg/tm/wrap-2.C
index 564fbf8..e2948c8 100644
--- a/gcc/testsuite/g++.dg/tm/wrap-2.C
+++ b/gcc/testsuite/g++.dg/tm/wrap-2.C
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-fgnu-tm" } */
+/* { dg-options "-fgnu-tm -ftrack-macro-expansion=0" } */
 
 #define W(X)	__attribute__((transaction_wrap(X)))
 void f1(void);
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer.C b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer.C
index 282ac13..3b6d1f3 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer.C
@@ -3,7 +3,7 @@
    gcc/testsuite/gcc.dg/Wconversion-real-integer.c */
 
 /* { dg-do compile }
-/* { dg-options "-Wconversion" } */
+/* { dg-options "-Wconversion -ftrack-macro-expansion=0" } */
 /* { dg-require-effective-target int32plus } */
 #include <limits.h>
 
diff --git a/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C b/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C
index 9b4044f..98d2eed 100644
--- a/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C
+++ b/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C
@@ -1,11 +1,11 @@
 /* { dg-do compile } */
-/* { dg-options "-Wdouble-promotion" } */
+/* { dg-options "-Wdouble-promotion -ftrack-macro-expansion=2" } */
 
 #include <stddef.h>
 
 /* Some targets do not provide <complex.h> so we define I ourselves.  */
 #define I 1.0iF
-#define ID ((_Complex double)I)
+#define ID ((_Complex double)I) // { dg-warning "implicit" }
 
 float f;
 double d;
@@ -36,7 +36,7 @@ usual_arithmetic_conversions(void)
 
   local_cf = cf + 1.0;       /* { dg-warning "implicit" } */
   local_cf = cf - d;         /* { dg-warning "implicit" } */
-  local_cf = cf + 1.0 * ID;  /* { dg-warning "implicit" } */
+  local_cf = cf + 1.0 * ID;  /* { dg-message "expanded from here" } */
   local_cf = cf - cd;        /* { dg-warning "implicit" } */
   
   local_f = i ? f : d;       /* { dg-warning "implicit" } */
diff --git a/gcc/testsuite/g++.dg/warn/Wsign-conversion.C b/gcc/testsuite/g++.dg/warn/Wsign-conversion.C
index 83fe2ed..f6a0ccc 100644
--- a/gcc/testsuite/g++.dg/warn/Wsign-conversion.C
+++ b/gcc/testsuite/g++.dg/warn/Wsign-conversion.C
@@ -3,7 +3,7 @@
    C++ equivalent of gcc/testsuite/gcc.dg/Wsign-conversion.c  */
 
 // { dg-do compile } 
-// { dg-options "-fsigned-char -Wsign-conversion" } 
+// { dg-options "-fsigned-char -Wsign-conversion -ftrack-macro-expansion=0" } 
 #include <limits.h>
 
 void fsc (signed char sc);
diff --git a/gcc/testsuite/g++.dg/warn/multiple-overflow-warn-1.C b/gcc/testsuite/g++.dg/warn/multiple-overflow-warn-1.C
index 4899302..c941c13 100644
--- a/gcc/testsuite/g++.dg/warn/multiple-overflow-warn-1.C
+++ b/gcc/testsuite/g++.dg/warn/multiple-overflow-warn-1.C
@@ -1,6 +1,6 @@
 /* PR c/19978 : Test for duplicated warnings (unary operators).  */
 /* { dg-do compile } */
-/* { dg-options "-Woverflow" } */
+/* { dg-options "-Woverflow -ftrack-macro-expansion=0" } */
 
 #include <limits.h>
 
diff --git a/gcc/testsuite/g++.old-deja/g++.mike/p10769b.C b/gcc/testsuite/g++.old-deja/g++.mike/p10769b.C
index 9210a45..9c2dd29 100644
--- a/gcc/testsuite/g++.old-deja/g++.mike/p10769b.C
+++ b/gcc/testsuite/g++.old-deja/g++.mike/p10769b.C
@@ -1,5 +1,5 @@
 // { dg-do assemble  }
-// { dg-options "" }
+// { dg-options "-ftrack-macro-expansion=0" }
 // prms-id: 10769
 
 #define PMF2PF(PMF) ((void (*)())(PMF))
diff --git a/gcc/testsuite/gcc.dg/assign-warn-1.c b/gcc/testsuite/gcc.dg/assign-warn-1.c
index ae70242..f26a544 100644
--- a/gcc/testsuite/gcc.dg/assign-warn-1.c
+++ b/gcc/testsuite/gcc.dg/assign-warn-1.c
@@ -1,7 +1,7 @@
 /* Test diagnostics for bad implicit type conversions.  */
 /* Origin: Joseph Myers <jsm@polyomino.org.uk> */
 /* { dg-do compile } */
-/* { dg-options "-pedantic" } */
+/* { dg-options "-pedantic -ftrack-macro-expansion=0" } */
 
 #define TESTARG(ID, TL, TR) void ID##F(TL); void ID##F2(TR x) { ID##F(x); } extern int dummy
 #define TESTARP(ID, TL, TR) struct { void (*x)(TL); } ID##Fp; void ID##F2(TR x) { ID##Fp.x(x); } extern int dummy
diff --git a/gcc/testsuite/gcc.dg/assign-warn-2.c b/gcc/testsuite/gcc.dg/assign-warn-2.c
index 7813b72..1e5eb1c 100644
--- a/gcc/testsuite/gcc.dg/assign-warn-2.c
+++ b/gcc/testsuite/gcc.dg/assign-warn-2.c
@@ -2,7 +2,7 @@
    -pedantic-errors test.  */
 /* Origin: Joseph Myers <jsm@polyomino.org.uk> */
 /* { dg-do compile } */
-/* { dg-options "-pedantic-errors" } */
+/* { dg-options "-pedantic-errors -ftrack-macro-expansion=0" } */
 
 #define TESTARG(ID, TL, TR) void ID##F(TL); void ID##F2(TR x) { ID##F(x); } extern int dummy
 #define TESTARP(ID, TL, TR) struct { void (*x)(TL); } ID##Fp; void ID##F2(TR x) { ID##Fp.x(x); } extern int dummy
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size.c b/gcc/testsuite/gcc.dg/attr-alloc_size.c
index 47d7c00..e8129ce 100644
--- a/gcc/testsuite/gcc.dg/attr-alloc_size.c
+++ b/gcc/testsuite/gcc.dg/attr-alloc_size.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -Wall" } */
+/* { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */
 
 extern void abort (void);
 
diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c
index e9fb7db..beecab6 100644
--- a/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c
+++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c
@@ -1,7 +1,7 @@
 /* Test whether buffer overflow warnings for __*_chk builtins
    are emitted properly.  */
 /* { dg-do compile } */
-/* { dg-options "-O2 -std=gnu99" } */
+/* { dg-options "-O2 -std=gnu99 -ftrack-macro-expansion=0" } */
 /* { dg-options "-mstructure-size-boundary=8 -O2 -std=gnu99" { target arm*-*-* } } */
 
 extern void abort (void);
diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c
index adccd0f..7c2bb60 100644
--- a/gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c
+++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c
@@ -3,7 +3,7 @@
    incorrectly determined to be 0 while it should be (size_t) -1
    (== unknown).  */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -ftrack-macro-expansion=0" } */
 
 #include "../gcc.c-torture/execute/builtins/chk.h"
    
diff --git a/gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c b/gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c
index 80d7b9d..44677f1 100644
--- a/gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c
+++ b/gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c
@@ -1,7 +1,7 @@
 /* Test whether buffer overflow warnings for __strncat_chk builtin
    are emitted properly.  */
 /* { dg-do compile } */
-/* { dg-options "-O2 -std=gnu99" } */
+/* { dg-options "-O2 -std=gnu99 -ftrack-macro-expansion=0" } */
 
 extern void abort (void);
 
diff --git a/gcc/testsuite/gcc.dg/c90-const-expr-9.c b/gcc/testsuite/gcc.dg/c90-const-expr-9.c
index 0d5d8c1..4793a14 100644
--- a/gcc/testsuite/gcc.dg/c90-const-expr-9.c
+++ b/gcc/testsuite/gcc.dg/c90-const-expr-9.c
@@ -3,7 +3,7 @@
    expansion.  */
 /* Origin: Joseph Myers <joseph@codesourcery.com> */
 /* { dg-do compile } */
-/* { dg-options "-std=iso9899:1990 -pedantic-errors" } */
+/* { dg-options "-std=iso9899:1990 -pedantic-errors -ftrack-macro-expansion=0" } */
 
 struct s {
   int a;
diff --git a/gcc/testsuite/gcc.dg/c99-const-expr-9.c b/gcc/testsuite/gcc.dg/c99-const-expr-9.c
index a0a6a96..11e0b2c 100644
--- a/gcc/testsuite/gcc.dg/c99-const-expr-9.c
+++ b/gcc/testsuite/gcc.dg/c99-const-expr-9.c
@@ -3,7 +3,7 @@
    expansion.  */
 /* Origin: Joseph Myers <joseph@codesourcery.com> */
 /* { dg-do compile } */
-/* { dg-options "-std=iso9899:1999 -pedantic-errors" } */
+/* { dg-options "-std=iso9899:1999 -pedantic-errors -ftrack-macro-expansion=0" } */
 
 struct s {
   int a;
diff --git a/gcc/testsuite/gcc.dg/cpp/direct2.c b/gcc/testsuite/gcc.dg/cpp/direct2.c
index 858dec7..1ce40bf 100644
--- a/gcc/testsuite/gcc.dg/cpp/direct2.c
+++ b/gcc/testsuite/gcc.dg/cpp/direct2.c
@@ -4,18 +4,20 @@
 /* Test of prohibition on directives which result from macro expansion.
    See also direct2s.c */
 
-/* { dg-do compile } */
+/* 
+   { dg-options "-ftrack-macro-expansion=0" }
+   { dg-do compile } */
 
 #define HASH #
 #define HASHDEFINE #define
 #define HASHINCLUDE #include
 
 HASH include "somerandomfile" /*{ dg-error "stray" "non-include" }*/
-/*{ dg-bogus "No such" "don't execute non-include" { target *-*-* } 13 }*/
-int resync_parser_1; /*{ dg-error "parse|syntax|expected" "" { target *-*-* } 13 }*/
+/*{ dg-bogus "No such" "don't execute non-include" { target *-*-* } 15 }*/
+int resync_parser_1; /*{ dg-error "parse|syntax|expected" "" { target *-*-* } 15 }*/
 
 HASHINCLUDE <somerandomfile> /*{ dg-error "stray|expected" "non-include 2" }*/
-/*{ dg-bogus "No such" "don't execute non-include 2" { target *-*-* } 17 }*/
+/*{ dg-bogus "No such" "don't execute non-include 2" { target *-*-* } 19 }*/
 int resync_parser_2;
 
 void g1 ()
@@ -43,4 +45,4 @@ void f ()
 #define starslash *##/
 
 slashstar starslash /* { dg-error "parse error|syntax error|expected" "not a comment" } */
-/* { dg-error "does not give" "paste warning(s)" { target *-*-* } 45 } */
+/* { dg-error "does not give" "paste warning(s)" { target *-*-* } 47 } */
diff --git a/gcc/testsuite/gcc.dg/cpp/direct2s.c b/gcc/testsuite/gcc.dg/cpp/direct2s.c
index 9d0cc01..5923214 100644
--- a/gcc/testsuite/gcc.dg/cpp/direct2s.c
+++ b/gcc/testsuite/gcc.dg/cpp/direct2s.c
@@ -6,7 +6,7 @@
    should be identical.  */
 
 /* { dg-do compile } */
-/* { dg-options "-save-temps -ansi -pedantic-errors" } */
+/* { dg-options "-save-temps -ansi -pedantic-errors -ftrack-macro-expansion=0" } */
 
 #define HASH #
 #define HASHDEFINE #define
diff --git a/gcc/testsuite/gcc.dg/cpp/pr28709.c b/gcc/testsuite/gcc.dg/cpp/pr28709.c
index 11cccbe..cb1755a 100644
--- a/gcc/testsuite/gcc.dg/cpp/pr28709.c
+++ b/gcc/testsuite/gcc.dg/cpp/pr28709.c
@@ -1,8 +1,10 @@
 /* Copyright (C) 2006 Free Software Foundation, Inc.  */
 /* PR preprocessor/28709 */
 
-/* { dg-do compile } */
+/* { dg-options "-ftrack-macro-expansion=0" }
+   { dg-do compile } */
+
 #define foo - ## >>
 foo;
-/* { dg-error "expected identifier.*'-'" "" { target *-*-* } 6 } */
-/* { dg-error pasting "" { target *-*-* } 6 } */
+/* { dg-error "expected identifier.*'-'" "" { target *-*-* } 8 } */
+/* { dg-error pasting "" { target *-*-* } 8 } */
diff --git a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c
index 3a2f9da..add7360 100644
--- a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c
+++ b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c
@@ -1,5 +1,5 @@
 /*
-  { dg-options "-Wuninitialized" }
+  { dg-options "-Wuninitialized -ftrack-macro-expansion=0" }
   { dg-do compile }
 */
 
diff --git a/gcc/testsuite/gcc.dg/dfp/composite-type.c b/gcc/testsuite/gcc.dg/dfp/composite-type.c
index 69ecb14..6bf35f5 100644
--- a/gcc/testsuite/gcc.dg/dfp/composite-type.c
+++ b/gcc/testsuite/gcc.dg/dfp/composite-type.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O -Wall" } */
+/* { dg-options "-O -Wall -ftrack-macro-expansion=0" } */
 
 /* C99 6.2.7: Compatible type and composite type.  */
 
diff --git a/gcc/testsuite/gcc.dg/uninit-6-O0.c b/gcc/testsuite/gcc.dg/uninit-6-O0.c
index e3fefe5..fe9815e 100644
--- a/gcc/testsuite/gcc.dg/uninit-6-O0.c
+++ b/gcc/testsuite/gcc.dg/uninit-6-O0.c
@@ -2,7 +2,7 @@
    This one inspired by java/class.c:build_utf8_ref.  */
 
 /* { dg-do compile } */
-/* { dg-options "-Wuninitialized" } */
+/* { dg-options "-Wuninitialized -ftrack-macro-expansion=2" } */
 
 #include <stddef.h>
 
@@ -24,7 +24,7 @@ do {								\
      tmp->car = 0; tmp->cdr = 0; tmp->type = TYPE;		\
      tmp->data = VALUE;						\
      if (TREE->car)						\
-	 LAST->cdr = tmp;					\
+	 LAST->cdr = tmp;	  /* { dg-bogus "field" "uninitialized variable warning" { xfail *-*-* } } */				\
      else							\
 	 TREE->car = tmp;					\
      LAST = tmp;						\
@@ -39,7 +39,7 @@ make_something(int a, int b, int c)
     rv = malloc (sizeof (struct tree));
     rv->car = 0;
 
-    APPEND(rv, field, INTEGER_T, a);  /* { dg-bogus "field" "uninitialized variable warning" { xfail *-*-* } } */
+    APPEND(rv, field, INTEGER_T, a);
     APPEND(rv, field, PTR_T, b);
     APPEND(rv, field, INTEGER_T, c);
 
diff --git a/libstdc++-v3/scripts/testsuite_flags.in b/libstdc++-v3/scripts/testsuite_flags.in
index f77784b..7b16bb1 100755
--- a/libstdc++-v3/scripts/testsuite_flags.in
+++ b/libstdc++-v3/scripts/testsuite_flags.in
@@ -54,7 +54,7 @@ case ${query} in
       echo ${CC}
       ;;
     --cxxflags)
-      CXXFLAGS_default="-D_GLIBCXX_ASSERT -fmessage-length=0"
+      CXXFLAGS_default="-D_GLIBCXX_ASSERT -fmessage-length=0 -ftrack-macro-expansion=0"
       CXXFLAGS_config="@SECTION_FLAGS@ @CXXFLAGS@ @EXTRA_CXX_FLAGS@"
       echo ${CXXFLAGS_default} ${CXXFLAGS_config}
       ;;
-- 
1.7.6.5

Re: [PATCH 12/13] Adjust relevant test cases wrt -ftrack-macro-expansion=[0|2]

[Gabriel Dos Reis]

On Wed, Apr 25, 2012 at 9:16 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Even after all the patches I have already submitted for this
> -ftrack-macro-expansion business, some test cases where errors happens
> on tokens that are defined in macros see their output change in an
> incompatible way, when you run them with or without
> -ftrack-macro-expansion.
>
> I think this is expected, because the (spelling) locus inside the
> definition of the macro pointed to with -ftrack-macro-expansion is
> different from the locus of the expansion point of the macro pointed
> to without -ftrack-macro-expansion.
>
> In those cases this patch either adjusts the test case and forces it
> be run either with -ftrack-macro-expansion, or it just forces it to be
> run without -ftrack-macro-expansion.
>
> There are so many libstdc++ tests that were failing because of that
> benign issue that I preferred to just run them with
> -ftrack-macro-expansion diabled, after inspecting each of them to be
> sure there was nothing more serious underneath.  I believe we can latter
> turn on -ftrack-macro-expansion there on a case by case basis.
>
> Boostrapped on x86_64-unknown-linux-gnu against trunk with and without
> -ftrack-macro-expansion turned on.

OK.

-- Gaby

Re: [PATCH 12/13] Adjust relevant test cases wrt -ftrack-macro-expansion=[0|2]

[Dodji Seketeli]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> OK.

Thank you.

While bootstrapping the tree again, it appeared that an output
regression of the objc test objc.dg/foreach-7.m flew below my radar.

It's one of those typical cases where the first location pointed to by
the diagnostics points into the definition of a macro, instead of
pointing to its expansion point.  I have just forced that unique test to
run without -ftrack-macro-expansion.

This looks fairly obvious to me, but I am CC-ing Mike Stump, just in
case.

I have updated this patch to include the adjustment for
objc.dg/foreach-7.m.  All the other parts remained unchanged.

Tested on x86_64-unknown-linux-gnu against trunk, bootstrap still
underway.

gcc/testsuite/

	* objc.dg/foreach-7.m: Force the test case to run without
	-ftrack-macro-expansion.
	* c-c++-common/tm/attrib-1.c: Likewise.
	* c-c++-common/warn-ommitted-condop.c: Likewise.
	* gcc.dg/assign-warn-1.c: Likewise.
	* gcc.dg/assign-warn-2.c: Likewise.
	* gcc.dg/attr-alloc_size.c: Likewise.
	* gcc.dg/builtin-stringop-chk-1.c: Likewise.
	* gcc.dg/builtin-stringop-chk-2.c: Likewise.
	* gcc.dg/builtin-strncat-chk-1.c: Likewise.
	* gcc.dg/c90-const-expr-9.c: Likewise.
	* gcc.dg/c99-const-expr-9.c: Likewise.
	* gcc.dg/cpp/direct2.c: Likewise.  Adjust.
	* gcc.dg/cpp/direct2s.c: Likewise.
	* gcc/testsuite/gcc.dg/cpp/pr28709.c: Likewise.
	* gcc.dg/cpp/pragma-diagnostic-1.c: Likewise.
	* gcc.dg/dfp/composite-type.c: Likewise.
	* gcc.dg/uninit-6-O0.c: Adjust the test case and force it to run
	with -ftrack-macro-expansion
	* g++.dg/cpp0x/constexpr-ex3.C: Likewise.
	* g++.dg/cpp0x/constexpr-overflow.C: Likewise.
	* g++.dg/ext/cleanup-1.C: Likewise.
	* g++.dg/ext/gnu-inline-global-reject.C: Likewise.
	* g++.dg/template/sfinae10.C: Likewise.
	* g++.dg/tm/wrap-2.C: Likewise.
	* g++.dg/warn/Wconversion-real-integer.C: Likewise.
	* g++.dg/warn/Wsign-conversion.C: Likewise.
	* g++.dg/warn/multiple-overflow-warn-1.C: Likewise.
	* g++.old-deja/g++.mike/p10769b.C: Likewise.
	* g++.dg/warn/Wdouble-promotion.C: Adjust the test case and force
	it to run with -ftrack-macro-expansion.
	* libstdc++-v3/scripts/testsuite_flags.in: By default, run the
	test cases without -ftrack-macro-expansion.
---
 gcc/testsuite/c-c++-common/tm/attrib-1.c           |    2 +-
 gcc/testsuite/c-c++-common/warn-ommitted-condop.c  |    2 +-
 gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C         |    2 +-
 gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C    |    2 +-
 gcc/testsuite/g++.dg/ext/cleanup-1.C               |    2 +-
 .../g++.dg/ext/gnu-inline-global-reject.C          |    2 +-
 gcc/testsuite/g++.dg/template/sfinae10.C           |    2 +-
 gcc/testsuite/g++.dg/tm/wrap-2.C                   |    2 +-
 .../g++.dg/warn/Wconversion-real-integer.C         |    2 +-
 gcc/testsuite/g++.dg/warn/Wdouble-promotion.C      |    6 +++---
 gcc/testsuite/g++.dg/warn/Wsign-conversion.C       |    2 +-
 .../g++.dg/warn/multiple-overflow-warn-1.C         |    2 +-
 gcc/testsuite/g++.old-deja/g++.mike/p10769b.C      |    2 +-
 gcc/testsuite/gcc.dg/assign-warn-1.c               |    2 +-
 gcc/testsuite/gcc.dg/assign-warn-2.c               |    2 +-
 gcc/testsuite/gcc.dg/attr-alloc_size.c             |    2 +-
 gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c      |    2 +-
 gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c      |    2 +-
 gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c       |    2 +-
 gcc/testsuite/gcc.dg/c90-const-expr-9.c            |    2 +-
 gcc/testsuite/gcc.dg/c99-const-expr-9.c            |    2 +-
 gcc/testsuite/gcc.dg/cpp/direct2.c                 |   12 +++++++-----
 gcc/testsuite/gcc.dg/cpp/direct2s.c                |    2 +-
 gcc/testsuite/gcc.dg/cpp/pr28709.c                 |    8 +++++---
 gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c     |    2 +-
 gcc/testsuite/gcc.dg/dfp/composite-type.c          |    2 +-
 gcc/testsuite/gcc.dg/uninit-6-O0.c                 |    6 +++---
 gcc/testsuite/objc.dg/foreach-7.m                  |    9 ++++++---
 libstdc++-v3/scripts/testsuite_flags.in            |    2 +-
 29 files changed, 48 insertions(+), 41 deletions(-)

diff --git a/gcc/testsuite/c-c++-common/tm/attrib-1.c b/gcc/testsuite/c-c++-common/tm/attrib-1.c
index 536aeb3..534fa0e 100644
--- a/gcc/testsuite/c-c++-common/tm/attrib-1.c
+++ b/gcc/testsuite/c-c++-common/tm/attrib-1.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-fgnu-tm" } */
+/* { dg-options "-fgnu-tm -ftrack-macro-expansion=0" } */
 
 #define TC	__attribute__((transaction_callable))
 #define TU	__attribute__((transaction_unsafe))
diff --git a/gcc/testsuite/c-c++-common/warn-ommitted-condop.c b/gcc/testsuite/c-c++-common/warn-ommitted-condop.c
index de92b8f..0726f04 100644
--- a/gcc/testsuite/c-c++-common/warn-ommitted-condop.c
+++ b/gcc/testsuite/c-c++-common/warn-ommitted-condop.c
@@ -1,4 +1,4 @@
-/* { dg-options "-Wparentheses" } */
+/* { dg-options "-Wparentheses -ftrack-macro-expansion=0" } */
 
 extern void f2 (int);
 
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C
index 08552cd..5c0b1e2 100644
--- a/gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-ex3.C
@@ -1,4 +1,4 @@
-// { dg-options "-std=c++0x" }
+// { dg-options "-std=c++0x -ftrack-macro-expansion=0" }
 
 #define SA(X) static_assert (X, #X)
 
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C
index 9b3b1fa..3eb27aa 100644
--- a/gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-overflow.C
@@ -1,4 +1,4 @@
-// { dg-options "-std=c++0x -w" }
+// { dg-options "-std=c++0x -w -ftrack-macro-expansion=0" }
 
 #include <limits.h>
 extern constexpr int max_s = INT_MAX + 1;  // { dg-error "" }
diff --git a/gcc/testsuite/g++.dg/ext/cleanup-1.C b/gcc/testsuite/g++.dg/ext/cleanup-1.C
index 8e83537..7cf14c9 100644
--- a/gcc/testsuite/g++.dg/ext/cleanup-1.C
+++ b/gcc/testsuite/g++.dg/ext/cleanup-1.C
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-Wall" } */
+/* { dg-options "-Wall -ftrack-macro-expansion=0" } */
 /* Validate expected warnings and errors.  */
 
 #define U	__attribute__((unused))
diff --git a/gcc/testsuite/g++.dg/ext/gnu-inline-global-reject.C b/gcc/testsuite/g++.dg/ext/gnu-inline-global-reject.C
index d9e2660..fc7385d 100644
--- a/gcc/testsuite/g++.dg/ext/gnu-inline-global-reject.C
+++ b/gcc/testsuite/g++.dg/ext/gnu-inline-global-reject.C
@@ -4,7 +4,7 @@
 */
 
 /* { dg-do compile } */
-/* { dg-options " -ansi -Wno-long-long" } */
+/* { dg-options " -ansi -Wno-long-long -ftrack-macro-expansion=0" } */
 
 #include "gnu-inline-common.h"
 
diff --git a/gcc/testsuite/g++.dg/template/sfinae10.C b/gcc/testsuite/g++.dg/template/sfinae10.C
index c6cb12f..3b1d26b 100644
--- a/gcc/testsuite/g++.dg/template/sfinae10.C
+++ b/gcc/testsuite/g++.dg/template/sfinae10.C
@@ -1,7 +1,7 @@
 // DR 339
 //
 // Test of the use of various unary operators with SFINAE
-
+// { dg-options "-fmessage-length=0 -pedantic-errors -Wno-long-long -ftrack-macro-expansion=0 " }
 // Boilerplate helpers
 typedef char yes_type;
 struct no_type { char data[2]; };
diff --git a/gcc/testsuite/g++.dg/tm/wrap-2.C b/gcc/testsuite/g++.dg/tm/wrap-2.C
index 564fbf8..e2948c8 100644
--- a/gcc/testsuite/g++.dg/tm/wrap-2.C
+++ b/gcc/testsuite/g++.dg/tm/wrap-2.C
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-fgnu-tm" } */
+/* { dg-options "-fgnu-tm -ftrack-macro-expansion=0" } */
 
 #define W(X)	__attribute__((transaction_wrap(X)))
 void f1(void);
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer.C b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer.C
index 282ac13..3b6d1f3 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer.C
@@ -3,7 +3,7 @@
    gcc/testsuite/gcc.dg/Wconversion-real-integer.c */
 
 /* { dg-do compile }
-/* { dg-options "-Wconversion" } */
+/* { dg-options "-Wconversion -ftrack-macro-expansion=0" } */
 /* { dg-require-effective-target int32plus } */
 #include <limits.h>
 
diff --git a/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C b/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C
index 9b4044f..98d2eed 100644
--- a/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C
+++ b/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C
@@ -1,11 +1,11 @@
 /* { dg-do compile } */
-/* { dg-options "-Wdouble-promotion" } */
+/* { dg-options "-Wdouble-promotion -ftrack-macro-expansion=2" } */
 
 #include <stddef.h>
 
 /* Some targets do not provide <complex.h> so we define I ourselves.  */
 #define I 1.0iF
-#define ID ((_Complex double)I)
+#define ID ((_Complex double)I) // { dg-warning "implicit" }
 
 float f;
 double d;
@@ -36,7 +36,7 @@ usual_arithmetic_conversions(void)
 
   local_cf = cf + 1.0;       /* { dg-warning "implicit" } */
   local_cf = cf - d;         /* { dg-warning "implicit" } */
-  local_cf = cf + 1.0 * ID;  /* { dg-warning "implicit" } */
+  local_cf = cf + 1.0 * ID;  /* { dg-message "expanded from here" } */
   local_cf = cf - cd;        /* { dg-warning "implicit" } */
   
   local_f = i ? f : d;       /* { dg-warning "implicit" } */
diff --git a/gcc/testsuite/g++.dg/warn/Wsign-conversion.C b/gcc/testsuite/g++.dg/warn/Wsign-conversion.C
index 83fe2ed..f6a0ccc 100644
--- a/gcc/testsuite/g++.dg/warn/Wsign-conversion.C
+++ b/gcc/testsuite/g++.dg/warn/Wsign-conversion.C
@@ -3,7 +3,7 @@
    C++ equivalent of gcc/testsuite/gcc.dg/Wsign-conversion.c  */
 
 // { dg-do compile } 
-// { dg-options "-fsigned-char -Wsign-conversion" } 
+// { dg-options "-fsigned-char -Wsign-conversion -ftrack-macro-expansion=0" } 
 #include <limits.h>
 
 void fsc (signed char sc);
diff --git a/gcc/testsuite/g++.dg/warn/multiple-overflow-warn-1.C b/gcc/testsuite/g++.dg/warn/multiple-overflow-warn-1.C
index 4899302..c941c13 100644
--- a/gcc/testsuite/g++.dg/warn/multiple-overflow-warn-1.C
+++ b/gcc/testsuite/g++.dg/warn/multiple-overflow-warn-1.C
@@ -1,6 +1,6 @@
 /* PR c/19978 : Test for duplicated warnings (unary operators).  */
 /* { dg-do compile } */
-/* { dg-options "-Woverflow" } */
+/* { dg-options "-Woverflow -ftrack-macro-expansion=0" } */
 
 #include <limits.h>
 
diff --git a/gcc/testsuite/g++.old-deja/g++.mike/p10769b.C b/gcc/testsuite/g++.old-deja/g++.mike/p10769b.C
index 9210a45..9c2dd29 100644
--- a/gcc/testsuite/g++.old-deja/g++.mike/p10769b.C
+++ b/gcc/testsuite/g++.old-deja/g++.mike/p10769b.C
@@ -1,5 +1,5 @@
 // { dg-do assemble  }
-// { dg-options "" }
+// { dg-options "-ftrack-macro-expansion=0" }
 // prms-id: 10769
 
 #define PMF2PF(PMF) ((void (*)())(PMF))
diff --git a/gcc/testsuite/gcc.dg/assign-warn-1.c b/gcc/testsuite/gcc.dg/assign-warn-1.c
index ae70242..f26a544 100644
--- a/gcc/testsuite/gcc.dg/assign-warn-1.c
+++ b/gcc/testsuite/gcc.dg/assign-warn-1.c
@@ -1,7 +1,7 @@
 /* Test diagnostics for bad implicit type conversions.  */
 /* Origin: Joseph Myers <jsm@polyomino.org.uk> */
 /* { dg-do compile } */
-/* { dg-options "-pedantic" } */
+/* { dg-options "-pedantic -ftrack-macro-expansion=0" } */
 
 #define TESTARG(ID, TL, TR) void ID##F(TL); void ID##F2(TR x) { ID##F(x); } extern int dummy
 #define TESTARP(ID, TL, TR) struct { void (*x)(TL); } ID##Fp; void ID##F2(TR x) { ID##Fp.x(x); } extern int dummy
diff --git a/gcc/testsuite/gcc.dg/assign-warn-2.c b/gcc/testsuite/gcc.dg/assign-warn-2.c
index 7813b72..1e5eb1c 100644
--- a/gcc/testsuite/gcc.dg/assign-warn-2.c
+++ b/gcc/testsuite/gcc.dg/assign-warn-2.c
@@ -2,7 +2,7 @@
    -pedantic-errors test.  */
 /* Origin: Joseph Myers <jsm@polyomino.org.uk> */
 /* { dg-do compile } */
-/* { dg-options "-pedantic-errors" } */
+/* { dg-options "-pedantic-errors -ftrack-macro-expansion=0" } */
 
 #define TESTARG(ID, TL, TR) void ID##F(TL); void ID##F2(TR x) { ID##F(x); } extern int dummy
 #define TESTARP(ID, TL, TR) struct { void (*x)(TL); } ID##Fp; void ID##F2(TR x) { ID##Fp.x(x); } extern int dummy
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size.c b/gcc/testsuite/gcc.dg/attr-alloc_size.c
index 47d7c00..e8129ce 100644
--- a/gcc/testsuite/gcc.dg/attr-alloc_size.c
+++ b/gcc/testsuite/gcc.dg/attr-alloc_size.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -Wall" } */
+/* { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */
 
 extern void abort (void);
 
diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c
index e9fb7db..beecab6 100644
--- a/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c
+++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c
@@ -1,7 +1,7 @@
 /* Test whether buffer overflow warnings for __*_chk builtins
    are emitted properly.  */
 /* { dg-do compile } */
-/* { dg-options "-O2 -std=gnu99" } */
+/* { dg-options "-O2 -std=gnu99 -ftrack-macro-expansion=0" } */
 /* { dg-options "-mstructure-size-boundary=8 -O2 -std=gnu99" { target arm*-*-* } } */
 
 extern void abort (void);
diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c
index adccd0f..7c2bb60 100644
--- a/gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c
+++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-2.c
@@ -3,7 +3,7 @@
    incorrectly determined to be 0 while it should be (size_t) -1
    (== unknown).  */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -ftrack-macro-expansion=0" } */
 
 #include "../gcc.c-torture/execute/builtins/chk.h"
    
diff --git a/gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c b/gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c
index 80d7b9d..44677f1 100644
--- a/gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c
+++ b/gcc/testsuite/gcc.dg/builtin-strncat-chk-1.c
@@ -1,7 +1,7 @@
 /* Test whether buffer overflow warnings for __strncat_chk builtin
    are emitted properly.  */
 /* { dg-do compile } */
-/* { dg-options "-O2 -std=gnu99" } */
+/* { dg-options "-O2 -std=gnu99 -ftrack-macro-expansion=0" } */
 
 extern void abort (void);
 
diff --git a/gcc/testsuite/gcc.dg/c90-const-expr-9.c b/gcc/testsuite/gcc.dg/c90-const-expr-9.c
index 0d5d8c1..4793a14 100644
--- a/gcc/testsuite/gcc.dg/c90-const-expr-9.c
+++ b/gcc/testsuite/gcc.dg/c90-const-expr-9.c
@@ -3,7 +3,7 @@
    expansion.  */
 /* Origin: Joseph Myers <joseph@codesourcery.com> */
 /* { dg-do compile } */
-/* { dg-options "-std=iso9899:1990 -pedantic-errors" } */
+/* { dg-options "-std=iso9899:1990 -pedantic-errors -ftrack-macro-expansion=0" } */
 
 struct s {
   int a;
diff --git a/gcc/testsuite/gcc.dg/c99-const-expr-9.c b/gcc/testsuite/gcc.dg/c99-const-expr-9.c
index a0a6a96..11e0b2c 100644
--- a/gcc/testsuite/gcc.dg/c99-const-expr-9.c
+++ b/gcc/testsuite/gcc.dg/c99-const-expr-9.c
@@ -3,7 +3,7 @@
    expansion.  */
 /* Origin: Joseph Myers <joseph@codesourcery.com> */
 /* { dg-do compile } */
-/* { dg-options "-std=iso9899:1999 -pedantic-errors" } */
+/* { dg-options "-std=iso9899:1999 -pedantic-errors -ftrack-macro-expansion=0" } */
 
 struct s {
   int a;
diff --git a/gcc/testsuite/gcc.dg/cpp/direct2.c b/gcc/testsuite/gcc.dg/cpp/direct2.c
index 858dec7..1ce40bf 100644
--- a/gcc/testsuite/gcc.dg/cpp/direct2.c
+++ b/gcc/testsuite/gcc.dg/cpp/direct2.c
@@ -4,18 +4,20 @@
 /* Test of prohibition on directives which result from macro expansion.
    See also direct2s.c */
 
-/* { dg-do compile } */
+/* 
+   { dg-options "-ftrack-macro-expansion=0" }
+   { dg-do compile } */
 
 #define HASH #
 #define HASHDEFINE #define
 #define HASHINCLUDE #include
 
 HASH include "somerandomfile" /*{ dg-error "stray" "non-include" }*/
-/*{ dg-bogus "No such" "don't execute non-include" { target *-*-* } 13 }*/
-int resync_parser_1; /*{ dg-error "parse|syntax|expected" "" { target *-*-* } 13 }*/
+/*{ dg-bogus "No such" "don't execute non-include" { target *-*-* } 15 }*/
+int resync_parser_1; /*{ dg-error "parse|syntax|expected" "" { target *-*-* } 15 }*/
 
 HASHINCLUDE <somerandomfile> /*{ dg-error "stray|expected" "non-include 2" }*/
-/*{ dg-bogus "No such" "don't execute non-include 2" { target *-*-* } 17 }*/
+/*{ dg-bogus "No such" "don't execute non-include 2" { target *-*-* } 19 }*/
 int resync_parser_2;
 
 void g1 ()
@@ -43,4 +45,4 @@ void f ()
 #define starslash *##/
 
 slashstar starslash /* { dg-error "parse error|syntax error|expected" "not a comment" } */
-/* { dg-error "does not give" "paste warning(s)" { target *-*-* } 45 } */
+/* { dg-error "does not give" "paste warning(s)" { target *-*-* } 47 } */
diff --git a/gcc/testsuite/gcc.dg/cpp/direct2s.c b/gcc/testsuite/gcc.dg/cpp/direct2s.c
index 9d0cc01..5923214 100644
--- a/gcc/testsuite/gcc.dg/cpp/direct2s.c
+++ b/gcc/testsuite/gcc.dg/cpp/direct2s.c
@@ -6,7 +6,7 @@
    should be identical.  */
 
 /* { dg-do compile } */
-/* { dg-options "-save-temps -ansi -pedantic-errors" } */
+/* { dg-options "-save-temps -ansi -pedantic-errors -ftrack-macro-expansion=0" } */
 
 #define HASH #
 #define HASHDEFINE #define
diff --git a/gcc/testsuite/gcc.dg/cpp/pr28709.c b/gcc/testsuite/gcc.dg/cpp/pr28709.c
index 11cccbe..cb1755a 100644
--- a/gcc/testsuite/gcc.dg/cpp/pr28709.c
+++ b/gcc/testsuite/gcc.dg/cpp/pr28709.c
@@ -1,8 +1,10 @@
 /* Copyright (C) 2006 Free Software Foundation, Inc.  */
 /* PR preprocessor/28709 */
 
-/* { dg-do compile } */
+/* { dg-options "-ftrack-macro-expansion=0" }
+   { dg-do compile } */
+
 #define foo - ## >>
 foo;
-/* { dg-error "expected identifier.*'-'" "" { target *-*-* } 6 } */
-/* { dg-error pasting "" { target *-*-* } 6 } */
+/* { dg-error "expected identifier.*'-'" "" { target *-*-* } 8 } */
+/* { dg-error pasting "" { target *-*-* } 8 } */
diff --git a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c
index 3a2f9da..add7360 100644
--- a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c
+++ b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-1.c
@@ -1,5 +1,5 @@
 /*
-  { dg-options "-Wuninitialized" }
+  { dg-options "-Wuninitialized -ftrack-macro-expansion=0" }
   { dg-do compile }
 */
 
diff --git a/gcc/testsuite/gcc.dg/dfp/composite-type.c b/gcc/testsuite/gcc.dg/dfp/composite-type.c
index 69ecb14..6bf35f5 100644
--- a/gcc/testsuite/gcc.dg/dfp/composite-type.c
+++ b/gcc/testsuite/gcc.dg/dfp/composite-type.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O -Wall" } */
+/* { dg-options "-O -Wall -ftrack-macro-expansion=0" } */
 
 /* C99 6.2.7: Compatible type and composite type.  */
 
diff --git a/gcc/testsuite/gcc.dg/uninit-6-O0.c b/gcc/testsuite/gcc.dg/uninit-6-O0.c
index e3fefe5..fe9815e 100644
--- a/gcc/testsuite/gcc.dg/uninit-6-O0.c
+++ b/gcc/testsuite/gcc.dg/uninit-6-O0.c
@@ -2,7 +2,7 @@
    This one inspired by java/class.c:build_utf8_ref.  */
 
 /* { dg-do compile } */
-/* { dg-options "-Wuninitialized" } */
+/* { dg-options "-Wuninitialized -ftrack-macro-expansion=2" } */
 
 #include <stddef.h>
 
@@ -24,7 +24,7 @@ do {								\
      tmp->car = 0; tmp->cdr = 0; tmp->type = TYPE;		\
      tmp->data = VALUE;						\
      if (TREE->car)						\
-	 LAST->cdr = tmp;					\
+	 LAST->cdr = tmp;	  /* { dg-bogus "field" "uninitialized variable warning" { xfail *-*-* } } */				\
      else							\
 	 TREE->car = tmp;					\
      LAST = tmp;						\
@@ -39,7 +39,7 @@ make_something(int a, int b, int c)
     rv = malloc (sizeof (struct tree));
     rv->car = 0;
 
-    APPEND(rv, field, INTEGER_T, a);  /* { dg-bogus "field" "uninitialized variable warning" { xfail *-*-* } } */
+    APPEND(rv, field, INTEGER_T, a);
     APPEND(rv, field, PTR_T, b);
     APPEND(rv, field, INTEGER_T, c);
 
diff --git a/gcc/testsuite/objc.dg/foreach-7.m b/gcc/testsuite/objc.dg/foreach-7.m
index c565211..a828692 100644
--- a/gcc/testsuite/objc.dg/foreach-7.m
+++ b/gcc/testsuite/objc.dg/foreach-7.m
@@ -1,5 +1,8 @@
 /* Test basic Objective-C foreach syntax.  This tests warnings and errors.  */
-/* { dg-do compile } */
+/* 
+   { dg-options "-ftrack-macro-expansion=0" }
+   { dg-do compile } 
+*/
 
 #import "../objc-obj-c++-shared/TestsuiteObject.h"
 #import <objc/objc.h>
@@ -37,10 +40,10 @@ int main (void)
   id object = nil;
 
   for (typedef int my_typedef in array) /* { dg-error "declaration of non-variable" } */
-    ;                                   /* { dg-error "iterating variable in fast enumeration is not an object" "" { target *-*-* } 39 } */
+    ;                                   /* { dg-error "iterating variable in fast enumeration is not an object" "" { target *-*-* } 42 } */
 
   for (function () in nil) /* { dg-error "invalid iterating variable in fast enumeration" } */
-    ;                      /* { dg-error "iterating variable in fast enumeration is not an object" "" { target *-*-* } 42 } */
+    ;                      /* { dg-error "iterating variable in fast enumeration is not an object" "" { target *-*-* } 45 } */
 
   for (object_function () in nil) /* { dg-error "invalid iterating variable in fast enumeration" } */
     ;
diff --git a/libstdc++-v3/scripts/testsuite_flags.in b/libstdc++-v3/scripts/testsuite_flags.in
index f77784b..7b16bb1 100755
--- a/libstdc++-v3/scripts/testsuite_flags.in
+++ b/libstdc++-v3/scripts/testsuite_flags.in
@@ -54,7 +54,7 @@ case ${query} in
       echo ${CC}
       ;;
     --cxxflags)
-      CXXFLAGS_default="-D_GLIBCXX_ASSERT -fmessage-length=0"
+      CXXFLAGS_default="-D_GLIBCXX_ASSERT -fmessage-length=0 -ftrack-macro-expansion=0"
       CXXFLAGS_config="@SECTION_FLAGS@ @CXXFLAGS@ @EXTRA_CXX_FLAGS@"
       echo ${CXXFLAGS_default} ${CXXFLAGS_config}
       ;;
-- 
		Dodji

Re: [PATCH 12/13] Adjust relevant test cases wrt -ftrack-macro-expansion=[0|2]

[Dodji Seketeli]

Dodji Seketeli <dodji@redhat.com> writes:

> Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
>
>> OK.
>
> Thank you.
>
> While bootstrapping the tree again, it appeared that an output
> regression of the objc test objc.dg/foreach-7.m flew below my radar.
>
> It's one of those typical cases where the first location pointed to by
> the diagnostics points into the definition of a macro, instead of
> pointing to its expansion point.  I have just forced that unique test to
> run without -ftrack-macro-expansion.
>
> This looks fairly obvious to me, but I am CC-ing Mike Stump, just in
> case.
>
> I have updated this patch to include the adjustment for
> objc.dg/foreach-7.m.  All the other parts remained unchanged.
>
> Tested on x86_64-unknown-linux-gnu against trunk, bootstrap still
> underway.

FWIW, this passed bootstrap fine.

-- 
		Dodji

Re: [PATCH 12/13] Adjust relevant test cases wrt -ftrack-macro-expansion=[0|2]

[Gabriel Dos Reis]

It is still OK :-)

On Mon, Apr 30, 2012 at 1:22 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Dodji Seketeli <dodji@redhat.com> writes:
>
>> Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
>>
>>> OK.
>>
>> Thank you.
>>
>> While bootstrapping the tree again, it appeared that an output
>> regression of the objc test objc.dg/foreach-7.m flew below my radar.
>>
>> It's one of those typical cases where the first location pointed to by
>> the diagnostics points into the definition of a macro, instead of
>> pointing to its expansion point.  I have just forced that unique test to
>> run without -ftrack-macro-expansion.
>>
>> This looks fairly obvious to me, but I am CC-ing Mike Stump, just in
>> case.
>>
>> I have updated this patch to include the adjustment for
>> objc.dg/foreach-7.m.  All the other parts remained unchanged.
>>
>> Tested on x86_64-unknown-linux-gnu against trunk, bootstrap still
>> underway.
>
> FWIW, this passed bootstrap fine.
>
> --
>                Dodji

Re: [PATCH 12/13] Adjust relevant test cases wrt -ftrack-macro-expansion=[0|2]

[Mike Stump]

On Apr 29, 2012, at 10:38 AM, Dodji Seketeli wrote:
> While bootstrapping the tree again, it appeared that an output
> regression of the objc test objc.dg/foreach-7.m flew below my radar.

> This looks fairly obvious to me, but I am CC-ing Mike Stump, just in
> case.

That's fine.

RE: [PATCH 12/13] Adjust relevant test cases wrt -ftrack-macro-expansion=[0|2]

[Greta Yorsh]

There are a couple more test that need adjusting:
gcc.dg/fixed-point/operator-bitwise.c
gcc.dg/fixed-point/composite-type.c

These tests fail on arm-none-eabi. 
Below is a patch that fixes them.

Thanks,
Greta

gcc/testsuite

2012-05-02  Greta Yorsh  <Greta.Yorsh@arm.com>

	* gcc.dg/fixed-point/composite-type.c (dg-options): Add
	option -ftrack-macro-expansion=0.
	* gcc.dg/fixed-point/operator-bitwise.c (dg-options): Add
	option -ftrack-macro-expansion=0.

diff --git a/gcc/testsuite/gcc.dg/fixed-point/composite-type.c
b/gcc/testsuite/gcc.dg/fixed-point/composite-type.c
index 5ae1198..026bdaf 100644
--- a/gcc/testsuite/gcc.dg/fixed-point/composite-type.c
+++ b/gcc/testsuite/gcc.dg/fixed-point/composite-type.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-std=gnu99 -O -Wall -Wno-unused" } */
+/* { dg-options "-std=gnu99 -O -Wall -Wno-unused -ftrack-macro-expansion=0"
} */
 
 /* C99 6.2.7: Compatible type and composite type.  */
 
diff --git a/gcc/testsuite/gcc.dg/fixed-point/operator-bitwise.c
b/gcc/testsuite/gcc.dg/fixed-point/operator-bitwise.c
index 31aecf5..6ba817d 100644
--- a/gcc/testsuite/gcc.dg/fixed-point/operator-bitwise.c
+++ b/gcc/testsuite/gcc.dg/fixed-point/operator-bitwise.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-std=gnu99" } */
+/* { dg-options "-std=gnu99 -ftrack-macro-expansion=0" } */
 
 /* C99 6.5.10: Bitwise AND operator.
    C99 6.5.11: Bitwise exclusive OR operator.


> -----Original Message-----
> From: Mike Stump [mailto:mikestump@comcast.net]
> Sent: 30 April 2012 17:09
> To: Dodji Seketeli
> Cc: Gabriel Dos Reis; GCC Patches; Tom Tromey; Jason Merrill; Paolo
> Carlini; Benjamin De Kosnik
> Subject: Re: [PATCH 12/13] Adjust relevant test cases wrt -ftrack-
> macro-expansion=[0|2]
> 
> On Apr 29, 2012, at 10:38 AM, Dodji Seketeli wrote:
> > While bootstrapping the tree again, it appeared that an output
> > regression of the objc test objc.dg/foreach-7.m flew below my radar.
> 
> > This looks fairly obvious to me, but I am CC-ing Mike Stump, just in
> > case.
> 
> That's fine.

Re: [PATCH 12/13] Adjust relevant test cases wrt -ftrack-macro-expansion=[0|2]

[Benjamin Kosnik]

Hey hey! Looking good Dodji. Can't wait to use this stuff.

> There are so many libstdc++ tests that were failing because of that
> benign issue that I preferred to just run them with
> -ftrack-macro-expansion diabled, after inspecting each of them to be
> sure there was nothing more serious underneath.  I believe we can
> latter turn on -ftrack-macro-expansion there on a case by case basis.

Seems perfectly reasonable. I volunteer to look at the
libstdc++testsuite  changes once your patches are in checked in to
trunk. 

-benjamin

[PATCH 13/13] Switch -ftrack-macro-expansion=2 on by default.

[Dodji Seketeli]

Hopefully closing the series, this patch switches the compiler to
-ftrack-macro-expansion=2 by default.

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

libcpp/

	* init.c (cpp_create_reader): Switch -ftrack-macro-expansion=2 on
	by default.  Add comments.

gcc/docs/

	* cppopts.texi: Adjust for enabling -ftrack-macro-expansion=2 by
	default.
---
 gcc/doc/cppopts.texi |    2 ++
 libcpp/init.c        |    6 ++++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/gcc/doc/cppopts.texi b/gcc/doc/cppopts.texi
index 205d870..27b1095 100644
--- a/gcc/doc/cppopts.texi
+++ b/gcc/doc/cppopts.texi
@@ -617,6 +617,8 @@ tokens locations completely. This value is the most memory hungry.
 When this option is given no argument, the default parameter value is
 @samp{2}.
 
+Note that -ftrack-macro-expansion=2 is activated by default.
+
 @item -fexec-charset=@var{charset}
 @opindex fexec-charset
 @cindex character set, execution
diff --git a/libcpp/init.c b/libcpp/init.c
index 5fa82ca..ba1e9cd 100644
--- a/libcpp/init.c
+++ b/libcpp/init.c
@@ -174,6 +174,12 @@ cpp_create_reader (enum c_lang lang, hash_table *table,
   CPP_OPTION (pfile, warn_dollars) = 1;
   CPP_OPTION (pfile, warn_variadic_macros) = 1;
   CPP_OPTION (pfile, warn_builtin_macro_redefined) = 1;
+  /* By default, track locations of tokens resulting from macro
+     expansion.  The '2' means, track the locations with the highest
+     accuracy.  Read the comments for struct
+     cpp_options::track_macro_expansion to learn about the other
+     values.  */
+  CPP_OPTION (pfile, track_macro_expansion) = 2;
   CPP_OPTION (pfile, warn_normalize) = normalized_C;
 
   /* Default CPP arithmetic to something sensible for the host for the
-- 
		Dodji

Re: [PATCH 13/13] Switch -ftrack-macro-expansion=2 on by default.

[Gabriel Dos Reis]

On Wed, Apr 25, 2012 at 9:33 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Hopefully closing the series, this patch switches the compiler to
> -ftrack-macro-expansion=2 by default.
>
> Tested and bootstrapped on x86_64-unknown-linux-gnu against trunk.

Makes sense to me; Tom?

-- Gaby

Re: [PATCH 13/13] Switch -ftrack-macro-expansion=2 on by default.

[Tom Tromey]

>>>>> "Gaby" == Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

Gaby> On Wed, Apr 25, 2012 at 9:33 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>> Hopefully closing the series, this patch switches the compiler to
>> -ftrack-macro-expansion=2 by default.
>> 
>> Tested and bootstrapped on x86_64-unknown-linux-gnu against trunk.

Gaby> Makes sense to me; Tom?

Yes, do it.  Thanks.

Tom

Re: Patches to enable -ftrack-macro-expansion by default

[Dodji Seketeli]

Dodji Seketeli <dodji@redhat.com> writes:

> I am proposing a series of patches which is supposed to address the
> remaining issues (I am aware of) preventing us from enabling the
> -ftrack-macro-expansion by default.
>
> The idea is to address each issue I notice in the course of trying to
> bootstrap the compiler and running the tests with
> -ftrack-macro-expansion enabled.
>
> Beside the fixes, I ended up disabling the -ftrack-macro-expansion for
> many test cases (sometimes globally in the dg-*.exp files, or on a
> case by case basis), because that option changes the compiler output
> and so requires that I either adapt the test case or disable the
> option.  For other tests, I chose to adapt the test case.

I have finally applied this series of 14 patches to the mainline today.
The SVN revisions are from r186965 to r186978.

Thank you all for your time and patience while reviewing this patch-set.

-- 
		Dodji

Re: Patches to enable -ftrack-macro-expansion by default

[Andreas Krebbel]

On 04/30/2012 01:46 PM, Dodji Seketeli wrote:
> Dodji Seketeli <dodji@redhat.com> writes:
> 
>> I am proposing a series of patches which is supposed to address the
>> remaining issues (I am aware of) preventing us from enabling the
>> -ftrack-macro-expansion by default.
>>
>> The idea is to address each issue I notice in the course of trying to
>> bootstrap the compiler and running the tests with
>> -ftrack-macro-expansion enabled.
>>
>> Beside the fixes, I ended up disabling the -ftrack-macro-expansion for
>> many test cases (sometimes globally in the dg-*.exp files, or on a
>> case by case basis), because that option changes the compiler output
>> and so requires that I either adapt the test case or disable the
>> option.  For other tests, I chose to adapt the test case.
> 
> I have finally applied this series of 14 patches to the mainline today.
> The SVN revisions are from r186965 to r186978.

s390 (31 bit) C++ bootstrap broke with revision 186977:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53280

Bye,

-Andreas-

Re: Patches to enable -ftrack-macro-expansion by default

[Gerald Pfeifer]

On Mon, 30 Apr 2012, Dodji Seketeli wrote:
> I have finally applied this series of 14 patches to the mainline today.
> The SVN revisions are from r186965 to r186978.

Shouldn't we document this in the release notes?

What do you guys think about the following?  Suggestions welcome.

Gerald

Index: changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.8/changes.html,v
retrieving revision 1.17
diff -u -3 -p -r1.17 changes.html
--- changes.html	20 Aug 2012 12:23:39 -0000	1.17
+++ changes.html	26 Aug 2012 00:27:32 -0000
@@ -56,9 +56,14 @@ by this change.</p>
 -->
 
 
-<!--
 <h3>C family</h3>
--->
+
+  <ul>
+    <li>The option <code>-ftrack-macro-expansion=2</code> is now
+    enabled by default. This allows the compiler to emit diagnostic
+    about the current macro expansion stack when a compilation error
+    occurs.</li>
+  </ul>
 
 <!--
 <h3>C</h3>

Re: Patches to enable -ftrack-macro-expansion by default

[Gabriel Dos Reis]

On Sat, Aug 25, 2012 at 7:28 PM, Gerald Pfeifer <gerald@pfeifer.com> wrote:
> On Mon, 30 Apr 2012, Dodji Seketeli wrote:
>> I have finally applied this series of 14 patches to the mainline today.
>> The SVN revisions are from r186965 to r186978.
>
> Shouldn't we document this in the release notes?
>
> What do you guys think about the following?  Suggestions welcome.

Good idea.  See suggestion below.

>
> Gerald
>
> Index: changes.html
> ===================================================================
> RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.8/changes.html,v
> retrieving revision 1.17
> diff -u -3 -p -r1.17 changes.html
> --- changes.html        20 Aug 2012 12:23:39 -0000      1.17
> +++ changes.html        26 Aug 2012 00:27:32 -0000
> @@ -56,9 +56,14 @@ by this change.</p>
>  -->
>
>
> -<!--
>  <h3>C family</h3>
> --->
> +
> +  <ul>
> +    <li>The option <code>-ftrack-macro-expansion=2</code> is now
> +    enabled by default. This allows the compiler to emit diagnostic
> +    about the current macro expansion stack when a compilation error
> +    occurs.</li>
> +  </ul>

What about replacing the last sentence with:

              This allows the compiler to display macro expansion stack in
               diagnostics.

I think we show the stack not just for errors, but for any diagnostics.

Re: Patches to enable -ftrack-macro-expansion by default

[Dodji Seketeli]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> I think we show the stack not just for errors, but for any diagnostics.

I agree, FWIW.

-- 
		Dodji

Re: Patches to enable -ftrack-macro-expansion by default

[Gerald Pfeifer]

On Sun, 26 Aug 2012, Dodji Seketeli wrote:
> Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
>> I think we show the stack not just for errors, but for any diagnostics.
> I agree, FWIW.

Good point, thanks for the feedback!

I have applied the following, updated patch.

Gerald

Index: changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.8/changes.html,v
retrieving revision 1.21
diff -u -3 -p -r1.21 changes.html
--- changes.html	26 Aug 2012 13:55:23 -0000	1.21
+++ changes.html	26 Aug 2012 15:59:47 -0000
@@ -56,9 +56,13 @@ by this change.</p>
 -->
 
 
-<!--
 <h3>C family</h3>
--->
+
+  <ul>
+    <li>The option <code>-ftrack-macro-expansion=2</code> is now
+    enabled by default. This allows the compiler to display the
+    macro expansion stack in diagnostics.</li>
+  </ul>
 
 <!--
 <h3>C</h3>

Re: Patches to enable -ftrack-macro-expansion by default

[Gabriel Dos Reis]

On Sun, Aug 26, 2012 at 3:00 PM, Gerald Pfeifer <gerald@pfeifer.com> wrote:

> I have applied the following, updated patch.

Thank you!

-- Gaby

Re: Patches to enable -ftrack-macro-expansion by default

[Uros Bizjak]

Hello!

> > I am proposing a series of patches which is supposed to address the
> > remaining issues (I am aware of) preventing us from enabling the
> > -ftrack-macro-expansion by default.
> >
> > The idea is to address each issue I notice in the course of trying to
> > bootstrap the compiler and running the tests with
> > -ftrack-macro-expansion enabled.
> >
> > Beside the fixes, I ended up disabling the -ftrack-macro-expansion for
> > many test cases (sometimes globally in the dg-*.exp files, or on a
> > case by case basis), because that option changes the compiler output
> > and so requires that I either adapt the test case or disable the
> > option.  For other tests, I chose to adapt the test case.
>
> I have finally applied this series of 14 patches to the mainline today.
> The SVN revisions are from r186965 to r186978.

Your patch introduced:

FAIL: gcc.dg/gomp/macro-4.c (internal compiler error)
FAIL: gcc.dg/gomp/macro-4.c (test for excess errors)

on alphaev68-pc-linux-gnu. The failure is silent on
x86_64-pc-linux-gnu, but can be uncovered by valgrind:

$ valgrind ~/gcc-build/gcc/cc1 -fopenmp -Wunknown-pragmas macro-4.c

==2461== Conditional jump or move depends on uninitialised value(s)
==2461==    at 0xE2B9D2: _cpp_pop_context (macro.c:2143)
==2461==    by 0xE1B1EF: skip_rest_of_line(cpp_reader*) (directives.c:207)
==2461==    by 0xE1BBFE: end_directive(cpp_reader*, int) (directives.c:298)
==2461==    by 0xE1E2B0:
_ZL19destringize_and_runP10cpp_readerPK10cpp_string.isra.6
(directives.c:1762)
==2461==    by 0xE1E473: _cpp_do__Pragma (directives.c:1833)
==2461==    by 0xE2B924: builtin_macro(cpp_reader*, cpp_hashnode*) (macro.c:409)
==2461==    by 0xE2BFC7: enter_macro_context(cpp_reader*,
cpp_hashnode*, cpp_token const*, unsigned int) (macro.c:1156)
==2461==    by 0xE2D66D: cpp_get_token_1(cpp_reader*, unsigned int*)
(macro.c:2391)
==2461==    by 0x584EB2: c_lex_with_flags(tree_node**, unsigned int*,
unsigned char*, int) (c-lex.c:302)
==2461==    by 0x532684: c_lex_one_token(c_parser*, c_token*) (c-parser.c:221)
==2461==    by 0x538F0D:
c_parser_compound_statement_nostart(c_parser*) (c-parser.c:401)
==2461==    by 0x54627B: c_parser_compound_statement(c_parser*)
(c-parser.c:4071)
==2461==

Uros.

Re: Patches to enable -ftrack-macro-expansion by default

[Dodji Seketeli]

Uros Bizjak <ubizjak@gmail.com> writes:

> Your patch introduced:
>
> FAIL: gcc.dg/gomp/macro-4.c (internal compiler error)
> FAIL: gcc.dg/gomp/macro-4.c (test for excess errors)
>
> on alphaev68-pc-linux-gnu. The failure is silent on
> x86_64-pc-linux-gnu, but can be uncovered by valgrind:
>
> $ valgrind ~/gcc-build/gcc/cc1 -fopenmp -Wunknown-pragmas macro-4.c
>
> ==2461== Conditional jump or move depends on uninitialised value(s)
> ==2461==    at 0xE2B9D2: _cpp_pop_context (macro.c:2143)
> ==2461==    by 0xE1B1EF: skip_rest_of_line(cpp_reader*) (directives.c:207)
> ==2461==    by 0xE1BBFE: end_directive(cpp_reader*, int)
> (directives.c:298)

I see.  Thank you for raising this, and sorry for the inconvenience.

I have a patchlet that seems to make the Valgrind error go away for me
on x86_64-unknown-linux-gnu.  The bootstrap is still underway.

Could you please test that it address the issue for you on
alphaev68-pc-linux-gnu?

Here is the patch, with its introductory text.  Thanks.

From 3e8a8ff118d6fbf038802348bac51ce1dbd26647 Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Tue, 1 May 2012 11:14:45 +0200
Subject: [PATCH] Properly initialize cpp_context in destringize_and_run

destringize_and_run forgets to initialize all the fields of the
cpp_context that it pushes.  Later _cpp_pop_context then gets confused
when it accesses context->tokens_kind via the call to macro_of_context
on context->prev.

The first hunk of this patch is the real obvious fix.  The second hunk
is just an assert that I am adding to err on the safe side.

Tested on x86_64-unknown-linux-gnu against trunk by running the
test gcc.dg/gomp/macro-4.c under Valgrind.  Bootstrap is still
underway.

libcpp/

	* directives.c (destringize_and_run): Properly initialize the new
	context.
	* macro.c (_cpp_pop_context): Assert that we shouldn't try to pop
	the initial base context, which has the same life time as the
	current instance of cpp_file.
---
 libcpp/directives.c |    4 +---
 libcpp/macro.c      |    4 ++++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/libcpp/directives.c b/libcpp/directives.c
index 0510c6e..c1c2415 100644
--- a/libcpp/directives.c
+++ b/libcpp/directives.c
@@ -1742,9 +1742,7 @@ destringize_and_run (cpp_reader *pfile, const cpp_string *in)
   saved_cur_run = pfile->cur_run;
 
   pfile->context = XNEW (cpp_context);
-  pfile->context->c.macro = 0;
-  pfile->context->prev = 0;
-  pfile->context->next = 0;
+  memset (pfile->context, 0, sizeof (cpp_context));
 
   /* Inline run_directive, since we need to delay the _cpp_pop_buffer
      until we've read all of the tokens that we want.  */
diff --git a/libcpp/macro.c b/libcpp/macro.c
index ab3e8f6..c4e2a23 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -2152,6 +2152,10 @@ _cpp_pop_context (cpp_reader *pfile)
 {
   cpp_context *context = pfile->context;
 
+  /* We should not be popping the base context.  */
+  if (context == &pfile->base_context)
+    abort ();
+
   if (context->c.macro)
     {
       cpp_hashnode *macro;
-- 
		Dodji

Re: Patches to enable -ftrack-macro-expansion by default

[Dodji Seketeli]

Dodji Seketeli <dodji@redhat.com> writes:

> Tested on x86_64-unknown-linux-gnu against trunk by running the
> test gcc.dg/gomp/macro-4.c under Valgrind.  Bootstrap is still
> underway.

For what it's worth, the patch passes bootstrap on
x86_64-unknown-linux-gnu.

> libcpp/
>
> 	* directives.c (destringize_and_run): Properly initialize the new
> 	context.
> 	* macro.c (_cpp_pop_context): Assert that we shouldn't try to pop
> 	the initial base context, which has the same life time as the
> 	current instance of cpp_file.

-- 
		Dodji

Re: Patches to enable -ftrack-macro-expansion by default

[Jason Merrill]

On 05/01/2012 05:36 AM, Dodji Seketeli wrote:
>     pfile->context = XNEW (cpp_context);
> +  memset (pfile->context, 0, sizeof (cpp_context));

You can use XCNEW instead of XNEW + memset.

OK with that change.

Jason

Re: Patches to enable -ftrack-macro-expansion by default

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> On 05/01/2012 05:36 AM, Dodji Seketeli wrote:
>>     pfile->context = XNEW (cpp_context);
>> +  memset (pfile->context, 0, sizeof (cpp_context));
>
> You can use XCNEW instead of XNEW + memset.
>
> OK with that change.

Thank you.

Here is the patch I will commit when Uros confirms that it fixes the
problem for him on alpha.

libcpp/

	* directives.c (destringize_and_run): Properly initialize the new
	context.
	* macro.c (_cpp_pop_context): Assert that we shouldn't try to pop
	the initial base context, which has the same life time as the
	current instance of cpp_file.
---
 libcpp/directives.c |    5 +----
 libcpp/macro.c      |    4 ++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/libcpp/directives.c b/libcpp/directives.c
index 0510c6e..e46280e 100644
--- a/libcpp/directives.c
+++ b/libcpp/directives.c
@@ -1741,10 +1741,7 @@ destringize_and_run (cpp_reader *pfile, const cpp_string *in)
   saved_cur_token = pfile->cur_token;
   saved_cur_run = pfile->cur_run;
 
-  pfile->context = XNEW (cpp_context);
-  pfile->context->c.macro = 0;
-  pfile->context->prev = 0;
-  pfile->context->next = 0;
+  pfile->context = XCNEW (cpp_context);
 
   /* Inline run_directive, since we need to delay the _cpp_pop_buffer
      until we've read all of the tokens that we want.  */
diff --git a/libcpp/macro.c b/libcpp/macro.c
index ab3e8f6..c4e2a23 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -2152,6 +2152,10 @@ _cpp_pop_context (cpp_reader *pfile)
 {
   cpp_context *context = pfile->context;
 
+  /* We should not be popping the base context.  */
+  if (context == &pfile->base_context)
+    abort ();
+
   if (context->c.macro)
     {
       cpp_hashnode *macro;
-- 
		Dodji

Re: Patches to enable -ftrack-macro-expansion by default

[Uros Bizjak]

On Tue, May 1, 2012 at 6:04 PM, Dodji Seketeli <dodji@redhat.com> wrote:

>>>     pfile->context = XNEW (cpp_context);
>>> +  memset (pfile->context, 0, sizeof (cpp_context));
>>
>> You can use XCNEW instead of XNEW + memset.
>>
>> OK with that change.
>
> Thank you.
>
> Here is the patch I will commit when Uros confirms that it fixes the
> problem for him on alpha.

Yes, the patch works OK on alpha and fixes the ICE in macro-4.c test.

Thanks,
Uros.

Re: Patches to enable -ftrack-macro-expansion by default

[Dodji Seketeli]

Uros Bizjak <ubizjak@gmail.com> writes:

> On Tue, May 1, 2012 at 6:04 PM, Dodji Seketeli <dodji@redhat.com> wrote:
>
>>>>     pfile->context = XNEW (cpp_context);
>>>> +  memset (pfile->context, 0, sizeof (cpp_context));
>>>
>>> You can use XCNEW instead of XNEW + memset.
>>>
>>> OK with that change.
>>
>> Thank you.
>>
>> Here is the patch I will commit when Uros confirms that it fixes the
>> problem for him on alpha.
>
> Yes, the patch works OK on alpha and fixes the ICE in macro-4.c test.

Thanks.  I have committed the patch to the mainline, at SVN revision
r187054.

-- 
		Dodji