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
