Patchwork [3/7] Emit macro expansion related diagnostics

login
register
mail settings
Submitter Dodji Seketeli
Date Sept. 19, 2011, 1:58 p.m.
Message ID <m3y5xk8whp.fsf@redhat.com>
Download mbox | patch
Permalink /patch/115347/
State New
Headers show

Comments

Dodji Seketeli - Sept. 19, 2011, 1:58 p.m.
Jason Merrill <jason@redhat.com> writes:

> On 09/16/2011 03:55 AM, Dodji Seketeli wrote:
> > +    test.c: In function ‘g’:
> > +    test.c:5:14: error: invalid operands to binary << (have ‘double’ and ‘int’)
> > +    test.c:2:9: note: in expansion of macro 'OPERATE'
> > +    test.c:5:3: note: expanded from here
> > +    test.c:5:14: note: in expansion of macro 'SHIFTL'
> > +    test.c:8:3: note: expanded from here
> > +    test.c:8:3: note: in expansion of macro 'MULT2'
> > +    test.c:13:3: note: expanded from here
> > +
> > +   The part that goes from the third to the sixth line of this
> > +   diagnostic (the lines containing the 'note:' string) is called the
> > +   unwound macro expansion trace.  That's the part generated by this
> > +   function.
> 
> No UTF-8 in the code, please.

Fixed, sorry.  I actually copy/pasted the output from my terminal
after running gcc on the test case, and the terminal is in UTF-8.  And
so is my editor that runs in that terminal.

> 
> Do you mean eighth rather than sixth?

Yes I do.  Fixed.  Thanks for catching that.

> 
> > +      /* If the token at RESOLVED_LOCATION [at macro definition point]
> > +         is itself inside an expanded macro then we keep unwinding the
> > +         trace by reaching the "parent macro" that got expanded inside
> > +         the definition of the macro of MAP...  */
> 
> This doesn't make sense to me; a macro definition can't be inside an
> expanded macro.

Inside the definition of a macro M, you can have another macro M'.
And M' is going to be eventually expanded into a token FOO.

So, to paraphrase what I said earlier, FOO [which is at a point in the
definition of the macro M] is itself inside the expanded macro M'.

>  Can you describe this situation better?

Yes, sorry for the confusion.  I am trying to provide a better
explanation in the updated patch below.

> 
> > +  if (first_exp_point_map)
> > +    *first_exp_point_map = map;
> > +
> > +  /* Walk LOC_VEC and print the macro expansion trace, unless the
> > +     first macro which expansion triggered this trace was expanded
> > +     inside a system header.  */
> > +  if (!LINEMAP_SYSP (resolved_map))
> 
> Should the SYSP check use "map" rather than "resolved_map"?

Using map is clearer (and I changed it to that) but I think
resolved_map works too.

From: Dodji Seketeli <dodji@redhat.com>
Date: Sat, 4 Dec 2010 16:31:35 +0100
Subject: [PATCH 3/7] Emit macro expansion related diagnostics

In this third instalment the diagnostic machinery -- when faced with
the virtual location of a token resulting from macro expansion -- uses
the new linemap APIs to unwind the stack of macro expansions that led
to that token and emits a [hopefully] more useful message than what we
have today.

diagnostic_report_current_module has been slightly changed to use the
location given by client code instead of the global input_location
variable.  This results in more precise diagnostic locations in
general but then the patch adjusts some C++ tests which output changed
as a result of this.

Three new regression tests have been added.

The mandatory screenshot goes like this:

[dodji@adjoa gcc]$ cat -n test.c
     1    #define OPERATE(OPRD1, OPRT, OPRD2) \
     2      OPRD1 OPRT OPRD2;
     3
     4    #define SHIFTL(A,B) \
     5      OPERATE (A,<<,B)
     6
     7    #define MULT(A) \
     8      SHIFTL (A,1)
     9
    10    void
    11    g ()
    12    {
    13      MULT (1.0);/* 1.0 << 1; <-- so this is an error.  */
    14    }

[dodji@adjoa gcc]$ ./cc1 -quiet -ftrack-macro-expansion test.c
test.c: In function 'g':
test.c:5:14: erreur: invalid operands to binary << (have 'double' and 'int')
test.c:2:9: note: in expansion of macro 'OPERATE'
test.c:5:3: note: expanded from here
test.c:5:14: note: in expansion of macro 'SHIFTL'
test.c:8:3: note: expanded from here
test.c:8:3: note: in expansion of macro 'MULT2'
test.c:13:3: note: expanded from here

The combination of this patch and the previous ones boostrapped with
--enable-languages=all,ada and passed regression tests on
x86_64-unknown-linux-gnu.

gcc/
	* gcc/diagnostic.h (diagnostic_report_current_module): Add a
	location parameter.
	* diagnostic.c (diagnostic_report_current_module): Add a location
	parameter to the function definition.  Use it instead of
	input_location.  Resolve the virtual location rather than just
	looking up its map and risking to touch a resulting macro map.
	(default_diagnostic_starter): Pass the relevant diagnostic
	location to diagnostic_report_current_module.
	* tree-diagnostic.c (maybe_unwind_expanded_macro_loc): New.
	(virt_loc_aware_diagnostic_finalizer): Likewise.
	(diagnostic_report_current_function): Pass the
	relevant location to diagnostic_report_current_module.
	* tree-diagnostic.h (virt_loc_aware_diagnostic_finalizer): Declare
	new function.
	* toplev.c (general_init): By default, use the new
	virt_loc_aware_diagnostic_finalizer as diagnostic finalizer.
	* Makefile.in: Add vec.h dependency to tree-diagnostic.c.

