All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf: check that objdump correctly works
@ 2016-12-01  0:04 Alexis Berlemont
  2016-12-01  0:04 ` [PATCH] perf annotate: " Alexis Berlemont
  2016-12-06 14:44 ` [PATCH] perf: " Arnaldo Carvalho de Melo
  0 siblings, 2 replies; 7+ messages in thread
From: Alexis Berlemont @ 2016-12-01  0:04 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexis Berlemont, peterz, mingo, acme, alexander.shishkin

Hi,

In perf todo list, there was an entry regarding objdump:

* Check if the objdump call in annotate worked or not, providing a
  popup window telling it didn't work, to test, just uninstall
  binutils or otherwise remove objdump from the path.

The patch below tries to fulfill that point.

Alexis.

Alexis Berlemont (1):
  perf annotate: check that objdump correctly works

 tools/perf/util/annotate.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
 tools/perf/util/annotate.h |  3 ++
 2 files changed, 81 insertions(+), 1 deletion(-)

-- 
2.10.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] perf annotate: check that objdump correctly works
  2016-12-01  0:04 [PATCH] perf: check that objdump correctly works Alexis Berlemont
@ 2016-12-01  0:04 ` Alexis Berlemont
  2016-12-06 19:38   ` Arnaldo Carvalho de Melo
  2016-12-06 14:44 ` [PATCH] perf: " Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 7+ messages in thread
From: Alexis Berlemont @ 2016-12-01  0:04 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexis Berlemont, peterz, mingo, acme, alexander.shishkin

Before disassembling, the tool objdump is called just to be sure:
* objdump is available in the path;
* objdump is an executable binary;
* objdump has no dependency issue or anything else.

This objdump "pre-"command is only necessary because the real objdump
command is followed by some " | grep ..."; this prevents the shell
from returning the exit code of objdump execution.

Signed-off-by: Alexis Berlemont <alexis.berlemont@gmail.com>
---
 tools/perf/util/annotate.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
 tools/perf/util/annotate.h |  3 ++
 2 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 3e34ee0..9d6c3a0 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -20,9 +20,12 @@
 #include "block-range.h"
 #include "arch/common.h"
 #include <regex.h>
+#include <unistd.h>
 #include <pthread.h>
 #include <linux/bitops.h>
 #include <sys/utsname.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 
 const char 	*disassembler_style;
 const char	*objdump_path;
@@ -1278,6 +1281,21 @@ int symbol__strerror_disassemble(struct symbol *sym __maybe_unused, struct map *
 			  "  --vmlinux vmlinux\n", build_id_msg ?: "");
 	}
 		break;
+
+	case SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP:
+		scnprintf(buf, buflen, "No objdump tool available in $PATH\n");
+		break;
+
+	case SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP:
+		scnprintf(buf, buflen,
+			"The objdump tool found in $PATH cannot be executed\n");
+		break;
+
+	case SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT:
+		scnprintf(buf, buflen,
+			"The objdump tool returned no disassembled code\n");
+		break;
+
 	default:
 		scnprintf(buf, buflen, "Internal error: Invalid %d error code\n", errnum);
 		break;
@@ -1321,6 +1339,61 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	return 0;
 }
 
+static int annotate__check_objdump(void)
+{
+	char command[PATH_MAX * 2];
+	int wstatus, err;
+	pid_t pid;
+
+	snprintf(command, sizeof(command),
+		"%s -v > /dev/null 2>&1",
+		objdump_path ? objdump_path : "objdump");
+
+	pid = fork();
+	if (pid < 0) {
+		pr_err("Failure forking to run %s\n", command);
+		return -1;
+	}
+
+	if (pid == 0) {
+		execl("/bin/sh", "sh", "-c", command, NULL);
+		exit(-1);
+	}
+
+	err = waitpid(pid, &wstatus, 0);
+	if (err < 0) {
+		pr_err("Failure calling waitpid: %s: (%s)\n",
+			strerror(errno), command);
+		return -1;
+	}
+
+	pr_err("%s: %d %d\n", command, pid, WEXITSTATUS(wstatus));
+
+	switch (WEXITSTATUS(wstatus)) {
+	case 0:
+		/* Success */
+		err = 0;
+		break;
+	case 127:
+		/* The shell did not find objdump in the path */
+		err = SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP;
+		break;
+	default:
+		/*
+		 * In the default case, we consider that objdump
+		 * cannot be executed; so it gathers many fault
+		 * scenarii:
+		 * - objdump is not an executable (126);
+		 * - objdump has some dependency issue;
+		 * - ...
+		 */
+		err = SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP;
+		break;
+	}
+
+	return err;
+}
+
 static const char *annotate__norm_arch(const char *arch_name)
 {
 	struct utsname uts;
@@ -1351,6 +1424,10 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 	if (err)
 		return err;
 
+	err = annotate__check_objdump();
+	if (err)
+		return err;
+
 	arch_name = annotate__norm_arch(arch_name);
 	if (!arch_name)
 		return -1;
@@ -1482,7 +1559,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 		delete_last_nop(sym);
 
 	fclose(file);
-	err = 0;
+	err = nline == 0 ? SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT : 0;
 out_remove_tmp:
 	close(stdout_fd[0]);
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 87e4cad..123f60c 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -172,6 +172,9 @@ enum symbol_disassemble_errno {
 	__SYMBOL_ANNOTATE_ERRNO__START		= -10000,
 
 	SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX	= __SYMBOL_ANNOTATE_ERRNO__START,
+	SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP,
+	SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP,
+	SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT,
 
 	__SYMBOL_ANNOTATE_ERRNO__END,
 };
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] perf: check that objdump correctly works
  2016-12-01  0:04 [PATCH] perf: check that objdump correctly works Alexis Berlemont
  2016-12-01  0:04 ` [PATCH] perf annotate: " Alexis Berlemont
