From 353dcaa2f979a04f9397306ae3165ccf9fc731df Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Thu, 10 Oct 2019 11:36:45 -0700 Subject: perf annotate: Avoid reallocation in objdump parsing Objdump output is parsed using getline which allocates memory for the read. Getline will realloc if the memory is too small, but currently the line is always freed after the call. Simplify parse_objdump_line by performing the reading in symbol__disassemble. Signed-off-by: Ian Rogers Tested-by: Arnaldo Carvalho de Melo Cc: Alexander Shishkin Cc: Jin Yao Cc: Jiri Olsa Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Song Liu Cc: Stephane Eranian Cc: clang-built-linux@googlegroups.com Link: http://lore.kernel.org/lkml/20191010183649.23768-2-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/annotate.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) (limited to 'tools/perf/util/annotate.c') diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 2b856b6b46f6..f9c39a742418 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1489,24 +1489,17 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start * means that it's not a disassembly line so should be treated differently. * The ops.raw part will be parsed further according to type of the instruction. */ -static int symbol__parse_objdump_line(struct symbol *sym, FILE *file, +static int symbol__parse_objdump_line(struct symbol *sym, struct annotate_args *args, - int *line_nr) + char *line, int *line_nr) { struct map *map = args->ms.map; struct annotation *notes = symbol__annotation(sym); struct disasm_line *dl; - char *line = NULL, *parsed_line, *tmp, *tmp2; - size_t line_len; + char *parsed_line, *tmp, *tmp2; s64 line_ip, offset = -1; regmatch_t match[2]; - if (getline(&line, &line_len, file) < 0) - return -1; - - if (!line) - return -1; - line_ip = -1; parsed_line = strim(line); @@ -1543,7 +1536,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, FILE *file, args->ms.sym = sym; dl = disasm_line__new(args); - free(line); (*line_nr)++; if (dl == NULL) @@ -1876,6 +1868,8 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) int lineno = 0; int nline; pid_t pid; + char *line; + size_t line_len; int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename)); if (err) @@ -1964,18 +1958,26 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) goto out_free_command; } + /* Storage for getline. */ + line = NULL; + line_len = 0; + nline = 0; while (!feof(file)) { + if (getline(&line, &line_len, file) < 0 || !line) + break; + /* * The source code line number (lineno) needs to be kept in * across calls to symbol__parse_objdump_line(), so that it * can associate it with the instructions till the next one. * See disasm_line__new() and struct disasm_line::line_nr. */ - if (symbol__parse_objdump_line(sym, file, args, &lineno) < 0) + if (symbol__parse_objdump_line(sym, args, line, &lineno) < 0) break; nline++; } + free(line); if (nline == 0) pr_err("No output from %s\n", command); -- cgit v1.2.3-59-g8ed1b From 4235949944d1bb244c85fd184cdc2f78e9df848b Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Thu, 10 Oct 2019 11:36:46 -0700 Subject: perf annotate: Use libsubcmd's run-command.h to fork objdump Reduce duplicated logic by using the subcmd library. Ensure when errors occur they are reported to the caller. Before this patch, if no lines are read the error status is 0. Signed-off-by: Ian Rogers Tested-by: Arnaldo Carvalho de Melo Cc: Alexander Shishkin Cc: Jin Yao Cc: Jiri Olsa Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Song Liu Cc: Stephane Eranian Cc: clang-built-linux@googlegroups.com Link: http://lore.kernel.org/lkml/20191010183649.23768-3-irogers@google.com Link: http://lore.kernel.org/lkml/20191015003418.62563-1-irogers@google.com [ merged follow up fix for NULL termination as in the 2nd link above ] Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/annotate.c | 72 ++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 35 deletions(-) (limited to 'tools/perf/util/annotate.c') diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index f9c39a742418..9835666db5a7 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -43,6 +43,7 @@ #include #include #include +#include /* FIXME: For the HE_COLORSET */ #include "ui/browser.h" @@ -1864,12 +1865,19 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) struct kcore_extract kce; bool delete_extract = false; bool decomp = false; - int stdout_fd[2]; int lineno = 0; int nline; - pid_t pid; char *line; size_t line_len; + const char *objdump_argv[] = { + "/bin/sh", + "-c", + NULL, /* Will be the objdump command to run. */ + "--", + NULL, /* Will be the symfs path. */ + NULL, + }; + struct child_process objdump_process; int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename)); if (err) @@ -1899,7 +1907,7 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) if (dso__decompress_kmodule_path(dso, symfs_filename, tmp, sizeof(tmp)) < 0) - goto out; + return -1; decomp = true; strcpy(symfs_filename, tmp); @@ -1924,38 +1932,28 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) pr_debug("Executing: %s\n", command); - err = -1; - if (pipe(stdout_fd) < 0) { - pr_err("Failure creating the pipe to run %s\n", command); - goto out_free_command; - } - - pid = fork(); - if (pid < 0) { - pr_err("Failure forking to run %s\n", command); - goto out_close_stdout; - } + objdump_argv[2] = command; + objdump_argv[4] = symfs_filename; - if (pid == 0) { - close(stdout_fd[0]); - dup2(stdout_fd[1], 1); - close(stdout_fd[1]); - execl("/bin/sh", "sh", "-c", command, "--", symfs_filename, - NULL); - perror(command); - exit(-1); + /* Create a pipe to read from for stdout */ + memset(&objdump_process, 0, sizeof(objdump_process)); + objdump_process.argv = objdump_argv; + objdump_process.out = -1; + if (start_command(&objdump_process)) { + pr_err("Failure starting to run %s\n", command); + err = -1; + goto out_free_command; } - close(stdout_fd[1]); - - file = fdopen(stdout_fd[0], "r"); + file = fdopen(objdump_process.out, "r"); if (!file) { pr_err("Failure creating FILE stream for %s\n", command); /* * If we were using debug info should retry with * original binary. */ - goto out_free_command; + err = -1; + goto out_close_stdout; } /* Storage for getline. */ @@ -1979,8 +1977,14 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) } free(line); - if (nline == 0) + err = finish_command(&objdump_process); + if (err) + pr_err("Error running %s\n", command); + + if (nline == 0) { + err = -1; pr_err("No output from %s\n", command); + } /* * kallsyms does not have symbol sizes so there may a nop at the end. @@ -1990,23 +1994,21 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) delete_last_nop(sym); fclose(file); - err = 0; + +out_close_stdout: + close(objdump_process.out); + out_free_command: free(command); -out_remove_tmp: - close(stdout_fd[0]); +out_remove_tmp: if (decomp) unlink(symfs_filename); if (delete_extract) kcore_extract__delete(&kce); -out: - return err; -out_close_stdout: - close(stdout_fd[1]); - goto out_free_command; + return err; } static void calc_percent(struct sym_hist *sym_hist, -- cgit v1.2.3-59-g8ed1b From 7a675de428364a16038a8e6ed557daf0a009ce9c Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Thu, 10 Oct 2019 11:36:47 -0700 Subject: perf annotate: Don't pipe objdump output through 'grep' command Simplify the objdump command by not piping the output of objdump through grep. Instead, drop lines that match the grep pattern during the reading loop. Signed-off-by: Ian Rogers Tested-by: Arnaldo Carvalho de Melo Cc: Alexander Shishkin Cc: Jin Yao Cc: Jiri Olsa Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Song Liu Cc: Stephane Eranian Cc: clang-built-linux@googlegroups.com Link: http://lore.kernel.org/lkml/20191010183649.23768-4-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/annotate.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) (limited to 'tools/perf/util/annotate.c') diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 9835666db5a7..0e052e253835 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1916,7 +1916,7 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) err = asprintf(&command, "%s %s%s --start-address=0x%016" PRIx64 " --stop-address=0x%016" PRIx64 - " -l -d %s %s -C \"$1\" 2>/dev/null|grep -v \"$1:\"|expand", + " -l -d %s %s -C \"$1\" 2>/dev/null|expand", opts->objdump_path ?: "objdump", opts->disassembler_style ? "-M " : "", opts->disassembler_style ?: "", @@ -1962,9 +1962,16 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) nline = 0; while (!feof(file)) { + const char *match; + if (getline(&line, &line_len, file) < 0 || !line) break; + /* Skip lines containing "filename:" */ + match = strstr(line, symfs_filename); + if (match && match[strlen(symfs_filename)] == ':') + continue; + /* * The source code line number (lineno) needs to be kept in * across calls to symbol__parse_objdump_line(), so that it -- cgit v1.2.3-59-g8ed1b From b34b45eef16d814a93a52f2e76db803bb38939a0 Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Thu, 10 Oct 2019 11:36:48 -0700 Subject: perf annotate: Don't pipe objdump output through 'expand' command Avoiding a pipe allows objdump command failures to surface. Move to the caller of symbol__parse_objdump_line the call to strim that removes leading and trailing tabs. Add a new expand_tabs function that if a tab is present allocate a new line in which tabs are expanded. In symbol__parse_objdump_line the line had no leading spaces, so simplify the line_ip processing. Signed-off-by: Ian Rogers Tested-by: Arnaldo Carvalho de Melo Cc: Alexander Shishkin Cc: Jin Yao Cc: Jiri Olsa Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Song Liu Cc: Stephane Eranian Cc: clang-built-linux@googlegroups.com Link: http://lore.kernel.org/lkml/20191010183649.23768-5-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/annotate.c | 95 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 76 insertions(+), 19 deletions(-) (limited to 'tools/perf/util/annotate.c') diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 0e052e253835..efc5bfef790a 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1492,35 +1492,24 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start */ static int symbol__parse_objdump_line(struct symbol *sym, struct annotate_args *args, - char *line, int *line_nr) + char *parsed_line, int *line_nr) { struct map *map = args->ms.map; struct annotation *notes = symbol__annotation(sym); struct disasm_line *dl; - char *parsed_line, *tmp, *tmp2; + char *tmp; s64 line_ip, offset = -1; regmatch_t match[2]; - line_ip = -1; - parsed_line = strim(line); - /* /filename:linenr ? Save line number and ignore. */ if (regexec(&file_lineno, parsed_line, 2, match, 0) == 0) { *line_nr = atoi(parsed_line + match[1].rm_so); return 0; } - tmp = skip_spaces(parsed_line); - if (*tmp) { - /* - * Parse hexa addresses followed by ':' - */ - line_ip = strtoull(tmp, &tmp2, 16); - if (*tmp2 != ':' || tmp == tmp2 || tmp2[1] == '\0') - line_ip = -1; - } - - if (line_ip != -1) { + /* Process hex address followed by ':'. */ + line_ip = strtoull(parsed_line, &tmp, 16); + if (parsed_line != tmp && tmp[0] == ':' && tmp[1] != '\0') { u64 start = map__rip_2objdump(map, sym->start), end = map__rip_2objdump(map, sym->end); @@ -1528,7 +1517,7 @@ static int symbol__parse_objdump_line(struct symbol *sym, if ((u64)line_ip < start || (u64)line_ip >= end) offset = -1; else - parsed_line = tmp2 + 1; + parsed_line = tmp + 1; } args->offset = offset; @@ -1854,6 +1843,67 @@ static int symbol__disassemble_bpf(struct symbol *sym __maybe_unused, } #endif // defined(HAVE_LIBBFD_SUPPORT) && defined(HAVE_LIBBPF_SUPPORT) +/* + * Possibly create a new version of line with tabs expanded. Returns the + * existing or new line, storage is updated if a new line is allocated. If + * allocation fails then NULL is returned. + */ +static char *expand_tabs(char *line, char **storage, size_t *storage_len) +{ + size_t i, src, dst, len, new_storage_len, num_tabs; + char *new_line; + size_t line_len = strlen(line); + + for (num_tabs = 0, i = 0; i < line_len; i++) + if (line[i] == '\t') + num_tabs++; + + if (num_tabs == 0) + return line; + + /* + * Space for the line and '\0', less the leading and trailing + * spaces. Each tab may introduce 7 additional spaces. + */ + new_storage_len = line_len + 1 + (num_tabs * 7); + + new_line = malloc(new_storage_len); + if (new_line == NULL) { + pr_err("Failure allocating memory for tab expansion\n"); + return NULL; + } + + /* + * Copy regions starting at src and expand tabs. If there are two + * adjacent tabs then 'src == i', the memcpy is of size 0 and the spaces + * are inserted. + */ + for (i = 0, src = 0, dst = 0; i < line_len && num_tabs; i++) { + if (line[i] == '\t') { + len = i - src; + memcpy(&new_line[dst], &line[src], len); + dst += len; + new_line[dst++] = ' '; + while (dst % 8 != 0) + new_line[dst++] = ' '; + src = i + 1; + num_tabs--; + } + } + + /* Expand the last region. */ + len = line_len + 1 - src; + memcpy(&new_line[dst], &line[src], len); + dst += len; + new_line[dst] = '\0'; + + free(*storage); + *storage = new_line; + *storage_len = new_storage_len; + return new_line; + +} + static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) { struct annotation_options *opts = args->options; @@ -1916,7 +1966,7 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) err = asprintf(&command, "%s %s%s --start-address=0x%016" PRIx64 " --stop-address=0x%016" PRIx64 - " -l -d %s %s -C \"$1\" 2>/dev/null|expand", + " -l -d %s %s -C \"$1\" 2>/dev/null", opts->objdump_path ?: "objdump", opts->disassembler_style ? "-M " : "", opts->disassembler_style ?: "", @@ -1963,6 +2013,7 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) nline = 0; while (!feof(file)) { const char *match; + char *expanded_line; if (getline(&line, &line_len, file) < 0 || !line) break; @@ -1972,13 +2023,19 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) if (match && match[strlen(symfs_filename)] == ':') continue; + expanded_line = strim(line); + expanded_line = expand_tabs(expanded_line, &line, &line_len); + if (!expanded_line) + break; + /* * The source code line number (lineno) needs to be kept in * across calls to symbol__parse_objdump_line(), so that it * can associate it with the instructions till the next one. * See disasm_line__new() and struct disasm_line::line_nr. */ - if (symbol__parse_objdump_line(sym, args, line, &lineno) < 0) + if (symbol__parse_objdump_line(sym, args, expanded_line, + &lineno) < 0) break; nline++; } -- cgit v1.2.3-59-g8ed1b From c5baf9089246c1356705c9ba36d767ee8ce43dd2 Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Thu, 10 Oct 2019 11:36:49 -0700 Subject: perf annotate: Fix objdump --no-show-raw-insn flag Remove redirection of objdump's stderr to /dev/null to help diagnose failures. Fix the '--no-show-raw' flag to be '--no-show-raw-insn' which binutils is permissive and allows, but fails with LLVM objdump. Signed-off-by: Ian Rogers Cc: Alexander Shishkin Cc: Jin Yao Cc: Jiri Olsa Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Song Liu Cc: Stephane Eranian Cc: clang-built-linux@googlegroups.com Link: http://lore.kernel.org/lkml/20191010183649.23768-6-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/annotate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tools/perf/util/annotate.c') diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index efc5bfef790a..eef8aa87db66 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1966,13 +1966,13 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) err = asprintf(&command, "%s %s%s --start-address=0x%016" PRIx64 " --stop-address=0x%016" PRIx64 - " -l -d %s %s -C \"$1\" 2>/dev/null", + " -l -d %s %s -C \"$1\"", opts->objdump_path ?: "objdump", opts->disassembler_style ? "-M " : "", opts->disassembler_style ?: "", map__rip_2objdump(map, sym->start), map__rip_2objdump(map, sym->end), - opts->show_asm_raw ? "" : "--no-show-raw", + opts->show_asm_raw ? "" : "--no-show-raw-insn", opts->annotate_src ? "-S" : ""); if (err < 0) { -- cgit v1.2.3-59-g8ed1b