[PATCH diagnostics] Make unwound macro expansion trace less redundant
Dodji Seketeli
dodji@redhat.com
Tue May 15 11:15:00 GMT 2012
Hello,
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.
---
.../g++.dg/warn/Wconversion-real-integer2.C | 2 +-
gcc/testsuite/g++.dg/warn/Wdouble-promotion.C | 2 +-
gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c | 8 +-
gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c | 9 +-
gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c | 6 +-
gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c | 5 +-
gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c | 6 +-
gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c | 2 +-
gcc/tree-diagnostic.c | 93 +++++++++++++++-----
9 files changed, 86 insertions(+), 47 deletions(-)
diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
index 29130f1..6a95b0e 100644
--- a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
+++ b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C
@@ -29,5 +29,5 @@ float vfloat;
void h (void)
{
- vfloat = INT_MAX; // { dg-message "expanded from here" }
+ vfloat = INT_MAX; // { dg-message "in expansion of macro 'INT_MAX'" }
}
diff --git a/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C b/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C
index 98d2eed..afd9a20 100644
--- a/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C
+++ b/gcc/testsuite/g++.dg/warn/Wdouble-promotion.C
@@ -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-message "expanded from here" } */
+ local_cf = cf + 1.0 * ID; /* { dg-message "in expansion of macro 'ID'" } */
local_cf = cf - cd; /* { dg-warning "implicit" } */
local_f = i ? f : d; /* { dg-warning "implicit" } */
diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c
index d975c8c..28ef795 100644
--- a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c
@@ -6,16 +6,14 @@
#define OPERATE(OPRD1, OPRT, OPRD2) \
do \
{ \
- OPRD1 OPRT OPRD2; /* { dg-message "expansion" }*/ \
+ OPRD1 OPRT OPRD2; /* { dg-message "definition" }*/ \
} while (0)
#define SHIFTL(A,B) \
- OPERATE (A,<<,B) /* { dg-message "expanded|expansion" } */
+ OPERATE (A,<<,B) /* { dg-error "invalid operands" } */
void
foo ()
{
- SHIFTL (0.1,0.2); /* { dg-message "expanded" } */
+ SHIFTL (0.1,0.2); /* { dg-message "in expansion of macro \[^\n\r\]SHIFTL" } */
}
-
-/* { 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
index 684af4c..2367765 100644
--- a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c
@@ -4,18 +4,17 @@
*/
#define OPERATE(OPRD1, OPRT, OPRD2) \
- OPRD1 OPRT OPRD2; /* { dg-message "expansion" } */
+ OPRD1 OPRT OPRD2; /* { dg-message "in definition of macro 'OPERATE'" } */
#define SHIFTL(A,B) \
- OPERATE (A,<<,B) /* { dg-message "expanded|expansion" } */
+ OPERATE (A,<<,B) /* { dg-message "invalid operands to binary <<" } */
#define MULT(A) \
- SHIFTL (A,1) /* { dg-message "expanded|expansion" } */
+ SHIFTL (A,1) /* { dg-message "in expansion of macro 'SHIFTL'" } */
void
foo ()
{
- MULT (1.0); /* { dg-message "expanded" } */
+ MULT (1.0); /* { dg-message "in expansion of macro 'MULT'" } */
}
-/* { 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
index 119053e..b47726d 100644
--- a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c
@@ -3,12 +3,10 @@
{ dg-do compile }
*/
-#define SQUARE(A) A * A /* { dg-message "expansion" } */
+#define SQUARE(A) A * A /* { dg-message "in definition of macro 'SQUARE'" } */
void
foo()
{
- SQUARE (1 << 0.1); /* { dg-message "expanded" } */
+ SQUARE (1 << 0.1); /* { dg-error "16:invalid operands to binary <<" } */
}
-
-/* { 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
index 1f9fe6a..401b846 100644
--- a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c
@@ -3,12 +3,11 @@
{ dg-do compile }
*/
-#define SQUARE(A) A * A /* { dg-message "expansion" } */
+#define SQUARE(A) A * A /* { dg-message "in definition of macro 'SQUARE'" } */
void
foo()
{
- SQUARE (1 << 0.1); /* { dg-message "expanded" } */
+ SQUARE (1 << 0.1); /* { dg-message "13:invalid operands to binary <<" } */
}
-/* { dg-error "13:invalid operands to binary <<" "" { target *-*-* } { 11 } } */
diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c
index 7933660..abe456c 100644
--- a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c
+++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-5.c
@@ -3,16 +3,16 @@
{ dg-do compile }
*/
-#define PASTED var ## iable /* { dg-error "undeclared" } */
+#define PASTED var ## iable /* { dg-error "'variable' undeclared" } */
#define call_foo(p1, p2) \
foo (p1, \
- p2); /* { dg-message "in expansion of macro" } */
+ p2); /* { dg-message "in definition of macro 'call_foo'" } */
void foo(int, char);
void
bar()
{
- call_foo(1,PASTED); /* { dg-message "expanded from here" } */
+ call_foo(1,PASTED); /* { dg-message "in expansion of macro 'PASTED'" } */
}
diff --git a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c
index 57f3f01..38fc77c 100644
--- a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c
+++ b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c
@@ -24,5 +24,5 @@ g (void)
void
h (void)
{
- CODE_WITH_WARNING; /* { dg-message "expanded" } */
+ CODE_WITH_WARNING; /* { dg-message "in expansion of macro 'CODE_WITH_WARNING'" } */
}
diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c
index cbdbb77..774b6c4 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);
--
Dodji
More information about the Gcc-patches
mailing list