[PATCH] perf tools: Remove cast of non-varadic function to varadic

[PATCH] perf tools: Remove cast of non-varadic function to varadic

[Michael Hudson-Doyle]

4fb71074a570 (perf ui/hist: Consolidate hpp helpers) introduced a cast of
percent_color_snprintf to a function pointer type with varargs.  Change
percent_color_snprintf to be varadic and remove the cast.

The symptom of this was all percentages being reported as 0.00% in perf
report --stdio output on armhf.

Signed-off-by: Michael Hudson-Doyle <michael.hudson@linaro.org>
---
 tools/perf/ui/hist.c    |  2 +-
 tools/perf/util/color.c | 11 +++++++++--
 tools/perf/util/color.h |  2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 0a19328..78f4c92 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -117,7 +117,7 @@ static int hpp__color_##_type(struct perf_hpp_fmt *fmt __maybe_unused,		\
 			      struct perf_hpp *hpp, struct hist_entry *he) 	\
 {										\
 	return __hpp__fmt(hpp, he, he_get_##_field, " %6.2f%%",			\
-			  (hpp_snprint_fn)percent_color_snprintf, true);	\
+			  percent_color_snprintf, true);			\
 }
 
 #define __HPP_ENTRY_PERCENT_FN(_type, _field)					\
diff --git a/tools/perf/util/color.c b/tools/perf/util/color.c
index 11e46da..66e44a5 100644
--- a/tools/perf/util/color.c
+++ b/tools/perf/util/color.c
@@ -318,8 +318,15 @@ int percent_color_fprintf(FILE *fp, const char *fmt, double percent)
 	return r;
 }
 
-int percent_color_snprintf(char *bf, size_t size, const char *fmt, double percent)
+int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...)
 {
-	const char *color = get_percent_color(percent);
+	va_list args;
+	double percent;
+	const char *color;
+
+	va_start(args, fmt);
+	percent = va_arg(args, double);
+	va_end(args);
+	color = get_percent_color(percent);
 	return color_snprintf(bf, size, color, fmt, percent);
 }
diff --git a/tools/perf/util/color.h b/tools/perf/util/color.h
index dea082b..fced384 100644
--- a/tools/perf/util/color.h
+++ b/tools/perf/util/color.h
@@ -39,7 +39,7 @@ int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 int color_snprintf(char *bf, size_t size, const char *color, const char *fmt, ...);
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
 int color_fwrite_lines(FILE *fp, const char *color, size_t count, const char *buf);
-int percent_color_snprintf(char *bf, size_t size, const char *fmt, double percent);
+int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...);
 int percent_color_fprintf(FILE *fp, const char *fmt, double percent);
 const char *get_percent_color(double percent);
 
-- 
1.8.3.2

Re: [PATCH] perf tools: Remove cast of non-varadic function to varadic

[Will Deacon]

Hi Michael,

On Thu, Oct 31, 2013 at 07:50:25PM +0000, Michael Hudson-Doyle wrote:
> 4fb71074a570 (perf ui/hist: Consolidate hpp helpers) introduced a cast of
> percent_color_snprintf to a function pointer type with varargs.  Change
> percent_color_snprintf to be varadic and remove the cast.

nit: s/varadic/variadic/

> The symptom of this was all percentages being reported as 0.00% in perf
> report --stdio output on armhf.

  Acked-by: Will Deacon <will.deacon@arm.com>

Probably worth CCing stable too.

Will

Re: [PATCH] perf tools: Remove cast of non-varadic function to varadic

[Arnaldo Carvalho de Melo]

Em Thu, Oct 31, 2013 at 12:50:25PM -0700, Michael Hudson-Doyle escreveu:
> 4fb71074a570 (perf ui/hist: Consolidate hpp helpers) introduced a cast of
> percent_color_snprintf to a function pointer type with varargs.  Change
> percent_color_snprintf to be varadic and remove the cast.
> 
> The symptom of this was all percentages being reported as 0.00% in perf
> report --stdio output on armhf.

git bisect + visual inspection, right? Anyway, good catch, interesting
it caused problems on just one arch, or have you tested this in other
arches?

Namhyung?

- Arnaldo
 
> Signed-off-by: Michael Hudson-Doyle <michael.hudson@linaro.org>
> ---
>  tools/perf/ui/hist.c    |  2 +-
>  tools/perf/util/color.c | 11 +++++++++--
>  tools/perf/util/color.h |  2 +-
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 0a19328..78f4c92 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -117,7 +117,7 @@ static int hpp__color_##_type(struct perf_hpp_fmt *fmt __maybe_unused,		\
>  			      struct perf_hpp *hpp, struct hist_entry *he) 	\
>  {										\
>  	return __hpp__fmt(hpp, he, he_get_##_field, " %6.2f%%",			\
> -			  (hpp_snprint_fn)percent_color_snprintf, true);	\
> +			  percent_color_snprintf, true);			\
>  }
>  
>  #define __HPP_ENTRY_PERCENT_FN(_type, _field)					\
> diff --git a/tools/perf/util/color.c b/tools/perf/util/color.c
> index 11e46da..66e44a5 100644
> --- a/tools/perf/util/color.c
> +++ b/tools/perf/util/color.c
> @@ -318,8 +318,15 @@ int percent_color_fprintf(FILE *fp, const char *fmt, double percent)
>  	return r;
>  }
>  
> -int percent_color_snprintf(char *bf, size_t size, const char *fmt, double percent)
> +int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...)
>  {
> -	const char *color = get_percent_color(percent);
> +	va_list args;
> +	double percent;
> +	const char *color;
> +
> +	va_start(args, fmt);
> +	percent = va_arg(args, double);
> +	va_end(args);
> +	color = get_percent_color(percent);
>  	return color_snprintf(bf, size, color, fmt, percent);
>  }
> diff --git a/tools/perf/util/color.h b/tools/perf/util/color.h
> index dea082b..fced384 100644
> --- a/tools/perf/util/color.h
> +++ b/tools/perf/util/color.h
> @@ -39,7 +39,7 @@ int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
>  int color_snprintf(char *bf, size_t size, const char *color, const char *fmt, ...);
>  int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
>  int color_fwrite_lines(FILE *fp, const char *color, size_t count, const char *buf);
> -int percent_color_snprintf(char *bf, size_t size, const char *fmt, double percent);
> +int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...);
>  int percent_color_fprintf(FILE *fp, const char *fmt, double percent);
>  const char *get_percent_color(double percent);
>  
> -- 
> 1.8.3.2

