PING: http://gcc.gnu.org/ml/gcc-patches/2012-05/msg01003.html
Dodji Seketeli <do...@redhat.com> writes: > 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