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

[Jin, Yao]

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@xxxxxxxxxxxxxxx>
> > > ---
> > >   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