@ 2016-12-06 14:44 ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-12-06 14:44 UTC (permalink / raw)
  To: Alexis Berlemont; +Cc: linux-kernel, peterz, mingo, alexander.shishkin

Em Thu, Dec 01, 2016 at 01:04:35AM +0100, Alexis Berlemont escreveu:
> Hi,
> 
> In perf todo list, there was an entry regarding objdump:
> 
> * Check if the objdump call in annotate worked or not, providing a
>   popup window telling it didn't work, to test, just uninstall
>   binutils or otherwise remove objdump from the path.
> 
> The patch below tries to fulfill that point.

Thanks for working on this, will try and process it ASAP.

- Arnaldo
 
> Alexis.
> 
> Alexis Berlemont (1):
>   perf annotate: check that objdump correctly works
> 
>  tools/perf/util/annotate.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
>  tools/perf/util/annotate.h |  3 ++
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> -- 
> 2.10.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] perf annotate: check that objdump correctly works
  2016-12-01  0:04 ` [PATCH] perf annotate: " Alexis Berlemont
@ 2016-12-06 19:38   ` Arnaldo Carvalho de Melo
  2016-12-09 23:57     ` Alexis Berlemont
                       ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-12-06 19:38 UTC (permalink / raw)
  To: Alexis Berlemont; +Cc: linux-kernel, peterz, mingo, alexander.shishkin

Em Thu, Dec 01, 2016 at 01:04:36AM +0100, Alexis Berlemont escreveu:
> Before disassembling, the tool objdump is called just to be sure:
> * objdump is available in the path;
> * objdump is an executable binary;
> * objdump has no dependency issue or anything else.
> 
> This objdump "pre-"command is only necessary because the real objdump
> command is followed by some " | grep ..."; this prevents the shell
> from returning the exit code of objdump execution.
> 
> Signed-off-by: Alexis Berlemont <alexis.berlemont@gmail.com>
> ---
>  tools/perf/util/annotate.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
>  tools/perf/util/annotate.h |  3 ++
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 3e34ee0..9d6c3a0 100644
> --- a/tools/perf/util/annotate.c
  
> +static int annotate__check_objdump(void)
> +{
> +	char command[PATH_MAX * 2];
> +	int wstatus, err;
> +	pid_t pid;
> +
> +	snprintf(command, sizeof(command),
> +		"%s -v > /dev/null 2>&1",
> +		objdump_path ? objdump_path : "objdump");
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		pr_err("Failure forking to run %s\n", command);
> +		return -1;
> +	}
> +
> +	if (pid == 0) {
> +		execl("/bin/sh", "sh", "-c", command, NULL);
> +		exit(-1);
> +	}
> +
> +	err = waitpid(pid, &wstatus, 0);
> +	if (err < 0) {
> +		pr_err("Failure calling waitpid: %s: (%s)\n",
> +			strerror(errno), command);
> +		return -1;
> +	}
> +
> +	pr_err("%s: %d %d\n", command, pid, WEXITSTATUS(wstatus));

So this will appear in all cases, no need for that, i.e. in the success
case we don't need to get that flashing on the screen, on the last line.

> +	switch (WEXITSTATUS(wstatus)) {
> +	case 0:
> +		/* Success */
> +		err = 0;
> +		break;

So probably you want to return 0;  here instead and then at the error
case, i.e. when you set err to !0 you do that pr_err() call above, but I
think it would be better to use pr_debug(), the warning on the popup box
is what by default is more polished to tell the user, the details are
for developers or people wanting to dig in.

But while doing this I thought that you could instead call this only
after objdump fails, i.e. if all is right, no need for checking what
went wrong.

I.e. you would do the grep step separately, after checking objdump's
error.

If you think that is too much work, then please just do the
pr_err->pr_debug conversion, which would remove the flashing for the
success case.

I tested it, btw, using:

  perf annotate --objdump /dev/null page_fault

Which produced a better output than what we have now (nothing):

   ┌─Error:───────────────────────────────────────────┐
   │Couldn't annotate page_fault:                     │
   │The objdump tool found in $PATH cannot be executed│
   │                                                  │
   │                                                  │
   │Press any key...                                  │
   └──────────────────────────────────────────────────┘



/dev/null -v > /dev/null 2>&1: 10336 126


-------------------

summary: make that last line appear only when -v is used (pr_debug) and
consider covering the case where --objdump was used, where talking about $PATH
is misleading.


> +	case 127:
> +		/* The shell did not find objdump in the path */
> +		err = SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP;
> +		break;
> +	default:
> +		/*
> +		 * In the default case, we consider that objdump
> +		 * cannot be executed; so it gathers many fault
> +		 * scenarii:
> +		 * - objdump is not an executable (126);
> +		 * - objdump has some dependency issue;
> +		 * - ...
> +		 */
> +		err = SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP;
> +		break;
> +	}
> +
> +	return err;
> +}
> +
>  static const char *annotate__norm_arch(const char *arch_name)
>  {
>  	struct utsname uts;
> @@ -1351,6 +1424,10 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
>  	if (err)
>  		return err;
>  
> +	err = annotate__check_objdump();
> +	if (err)
> +		return err;
> +
>  	arch_name = annotate__norm_arch(arch_name);
>  	if (!arch_name)
>  		return -1;
> @@ -1482,7 +1559,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
>  		delete_last_nop(sym);
>  
>  	fclose(file);
> -	err = 0;
> +	err = nline == 0 ? SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT : 0;
>  out_remove_tmp:
>  	close(stdout_fd[0]);
>  
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 87e4cad..123f60c 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -172,6 +172,9 @@ enum symbol_disassemble_errno {
>  	__SYMBOL_ANNOTATE_ERRNO__START		= -10000,
>  
>  	SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX	= __SYMBOL_ANNOTATE_ERRNO__START,
> +	SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP,
> +	SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP,
> +	SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT,
>  
>  	__SYMBOL_ANNOTATE_ERRNO__END,
>  };
> -- 
> 2.10.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] perf annotate: check that objdump correctly works
  2016-12-06 19:38   ` Arnaldo Carvalho de Melo