gcc/cp/

	* error.c (cp_diagnostic_starter): Pass the relevant location to
	diagnostic_report_current_module.
	(cp_diagnostic_finalizer): Call virt_loc_aware_diagnostic_finalizer.

gcc/testsuite/

	* lib/prune.exp (prune_gcc_output):  Prune output referring to
	included files.
	* gcc.dg/cpp/macro-exp-tracking-1.c: New test.
	* gcc.dg/cpp/macro-exp-tracking-2.c: Likewise.
	* gcc.dg/cpp/macro-exp-tracking-3.c: Likewise.
	* gcc.dg/cpp/pragma-diagnostic-2.c: Likewise.
---
 gcc/Makefile.in                                 |    3 +-
 gcc/cp/error.c                                  |    5 +-
 gcc/diagnostic.c                                |   13 +-
 gcc/diagnostic.h                                |    2 +-
 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c |   21 +++
 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c |   21 +++
 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c |   14 ++
 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c |   14 ++
 gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c  |   34 ++++
 gcc/testsuite/lib/prune.exp                     |    1 +
 gcc/toplev.c                                    |    3 +
 gcc/tree-diagnostic.c                           |  212 ++++++++++++++++++++++-
 gcc/tree-diagnostic.h                           |    3 +-
 13 files changed, 335 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c
Jason Merrill - Sept. 19, 2011, 6:29 p.m.
On 09/19/2011 09:58 AM, Dodji Seketeli wrote:
> +   The part that goes from the third to the heighth line of this

An extra 'h' snuck in there. :)

> Inside the definition of a macro M, you can have another macro M'.
> And M' is going to be eventually expanded into a token FOO.
>
> So, to paraphrase what I said earlier, FOO [which is at a point in the
> definition of the macro M] is itself inside the expanded macro M'.

So what we're dealing with here is the token FOO.  We start with its 
fully-expanded location, and then step out to see where it was expanded 
from.  If that location is still within a macro expansion, we step out 
again until we are at the point in the source that originally triggered 
a macro expansion.  Right?

I don't understand how that unwinding operation maps onto a function 
called linemap_macro_map_loc_to_def_point, and why we need to use both 
that function and linemap_macro_map_loc_to_exp_point here.

I'm afraid this is leading me to question the basic model again.

> +      1    #define OPERATE(OPRD1, OPRT, OPRD2) \
> +      2      OPRD1 OPRT OPRD2;
> +      3
> +      4    #define SHIFTL(A,B) \
> +      5      OPERATE (A,<<,B)
> +      6
> +      7    #define MULT(A) \
> +      8      SHIFTL (A,1)
> +      9
> +     10    void
> +     11    g ()
> +     12    {
> +     13      MULT (1.0);// 1.0 << 1; <-- so this is an error.
> +     14    }

Here "1.0" has the locations 2(OPRD1), 5(A), 8(A), 13(1.0).  "<<" has 
the locations 2(OPRT), 5(<<), 8(SHIFTL), 13(MULT).  Each token has 4 
locations, one for each of the macros involved plus one for the original 
expansion location.  So it seems like we ought to be able to get away 
with only storing one location per token in a macro expansion map.  Am I 
missing something?

Jason
Jason Merrill - Sept. 19, 2011, 8:57 p.m.
On 09/19/2011 02:29 PM, Jason Merrill wrote:
> expansion location. So it seems like we ought to be able to get away
> with only storing one location per token in a macro expansion map. Am I
> missing something?

I notice that here:

> +        /* Resolve the location iter->where into the locus 1/ of the
> +           comment above.  */
> +        resolved_def_loc =
> +          linemap_resolve_location (line_table, iter->where,
> +                                    LRK_MACRO_PARM_REPLACEMENT_POINT, NULL);
> +
> +        /* 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.  */
> +        resolved_exp_loc =
> +          linemap_resolve_location (line_table,
> +                                    MACRO_MAP_EXPANSION_POINT_LOCATION (iter->map),
> +                                    LRK_MACRO_PARM_REPLACEMENT_POINT, NULL);

You're using LRK_MACRO_PARM_REPLACEMENT_POINT in both places for 
printing (and thus the second of the two token locations), whereas you 
used the first location in the unwinding process.

It is sounding to me like the first location (xI) gets you the next 
virtual location in the unwinding process, whereas the second location 
(yI) gets you the spelling location of the token in the definition of a 
macro.  Certainly all the calls to tokens_buff_add_token pass 
src->src_loc for the second.  So why don't we look up the second 
location in the macro definition when we need it rather than store a 
copy in the map?

Jason
Dodji Seketeli - Sept. 19, 2011, 10:30 p.m.
Jason Merrill <jason@redhat.com> writes:

> On 09/19/2011 09:58 AM, Dodji Seketeli wrote:
> > +   The part that goes from the third to the heighth line of this
> 
> An extra 'h' snuck in there. :)

Oops, fixed in my local copy, sorry.

> 
> > Inside the definition of a macro M, you can have another macro M'.
> > And M' is going to be eventually expanded into a token FOO.
> >
> > So, to paraphrase what I said earlier, FOO [which is at a point in the
> > definition of the macro M] is itself inside the expanded macro M'.
> 
> So what we're dealing with here is the token FOO.  We start with its
> fully-expanded location, and then step out to see where it was
> expanded from.  If that location is still within a macro expansion, we
> step out again until we are at the point in the source that originally
> triggered a macro expansion.  Right?

Right.

> I don't understand how that unwinding operation maps onto a function
> called linemap_macro_map_loc_to_def_point,

In the example I used in the comment of that function, suppose that
during 'stepping out', as you put it, we don't deal with the place in
the source where '<<' appears in the definition of a macro.  We'd then
only deal with the expansion points of the macros involved.  That
would mean that we would only record in the trace the following
locations (and be able to only display this trace):

