linux-kernel - Re: [PATCH] perf annotate: improve --stdio mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bdccc648-41e4-2719-cfbb-a9f44ff3b489@suse.cz>
Date:   Mon, 19 Apr 2021 09:39:37 +0200
From:   Martin Liška <mliska@...e.cz>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] perf annotate: improve --stdio mode

@Arnaldo: May I please ping this?

Thanks,
Martin

On 4/8/21 12:08 PM, Martin Liška wrote:
> On 4/7/21 10:25 PM, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Apr 07, 2021 at 04:30:46PM -0300, Arnaldo Carvalho de Melo escreveu:
>>> Em Fri, Feb 26, 2021 at 10:24:00AM +0100, Martin Liška escreveu:
>>>> On 2/23/21 8:47 PM, Arnaldo Carvalho de Melo wrote:
>>>> Sure. But I think the current format provides quite broken visual layout:
>>>>
>>>>       0.00 :   405ef1: inc    %r15
>>>>       0.01 :   405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3        # 4318b0 <_IO_stdin_used+0x8b0>
>>>>    eff.c:1811    0.67 :   405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3        # 4318b8 <_IO_stdin_used+0x8b8>
>>>>           :            TA + tmpsd * (TB +
>>>>
>>>> vs.
>>>>
>>>>       0.00 :   405ef1: inc    %r15
>>>>       0.01 :   405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3        # 4318b0 <_IO_stdin_used+0x8b0>
>>>>       0.67 :   405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3        # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
>>>>            : 1810   TA + tmpsd * (TB +
>>>>
>>>> I bet also the current users of --stdio mode would benefit from it.
>>>> What do you think?
>>  
>>> Agreed, I tried applying but it bitrotted, it seems :-\
>>
>> I refreshed it, please check.
> 
> Thanks! I've just tested the patch on top of acme/perf/core and it works as was planned.
> I'm attaching 2 perf annotate snippets (perf annotate --stdio -l --stdio-color=always) before
> and after the revision:
> 
> https://splichal.eu/tmp/perf-before.html
> https://splichal.eu/tmp/perf-after.html
> 
> I hope it nicely describes that it's an improvement.
> 
> Cheers,
> Martin
> 
>>
>> - Arnaldo
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 18eee25b4976bea8..abe1499a91645375 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -1368,7 +1368,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>>  {
>>  	struct disasm_line *dl = container_of(al, struct disasm_line, al);
>>  	static const char *prev_line;
>> -	static const char *prev_color;
>>  
>>  	if (al->offset != -1) {
>>  		double max_percent = 0.0;
>> @@ -1407,20 +1406,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>>  
>>  		color = get_percent_color(max_percent);
>>  
>> -		/*
>> -		 * Also color the filename and line if needed, with
>> -		 * the same color than the percentage. Don't print it
>> -		 * twice for close colored addr with the same filename:line
>> -		 */
>> -		if (al->path) {
>> -			if (!prev_line || strcmp(prev_line, al->path)
>> -				       || color != prev_color) {
>> -				color_fprintf(stdout, color, " %s", al->path);
>> -				prev_line = al->path;
>> -				prev_color = color;
>> -			}
>> -		}
>> -
>>  		for (i = 0; i < nr_percent; i++) {
>>  			struct annotation_data *data = &al->data[i];
>>  			double percent;
>> @@ -1441,6 +1426,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>>  		printf(" : ");
>>  
>>  		disasm_line__print(dl, start, addr_fmt_width);
>> +
>> +		/*
>> +		 * Also color the filename and line if needed, with
>> +		 * the same color than the percentage. Don't print it
>> +		 * twice for close colored addr with the same filename:line
>> +		 */
>> +		if (al->path) {
>> +			if (!prev_line || strcmp(prev_line, al->path)) {
>> +				color_fprintf(stdout, color, " // %s", al->path);
>> +				prev_line = al->path;
>> +			}
>> +		}
>> +
>>  		printf("\n");
>>  	} else if (max_lines && printed >= max_lines)
>>  		return 1;
>> @@ -1456,7 +1454,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>>  		if (!*al->line)
>>  			printf(" %*s:\n", width, " ");
>>  		else
>> -			printf(" %*s:     %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
>> +			printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
>>  	}
>>  
>>  	return 0;
>>
>