Make unwound macro expansion trace less redundant

As discussed previously, the unwinder for macro expansion is quite
verbose [1].  This patch proposes to address that shortcoming.

Consider this test case:

    $ cat -n test.c
	 1	#define MYMAX(A,B) __extension__ ({ __typeof__(A) __a = (A); \
	 2	 __typeof__(B) __b = (B); __a < __b ? __b : __a; })
	 3
	 4	struct mystruct {};
	 5	void
	 6	foo()
	 7	{
	 8	  struct mystruct p;
	 9	  float f = 0.0;
	10	  MYMAX (p, f);
	11	}
    $

The output of the compiler from trunk yields:

    $ cc1 -quiet ./test.c
    ./test.c: In function ‘foo’:
    ./test.c:2:31: error: invalid operands to binary < (have ‘struct mystruct’ and ‘float’)
      __typeof__(B) __b = (B); __a < __b ? __b : __a; })
				   ^
    ./test.c:2:31: note: in expansion of macro 'MYMAX'
      __typeof__(B) __b = (B); __a < __b ? __b : __a; })
				   ^
    ./test.c:10:3: note: expanded from here
       MYMAX (p, f);
       ^
    $

After this patch, the compiler yields:

    $ ./cc1 -quiet ./test.c
    ./test.c: In function ‘foo’:
    ./test.c:2:31: error: invalid operands to binary < (have ‘struct mystruct’ and ‘float’)
      __typeof__(B) __b = (B); __a < __b ? __b : __a; })
				   ^
    ./test.c:10:3: note: in expansion of macro 'MYMAX'
       MYMAX (p, f);
       ^
    $

The gotcha is, in the general case, we cannot simply eliminate the
context of the macro definition.  That is, the line from the first
output that is redundant with the first diagnostic line that has
line/column number:

    ./test.c:2:31: note: in expansion of macro 'MYMAX'
      __typeof__(B) __b = (B); __a < __b ? __b : __a; })
                                   ^

We cannot simply eliminate that context of macro definition because
there are cases where the first diagnostic that has a line/column
number doesn't point to a location inside the definition of the macro
where the relevant token is used.  For instance:

    $ cat -n test2.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	}
    $

Which yields without the patch:

    $ cc1 -quiet ./test2.c
    ./test2.c: In function ‘g’:
    ./test2.c:5:14: error: invalid operands to binary << (have ‘double’ and ‘int’)
       OPERATE (A,<<,B)
		  ^
    ./test2.c:2:9: note: in expansion of macro 'OPERATE'
       OPRD1 OPRT OPRD2;
	     ^
    ./test2.c:5:3: note: expanded from here
       OPERATE (A,<<,B)
       ^
    ./test2.c:5:14: note: in expansion of macro 'SHIFTL'
       OPERATE (A,<<,B)
		  ^
    ./test2.c:8:3: note: expanded from here
       SHIFTL (A,1)
       ^
    ./test2.c:8:3: note: in expansion of macro 'MULT'
       SHIFTL (A,1)
       ^
    ./test2.c:13:3: note: expanded from here
       MULT (1.0);// 1.0 << 1; <-- so this is an error.
       ^
    $

Here, the line that has the context of macro definition:

    ./test2.c:2:9: note: in expansion of macro 'OPERATE'
       OPRD1 OPRT OPRD2;
	     ^
is useful, because the first diagnostic that has line/column number
wasn't pointing into the definition of the macro OPERATE, where the
token '<<' is used.

    ./test2.c:5:14: error: invalid operands to binary << (have ‘double’ and ‘int’)
       OPERATE (A,<<,B)
		  ^
So in this this case, displaying the macro definition context is not
redundant.  I think it is even desirable.

The patch changes the output in that case to be:

    ./test2.c: In function ‘g’:
    ./test2.c:5:14: erreur: invalid operands to binary << (have ‘double’ and ‘int’)
       OPERATE (A,<<,B)
		  ^
    ./test2.c:2:9: note: in definition of macro 'OPERATE'
       OPRD1 OPRT OPRD2;
	     ^
    ./test2.c:8:3: note: in expansion of macro 'SHIFTL'
       SHIFTL (A,1)
       ^
    ./test2.c:13:3: note: in expansion of macro 'MULT'
       MULT (1.0);// 1.0 << 1; <-- so this is an error.
       ^
    $

It's shorter, but I believe it has all the information that was
present before the patch.

[1]: http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00321.html

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

gcc/

	Make unwound macro expansion trace less redundant
	* tree-diagnostic.c (maybe_unwind_expanded_macro_loc): Don't print
	context of macro definition in the trace, when it's redundant.
	Update comments.

gcc/testsuite/

	Make unwound macro expansion trace less redundant
	* gcc.dg/cpp/macro-exp-tracking-1.c: Adjust.
	* gcc.dg/cpp/macro-exp-tracking-2.c: Likewise.
	* gcc.dg/cpp/macro-exp-tracking-3.c: Likewise.
	* gcc.dg/cpp/macro-exp-tracking-4.c: Likewise.
	* gcc.dg/cpp/macro-exp-tracking-5.c: Likewise.
	* gcc.dg/cpp/pragma-diagnostic-2.c: Likewise.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@187845 138bc75d-0d04-0410-961f-82ee72b054a4