1/

    test.c:5:14: error: invalid operands to binary << (have 'double' and 'int')
    test.c:5:3: note: expanded from here
    test.c:8:3: note: expanded from here
    test.c:13:3: note: expanded from here

But I also want to display where "<<" appears in the definition of the
relevant macro ...

> and why we need to use both that function and
> linemap_macro_map_loc_to_exp_point here.

... so that I can have a trace that shows the locations also in the
context of the definitions of the macros:

2/

    test.c:5:14: error: invalid operands to binary << (have 'double' and 'int')
    test.c:2:9: note: in expansion of macro 'OPERATE'
    test.c:5:3: note: expanded from here
    test.c:5:14: note: in expansion of macro 'SHIFTL'
    test.c:8:3: note: expanded from here
    test.c:8:3: note: in expansion of macro 'MULT2'
    test.c:13:3: note: expanded from here

> 
> I'm afraid this is leading me to question the basic model again.
> 
> > +      1    #define OPERATE(OPRD1, OPRT, OPRD2) \
> > +      2      OPRD1 OPRT OPRD2;
> > +      3
> > +      4    #define SHIFTL(A,B) \
> > +      5      OPERATE (A,<<,B)
> > +      6
> > +      7    #define MULT(A) \
> > +      8      SHIFTL (A,1)
> > +      9
> > +     10    void
> > +     11    g ()
> > +     12    {
> > +     13      MULT (1.0);// 1.0 << 1; <-- so this is an error.
> > +     14    }
> 
> Here "1.0" has the locations 2(OPRD1), 5(A), 8(A), 13(1.0).  "<<"
> has the locations 2(OPRT), 5(<<), 8(SHIFTL), 13(MULT).  Each token
> has 4 locations, one for each of the macros involved plus one for
> the original expansion location.  So it seems like we ought to be
> able to get away with only storing one location per token in a macro
> expansion map.

This is essentially what we do, for tokens of macro replacement list
that are not macro parameters.

In a function-like macro, tokens of macro parameters are replaced by
the tokens of the arguments; if we just store the spelling location of
a given parameter (and forget about the location of the matching
argument as you are suggesting), getting the location of that argument
at virtual location resolution time can be difficult.

In your example, suppose we didn't store the location of "<<" inside
the map for OPERATE, and that we only stored the location (2:9) of
OPRT, as you suggest.  How, to generate the trace, would we step out
to get the location (5:14) (of "<<") from that (2:9)?  For that, I
guess we'd need to store the locations of the arguments of OPERATE,
somehow, figure out that OPRT is the parameter for the argument "<<"
and act from there.  This is the argument replacement replay I talked
about in another thread when you first raised this point.  We'd need
to do some parts of what replace_args does and handle cases like
pasting etc.  Tom and I figured this would be a bit more difficult so
we preferred staying with this simpler scheme for now.  The simpler
scheme being to store the location of the argument "<<" and the
location of the parameter OPRT in the map.
Dodji Seketeli - Sept. 20, 2011, 7:23 a.m.
Jason Merrill <jason@redhat.com> writes:

> It is sounding to me like the first location (xI) gets you the next
> virtual location in the unwinding process, whereas the second location
> (yI) gets you the spelling location of the token in the definition of
> a macro.

Right.

> Certainly all the calls to tokens_buff_add_token pass src->src_loc for
> the second.  So why don't we look up the second location in the macro
> definition when we need it rather than store a copy in the map?

Because when you have the first location, looking up the second is not
easy.  Note that the information about the arguments of a function-like
macro is freed right in enter_macro_context by delete_macro_args once we
have recorded the locations for the macro expansion.  Getting the src
that matches the loc of a token coming from an argument of the macro
expansion, once that argument has been freed is not easier than just
storing a copy of the src->src_loc we need.  So Tom and I decided to let
that optimization for later once we are sure the whole thing works and
performs well enough.
Jason Merrill - Sept. 20, 2011, 1:59 p.m.
On 09/20/2011 03:23 AM, Dodji Seketeli wrote:
> Jason Merrill<jason@redhat.com>  writes:

>> Certainly all the calls to tokens_buff_add_token pass src->src_loc for
>> the second.  So why don't we look up the second location in the macro
>> definition when we need it rather than store a copy in the map?
>
> Because when you have the first location, looking up the second is not
> easy.

In linemap_macro_map_loc_to_def_point you get the token number and then 
use that to index into MACRO_MAP_LOCATIONS.  Can't you use the same 
token number to index into macro->exp.tokens instead?

Jason
Dodji Seketeli - Sept. 20, 2011, 2:05 p.m.
Jason Merrill <jason@redhat.com> writes:

> On 09/20/2011 03:23 AM, Dodji Seketeli wrote:
>> Jason Merrill<jason@redhat.com>  writes:
>
>>> Certainly all the calls to tokens_buff_add_token pass src->src_loc for
>>> the second.  So why don't we look up the second location in the macro
>>> definition when we need it rather than store a copy in the map?
>>
>> Because when you have the first location, looking up the second is not
>> easy.
>
> In linemap_macro_map_loc_to_def_point you get the token number and
> then use that to index into MACRO_MAP_LOCATIONS.  Can't you use the
> same token number to index into macro->exp.tokens instead?

No, because a macro argument can be made of several tokens.  So after
the first macro parameter of the replacement-list has been replaced by
the tokens of the argument, there is a shift between the indexes of the
tokens resulting from the replacement, and the original tokens of the
macro replacement list.
Jason Merrill - Sept. 21, 2011, 1:20 a.m.
On 09/20/2011 10:05 AM, Dodji Seketeli wrote:
> Jason Merrill<jason@redhat.com>  writes:

