2013-10-31 19:50:35

by Michael Hudson-Doyle

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

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 <[email protected]>
---
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

2013-10-31 20:06:28

by Will Deacon

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

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 <[email protected]>

Probably worth CCing stable too.

Will

2013-10-31 20:10:09

by Arnaldo Carvalho de Melo

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

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 <[email protected]>
> ---
> 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

2013-10-31 21:19:41

by Michael Hudson-Doyle

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

Arnaldo Carvalho de Melo <[email protected]> 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 <[email protected]>
>> ---
>> 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