Re: [PATCH] perf tools: Remove cast of non-varadic function to varadic

[Michael Hudson-Doyle]

Arnaldo Carvalho de Melo <acme@ghostprotocols.net> writes:

> Em Thu, Oct 31, 2013 at 12:50:25PM -0700, Michael Hudson-Doyle escreveu:
>> 4fb71074a570 (perf ui/hist: Consolidate hpp helpers) introduced a cast of
>> percent_color_snprintf to a function pointer type with varargs.  Change
>> percent_color_snprintf to be varadic and remove the cast.
>> 
>> The symptom of this was all percentages being reported as 0.00% in perf
>> report --stdio output on armhf.
>
> git bisect + visual inspection, right?

Well annotate + visual inspection yes.  I haven't tested that it was
that commit.

> Anyway, good catch, interesting it caused problems on just one arch,
> or have you tested this in other arches?

I've only observed the problem on the one platform and don't know the
ABIs well enough to know what to expect on other platforms... even on
armhf it's dependent on details of what the compiler decides to do.

Cheers,
mwh

> Namhyung?
>
> - Arnaldo
>  
>> Signed-off-by: Michael Hudson-Doyle <michael.hudson@linaro.org>
>> ---
>>  tools/perf/ui/hist.c    |  2 +-
>>  tools/perf/util/color.c | 11 +++++++++--
>>  tools/perf/util/color.h |  2 +-
>>  3 files changed, 11 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
>> index 0a19328..78f4c92 100644
>> --- a/tools/perf/ui/hist.c
>> +++ b/tools/perf/ui/hist.c
>> @@ -117,7 +117,7 @@ static int hpp__color_##_type(struct perf_hpp_fmt *fmt __maybe_unused,		\
>>  			      struct perf_hpp *hpp, struct hist_entry *he) 	\
>>  {										\
>>  	return __hpp__fmt(hpp, he, he_get_##_field, " %6.2f%%",			\
>> -			  (hpp_snprint_fn)percent_color_snprintf, true);	\
>> +			  percent_color_snprintf, true);			\
>>  }
>>  
>>  #define __HPP_ENTRY_PERCENT_FN(_type, _field)					\
>> diff --git a/tools/perf/util/color.c b/tools/perf/util/color.c
>> index 11e46da..66e44a5 100644
>> --- a/tools/perf/util/color.c
>> +++ b/tools/perf/util/color.c
>> @@ -318,8 +318,15 @@ int percent_color_fprintf(FILE *fp, const char *fmt, double percent)
>>  	return r;
>>  }
>>  
>> -int percent_color_snprintf(char *bf, size_t size, const char *fmt, double percent)
>> +int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...)
>>  {
>> -	const char *color = get_percent_color(percent);
>> +	va_list args;
>> +	double percent;
>> +	const char *color;
>> +
>> +	va_start(args, fmt);
>> +	percent = va_arg(args, double);
>> +	va_end(args);
>> +	color = get_percent_color(percent);
>>  	return color_snprintf(bf, size, color, fmt, percent);
>>  }
>> diff --git a/tools/perf/util/color.h b/tools/perf/util/color.h
>> index dea082b..fced384 100644
>> --- a/tools/perf/util/color.h
>> +++ b/tools/perf/util/color.h
>> @@ -39,7 +39,7 @@ int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
>>  int color_snprintf(char *bf, size_t size, const char *color, const char *fmt, ...);
>>  int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
>>  int color_fwrite_lines(FILE *fp, const char *color, size_t count, const char *buf);
>> -int percent_color_snprintf(char *bf, size_t size, const char *fmt, double percent);
>> +int percent_color_snprintf(char *bf, size_t size, const char *fmt, ...);
>>  int percent_color_fprintf(FILE *fp, const char *fmt, double percent);
>>  const char *get_percent_color(double percent);
>>  
>> -- 
>> 1.8.3.2

Re: [PATCH] perf tools: Remove cast of non-varadic function to varadic

[Namhyung Kim]

Hi Arnaldo and Michael,

On Thu, 31 Oct 2013 18:09:50 -0200, Arnaldo Carvalho de Melo wrote:
> Em Thu, Oct 31, 2013 at 12:50:25PM -0700, Michael Hudson-Doyle escreveu:
>> 4fb71074a570 (perf ui/hist: Consolidate hpp helpers) introduced a cast of
>> percent_color_snprintf to a function pointer type with varargs.  Change
>> percent_color_snprintf to be varadic and remove the cast.
>> 
>> The symptom of this was all percentages being reported as 0.00% in perf
>> report --stdio output on armhf.
>
> git bisect + visual inspection, right? Anyway, good catch, interesting
> it caused problems on just one arch, or have you tested this in other
> arches?
>
> Namhyung?

Yes, it'll affect the way of passing parameter on some arch's -  I only
tested it on x86_64.  So,

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung