From 75b49202d87c142a646e37edb466462ea6fbe0fb Mon Sep 17 00:00:00 2001 From: Arnaldo Carvalho de Melo Date: Thu, 24 Nov 2016 11:16:06 -0300 Subject: [PATCH] perf annotate: Remove duplicate 'name' field from disasm_line The disasm_line::name field is always equal to ins::name, being used just to locate the instruction's ins_ops from the per-arch instructions table. Eliminate this duplication, nuking that field and instead make ins__find() return an ins_ops, store it in disasm_line::ins.ops, and keep just in disasm_line::ins.name what was in disasm_line::name, this way we end up not keeping a reference to entries in the per-arch instructions table. This in turn will help supporting multiple ways to manage the per-arch instructions table, allowing resorting that array, for instance, when the entries will move after references to its addresses were made. The same problem is avoided when one grows the array with realloc. So architectures simply keeping a constant array will work as well as architectures building the table using regular expressions or other logic that involves resorting the table. Reviewed-by: Ravi Bangoria Cc: Adrian Hunter Cc: Alexander Shishkin Cc: Chris Riyder Cc: David Ahern Cc: Jiri Olsa Cc: Kim Phillips Cc: Markus Trippelsdorf Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Naveen N. Rao Cc: Pawel Moll Cc: Peter Zijlstra Cc: Russell King Cc: Taeung Song Cc: Wang Nan Link: http://lkml.kernel.org/n/tip-vr899azvabnw9gtuepuqfd9t@git.kernel.org Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/ui/browsers/annotate.c | 18 ++++---- tools/perf/util/annotate.c | 73 ++++++++++++++----------------- tools/perf/util/annotate.h | 17 ++++--- 3 files changed, 51 insertions(+), 57 deletions(-) diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c index e6e9f7d80dbd..cee0eee31ce6 100644 --- a/tools/perf/ui/browsers/annotate.c +++ b/tools/perf/ui/browsers/annotate.c @@ -213,17 +213,17 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int ui_browser__write_nstring(browser, bf, printed); if (change_color) ui_browser__set_color(browser, color); - if (dl->ins && dl->ins->ops->scnprintf) { - if (ins__is_jump(dl->ins)) { + if (dl->ins.ops && dl->ins.ops->scnprintf) { + if (ins__is_jump(&dl->ins)) { bool fwd = dl->ops.target.offset > (u64)dl->offset; ui_browser__write_graph(browser, fwd ? SLSMG_DARROW_CHAR : SLSMG_UARROW_CHAR); SLsmg_write_char(' '); - } else if (ins__is_call(dl->ins)) { + } else if (ins__is_call(&dl->ins)) { ui_browser__write_graph(browser, SLSMG_RARROW_CHAR); SLsmg_write_char(' '); - } else if (ins__is_ret(dl->ins)) { + } else if (ins__is_ret(&dl->ins)) { ui_browser__write_graph(browser, SLSMG_LARROW_CHAR); SLsmg_write_char(' '); } else { @@ -243,7 +243,7 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int static bool disasm_line__is_valid_jump(struct disasm_line *dl, struct symbol *sym) { - if (!dl || !dl->ins || !ins__is_jump(dl->ins) + if (!dl || !dl->ins.ops || !ins__is_jump(&dl->ins) || !disasm_line__has_offset(dl) || dl->ops.target.offset >= symbol__size(sym)) return false; @@ -492,7 +492,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser, }; char title[SYM_TITLE_MAX_SIZE]; - if (!ins__is_call(dl->ins)) + if (!ins__is_call(&dl->ins)) return false; if (map_groups__find_ams(&target) || @@ -545,7 +545,7 @@ static bool annotate_browser__jump(struct annotate_browser *browser) struct disasm_line *dl = browser->selection; s64 idx; - if (!ins__is_jump(dl->ins)) + if (!ins__is_jump(&dl->ins)) return false; dl = annotate_browser__find_offset(browser, dl->ops.target.offset, &idx); @@ -841,9 +841,9 @@ show_help: ui_helpline__puts("Huh? No selection. Report to linux-kernel@vger.kernel.org"); else if (browser->selection->offset == -1) ui_helpline__puts("Actions are only available for assembly lines."); - else if (!browser->selection->ins) + else if (!browser->selection->ins.ops) goto show_sup_ins; - else if (ins__is_ret(browser->selection->ins)) + else if (ins__is_ret(&browser->selection->ins)) goto out; else if (!(annotate_browser__jump(browser) || annotate_browser__callq(browser, evsel, hbt))) { diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 095d90a9077f..b48a39be071b 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -28,8 +28,8 @@ const char *disassembler_style; const char *objdump_path; static regex_t file_lineno; -static struct ins *ins__find(struct arch *arch, const char *name); -static int disasm_line__parse(char *line, char **namep, char **rawp); +static struct ins_ops *ins__find(struct arch *arch, const char *name); +static int disasm_line__parse(char *line, const char **namep, char **rawp); struct arch { const char *name; @@ -218,26 +218,20 @@ static int comment__symbol(char *raw, char *comment, u64 *addrp, char **namep) static int lock__parse(struct arch *arch, struct ins_operands *ops, struct map *map) { - char *name; - ops->locked.ops = zalloc(sizeof(*ops->locked.ops)); if (ops->locked.ops == NULL) return 0; - if (disasm_line__parse(ops->raw, &name, &ops->locked.ops->raw) < 0) + if (disasm_line__parse(ops->raw, &ops->locked.ins.name, &ops->locked.ops->raw) < 0) goto out_free_ops; - ops->locked.ins = ins__find(arch, name); - free(name); + ops->locked.ins.ops = ins__find(arch, ops->locked.ins.name); - if (ops->locked.ins == NULL) + if (ops->locked.ins.ops == NULL) goto out_free_ops; - if (!ops->locked.ins->ops) - return 0; - - if (ops->locked.ins->ops->parse && - ops->locked.ins->ops->parse(arch, ops->locked.ops, map) < 0) + if (ops->locked.ins.ops->parse && + ops->locked.ins.ops->parse(arch, ops->locked.ops, map) < 0) goto out_free_ops; return 0; @@ -252,19 +246,19 @@ static int lock__scnprintf(struct ins *ins, char *bf, size_t size, { int printed; - if (ops->locked.ins == NULL) + if (ops->locked.ins.ops == NULL) return ins__raw_scnprintf(ins, bf, size, ops); printed = scnprintf(bf, size, "%-6.6s ", ins->name); - return printed + ins__scnprintf(ops->locked.ins, bf + printed, + return printed + ins__scnprintf(&ops->locked.ins, bf + printed, size - printed, ops->locked.ops); } static void lock__delete(struct ins_operands *ops) { - struct ins *ins = ops->locked.ins; + struct ins *ins = &ops->locked.ins; - if (ins && ins->ops->free) + if (ins->ops && ins->ops->free) ins->ops->free(ops->locked.ops); else ins__delete(ops->locked.ops); @@ -425,8 +419,9 @@ static void ins__sort(struct arch *arch) qsort(arch->instructions, nmemb, sizeof(struct ins), ins__cmp); } -static struct ins *ins__find(struct arch *arch, const char *name) +static struct ins_ops *ins__find(struct arch *arch, const char *name) { + struct ins *ins; const int nmemb = arch->nr_instructions; if (!arch->sorted_instructions) { @@ -434,7 +429,8 @@ static struct ins *ins__find(struct arch *arch, const char *name) arch->sorted_instructions = true; } - return bsearch(name, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp); + ins = bsearch(name, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp); + return ins ? ins->ops : NULL; } static int arch__key_cmp(const void *name, const void *archp) @@ -691,19 +687,16 @@ int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 ip) static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map) { - dl->ins = ins__find(arch, dl->name); + dl->ins.ops = ins__find(arch, dl->ins.name); - if (dl->ins == NULL) + if (!dl->ins.ops) return; - if (!dl->ins->ops) - return; - - if (dl->ins->ops->parse && dl->ins->ops->parse(arch, &dl->ops, map) < 0) - dl->ins = NULL; + if (dl->ins.ops->parse && dl->ins.ops->parse(arch, &dl->ops, map) < 0) + dl->ins.ops = NULL; } -static int disasm_line__parse(char *line, char **namep, char **rawp) +static int disasm_line__parse(char *line, const char **namep, char **rawp) { char *name = line, tmp; @@ -736,7 +729,8 @@ static int disasm_line__parse(char *line, char **namep, char **rawp) return 0; out_free_name: - zfree(namep); + free((void *)namep); + *namep = NULL; return -1; } @@ -755,7 +749,7 @@ static struct disasm_line *disasm_line__new(s64 offset, char *line, goto out_delete; if (offset != -1) { - if (disasm_line__parse(dl->line, &dl->name, &dl->ops.raw) < 0) + if (disasm_line__parse(dl->line, &dl->ins.name, &dl->ops.raw) < 0) goto out_free_line; disasm_line__init_ins(dl, arch, map); @@ -774,20 +768,21 @@ out_delete: void disasm_line__free(struct disasm_line *dl) { zfree(&dl->line); - zfree(&dl->name); - if (dl->ins && dl->ins->ops->free) - dl->ins->ops->free(&dl->ops); + if (dl->ins.ops && dl->ins.ops->free) + dl->ins.ops->free(&dl->ops); else ins__delete(&dl->ops); + free((void *)dl->ins.name); + dl->ins.name = NULL; free(dl); } int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool raw) { - if (raw || !dl->ins) - return scnprintf(bf, size, "%-6.6s %s", dl->name, dl->ops.raw); + if (raw || !dl->ins.ops) + return scnprintf(bf, size, "%-6.6s %s", dl->ins.name, dl->ops.raw); - return ins__scnprintf(dl->ins, bf, size, &dl->ops); + return ins__scnprintf(&dl->ins, bf, size, &dl->ops); } static void disasm__add(struct list_head *head, struct disasm_line *line) @@ -1143,7 +1138,7 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map, map__rip_2objdump(map, sym->start); /* kcore has no symbols, so add the call target name */ - if (dl->ins && ins__is_call(dl->ins) && !dl->ops.target.name) { + if (dl->ins.ops && ins__is_call(&dl->ins) && !dl->ops.target.name) { struct addr_map_symbol target = { .map = map, .addr = dl->ops.target.addr, @@ -1173,8 +1168,8 @@ static void delete_last_nop(struct symbol *sym) while (!list_empty(list)) { dl = list_entry(list->prev, struct disasm_line, node); - if (dl->ins && dl->ins->ops) { - if (dl->ins->ops != &nop_ops) + if (dl->ins.ops) { + if (dl->ins.ops != &nop_ops) return; } else { if (!strstr(dl->line, " nop ") && @@ -1767,7 +1762,7 @@ static size_t disasm_line__fprintf(struct disasm_line *dl, FILE *fp) if (dl->offset == -1) return fprintf(fp, "%s\n", dl->line); - printed = fprintf(fp, "%#" PRIx64 " %s", dl->offset, dl->name); + printed = fprintf(fp, "%#" PRIx64 " %s", dl->offset, dl->ins.name); if (dl->ops.raw[0] != '\0') { printed += fprintf(fp, "%.*s %s\n", 6 - (int)printed, " ", diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h index 8e490b5c91bc..87e4cadc5d27 100644 --- a/tools/perf/util/annotate.h +++ b/tools/perf/util/annotate.h @@ -11,7 +11,12 @@ #include #include -struct ins; +struct ins_ops; + +struct ins { + const char *name; + struct ins_ops *ops; +}; struct ins_operands { char *raw; @@ -28,7 +33,7 @@ struct ins_operands { u64 addr; } source; struct { - struct ins *ins; + struct ins ins; struct ins_operands *ops; } locked; }; @@ -43,11 +48,6 @@ struct ins_ops { struct ins_operands *ops); }; -struct ins { - const char *name; - struct ins_ops *ops; -}; - bool ins__is_jump(const struct ins *ins); bool ins__is_call(const struct ins *ins); bool ins__is_ret(const struct ins *ins); @@ -59,8 +59,7 @@ struct disasm_line { struct list_head node; s64 offset; char *line; - char *name; - struct ins *ins; + struct ins ins; int line_nr; float ipc; u64 cycles;