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

[Arnaldo Carvalho de Melo]

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;
 }