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

[Jiri Olsa]

On Mon, Dec 07, 2020 at 03:19:06PM -0500, Liang, Kan wrote:
> 
> 
> On 12/4/2020 6:27 PM, Jiri Olsa wrote:
> > 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?
> 
> 
> To keep the fmt consistent.
> 
> The patch divides the "printf(fmt,..." into two part. In the first half
> part, the symbol_conf.field_sep may be modified to " ";
> In the second half part, we should not use the modified
> symbol_conf.field_sep for the below check. The field_sep keeps the original
> value and should be used.

ok, I missed it's being moified.. thanks

jirka