Re: [PATCH v2 3/4] perf annotate: add powerpc support

[Ravi Bangoria]

Thanks Naveen,

On Wednesday 29 June 2016 08:15 PM, Naveen N. Rao wrote:
> On 2016/06/29 04:45PM, Ravi Bangoria wrote:
> > From: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>
> >
> > Powerpc has long list of branch instructions and hardcoding them in
> > table appears to be error-prone. So, add new function to find
> > instruction instead of creating table. This function dynamically
> > create table(list of 'struct ins'), and instead of creating object
> > every time, first check if list already contain object for that
> > nemonics.
> >
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >    - Corrected few memory leaks.
> >    - Created Dynamic list for powerpc to optimize memory consumption
> >
> >   tools/perf/util/annotate.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 121 insertions(+)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 36a5825..812bfad 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -461,6 +461,11 @@ static struct ins instructions_arm[] = {
> >   	{ .name = "bne",   .ops  = &jump_ops, },
> >   };
> >
> > +struct instructions_powerpc {
> > +	struct ins *ins;
> > +	struct list_head list;
> > +};
> > +
> >   static int ins__key_cmp(const void *name, const void *insp)
> >   {
> >   	const struct ins *ins = insp;
> > @@ -476,6 +481,120 @@ static int ins__cmp(const void *a, const void *b)
> >   	return strcmp(ia->name, ib->name);
> >   }
> >
> > +static int list_add__ins_powerpc(struct instructions_powerpc *head,
> > +				 struct ins *ins)
> > +{
> > +	struct instructions_powerpc *ins_powerpc;
> > +
> > +	ins_powerpc = zalloc(sizeof(struct instructions_powerpc));
> > +	if (!ins_powerpc)
> > +		return -1;
> > +
> > +	ins_powerpc->ins = ins;
> > +	list_add_tail(&(ins_powerpc->list), &(head->list));
> > +
> > +	return 0;
> > +}
> > +
> > +static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head,
> > +					    const char *name)
> > +{
> > +	struct instructions_powerpc *pos;
> > +
> > +	list_for_each_entry(pos, &head->list, list) {
> > +		if (!strcmp(pos->ins->name, name))
> > +			return pos->ins;
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static struct ins *ins__find_powerpc(const char *name)
> > +{
> > +	int i;
> > +	struct ins *ins;
> > +	static struct instructions_powerpc head;
> > +	static bool list_initialized;
> > +
> > +	if (!list_initialized) {
> > +		INIT_LIST_HEAD(&head.list);
> > +		list_initialized = true;
> > +	}
> > +
> > +	/*
> > +	 * Search if we already created object of 'struct ins'
> > +	 * for this instruction
> > +	 */
> > +	ins = list_search__ins_powerpc(&head, name);
> > +	if (ins)
> > +		return ins;
> > +
> > +	ins = zalloc(sizeof(struct ins));
> > +	if (!ins)
> > +		return NULL;
> > +
> > +	ins->name = strdup(name);
> > +	if (!ins->name)
> > +		goto err;
> You can move the above two inside the below if condition, so that you
> only allocate memory if needed.
>
> Or, what would be better would be to pass 'name' and the appropriate ops
> pointer to the helper above (list_add__ins_powerpc) and have that
> allocate 'struct ins' and insert into the list.

Yes I will think about this.

> > +
> > +	if (name[0] == 'b') {
> > +		/* branch instructions */
> > +		ins->ops = &jump_ops;
> > +
> > +		/*
> > +		 * - Few start with 'b', but aren't branch instructions.
> > +		 * - Let's also ignore instructions involving 'ctr' and
> > +		 *   'tar' since target branch addresses for those can't
> > +		 *   be determined statically.
> > +		 */
> > +		if (!strncmp(name, "bcd", 3)   ||
> > +		    !strncmp(name, "brinc", 5) ||
> > +		    !strncmp(name, "bper", 4)  ||
> > +		    strstr(name, "ctr")        ||
> > +		    strstr(name, "tar"))
> > +			goto err;
> You are still leaking ins->name here.

Ah!! Sorry. I missed that we are using strdup here. Will correct it.

-Ravi