On Thu, 12 May 2016 11:02:12 +0900
Masami Hiramatsu <mhira...@kernel.org> wrote:
> On Wed, 11 May 2016 12:45:00 -0300
> Arnaldo Carvalho de Melo <a...@kernel.org> 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 <mhira...@kernel.org>
> > > ---
> > > 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 <mhira...@kernel.org>
--
Masami Hiramatsu <mhira...@kernel.org>
