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
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 <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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
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 <[email protected]>
---
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