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

[Alexis Berlemont <alexis.berlemont@gmail.com>]

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