Re: [PATCH perf/core v7 02/21] perf buildid: Fix to set correct dso name for kallsyms

[Masami Hiramatsu]

On Thu, 12 May 2016 11:02:12 +0900
Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:

> On Wed, 11 May 2016 12:45:00 -0300
> Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:
> 
> > Em Wed, May 11, 2016 at 10:51:37PM +0900, Masami Hiramatsu escreveu:
> > > Fix to set correct dso name ("[kernel.kallsyms]") for
> > > kallsyms, as for vdso does.
> > 
> > Can you rewrite the above comment and also break this down in two
> > patches,
> 
> No, could you explain what parts this consists of?
> I think this patch is done just one thing. Actually I can rewrite
> this as one-line patch like below
> 
>   	if (asprintf(&filename, "%s%s%s", buildid_dir, slash ? "/" : "",
> -		     is_vdso ? DSO__NAME_VDSO : realname) < 0)
> +		     is_vdso ? DSO__NAME_VDSO : (is_kallsyms ? "[kernel.kallsyms]": realname)) < 0)
> 
> But indeed this is not readable so clean it up like below.
> 
> > probably decribing what is the problem that it fixes?
> 
> Ah, indeed. This is actually not a fix for existing bug, instead
> it will prevent buggy behavior.　Current function can get any filepath
> with is_vdso = true or is_kallsyms = true, but it seems easily giving
> contradictory combination.
> 
> Hmm, OK, I think I have to solve this issue in another way.

OK, from further investigation, I found this is not a real bug, but
just an unneeded behavior changing. At this moment, I should drop this
from the series. Here is what I understand.

At first, the path of the original binary is used only when storing
build-id caches.
When loading the cache, perf uses build-id symlink to access it.

OK, so this is the callgraph of build_id_cache__dirname_from_path.
'<-' shows the caller, and with what parameters given.
 
build_id_cache__dirname_from_path
	<- build_id_cache__add_s, with (pathname, is_kallsyms, is_vdso)
		<- build_id_cache__add_b, with (sbuild_id, name, is_kallsyms, is_vdso)
			<-dso__cache_build_id
		<- build_id_cache__add_file, with (sbuild_id,filepath,false,false)
		<- build_id_cache__update_file, with (sbuild_id,filepath,false,false)
	<- build_id_cache__list_build_ids, with (pathname, false, false)

Focusing on the is_kallsyms and is_vdso, the callgraph to
build_id_cache__list_build_ids(), build_id_cache__add_file()
and build_id_cache__update_file() are not using those parameters.
These functions just pass the file path, so we can ignore.
(BTW, in this case we can not remove/purge the caches for vdso, kallsyms and
 kcore from buildid directory ...)

The last callgraph is dso__cache_build_id(), and only it uses is_kallsyms and is_vdso.
Those parameters are given as below

  is_kallsyms = dso->kernel && dso->long_name[0] != '/'
  is_vdso = dso__is_vdso(dso) <- check the dso->short_name is DSO__NAME_VDSO*
  pathname = dso->long_name

So, is_vdso == true, build_id_cache__dirname_from_path() stores all
DSO__NAME_VDSO* ("[vdso]", "[vdso32]", and "[vdsox32]") caches under
DSO__NAME_VDSO("[vdso]").

This seems is_kallsyms == true, the pathname has no real filename.

And the kernel dso will be made by machine__get_kernel(), which sets the
kernel dso's long name is whether the user given (guest)vmlinux name or
the name given by machine__mmap_name(). The machine__mmap_name() returns
[kernel.kallsyms], [guest.kernel.kallsyms] or [guest.kernel.kallsyms.<PID>].

OK, so the question is, should we do same thing on kallsyms as vdso?
This patch actually tried it, all the "[*kernel.kallsyms*]" dso caches
stored under "[kernel.kallsyms]".

Thank you,


> >  
> > > Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > > ---
> > >  tools/perf/util/build-id.c |   12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> > > index b6ecf87..234d8a1 100644
> > > --- a/tools/perf/util/build-id.c
> > > +++ b/tools/perf/util/build-id.c
> > > @@ -343,21 +343,25 @@ void disable_buildid_cache(void)
> > >  static char *build_id_cache__dirname_from_path(const char *name,
> > >  					       bool is_kallsyms, bool is_vdso)
> > >  {
> > > -	char *realname = (char *)name, *filename;
> > > +	const char *realname = name;
> > > +	char *filename;
> > >  	bool slash = is_kallsyms || is_vdso;
> > >  
> > >  	if (!slash) {
> > >  		realname = realpath(name, NULL);
> > >  		if (!realname)
> > >  			return NULL;
> > > -	}
> > > +	} else if (is_vdso)
> > > +		realname = DSO__NAME_VDSO;
> > > +	else
> > > +		realname = "[kernel.kallsyms]";
> > >  
> > >  	if (asprintf(&filename, "%s%s%s", buildid_dir, slash ? "/" : "",
> > > -		     is_vdso ? DSO__NAME_VDSO : realname) < 0)
> > > +		     realname) < 0)
> > >  		filename = NULL;
> > >  
> > >  	if (!slash)
> > > -		free(realname);
> > > +		free((char *)realname);
> > >  
> > >  	return filename;
> > >  }
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@xxxxxxxxxx>


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>