@ 2016-12-09 23:57     ` Alexis Berlemont
  2016-12-20  0:11     ` [PATCH v2 0/1] perf: " Alexis Berlemont
  2016-12-20  0:11     ` [PATCH v2 1/1] perf annotate: " Alexis Berlemont
  2 siblings, 0 replies; 7+ messages in thread
From: Alexis Berlemont @ 2016-12-09 23:57 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, peterz, mingo, alexander.shishkin

Arnaldo Carvalho de Melo wrote:
> Em Thu, Dec 01, 2016 at 01:04:36AM +0100, Alexis Berlemont escreveu:
> > Before disassembling, the tool objdump is called just to be sure:
> > * objdump is available in the path;
> > * objdump is an executable binary;
> > * objdump has no dependency issue or anything else.
> > 
> > This objdump "pre-"command is only necessary because the real objdump
> > command is followed by some " | grep ..."; this prevents the shell
> > from returning the exit code of objdump execution.
> > 
> > Signed-off-by: Alexis Berlemont <alexis.berlemont@gmail.com>
> > ---
> >  tools/perf/util/annotate.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
> >  tools/perf/util/annotate.h |  3 ++
> >  2 files changed, 81 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 3e34ee0..9d6c3a0 100644
> > --- a/tools/perf/util/annotate.c
>   
> > +static int annotate__check_objdump(void)
> > +{
> > +	char command[PATH_MAX * 2];
> > +	int wstatus, err;
> > +	pid_t pid;
> > +
> > +	snprintf(command, sizeof(command),
> > +		"%s -v > /dev/null 2>&1",
> > +		objdump_path ? objdump_path : "objdump");
> > +
> > +	pid = fork();
> > +	if (pid < 0) {
> > +		pr_err("Failure forking to run %s\n", command);
> > +		return -1;
> > +	}
> > +
> > +	if (pid == 0) {
> > +		execl("/bin/sh", "sh", "-c", command, NULL);
> > +		exit(-1);
> > +	}
> > +
> > +	err = waitpid(pid, &wstatus, 0);
> > +	if (err < 0) {
> > +		pr_err("Failure calling waitpid: %s: (%s)\n",
> > +			strerror(errno), command);
> > +		return -1;
> > +	}
> > +
> > +	pr_err("%s: %d %d\n", command, pid, WEXITSTATUS(wstatus));
> 
> So this will appear in all cases, no need for that, i.e. in the success
> case we don't need to get that flashing on the screen, on the last line.
>

Many thanks for your answer and your time.

Sorry for the late anwser and such an obvious error.

> > +	switch (WEXITSTATUS(wstatus)) {
> > +	case 0:
> > +		/* Success */
> > +		err = 0;
> > +		break;
> 
> So probably you want to return 0;  here instead and then at the error
> case, i.e. when you set err to !0 you do that pr_err() call above, but I
> think it would be better to use pr_debug(), the warning on the popup box
> is what by default is more polished to tell the user, the details are
> for developers or people wanting to dig in.
> 
> But while doing this I thought that you could instead call this only
> after objdump fails, i.e. if all is right, no need for checking what
> went wrong.
> 
> I.e. you would do the grep step separately, after checking objdump's
> error.
> 
> If you think that is too much work, then please just do the
> pr_err->pr_debug conversion, which would remove the flashing for the
> success case.

I will do the grep separately; no problem.

Alexis.

> 
> I tested it, btw, using:
> 
>   perf annotate --objdump /dev/null page_fault
> 
> Which produced a better output than what we have now (nothing):
> 
>    ??????Error:????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????
>    ???Couldn't annotate page_fault:                     ???
>    ???The objdump tool found in $PATH cannot be executed???
>    ???                                                  ???
>    ???                                                  ???
>    ???Press any key...                                  ???
>    ????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????
> 
> 
> 
> /dev/null -v > /dev/null 2>&1: 10336 126
> 
> 
> -------------------
> 
> summary: make that last line appear only when -v is used (pr_debug) and
> consider covering the case where --objdump was used, where talking about $PATH
> is misleading.
> 
> 
> > +	case 127:
> > +		/* The shell did not find objdump in the path */
> > +		err = SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP;
> > +		break;
> > +	default:
> > +		/*
> > +		 * In the default case, we consider that objdump
> > +		 * cannot be executed; so it gathers many fault
> > +		 * scenarii:
> > +		 * - objdump is not an executable (126);
> > +		 * - objdump has some dependency issue;
> > +		 * - ...
> > +		 */
> > +		err = SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP;
> > +		break;
> > +	}
> > +
> > +	return err;
> > +}
> > +
> >  static const char *annotate__norm_arch(const char *arch_name)
> >  {
> >  	struct utsname uts;
> > @@ -1351,6 +1424,10 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
> >  	if (err)
> >  		return err;
> >  
> > +	err = annotate__check_objdump();
> > +	if (err)
> > +		return err;
> > +
> >  	arch_name = annotate__norm_arch(arch_name);
> >  	if (!arch_name)
> >  		return -1;
> > @@ -1482,7 +1559,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
> >  		delete_last_nop(sym);
> >  
> >  	fclose(file);
> > -	err = 0;
> > +	err = nline == 0 ? SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT : 0;
> >  out_remove_tmp:
> >  	close(stdout_fd[0]);
> >  
> > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> > index 87e4cad..123f60c 100644
> > --- a/tools/perf/util/annotate.h
> > +++ b/tools/perf/util/annotate.h
> > @@ -172,6 +172,9 @@ enum symbol_disassemble_errno {
> >  	__SYMBOL_ANNOTATE_ERRNO__START		= -10000,
> >  
> >  	SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX	= __SYMBOL_ANNOTATE_ERRNO__START,
> > +	SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP,
> > +	SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP,
> > +	SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT,
> >  
> >  	__SYMBOL_ANNOTATE_ERRNO__END,
> >  };
> > -- 
> > 2.10.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 0/1] perf: check that objdump correctly works
  2016-12-06 19:38   ` Arnaldo Carvalho de Melo
  2016-12-09 23:57     ` Alexis Berlemont
@ 2016-12-20  0:11     ` Alexis Berlemont
  2016-12-20  0:11     ` [PATCH v2 1/1] perf annotate: " Alexis Berlemont
  2 siblings, 0 replies; 7+ messages in thread
From: Alexis Berlemont @ 2016-12-20  0:11 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexis Berlemont, peterz, mingo, acme, alexander.shishkin

Hi Arnaldo,

Here is a patch which checks that objdump works without calling it
first with the "-v" option.

Thanks to the shell pipefail option (which returns the rightmost error
status in the shell pipe) and the grep subshell which prevents
"no-match" errors, we are able the use waitpid on the whole pipeline
and get the appropriate return code.

This is the most simple and straightforward solution I found. The
downside might be that it relies on the availability of "pipefail"
shell option. However, I checked that even busybox's shell implemented
it.

Alexis.

Alexis Berlemont (1):
  perf annotate: check that objdump correctly works

 tools/perf/util/annotate.c | 69 +++++++++++++++++++++++++++++++++++++++++++---
 tools/perf/util/annotate.h |  3 ++
 2 files changed, 68 insertions(+), 4 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/1] perf annotate: check that objdump correctly works
  2016-12-06 19:38   ` Arnaldo Carvalho de Melo
  2016-12-09 23:57     ` Alexis Berlemont
  2016-12-20  0:11     ` [PATCH v2 0/1] perf: " Alexis Berlemont