1 files changed, 69 insertions, 24 deletions

diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c
index cbdbb778259..774b6c44dd0 100644
--- a/gcc/tree-diagnostic.c
+++ b/gcc/tree-diagnostic.c
@@ -89,16 +89,13 @@ DEF_VEC_ALLOC_O (loc_map_pair, heap);
 
    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 'MULT'
-    test.c:13:3: note: expanded from here
-
-   The part that goes from the third to the eighth line of this
+    test.c: In function ‘g’:
+    test.c:5:14: error: invalid operands to binary << (have ‘double’ and ‘int’)
+    test.c:2:9: note: in definition of macro 'OPERATE'
+    test.c:8:3: note: in expansion of macro 'SHIFTL'
+    test.c:13:3: note: in expansion of macro 'MULT'
+
+   The part that goes from the third to the fifth 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.  */
@@ -150,10 +147,38 @@ maybe_unwind_expanded_macro_loc (diagnostic_context *context,
   if (!LINEMAP_SYSP (map))
     FOR_EACH_VEC_ELT (loc_map_pair, loc_vec, ix, iter)
       {
-        source_location resolved_def_loc = 0, resolved_exp_loc = 0;
+        source_location resolved_def_loc = 0, resolved_exp_loc = 0,
+	  saved_location = 0;
+	int resolved_def_loc_line = 0, saved_location_line = 0;
         diagnostic_t saved_kind;
         const char *saved_prefix;
-        source_location saved_location;
+	/* Sometimes, in the unwound macro expansion trace, we want to
+	   print a part of the context that shows where, in the
+	   definition of the relevant macro, is the token (we are
+	   looking at) used.  That is the case in the introductory
+	   comment of this function, where we print:
+
+	       test.c:2:9: note: in definition of macro 'OPERATE'.
+
+	   We print that "macro definition context" because the
+	   diagnostic line (emitted by the call to
+	   pp_ouput_formatted_text in diagnostic_report_diagnostic):
+
+	       test.c:5:14: error: invalid operands to binary << (have ‘double’ and ‘int’)
+
+	   does not point into the definition of the macro where the
+	   token '<<' (that is an argument to the function-like macro
+	   OPERATE) is used.  So we must "display" the line of that
+	   macro definition context to the user somehow.
+
+	   A contrario, when the first interesting diagnostic line
+	   points into the definition of the macro, we don't need to
+	   display any line for that macro definition in the trace
+	   anymore, otherwise it'd be redundant.
+
+	   This flag is true when we need to display the context of
+	   the macro definition.  */
+	bool print_definition_context_p = false;
 
         /* Okay, now here is what we want.  For each token resulting
            from macro expansion we want to show: 1/ where in the
@@ -176,6 +201,8 @@ maybe_unwind_expanded_macro_loc (diagnostic_context *context,
 	  if (l < RESERVED_LOCATION_COUNT
 	      || LINEMAP_SYSP (m))
 	    continue;
+
+	  resolved_def_loc_line = SOURCE_LINE (m, l);
 	}
 
         /* Resolve the location of the expansion point of the macro
@@ -189,22 +216,40 @@ maybe_unwind_expanded_macro_loc (diagnostic_context *context,
         saved_kind = diagnostic->kind;
         saved_prefix = pp_get_prefix (context->printer);
         saved_location = diagnostic->location;
+	saved_location_line =
+	  expand_location_to_spelling_point (saved_location).line;
 
         diagnostic->kind = DK_NOTE;
-        diagnostic->location = resolved_def_loc;
-        pp_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_show_locus (context, diagnostic);
 
-        diagnostic->location = resolved_exp_loc;
-        pp_set_prefix (context->printer,
+	/* We need to print the context of the macro definition only
+	   when the locus of the first displayed diagnostic (displayed
+	   before this trace) was inside the definition of the
+	   macro.  */
+	print_definition_context_p =
+	  (ix == 0 && (saved_location_line != resolved_def_loc_line));
+
+	if (print_definition_context_p)
+	  {
+	    diagnostic->location = resolved_def_loc;
+	    pp_set_prefix (context->printer,
+			   diagnostic_build_prefix (context, diagnostic));
+	    pp_newline (context->printer);
+	    pp_printf (context->printer, "in definition of macro '%s'",
+		       linemap_map_get_macro_name (iter->map));
+	    pp_destroy_prefix (context->printer);
+	    diagnostic_show_locus (context, diagnostic);
+	    /* At this step, as we've printed the context of the macro
+	       definition, we don't want to print the context of its
+	       expansion, otherwise, it'd be redundant.  */
+	    continue;
+	  }
+
+	diagnostic->location = resolved_exp_loc;
+	pp_set_prefix (context->printer,
                        diagnostic_build_prefix (context, diagnostic));
-        pp_newline (context->printer);
-        pp_string (context->printer, "expanded from here");
+	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_show_locus (context, diagnostic);