>> In linemap_macro_map_loc_to_def_point you get the token number and
>> then use that to index into MACRO_MAP_LOCATIONS.  Can't you use the
>> same token number to index into macro->exp.tokens instead?

> No, because a macro argument can be made of several tokens.

Ah, I see.

>> I don't understand how that unwinding operation maps onto a function
>> called linemap_macro_map_loc_to_def_point,

> In the example I used in the comment of that function, suppose that
> during 'stepping out', as you put it, we don't deal with the place in
> the source where '<<' appears in the definition of a macro.  We'd then
> only deal with the expansion points of the macros involved.

I think I wasn't expressing clearly enough the point I was trying to 
make.  My point was more that the name of the function is confusing; if 
what you get back is another virtual location, that's not what I would 
consider a "def point".  The only time you get a source location in the 
actual definition of the macro is when you ask for the "macro parm usage 
point".  And so when you go to print actual diagnostics you use 
LRK_MACRO_PARM_REPLACEMENT_POINT.

When we start out, we have a virtual location that represents << in the 
expansion of OPERATE.  Then we call linemap_macro_map_loc_to_def_point 
to get a virtual location that represents << in the expansion of SHIFTL. 
  This is not part of the definition of OPERATE, and shouldn't be 
described as such.

It seems that this function steps out until we hit the spelling 
location, and then we have a real source location and stop, which is why 
the loop in maybe_unwind_expanded_macro_loc needs to use 
linemap_macro_map_loc_to_exp_point as well.  Right?  In the example, 
once we hit << in SHIFTL unwinding needs to switch to the expansion point.

It seems to me that we should encapsulate all of this in a function that 
expresses this unwinding operation, say 
"linemap_macro_map_loc_unwind_once".  So the loop would look like

+  do
+    {
+      loc.where = where;
+      loc.map = map;
+
+      VEC_safe_push (loc_t, heap, loc_vec, &loc);
+
+      /* WHERE is the location of a token inside the expansion of a
+         macro.  MAP is the map holding the locations of that macro
+         expansion.  Let's get the location of the token in the
+         context that triggered the expansion of this macro.
+         This is basically how we go "down" in the trace of macro
+         expansions that led to WHERE.  */
+      where = linemap_unwind_once (where, &map);
+    } while (linemap_macro_expansion_map_p (map));

I think that linemap_macro_loc_to_def_point when called with false 
returns the unwound spelling location of the token (and thus is used for 
LRK_SPELLING_LOCATION), or when called with true returns the 
(not-unwound) location in the expanded macro (and so I think 
LRK_MACRO_PARM_REPLACEMENT_POINT should be renamed to 
LRK_MACRO_EXPANDED_LOCATION or something similar).

It seems to me that either we should split those functions apart in to 
two functions with clearer names, or bundle them and 
linemap_macro_loc_to_exp_point into linemap_resolve_location; if 
linemap_location_in_system_header_p used linemap_resolve_location it 
would be more readable anyway.

I'm having trouble thinking of a less misleading name for 
linemap_macro_map_loc_to_def_point, since the two locations serve 
completely different purposes.  Maybe something vague like 
linemap_macro_map_loc_get_stored_loc.  Or split it into two functions 
like linemap_virtual_loc_unwind_once_toward_spelling and 
linemap_virtual_loc_get_expanded_location or something like that.

What do you think?

Jason
Dodji Seketeli - Sept. 21, 2011, 6:32 p.m.
Jason Merrill <jason@redhat.com> writes:

> My point was more that the name of the function is confusing;

Sorry about that.

> if what you get back is another virtual location, that's not what I
> would consider a "def point".

For tokens that are not function-like macro arguments, I think the
linemap_macro_loc_to_def_point makes sense because what we get then is
the source location in the actual definition of the macro.  Actually I
think that's where the confusion comes from.  I first devised the name
while thinking of the case of macros that are not function-like.  And
now it falls short for the general case.  I should have caught that
but for some reason I was just seeing through the name is if it was
transparent.  Sigh.

I take your point; that name doesn't make sense in the general case.

> The only time you get a source location in the actual definition of
> the macro is when you ask for the "macro parm usage point".

Yes that, and in the case above; in which case xI and yI are equal, by
the way.

> When we start out, we have a virtual location that represents << in
> the expansion of OPERATE.  Then we call
> linemap_macro_map_loc_to_def_point to get a virtual location that
> represents << in the expansion of SHIFTL. This is not part of the
> definition of OPERATE, and shouldn't be described as such.

Agreed.

> It seems that this function steps out until we hit the spelling
> location, and then we have a real source location and stop, which is
> why the loop in maybe_unwind_expanded_macro_loc needs to use
> linemap_macro_map_loc_to_exp_point as well.  Right?

Right.

> It seems to me that we should encapsulate all of this in a function
> that expresses this unwinding operation, say
> "linemap_macro_map_loc_unwind_once".  So the loop would look like
> 
> +  do
> +    {
> +      loc.where = where;
> +      loc.map = map;
> +
> +      VEC_safe_push (loc_t, heap, loc_vec, &loc);
> +
> +      /* WHERE is the location of a token inside the expansion of a
> +         macro.  MAP is the map holding the locations of that macro
> +         expansion.  Let's get the location of the token in the
> +         context that triggered the expansion of this macro.
> +         This is basically how we go "down" in the trace of macro
> +         expansions that led to WHERE.  */
> +      where = linemap_unwind_once (where, &map);
> +    } while (linemap_macro_expansion_map_p (map));
> 

OK.

> I think that linemap_macro_loc_to_def_point when called with false
> returns the unwound spelling location of the token (and thus is used
> for LRK_SPELLING_LOCATION),

Right.

