Re: [PATCH 1/3] perf tools: Allow vmlinux to fallback to kallsyms on NO_LIBELF=1

[Namhyung Kim <namhyung@kernel.org>]

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