Re: [PATCH] perf report: Fix calculation of the symbol column width

[Arnaldo Carvalho de Melo]

Em Tue, Mar 13, 2012 at 02:09:29PM +0900, Namhyung Kim escreveu:
> The calculation of each column width is determining longest string or
> number in the data (including header). However printing symbol column
> doesn't honor the width. This is fine if symbol column is printed at
> last (default behavior), but if user changes the order using -s option
> the output will be messed up:

This one is clashing with b5387528:

"perf tools: Add code to support PERF_SAMPLE_BRANCH_STACK"

Ingo merged it recently, can you please respin this patch on perf/core?

- Arnaldo
 
>  $ perf report -s sym,dso,comm
>  ...
>  # Overhead  Symbol     Shared Object  Command
>  # ........  ......  ................  .......
>  #
>      99.96%  [.] main  noploop           noploop
>       0.03%  [.] memset  ld-2.12.1.so      noploop
>       0.00%  [.] 0x7ff751107b70    [unknown]          :18961
>       0.00%  [.] _start  ld-2.12.1.so      noploop
> 
> Also, the symbol column adds 4 characters which represents the cpu mode
> before the symbol and the width of unresolved symbols were not counted.
> 
> In addition, if user gave -v option it would print raw ip value and
> symbol origin character before the symbol name. After all of these
> concerns are addressed, the end result will look like this:
> 
>  $ perf report -s sym,dso,comm
>  ...
>  # Overhead              Symbol  Shared Object  Command
>  # ........  ..................  .............  .......
>  #
>      99.96%  [.] main            noploop        noploop
>       0.03%  [.] memset          ld-2.12.1.so   noploop
>       0.00%  [.] 0x7ff751107b70  [unknown]       :18961
>       0.00%  [.] _start          ld-2.12.1.so   noploop
> 
> Signed-off-by: Namhyung Kim <namhyung.kim@xxxxxxx>
> ---
>  tools/perf/util/hist.c |   27 ++++++++++++++++++++-------
>  tools/perf/util/sort.c |   14 ++++++++------
>  2 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 6f505d1abac7..2f5f8ff16e7e 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -54,16 +54,25 @@ static void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
>  {
>  	u16 len;
>  
> -	if (h->ms.sym)
> -		hists__new_col_len(hists, HISTC_SYMBOL, h->ms.sym->namelen);
> +	if (h->ms.sym) {
> +		len = h->ms.sym->namelen;
> +		if (!sort_dso.elide)
> +			len += 4;
> +		hists__new_col_len(hists, HISTC_SYMBOL, len);
> +	}
>  	else {
> -		const unsigned int unresolved_col_width = BITS_PER_LONG / 4;
> -
> -		if (hists__col_len(hists, HISTC_DSO) < unresolved_col_width &&
> +		char buf[BITS_PER_LONG / 4 + 3];
> +		len = snprintf(buf, BITS_PER_LONG / 4 + 2, "%#llx",
> +			       (unsigned long long)h->ip);
> +		if (!sort_dso.elide)
> +			len += 4;
> +		hists__new_col_len(hists, HISTC_SYMBOL, len);
> +
> +		len = strlen("[unknown]");
> +		if (hists__col_len(hists, HISTC_DSO) < len &&
>  		    !symbol_conf.col_width_list_str && !symbol_conf.field_sep &&
>  		    !symbol_conf.dso_list)
> -			hists__set_col_len(hists, HISTC_DSO,
> -					   unresolved_col_width);
> +			hists__set_col_len(hists, HISTC_DSO, len);
>  	}
>  
>  	len = thread__comm_len(h->thread);
> @@ -984,6 +993,8 @@ size_t hists__fprintf(struct hists *hists, struct hists *pair,
>  		}
>  		if (!hists__new_col_len(hists, se->se_width_idx, width))
>  			width = hists__col_len(hists, se->se_width_idx);
> +		if (verbose && se->se_width_idx == HISTC_SYMBOL)
> +			width += BITS_PER_LONG / 4 + 2 + 3;
>  		fprintf(fp, "  %*s", width, se->se_header);
>  	}
>  
> @@ -1016,6 +1027,8 @@ size_t hists__fprintf(struct hists *hists, struct hists *pair,
>  		width = hists__col_len(hists, se->se_width_idx);
>  		if (width == 0)
>  			width = strlen(se->se_header);
> +		if (verbose && se->se_width_idx == HISTC_SYMBOL)
> +			width += BITS_PER_LONG / 4 + 2 + 3;
>  		for (i = 0; i < width; i++)
>  			fprintf(fp, ".");
>  	}
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 16da30d8d765..806752870b9c 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -167,25 +167,27 @@ sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
>  }
>  
>  static int hist_entry__sym_snprintf(struct hist_entry *self, char *bf,
> -				    size_t size, unsigned int width __used)
> +				    size_t size, unsigned int width)
>  {
>  	size_t ret = 0;
>  
>  	if (verbose) {
>  		char o = self->ms.map ? dso__symtab_origin(self->ms.map->dso) : '!';
>  		ret += repsep_snprintf(bf, size, "%-#*llx %c ",
> -				       BITS_PER_LONG / 4, self->ip, o);
> +				       BITS_PER_LONG / 4 + 2, self->ip, o);
>  	}
>  
> -	if (!sort_dso.elide)
> +	if (!sort_dso.elide) {
>  		ret += repsep_snprintf(bf + ret, size - ret, "[%c] ", self->level);
> +		width -= 4;
> +	}
>  
>  	if (self->ms.sym)
> -		ret += repsep_snprintf(bf + ret, size - ret, "%s",
> +		ret += repsep_snprintf(bf + ret, size - ret, "%-*s", width,
>  				       self->ms.sym->name);
>  	else
> -		ret += repsep_snprintf(bf + ret, size - ret, "%-#*llx",
> -				       BITS_PER_LONG / 4, self->ip);
> +		ret += repsep_snprintf(bf + ret, size - ret, "%-#*llx", width,
> +				       self->ip);
>  
>  	return ret;
>  }
> -- 
> 1.7.9
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/