> or when called with true returns the (not-unwound) location in the
> expanded macro (and so I think LRK_MACRO_PARM_REPLACEMENT_POINT
> should be renamed to LRK_MACRO_EXPANDED_LOCATION or something
> similar).

FWIW, I'd like the LRK_MACRO_PARM_REPLACEMENT name (or its
replacement.  ha ha) to hint at the fact that it really has to do with
a token that is an /argument/ for a function-like macro.  So maybe
LRK_MACRO_PARM_FOR_ARG_LOCATION?  LRK_MACRO_EXPANDED_LOCATION really
seems too vague to me.  After all, pretty much everything is about
*EXPAND*ing macros here.  :-)

> It seems to me that either we should split those functions apart in
> to two functions with clearer names, or bundle them and
> linemap_macro_loc_to_exp_point into linemap_resolve_location; if
> linemap_location_in_system_header_p used linemap_resolve_location it
> would be more readable anyway.

OK.

> I'm having trouble thinking of a less misleading name for
> linemap_macro_map_loc_to_def_point, since the two locations serve
> completely different purposes.  Maybe something vague like
> linemap_macro_map_loc_get_stored_loc.  Or split it into two
> functions like linemap_virtual_loc_unwind_once_toward_spelling and
> linemap_virtual_loc_get_expanded_location or something like that.

So would a function named linemap_macro_map_loc_to_def_point that
returns the second location (yI) only, and a second function
linemap_macro_map_loc_unwind_once be less confusing to you?  If yes,
then I'll send an updated patch for that in a short while.

Thanks.
Jason Merrill - Sept. 22, 2011, 2:43 p.m.
On 09/21/2011 02:32 PM, Dodji Seketeli wrote:
> FWIW, I'd like the LRK_MACRO_PARM_REPLACEMENT name (or its
> replacement.  ha ha) to hint at the fact that it really has to do with
> a token that is an /argument/ for a function-like macro.

I disagree; arguments are the situation when the two locations differ, 
but this one (yI) is always the source location in the macro definition, 
while the first one (xI) is either that or, for macro arguments, a 
virtual location.  So the association with arguments seems backwards to me.

> So would a function named linemap_macro_map_loc_to_def_point that
> returns the second location (yI) only, and a second function
> linemap_macro_map_loc_unwind_once be less confusing to you?

Yes, that sounds good.

Jason
Dodji Seketeli - Sept. 26, 2011, 8:21 p.m.
Jason Merrill <jason@redhat.com> writes:

> On 09/21/2011 02:32 PM, Dodji Seketeli wrote:
>> FWIW, I'd like the LRK_MACRO_PARM_REPLACEMENT name (or its
>> replacement.  ha ha) to hint at the fact that it really has to do with
>> a token that is an /argument/ for a function-like macro.
>
> I disagree; arguments are the situation when the two locations differ,
> but this one (yI) is always the source location in the macro
> definition

I think an interesting question to ask here is "the source location of
what exactly?".

The value of the source location is always the same, yes, but its
meaning is different depending on if the token in question is an
argument or not.

When the token is not an argument, then yI is a source location for *that*
token in the macro definition.  It's then the spelling location for the
token in question.

When the token is an argument, then yI is a source location for
*another* token.  Namely, the parameter for that argument.  Not
necessarily the spelling location for the token we are observing.

> while the first one (xI) is either that or, for macro arguments, a
> virtual location.

Yes, but it's always a locus of the observed token.

Would LRK_MACRO_DEFINITION_LOCATION be better then?  :-)
Jason Merrill - Sept. 26, 2011, 8:45 p.m.
On 09/26/2011 04:21 PM, Dodji Seketeli wrote:
> Jason Merrill<jason@redhat.com>  writes:
>
>> On 09/21/2011 02:32 PM, Dodji Seketeli wrote:
>>> FWIW, I'd like the LRK_MACRO_PARM_REPLACEMENT name (or its
>>> replacement.  ha ha) to hint at the fact that it really has to do with
>>> a token that is an /argument/ for a function-like macro.
>>
>> I disagree; arguments are the situation when the two locations differ,
>> but this one (yI) is always the source location in the macro
>> definition
>
> I think an interesting question to ask here is "the source location of
> what exactly?".

It's the source location of what we're currently looking at, which is a 
virtual token in the macro expansion.

My point is that yI is always a source location, whereas xI may or may 
not be.

> Would LRK_MACRO_DEFINITION_LOCATION be better then?  :-)

Yes.  :)

Jason

Patch

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 92016f2..d26c682 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -2796,7 +2796,8 @@  tree-pretty-print.o : tree-pretty-print.c $(CONFIG_H) $(SYSTEM_H) \
    $(TM_H) coretypes.h tree-iterator.h $(SCEV_H) langhooks.h \
    $(TREE_PASS_H) value-prof.h output.h tree-pretty-print.h
 tree-diagnostic.o : tree-diagnostic.c $(CONFIG_H) $(SYSTEM_H) coretypes.h \
