[PATCH 0/5] Improve objdump parsing, fix LLVM objdump

[PATCH 0/5] Improve objdump parsing, fix LLVM objdump

[Ian Rogers]

The objdump command line is piped through grep and expand meaning
failures don't surface. Refactor symbol__disassemble so that it manages
the memory for the read line, as well as trimming and expanding tabs.
Correct the objdump flag '--no-show-raw' to be '--no-show-raw-insn'
which binutils objdump permissively allows but fails with LLVM objdump.

Ian Rogers (5):
  perf annotate: avoid reallocation in objdump parsing
  perf annotate: use run-command.h to fork objdump
  perf annotate: don't pipe objdump output through grep
  perf annotate: don't pipe objdump output through expand
  perf annotate: fix objdump --no-show-raw-insn flag

 tools/perf/util/annotate.c | 195 +++++++++++++++++++++++++------------
 1 file changed, 131 insertions(+), 64 deletions(-)

-- 
2.23.0.581.g78d2f28ef7-goog

[PATCH 1/5] perf annotate: avoid reallocation in objdump parsing

[Ian Rogers]

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 <irogers@google.com>
---
 tools/perf/util/annotate.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index e830eadfca2a..1487849a191a 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1485,24 +1485,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);
 
@@ -1539,7 +1532,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)
@@ -1855,6 +1847,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)
@@ -1943,18 +1937,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);
-- 
2.23.0.581.g78d2f28ef7-goog

Re: [PATCH 1/5] perf annotate: avoid reallocation in objdump parsing

[Arnaldo Carvalho de Melo]

Em Thu, Oct 10, 2019 at 11:36:45AM -0700, Ian Rogers escreveu:
> 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.

Thanks, applied.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/annotate.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index e830eadfca2a..1487849a191a 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1485,24 +1485,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);
>  
> @@ -1539,7 +1532,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)
> @@ -1855,6 +1847,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)
> @@ -1943,18 +1937,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);
> -- 
> 2.23.0.581.g78d2f28ef7-goog

-- 

- Arnaldo

[tip: perf/core] perf annotate: Avoid reallocation in objdump parsing

[tip-bot2 for Ian Rogers]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     353dcaa2f979a04f9397306ae3165ccf9fc731df
Gitweb:        https://git.kernel.org/tip/353dcaa2f979a04f9397306ae3165ccf9fc731df
Author:        Ian Rogers <irogers@google.com>
AuthorDate:    Thu, 10 Oct 2019 11:36:45 -07:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 15 Oct 2019 08:36:22 -03:00

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 <irogers@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stephane Eranian <eranian@google.com>
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 <acme@redhat.com>
---
 tools/perf/util/annotate.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 2b856b6..f9c39a7 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);

[PATCH 2/5] perf annotate: use run-command.h to fork objdump

[Ian Rogers]

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 <irogers@google.com>
---
 tools/perf/util/annotate.c | 71 +++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 35 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 1487849a191a..fc12c5cfe112 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -43,6 +43,7 @@
 #include <linux/string.h>
 #include <bpf/libbpf.h>
 #include <subcmd/parse-options.h>
+#include <subcmd/run-command.h>
 
 /* FIXME: For the HE_COLORSET */
 #include "ui/browser.h"
@@ -1843,12 +1844,18 @@ 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. */
+	};
+	struct child_process objdump_process;
 	int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename));
 
 	if (err)
@@ -1878,7 +1885,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);
@@ -1903,38 +1910,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. */
@@ -1958,8 +1955,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.
@@ -1969,23 +1972,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,
-- 
2.23.0.581.g78d2f28ef7-goog

Re: [PATCH 2/5] perf annotate: use run-command.h to fork objdump

[Arnaldo Carvalho de Melo]

Em Thu, Oct 10, 2019 at 11:36:46AM -0700, Ian Rogers escreveu:
> 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.

Thanks, applied and tested.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/annotate.c | 71 +++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 1487849a191a..fc12c5cfe112 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -43,6 +43,7 @@
>  #include <linux/string.h>
>  #include <bpf/libbpf.h>
>  #include <subcmd/parse-options.h>
> +#include <subcmd/run-command.h>
>  
>  /* FIXME: For the HE_COLORSET */
>  #include "ui/browser.h"
> @@ -1843,12 +1844,18 @@ 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. */
> +	};
> +	struct child_process objdump_process;
>  	int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename));
>  
>  	if (err)
> @@ -1878,7 +1885,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);
> @@ -1903,38 +1910,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. */
> @@ -1958,8 +1955,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.
> @@ -1969,23 +1972,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,
> -- 
> 2.23.0.581.g78d2f28ef7-goog

-- 

- Arnaldo

Re: [PATCH 2/5] perf annotate: use run-command.h to fork objdump

[Ian Rogers]

Thanks for applying! Apologies, there was a missing null termination.
Sent in a follow-up patch.

Ian

On Mon, Oct 14, 2019 at 7:18 AM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, Oct 10, 2019 at 11:36:46AM -0700, Ian Rogers escreveu:
> > 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.
>
> Thanks, applied and tested.
>
> - Arnaldo
>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/annotate.c | 71 +++++++++++++++++++-------------------
> >  1 file changed, 36 insertions(+), 35 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 1487849a191a..fc12c5cfe112 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -43,6 +43,7 @@
> >  #include <linux/string.h>
> >  #include <bpf/libbpf.h>
> >  #include <subcmd/parse-options.h>
> > +#include <subcmd/run-command.h>
> >
> >  /* FIXME: For the HE_COLORSET */
> >  #include "ui/browser.h"
> > @@ -1843,12 +1844,18 @@ 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. */
> > +     };
> > +     struct child_process objdump_process;
> >       int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename));
> >
> >       if (err)
> > @@ -1878,7 +1885,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);
> > @@ -1903,38 +1910,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. */
> > @@ -1958,8 +1955,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.
> > @@ -1969,23 +1972,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,
> > --
> > 2.23.0.581.g78d2f28ef7-goog
>
> --
>
> - Arnaldo

[tip: perf/core] perf annotate: Use libsubcmd's run-command.h to fork objdump

[tip-bot2 for Ian Rogers]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     4235949944d1bb244c85fd184cdc2f78e9df848b
Gitweb:        https://git.kernel.org/tip/4235949944d1bb244c85fd184cdc2f78e9df848b
Author:        Ian Rogers <irogers@google.com>
AuthorDate:    Thu, 10 Oct 2019 11:36:46 -07:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 15 Oct 2019 08:39:01 -03:00

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 <irogers@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stephane Eranian <eranian@google.com>
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 <acme@redhat.com>
---
 tools/perf/util/annotate.c | 72 +++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index f9c39a7..9835666 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -43,6 +43,7 @@
 #include <linux/string.h>
 #include <bpf/libbpf.h>
 #include <subcmd/parse-options.h>
+#include <subcmd/run-command.h>
 
 /* 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,

[PATCH 3/5] perf annotate: don't pipe objdump output through grep

[Ian Rogers]

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 <irogers@google.com>
---
 tools/perf/util/annotate.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index fc12c5cfe112..0a7a6f3c55f4 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1894,7 +1894,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 ?: "",
@@ -1940,9 +1940,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
-- 
2.23.0.581.g78d2f28ef7-goog

Re: [PATCH 3/5] perf annotate: don't pipe objdump output through grep

[Arnaldo Carvalho de Melo]

Em Thu, Oct 10, 2019 at 11:36:47AM -0700, Ian Rogers escreveu:
> 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.

Thanks, applied and tested.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/annotate.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index fc12c5cfe112..0a7a6f3c55f4 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1894,7 +1894,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 ?: "",
> @@ -1940,9 +1940,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
> -- 
> 2.23.0.581.g78d2f28ef7-goog

-- 

- Arnaldo

[tip: perf/core] perf annotate: Don't pipe objdump output through 'grep' command

[tip-bot2 for Ian Rogers]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     7a675de428364a16038a8e6ed557daf0a009ce9c
Gitweb:        https://git.kernel.org/tip/7a675de428364a16038a8e6ed557daf0a009ce9c
Author:        Ian Rogers <irogers@google.com>
AuthorDate:    Thu, 10 Oct 2019 11:36:47 -07:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 15 Oct 2019 08:39:42 -03:00

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 <irogers@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stephane Eranian <eranian@google.com>
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 <acme@redhat.com>
---
 tools/perf/util/annotate.c |  9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 9835666..0e052e2 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

[PATCH 4/5] perf annotate: don't pipe objdump output through expand

[Ian Rogers]

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 <irogers@google.com>
---
 tools/perf/util/annotate.c | 95 ++++++++++++++++++++++++++++++--------
 1 file changed, 76 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 0a7a6f3c55f4..3d5b9236576a 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1488,35 +1488,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);
 
@@ -1524,7 +1513,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;
@@ -1833,6 +1822,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;
@@ -1894,7 +1944,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 ?: "",
@@ -1941,6 +1991,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;
@@ -1950,13 +2001,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++;
 	}
-- 
2.23.0.581.g78d2f28ef7-goog

Re: [PATCH 4/5] perf annotate: don't pipe objdump output through expand

[Arnaldo Carvalho de Melo]

Em Thu, Oct 10, 2019 at 11:36:48AM -0700, Ian Rogers escreveu:
> 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.

Thanks, applied.
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/annotate.c | 95 ++++++++++++++++++++++++++++++--------
>  1 file changed, 76 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 0a7a6f3c55f4..3d5b9236576a 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1488,35 +1488,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);
>  
> @@ -1524,7 +1513,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;
> @@ -1833,6 +1822,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;
> @@ -1894,7 +1944,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 ?: "",
> @@ -1941,6 +1991,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;
> @@ -1950,13 +2001,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++;
>  	}
> -- 
> 2.23.0.581.g78d2f28ef7-goog

-- 

- Arnaldo

[tip: perf/core] perf annotate: Don't pipe objdump output through 'expand' command

[tip-bot2 for Ian Rogers]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     b34b45eef16d814a93a52f2e76db803bb38939a0
Gitweb:        https://git.kernel.org/tip/b34b45eef16d814a93a52f2e76db803bb38939a0
Author:        Ian Rogers <irogers@google.com>
AuthorDate:    Thu, 10 Oct 2019 11:36:48 -07:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 15 Oct 2019 08:39:42 -03:00

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 <irogers@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stephane Eranian <eranian@google.com>
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 <acme@redhat.com>
---
 tools/perf/util/annotate.c | 95 +++++++++++++++++++++++++++++--------
 1 file changed, 76 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 0e052e2..efc5bfe 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++;
 	}

[PATCH 5/5] perf annotate: fix objdump --no-show-raw-insn flag

[Ian Rogers]

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 <irogers@google.com>
---
 tools/perf/util/annotate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 3d5b9236576a..2a71c90a4921 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1944,13 +1944,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) {
-- 
2.23.0.581.g78d2f28ef7-goog

Re: [PATCH 5/5] perf annotate: fix objdump --no-show-raw-insn flag

[Arnaldo Carvalho de Melo]

Em Thu, Oct 10, 2019 at 11:36:49AM -0700, Ian Rogers escreveu:
> 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.

Thanks, applied.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/annotate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 3d5b9236576a..2a71c90a4921 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1944,13 +1944,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) {
> -- 
> 2.23.0.581.g78d2f28ef7-goog

-- 

- Arnaldo

[tip: perf/core] perf annotate: Fix objdump --no-show-raw-insn flag

[tip-bot2 for Ian Rogers]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     c5baf9089246c1356705c9ba36d767ee8ce43dd2
Gitweb:        https://git.kernel.org/tip/c5baf9089246c1356705c9ba36d767ee8ce43dd2
Author:        Ian Rogers <irogers@google.com>
AuthorDate:    Thu, 10 Oct 2019 11:36:49 -07:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 15 Oct 2019 08:39:42 -03:00

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 <irogers@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stephane Eranian <eranian@google.com>
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 <acme@redhat.com>
---
 tools/perf/util/annotate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index efc5bfe..eef8aa8 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) {