Re: [PATCH v6 1/6] perf report: properly handle branch count in match_chain

[Milian Wolff <milian.wolff@kdab.com>]

On Donnerstag, 19. Oktober 2017 00:41:04 CEST Andi Kleen wrote:
> Milian Wolff <milian.wolff@kdab.com> writes:
> > +static enum match_result match_address_dso(struct dso *left_dso, u64
> > left_ip, +					   struct dso *right_dso, u64 right_ip)
> > +{
> > +	if (left_dso == right_dso && left_ip == right_ip)
> > +		return MATCH_EQ;
> > +	else if (left_ip < right_ip)
> > +		return MATCH_LT;
> > +	else
> > +		return MATCH_GT;
> > +}
> 
> So why does only the first case check the dso? Does it not matter
> for the others?
> 
> Either should be checked by none or by all.

I don't see why it should be checked. It is only required to prevent two 
addresses to be considered equal while they are not. So only the one check is 
required, otherwise we return either LT or GT.

Am I missing something?

> > +	case CCKEY_FUNCTION:
> > +		if (node->sym && cnode->ms.sym) {
> > +			/*
> > +			 * Compare inlined frames based on their symbol name
> > +			 * because different inlined frames will have the same
> > +			 * symbol start. Otherwise do a faster comparison based
> > +			 * on the symbol start address.
> > +			 */
> > +			if (cnode->ms.sym->inlined || node->sym->inlined)
> > +				match = match_chain_strings(cnode->ms.sym->name,
> 
> node->sym->name);
> 
> So what happens when there are multiple symbols with the same name?
> 
> (e.g. local for a DSO or local in a file)
> 
> > +					  node->ip);
> > +		} else {
> > +			/*
> > +			 * It's "from" of a branch
> > +			 */
> > +			cnode->brtype_stat.branch_to = false;
> > +			cnode->cycles_count += node->branch_flags.cycles;
> > +			cnode->iter_count += node->nr_loop_iter;
> > +			cnode->iter_cycles += node->iter_cycles;
> 
> I assume you tested the cycle accounting still works?

Back then I did it, but it is a long time ago when I originally wrote this 
patch. I just tested it again, and indeed something crashes now. I will fix it 
and resend v7.

Sorry for that.

-- 
Milian Wolff | milian.wolff@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts