Messages in this thread | | | From | Namhyung Kim <> | Subject | Re: [PATCH 1/3] perf tools: Allow vmlinux to fallback to kallsyms on NO_LIBELF=1 | Date | Mon, 17 Nov 2014 10:55:22 +0900 |
| |
Hi Arnaldo,
Sorry for late reply. I was offline last week.
On Tue, 11 Nov 2014 10:02:29 -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Nov 11, 2014 at 01:24:38PM +0900, Namhyung Kim escreveu: >> Hi Peter, >> >> On Mon, Nov 10, 2014 at 9:11 PM, Peter Zijlstra <peterz@infradead.org> wrote: >> > On Mon, Nov 10, 2014 at 03:33:11PM +0900, Namhyung Kim wrote: >> >> Hmm.. I don't think it's specific to the minimal elf parser. The return >> >> value of dso__load_sym() is a number of symbols found so when it sees a >> >> dso with 0 symbols it'll fall back to the next option IMHO (not >> >> tested). Did you see a problem with the current code? >> > >> > So your patch: >> > >> > +++ b/tools/perf/util/symbol-minimal.c >> > @@ -335,6 +335,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused, >> > unsigned char *build_id[BUILD_ID_SIZE]; >> > int ret; >> > >> > + if (dso->kernel) >> > + return 0; /* always use kallsyms */ >> > + >> > >> > changes the symbol-minimal.c file to add this exception. That is very >> > much specific to the minimal elf parser, or am I just seeing things? >> >> What minimal parser does here is just skip kernel dsos (vmlinux) since >> it didn't deal with all the details of parsing vmlinux currently. >> >> >> > >> > What I was saying, why not have a util/symbol.c change that disregards >> > all DSOs with 0 symbols in. >> >> The util/symbol.c doesn't need this because it can handle vmlinux >> reliably. So after reading symbol table, it'll use the dso if it > > symbol.c should not be able to handle anything, since the actual ELF > loader is in symbol-elf.c or simbol-minimal.c, no?
Ah right, I was confused with symbol.c and symbol-elf.c - so I believe the generic logic in symbol.c already handles DSOs with 0 symbol. The problem was symbol-minimal.c didn't return 0 for the DSOs.
> > I.e.: > > symbol.c::dso__load() > symbol.c::dso__load_kernel_sym() > symbol.c::dso__load_vmlinux() > > And then it heads into one of the ELF loaders, right now, without your > patch, I see, when the minimal loader is used: > > symbol-minimal.c::dso__load_sym() > > reads the build-id and if it works, returns 1, which is a bug, it should > return 0 in this case, possibly -1 if it doesn't read the build-id :-\ > > I.e. it should return 0, because that way it signals: "I can't read it, > thus no symbols were loaded, symbol.c: please go on looking for them > somewhere else, kallsyms perhaps?".
Right. It's a bug for vmlinux. But by returning 0, it'll iterate over the candidate binaries in dso__load(). I'm afraid that this *might* overwrite the build-id of the DSO with different one. So I think it also needs to add a check for valid build-id.
I'll update this patch with above.
Thanks, Namhyung
> > And this is the bug, probably. Now to look at your patches to see if > this is touched somehow... > >> actually contains symbols or fallback to next dso if it has 0 symbols. >> IOW it already disregards all dsos with 0 symbols in. > > See above. > >> -- >> Thanks, >> Namhyung
| |