This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

PING: [PATCH diagnostics] Make unwound macro expansion trace less redundant


PING: http://gcc.gnu.org/ml/gcc-patches/2012-05/msg01003.html

Dodji Seketeli <dodji@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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]