-   $(TREE_H) $(DIAGNOSTIC_H) tree-diagnostic.h langhooks.h $(LANGHOOKS_DEF_H)
+   $(TREE_H) $(DIAGNOSTIC_H) tree-diagnostic.h langhooks.h $(LANGHOOKS_DEF_H) \
+   $(VEC_H)
 fold-const.o : fold-const.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) \
    $(TREE_H) $(FLAGS_H) $(DIAGNOSTIC_CORE_H) $(HASHTAB_H) $(EXPR_H) $(RTL_H) \
    $(GGC_H) $(TM_P_H) langhooks.h $(MD5_H) intl.h $(TARGET_H) \
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 598ddf1..8fa163f 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -2767,7 +2767,7 @@  static void
 cp_diagnostic_starter (diagnostic_context *context,
 		       diagnostic_info *diagnostic)
 {
-  diagnostic_report_current_module (context);
+  diagnostic_report_current_module (context, diagnostic->location);
   cp_print_error_function (context, diagnostic);
   maybe_print_instantiation_context (context);
   maybe_print_constexpr_context (context);
@@ -2777,8 +2777,9 @@  cp_diagnostic_starter (diagnostic_context *context,
 
 static void
 cp_diagnostic_finalizer (diagnostic_context *context,
-			 diagnostic_info *diagnostic ATTRIBUTE_UNUSED)
+			 diagnostic_info *diagnostic)
 {
+  virt_loc_aware_diagnostic_finalizer (context, diagnostic);
   pp_base_destroy_prefix (context->printer);
 }
 
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index b46eb35..0344937 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -255,9 +255,9 @@  diagnostic_action_after_output (diagnostic_context *context,
 }
 
 void
-diagnostic_report_current_module (diagnostic_context *context)
+diagnostic_report_current_module (diagnostic_context *context, location_t where)
 {
-  const struct line_map *map;
+  const struct line_map *map = NULL;
 
   if (pp_needs_newline (context->printer))
     {
@@ -265,10 +265,13 @@  diagnostic_report_current_module (diagnostic_context *context)
       pp_needs_newline (context->printer) = false;
     }
 
-  if (input_location <= BUILTINS_LOCATION)
+  if (where <= BUILTINS_LOCATION)
     return;
 
-  map = linemap_lookup (line_table, input_location);
+  linemap_resolve_location (line_table, where,
+			    LRK_MACRO_PARM_REPLACEMENT_POINT,
+			    &map);
+
   if (map && diagnostic_last_module_changed (context, map))
     {
       diagnostic_set_last_module (context, map);
@@ -301,7 +304,7 @@  void
 default_diagnostic_starter (diagnostic_context *context,
 			    diagnostic_info *diagnostic)
 {
-  diagnostic_report_current_module (context);
+  diagnostic_report_current_module (context, diagnostic->location);
   pp_set_prefix (context->printer, diagnostic_build_prefix (context,
 							    diagnostic));
 }
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 8074354..4b1265b 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -253,7 +253,7 @@  extern diagnostic_context *global_dc;
 /* Diagnostic related functions.  */
 extern void diagnostic_initialize (diagnostic_context *, int);
 extern void diagnostic_finish (diagnostic_context *);
-extern void diagnostic_report_current_module (diagnostic_context *);
+extern void diagnostic_report_current_module (diagnostic_context *, location_t);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
 extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *,
diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c
new file mode 100644
index 0000000..d975c8c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c
@@ -0,0 +1,21 @@ 
+/*
+   { dg-options "-ftrack-macro-expansion=1" }
+   { dg-do compile }
+*/
+
+#define OPERATE(OPRD1, OPRT, OPRD2) \
+do \
+{ \
+  OPRD1 OPRT OPRD2; /* { dg-message "expansion" }*/ 	   \
+} while (0)
+
+#define SHIFTL(A,B) \
+  OPERATE (A,<<,B) /* { dg-message "expanded|expansion" } */
+
+void
+foo ()
+{
+  SHIFTL (0.1,0.2); /* { dg-message "expanded" } */
+}
+
+/* { dg-error "invalid operands" "" { target *-*-* } 13 } */
diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c
new file mode 100644
index 0000000..684af4c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c
@@ -0,0 +1,21 @@ 
+/* 
+   { dg-options "-ftrack-macro-expansion=1" }
+   { dg-do compile }
+*/
+
+#define OPERATE(OPRD1, OPRT, OPRD2) \
+ OPRD1 OPRT OPRD2;		/* { dg-message "expansion" } */
+
+#define SHIFTL(A,B) \
+  OPERATE (A,<<,B) /* { dg-message "expanded|expansion" } */
+
+#define MULT(A) \
+  SHIFTL (A,1)			/* { dg-message "expanded|expansion" } */
+
+void
+foo ()
+{
+  MULT (1.0);			/* { dg-message "expanded" } */
+}
+
+/* { dg-error "invalid operands to binary <<" "" { target *-*-* } { 10 } } */
diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c
new file mode 100644
index 0000000..119053e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c
@@ -0,0 +1,14 @@ 
+/*
+  { dg-options "-fshow-column -ftrack-macro-expansion=1" }
+  { dg-do compile }
+ */
+
+#define SQUARE(A) A * A		/* { dg-message "expansion" } */
+
+void
+foo()
+{
+  SQUARE (1 << 0.1);		/* { dg-message "expanded" } */
+}
+
+/* { dg-error "16:invalid operands to binary <<" "" {target *-*-* } { 11 } } */
diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c
new file mode 100644
index 0000000..1f9fe6a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c
@@ -0,0 +1,14 @@ 
+/*
+  { dg-options "-fshow-column -ftrack-macro-expansion=2" }
+  { dg-do compile }
+ */
+
+#define SQUARE(A) A * A		/* { dg-message "expansion" } */
+
+void
+foo()
+{
+  SQUARE (1 << 0.1);		/* { dg-message "expanded" } */
+}
+
+/* { dg-error "13:invalid operands to binary <<" "" { target *-*-* } { 11 } } */
diff --git a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c
new file mode 100644
index 0000000..7ab95b0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c
@@ -0,0 +1,34 @@ 
+/*
+  { dg-options "-Wuninitialized -ftrack-macro-expansion=2" }
+  { dg-do compile }
+*/
+
+void f (unsigned);
+
+#define CODE_WITH_WARNING \
+  int a; /* { dg-message "expansion|declared here" } */  \
+  f (a)	 /* { dg-message "expansion" } */
+
+#pragma GCC diagnostic ignored "-Wuninitialized"
+
+void
+g (void)
+{
+  CODE_WITH_WARNING;
+}
+
+#pragma GCC diagnostic push
+
+#pragma GCC diagnostic error "-Wuninitialized"
+
+void
+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/testsuite/lib/prune.exp b/gcc/testsuite/lib/prune.exp
index 4683f93..09d2581 100644
--- a/gcc/testsuite/lib/prune.exp
+++ b/gcc/testsuite/lib/prune.exp
@@ -29,6 +29,7 @@  proc prune_gcc_output { text } {
     regsub -all "(^|\n)collect: re(compiling|linking)\[^\n\]*" $text "" text
     regsub -all "(^|\n)Please submit.*instructions\[^\n\]*" $text "" text
     regsub -all "(^|\n)\[0-9\]\[0-9\]* errors\." $text "" text
+    regsub -all "(^|\n)(In file included|\[ \]+from)\[^\n\]*" $text "" text
 
     # Ignore informational notes.
     regsub -all "(^|\n)\[^\n\]*: note: \[^\n\]*" $text "" text
diff --git a/gcc/toplev.c b/gcc/toplev.c
index de0a58a..5f63b69 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1132,6 +1132,9 @@  general_init (const char *argv0)
      can give warnings and errors.  */
   diagnostic_initialize (global_dc, N_OPTS);
   diagnostic_starter (global_dc) = default_tree_diagnostic_starter;
+  /* By default print macro expansion contexts in the diagnostic
+     finalizer -- for tokens resulting from macro macro expansion.  */
+  diagnostic_finalizer (global_dc) = virt_loc_aware_diagnostic_finalizer;
   /* Set a default printer.  Language specific initializations will
      override it later.  */
   pp_format_decoder (global_dc->printer) = &default_tree_printer;
diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c
index b456a2a..5a7e6c2 100644
--- a/gcc/tree-diagnostic.c
+++ b/gcc/tree-diagnostic.c
@@ -28,6 +28,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "tree-diagnostic.h"
 #include "langhooks.h"
 #include "langhooks-def.h"
+#include "vec.h"
 
 /* Prints out, if necessary, the name of the current function
    that caused an error.  Called from all error and warning functions.  */
@@ -35,7 +36,7 @@  void
 diagnostic_report_current_function (diagnostic_context *context,
 				    diagnostic_info *diagnostic)
 {
-  diagnostic_report_current_module (context);
+  diagnostic_report_current_module (context, diagnostic->location);
   lang_hooks.print_error_function (context, input_filename, diagnostic);
 }
 
@@ -47,3 +48,212 @@  default_tree_diagnostic_starter (diagnostic_context *context,
   pp_set_prefix (context->printer, diagnostic_build_prefix (context,
 							    diagnostic));
 }
+
+/* This is a pair made of a location and the line map it originated
+   from.  It's used in the maybe_unwind_expanded_macro_loc function
+   below.  */
+typedef struct
+{
+  const struct line_map *map;
+  source_location where;
+} loc_t;
+
+DEF_VEC_O (loc_t);
+DEF_VEC_ALLOC_O (loc_t, heap);
+
+/* Unwind the different macro expansions that lead to the token which
+   location is WHERE and emit diagnostics showing the resulting
+   unwound macro expansion trace.  Let's look at an example to see how
+   the trace looks like.  Suppose we have this piece of code,
+   artificially annotated with the line numbers to increase
+   legibility:
+
+    $ cat -n test.c
+      1    #define OPERATE(OPRD1, OPRT, OPRD2) \
+      2      OPRD1 OPRT OPRD2;
+      3
+      4    #define SHIFTL(A,B) \
+      5      OPERATE (A,<<,B)
+      6
+      7    #define MULT(A) \
+      8      SHIFTL (A,1)
+      9
+     10    void
+     11    g ()
+     12    {
+     13      MULT (1.0);// 1.0 << 1; <-- so this is an error.
+     14    }
+
+   Here is the diagnostic that we want the compiler to generate:
+
+    test.c: In function 'g':
+    test.c:5:14: error: invalid operands to binary << (have 'double' and 'int')
+    test.c:2:9: note: in expansion of macro 'OPERATE'
+    test.c:5:3: note: expanded from here
+    test.c:5:14: note: in expansion of macro 'SHIFTL'
+    test.c:8:3: note: expanded from here
+    test.c:8:3: note: in expansion of macro 'MULT2'
+    test.c:13:3: note: expanded from here
+
+   The part that goes from the third to the heighth line of this
+   diagnostic (the lines containing the 'note:' string) is called the
+   unwound macro expansion trace.  That's the part generated by this
+   function.
+
+   If FIRST_EXP_POINT_MAP is non-null, *FIRST_EXP_POINT_MAP is set to
+   the map of the location in the source that first triggered the
+   macro expansion.  This must be an ordinary map.  */
+
+static void
+maybe_unwind_expanded_macro_loc (diagnostic_context *context,
+                                 diagnostic_info *diagnostic,
+                                 source_location where,
+                                 const struct line_map **first_exp_point_map)
+{
+  const struct line_map *map, *resolved_map;
+  source_location resolved_location;
+  VEC(loc_t,heap) *loc_vec = NULL;
+  unsigned ix;
+  loc_t loc, *iter;
+
+  map = linemap_lookup (line_table, where);
+  if (!linemap_macro_expansion_map_p (map))
+    return;
+
+  /* Let's unwind the macros that got expanded and led to the token
+     which location is WHERE.  We are going to store these macros into
+     LOC_VEC, so that we can later walk it at our convenience to
+     display a somewhat meaningful trace of the macro expansion
+     history to the user.  Note that the first macro of the trace
+     (which is OPERATE in the example above) is going to be stored at
+     the beginning of LOC_VEC.  */
+
+  do
+    {
+      loc.where = where;
+      loc.map = map;
+
+      VEC_safe_push (loc_t, heap, loc_vec, &loc);
+
+      /* WHERE is the location of a token inside the expansion of a
+         macro.  MAP is the map holding the locations of that macro
+         expansion.  Let's get the location of the token inside the
+         *definition* of the macro of MAP, that got expanded at WHERE.
+         This is basically how we go "down" in the trace of macro
+         expansions that led to WHERE.  */
+      resolved_location =
+        linemap_macro_map_loc_to_def_point (map, where, false);
+      resolved_map = linemap_lookup (line_table, resolved_location);
+
+      /* In the definition of a macro M, we can have another macro M'.
+	 M' is eventually going to be expanded into a token FOO, which
+	 location is RESOLVED_LOCATION.  MAP would be the map of M.
+
+	 So if we are in this case, i.e If FOO is itself inside an
+         expanded macro M' then we keep unwinding the trace to reach
+         the "parent macro" M'.  RESOLVED_MAP would then be the map of
+         M'.  */
+      if (linemap_macro_expansion_map_p (resolved_map))
+        {
+          where = resolved_location;
+          map = resolved_map;
+        }
+      else
+        {
+          /* Otherwise, let's consider the location of the expansion
+             point of the macro of MAP.  Keep in mind that MAP is a
+             macro expansion map.  To get a "normal map" (i.e a non
+             macro expansion map) and be done with the unwinding, we
+             must either consider the location of the expansion point
+             of the macro or the location of the token inside the
+             macro definition that got expanded to WHERE.  */
+          where =
+            linemap_macro_map_loc_to_exp_point (map, where);
+          map = linemap_lookup (line_table, where);
+        }
+    } while (linemap_macro_expansion_map_p (map));
+
+  if (first_exp_point_map)
+    *first_exp_point_map = map;
+
+  /* Walk LOC_VEC and print the macro expansion trace, unless the
+     first macro which expansion triggered this trace was expanded
+     inside a system header.  */
+  if (!LINEMAP_SYSP (map))
+    FOR_EACH_VEC_ELT (loc_t, loc_vec, ix, iter)
+      {
+        source_location resolved_def_loc = 0, resolved_exp_loc = 0;
+        diagnostic_t saved_kind;
+        const char *saved_prefix;
+        source_location saved_location;
+
+        /* Okay, now here is what we want.  For each token resulting
+           from macro expansion we want to show: 1/ where in the
+           definition of the macro the token comes from; 2/ where the
+           macro got expanded.  */
+
+        /* Resolve the location iter->where into the locus 1/ of the
+           comment above.  */
+        resolved_def_loc =
+          linemap_resolve_location (line_table, iter->where,
+                                    LRK_MACRO_PARM_REPLACEMENT_POINT, NULL);
+
+        /* 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.  */
+        resolved_exp_loc =
+          linemap_resolve_location (line_table,
+                                    MACRO_MAP_EXPANSION_POINT_LOCATION (iter->map),
+                                    LRK_MACRO_PARM_REPLACEMENT_POINT, NULL);
+
+        saved_kind = diagnostic->kind;
+        saved_prefix = context->printer->prefix;
+        saved_location = diagnostic->location;
+
+        diagnostic->kind = DK_NOTE;
+        diagnostic->location = resolved_def_loc;
+        pp_base_set_prefix (context->printer,
+                            diagnostic_build_prefix (context,
+                                                     diagnostic));
+        pp_newline (context->printer);
+        pp_printf (context->printer, "in expansion of macro '%s'",
+                   linemap_map_get_macro_name (iter->map));
+        pp_destroy_prefix (context->printer);
+
+        diagnostic->location = resolved_exp_loc;
+        pp_base_set_prefix (context->printer,
+                            diagnostic_build_prefix (context,
+                                                     diagnostic));
+        pp_newline (context->printer);
+        pp_printf (context->printer, "expanded from here");
+        pp_destroy_prefix (context->printer);
+
+        diagnostic->kind = saved_kind;
+        diagnostic->location = saved_location;
+        context->printer->prefix = saved_prefix;
+      }
+
+  VEC_free (loc_t, heap, loc_vec);
+}
+
+/*  This is a diagnostic finalizer implementation that is aware of
+    virtual locations produced by libcpp.
+
+    It has to be called by the diagnostic finalizer of front ends that
+    uses libcpp and wish to get diagnostics involving tokens resulting
+    from macro expansion.
+
+    For a given location, if said location belongs to a token
+    resulting from a macro expansion, this starter prints the context
+    of the token.  E.g, for multiply nested macro expansion, it
+    unwinds the nested macro expansions and prints them in a manner
+    that is similar to what is done for function call stacks, or
+    template instantiation contexts.  */
+void
+virt_loc_aware_diagnostic_finalizer (diagnostic_context *context,
+				     diagnostic_info *diagnostic)
+{
+  maybe_unwind_expanded_macro_loc (context, diagnostic,
+				   diagnostic->location,
+				   NULL);
+}
diff --git a/gcc/tree-diagnostic.h b/gcc/tree-diagnostic.h
index 7d88089..6b8e8e6 100644
--- a/gcc/tree-diagnostic.h
+++ b/gcc/tree-diagnostic.h
@@ -52,5 +52,6 @@  along with GCC; see the file COPYING3.  If not see
 void default_tree_diagnostic_starter (diagnostic_context *, diagnostic_info *);
 extern void diagnostic_report_current_function (diagnostic_context *,
 						diagnostic_info *);
-
+void virt_loc_aware_diagnostic_finalizer (diagnostic_context *,
+					  diagnostic_info *);
 #endif /* ! GCC_TREE_DIAGNOSTIC_H */