Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1533693 > unrolled thread

[PATCH] perf: check that objdump correctly works

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf: check that objdump correctly works Alexis Berlemont <alexis.berlemont@gmail.com> - 2016-12-01 01:10 +0100
    [PATCH] perf annotate: check that objdump correctly works Alexis Berlemont <alexis.berlemont@gmail.com> - 2016-12-01 01:10 +0100
      Re: [PATCH] perf annotate: check that objdump correctly works Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-12-06 20:40 +0100
        Re: [PATCH] perf annotate: check that objdump correctly works Alexis Berlemont <alexis.berlemont@gmail.com> - 2016-12-10 01:00 +0100
    Re: [PATCH] perf: check that objdump correctly works Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-12-06 15:50 +0100

#1533693 — [PATCH] perf: check that objdump correctly works

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

[toc] | [next] | [standalone]

#1533695 — [PATCH] perf annotate: check that objdump correctly works

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

[toc] | [prev] | [next] | [standalone]

#1537240 — Re: [PATCH] perf annotate: check that objdump correctly works

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

[toc] | [prev] | [next] | [standalone]

#1539741 — Re: [PATCH] perf annotate: check that objdump correctly works

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

[toc] | [prev] | [next] | [standalone]

#1537010

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web