linux-kernel - Re: [PATCH v6 5/7] perf diff: Link same basic blocks among different data

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]

Date:   Wed, 3 Jul 2019 09:07:38 +0800
From:   "Jin, Yao" <yao.jin@...ux.intel.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     jolsa@...nel.org, peterz@...radead.org, mingo@...hat.com,
        alexander.shishkin@...ux.intel.com, Linux-kernel@...r.kernel.org,
        ak@...ux.intel.com, kan.liang@...el.com, yao.jin@...el.com
Subject: Re: [PATCH v6 5/7] perf diff: Link same basic blocks among different
 data



On 7/3/2019 12:20 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 02, 2019 at 01:17:39PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Fri, Jun 28, 2019 at 05:23:02PM +0800, Jin Yao escreveu:
>>> The target is to compare the performance difference (cycles
>>> diff) for the same basic blocks in different data files.
>>>
>>> The same basic block means same function, same start address
>>> and same end address. This patch finds the same basic blocks
>>> from different data files and link them together and resort
>>> by the cycles diff.
>>>
>>>   v3:
>>>   ---
>>>   The block stuffs are maintained by new structure 'block_hist',
>>>   so this patch is update accordingly.
>>>
>>>   v2:
>>>   ---
>>>   Since now the basic block hists is changed to per symbol,
>>>   the patch only links the basic block hists for the same
>>>   symbol in different data files.
>>>
>>> Signed-off-by: Jin Yao <yao.jin@...ux.intel.com>
>>> ---
>>>   tools/perf/builtin-diff.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 90 insertions(+)
>>>
>>> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
>>> index 83b8c0f..823f162 100644
>>> --- a/tools/perf/builtin-diff.c
>>> +++ b/tools/perf/builtin-diff.c
>>> @@ -641,6 +641,85 @@ static int process_block_per_sym(struct hist_entry *he)
>>>   	return 0;
>>>   }
>>>   
>>> +static int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
>>> +{
>>> +	struct block_info *bi_a = a->block_info;
>>> +	struct block_info *bi_b = b->block_info;
>>> +	int cmp;
>>> +
>>> +	if (!bi_a->sym || !bi_b->sym)
>>> +		return -1;
>>> +
>>> +	if (bi_a->sym->name && bi_b->sym->name) {
>>> +		cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
>>> +		if ((!cmp) && (bi_a->start == bi_b->start) &&
>>> +		    (bi_a->end == bi_b->end)) {
>>> +			return 0;
>>> +		}
>>
>>
>> builtin-diff.c:658:17: error: address of array 'bi_a->sym->name' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
>>          if (bi_a->sym->name && bi_b->sym->name) {
>>              ~~~~~~~~~~~^~~~ ~~
>> builtin-diff.c:658:36: error: address of array 'bi_b->sym->name' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
>>          if (bi_a->sym->name && bi_b->sym->name) {
>>
>> Because:
>>
>> struct symbol *symbol__new(u64 start, u64 len, u8 binding, u8 type, const char *name)
>> {
>>          size_t namelen = strlen(name) + 1;
>>          struct symbol *sym = calloc(1, (symbol_conf.priv_size +
>>                                          sizeof(*sym) + namelen));
>>
>>
>> So it will be at least a strlen(sym->name) == 0, i.e. we can use it
>> without checking anything.
>>
>> I'm chanign it to do the cmp straight away
> 
> i.e. I added this on top of this patch:
> 
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 823f162b0060..fafb7b3f58fb 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -650,13 +650,10 @@ static int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
>   	if (!bi_a->sym || !bi_b->sym)
>   		return -1;
>   
> -	if (bi_a->sym->name && bi_b->sym->name) {
> -		cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
> -		if ((!cmp) && (bi_a->start == bi_b->start) &&
> -		    (bi_a->end == bi_b->end)) {
> -			return 0;
> -		}
> -	}
> +	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
> +
> +	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
> +		return 0;
>   
>   	return -1;
>   }
> 

Hi Arnaldo,

Thanks so much for providing the fix. Maybe my gcc version is too old.
gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.9)

I will switch to gcc version 7.4.0.

Thanks
Jin Yao