Messages in this thread | | | Subject | Re: [PATCH v2 3/4] perf annotate: add powerpc support | From | Ravi Bangoria <> | Date | Wed, 29 Jun 2016 20:49:49 +0530 |
| |
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
| |