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

[Arnaldo Carvalho de Melo]

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@xxxxxxxxx>
> ---
>  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