Re: [PATCH V2 06/12] perf mem: Clean up output format

[Jiri Olsa]

On Mon, Nov 30, 2020 at 09:27:57AM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:

SNIP

> @@ -172,7 +172,7 @@ dump_raw_samples(struct perf_tool *tool,
>  {
>  	struct perf_mem *mem = container_of(tool, struct perf_mem, tool);
>  	struct addr_location al;
> -	const char *fmt;
> +	const char *fmt, *field_sep;
>  
>  	if (machine__resolve(machine, &al, sample) < 0) {
>  		fprintf(stderr, "problem processing %d event, skipping it.\n",
> @@ -186,60 +186,41 @@ dump_raw_samples(struct perf_tool *tool,
>  	if (al.map != NULL)
>  		al.map->dso->hit = 1;
>  
> -	if (mem->phys_addr) {
> -		if (symbol_conf.field_sep) {
> -			fmt = "%d%s%d%s0x%"PRIx64"%s0x%"PRIx64"%s0x%016"PRIx64
> -			      "%s%"PRIu64"%s0x%"PRIx64"%s%s:%s\n";
> -		} else {
> -			fmt = "%5d%s%5d%s0x%016"PRIx64"%s0x016%"PRIx64
> -			      "%s0x%016"PRIx64"%s%5"PRIu64"%s0x%06"PRIx64
> -			      "%s%s:%s\n";
> -			symbol_conf.field_sep = " ";
> -		}
> -
> -		printf(fmt,
> -			sample->pid,
> -			symbol_conf.field_sep,
> -			sample->tid,
> -			symbol_conf.field_sep,
> -			sample->ip,
> -			symbol_conf.field_sep,
> -			sample->addr,
> -			symbol_conf.field_sep,
> -			sample->phys_addr,
> -			symbol_conf.field_sep,
> -			sample->weight,
> -			symbol_conf.field_sep,
> -			sample->data_src,
> -			symbol_conf.field_sep,
> -			al.map ? (al.map->dso ? al.map->dso->long_name : "???") : "???",
> -			al.sym ? al.sym->name : "???");
> +	field_sep = symbol_conf.field_sep;

hum, what's the point of having field_sep?

> +	if (field_sep) {
> +		fmt = "%d%s%d%s0x%"PRIx64"%s0x%"PRIx64"%s";
>  	} else {
> -		if (symbol_conf.field_sep) {
> -			fmt = "%d%s%d%s0x%"PRIx64"%s0x%"PRIx64"%s%"PRIu64
> -			      "%s0x%"PRIx64"%s%s:%s\n";
> -		} else {
> -			fmt = "%5d%s%5d%s0x%016"PRIx64"%s0x016%"PRIx64
> -			      "%s%5"PRIu64"%s0x%06"PRIx64"%s%s:%s\n";
> -			symbol_conf.field_sep = " ";
> -		}
> +		fmt = "%5d%s%5d%s0x%016"PRIx64"%s0x016%"PRIx64"%s";
> +		symbol_conf.field_sep = " ";
> +	}
> +	printf(fmt,
> +		sample->pid,
> +		symbol_conf.field_sep,
> +		sample->tid,
> +		symbol_conf.field_sep,
> +		sample->ip,
> +		symbol_conf.field_sep,
> +		sample->addr,
> +		symbol_conf.field_sep);
>  
> -		printf(fmt,
> -			sample->pid,
> -			symbol_conf.field_sep,
> -			sample->tid,
> -			symbol_conf.field_sep,
> -			sample->ip,
> -			symbol_conf.field_sep,
> -			sample->addr,
> -			symbol_conf.field_sep,
> -			sample->weight,
> -			symbol_conf.field_sep,
> -			sample->data_src,
> -			symbol_conf.field_sep,
> -			al.map ? (al.map->dso ? al.map->dso->long_name : "???") : "???",
> -			al.sym ? al.sym->name : "???");
> +	if (mem->phys_addr) {
> +		printf("0x%016"PRIx64"%s",
> +			sample->phys_addr,
> +			symbol_conf.field_sep);
>  	}
> +
> +	if (field_sep)
> +		fmt = "%"PRIu64"%s0x%"PRIx64"%s%s:%s\n";
> +	else
> +		fmt = "%5"PRIu64"%s0x%06"PRIx64"%s%s:%s\n";
> +
> +	printf(fmt,
> +		sample->weight,
> +		symbol_conf.field_sep,
> +		sample->data_src,
> +		symbol_conf.field_sep,
> +		al.map ? (al.map->dso ? al.map->dso->long_name : "???") : "???",
> +		al.sym ? al.sym->name : "???");
>  out_put:
>  	addr_location__put(&al);
>  	return 0;
> @@ -287,10 +268,12 @@ static int report_raw_events(struct perf_mem *mem)
>  	if (ret < 0)
>  		goto out_delete;
>  
> +	printf("# PID, TID, IP, ADDR, ");
> +
>  	if (mem->phys_addr)
> -		printf("# PID, TID, IP, ADDR, PHYS ADDR, LOCAL WEIGHT, DSRC, SYMBOL\n");
> -	else
> -		printf("# PID, TID, IP, ADDR, LOCAL WEIGHT, DSRC, SYMBOL\n");
> +		printf("PHYS ADDR, ");
> +
> +	printf("LOCAL WEIGHT, DSRC, SYMBOL\n");

if phys addr is the only member, can't we just squeeze it via
  "%s", mem->phys_addr ? "PHYS ADDR" : "",
like I mentioned in the other reply? and same also above?

thanks,
jirka

>  
>  	ret = perf_session__process_events(session);
>  
> -- 
> 2.17.1
>