From 165ca58dc37d9c10443e5cf1daa836edb355a0f0 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Thu, 24 May 2012 19:37:45 +0000 Subject: [PATCH] Make unwound macro expansion trace less redundant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. From-SVN: r187845 --- gcc/ChangeLog | 7 ++ gcc/testsuite/ChangeLog | 10 ++ .../g++.dg/warn/Wconversion-real-integer2.C | 2 +- gcc/testsuite/g++.dg/warn/Wdouble-promotion.C | 2 +- .../gcc.dg/cpp/macro-exp-tracking-1.c | 8 +- .../gcc.dg/cpp/macro-exp-tracking-2.c | 9 +- .../gcc.dg/cpp/macro-exp-tracking-3.c | 6 +- .../gcc.dg/cpp/macro-exp-tracking-4.c | 5 +- .../gcc.dg/cpp/macro-exp-tracking-5.c | 6 +- .../gcc.dg/cpp/pragma-diagnostic-2.c | 2 +- gcc/tree-diagnostic.c | 91 ++++++++++++++----- 11 files changed, 102 insertions(+), 46 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 4c6b9c0b24f..e6e67cc33f1 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,10 @@ +2012-05-24 Dodji Seketeli + + 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. + 2012-05-24 Richard Guenther PR middle-end/53460 diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 065b5a33213..706d7f54030 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,13 @@ +2012-05-24 Dodji Seketeli + + 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. + 2012-05-24 Paolo Carlini PR c++/53464 diff --git a/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C b/gcc/testsuite/g++.dg/warn/Wconversion-real-integer2.C index 29130f1836f..6a95b0e3a8c 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 98d2eeda79e..afd9a20147e 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 d975c8cee97..28ef7955c9b 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 684af4c7507..2367765ccef 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 119053ef9fb..b47726d6b4b 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 1f9fe6ad26c..401b846cc3e 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 79336608569..abe456ceb6c 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 57f3f01939a..38fc77c47ba 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 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 + 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 eighth line of this + 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);