@ 2016-12-20  0:11     ` Alexis Berlemont
  2 siblings, 0 replies; 7+ messages in thread
From: Alexis Berlemont @ 2016-12-20  0:11 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexis Berlemont, peterz, mingo, acme, alexander.shishkin

Before disassembling, the tool objdump is called just to be sure:
* objdump is available in the path;
* objdump is an executable binary;
* objdump has no dependency issue or anything else.

This objdump "pre-"command is only necessary because the real objdump
command is followed by some " | grep ..."; this prevents the shell
from returning the exit code of objdump execution.

Signed-off-by: Alexis Berlemont <alexis.berlemont@gmail.com>
---
 tools/perf/util/annotate.c | 69 +++++++++++++++++++++++++++++++++++++++++++---
 tools/perf/util/annotate.h |  3 ++
 2 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index ea7e0de4b9c1..6fbaabbc9d2a 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -20,9 +20,12 @@
 #include "block-range.h"
 #include "arch/common.h"
 #include <regex.h>
+#include <unistd.h>
 #include <pthread.h>
 #include <linux/bitops.h>
 #include <sys/utsname.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 
 const char 	*disassembler_style;
 const char	*objdump_path;
@@ -1286,6 +1289,21 @@ int symbol__strerror_disassemble(struct symbol *sym __maybe_unused, struct map *
 			  "  --vmlinux vmlinux\n", build_id_msg ?: "");
 	}
 		break;
+
+	case SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP:
+		scnprintf(buf, buflen, "No objdump tool available in $PATH\n");
+		break;
+
+	case SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP:
+		scnprintf(buf, buflen,
+			"The objdump tool found in $PATH cannot be executed\n");
+		break;
+
+	case SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT:
+		scnprintf(buf, buflen,
+			"The objdump tool returned no disassembled code\n");
+		break;
+
 	default:
 		scnprintf(buf, buflen, "Internal error: Invalid %d error code\n", errnum);
 		break;
@@ -1329,6 +1347,44 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 	return 0;
 }
 
+static int annotate__check_exec(int pid, const char *command)
+{
+	int wstatus, err;
+
+	err = waitpid(pid, &wstatus, 0);
+	if (err < 0) {
+		pr_err("Failure calling waitpid: %s: (%s)\n",
+			strerror(errno), command);
+		return -1;
+	}
+
+	pr_debug("%s: %d %d\n", command, pid, WEXITSTATUS(wstatus));
+
+	switch (WEXITSTATUS(wstatus)) {
+	case 0:
+		/* Success */
+		err = 0;
+		break;
+	case 127:
+		/* The shell did not find objdump in the path */
+		err = SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP;
+		break;
+	default:
+		/*
+		 * In the default case, we consider that objdump
+		 * cannot be executed; so it gathers many fault
+		 * scenarii:
+		 * - objdump is not an executable (126);
+		 * - objdump has some dependency issue;
+		 * - ...
+		 */
+		err = SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP;
+		break;
+	}
+
+	return err;
+}
+
 static const char *annotate__norm_arch(const char *arch_name)
 {
 	struct utsname uts;
@@ -1426,7 +1482,8 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 	snprintf(command, sizeof(command),
 		 "%s %s%s --start-address=0x%016" PRIx64
 		 " --stop-address=0x%016" PRIx64
-		 " -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
+		 " -l -d %s %s -C %s 2>/dev/null | "
+		 "{ grep -v %s || true; } | expand",
 		 objdump_path ? objdump_path : "objdump",
 		 disassembler_style ? "-M " : "",
 		 disassembler_style ? disassembler_style : "",
@@ -1454,7 +1511,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 		close(stdout_fd[0]);
 		dup2(stdout_fd[1], 1);
 		close(stdout_fd[1]);
-		execl("/bin/sh", "sh", "-c", command, NULL);
+		execl("/bin/sh", "sh", "-o", "pipefail", "-c", command, NULL);
 		perror(command);
 		exit(-1);
 	}
@@ -1479,8 +1536,12 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 		nline++;
 	}
 
-	if (nline == 0)
+	err = annotate__check_exec(pid, command);
+
+	if (err == 0 && nline == 0) {
 		pr_err("No output from %s\n", command);
+		err = SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT;
+	}
 
 	/*
 	 * kallsyms does not have symbol sizes so there may a nop at the end.
@@ -1490,7 +1551,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 		delete_last_nop(sym);
 
 	fclose(file);
-	err = 0;
+
 out_remove_tmp:
 	close(stdout_fd[0]);
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 87e4cadc5d27..123f60c509a9 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -172,6 +172,9 @@ enum symbol_disassemble_errno {
 	__SYMBOL_ANNOTATE_ERRNO__START		= -10000,
 
 	SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX	= __SYMBOL_ANNOTATE_ERRNO__START,
+	SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP,
+	SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP,
+	SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT,
 
 	__SYMBOL_ANNOTATE_ERRNO__END,
 };
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-12-20  0:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-01  0:04 [PATCH] perf: check that objdump correctly works Alexis Berlemont
2016-12-01  0:04 ` [PATCH] perf annotate: " Alexis Berlemont
2016-12-06 19:38   ` Arnaldo Carvalho de Melo
2016-12-09 23:57     ` Alexis Berlemont
2016-12-20  0:11     ` [PATCH v2 0/1] perf: " Alexis Berlemont
2016-12-20  0:11     ` [PATCH v2 1/1] perf annotate: " Alexis Berlemont
2016-12-06 14:44 ` [PATCH] perf: " Arnaldo Carvalho de Melo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.