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

[Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>]

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@linux.vnet.ibm.com>
>>
>> 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@linux.vnet.ibm.com>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>> ---
>> 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