Re: [PATCH 2/3] perf symbols: Try reading the symbol table with libbfd

[Jiri Olsa <jolsa@redhat.com>]

On Wed, Jul 08, 2020 at 11:44:24AM +0200, Rémi Bernon wrote:
> On 2020-06-24 15:40, Rémi Bernon wrote:
> > On 2020-06-24 15:25, Jiri Olsa wrote:
> > > On Wed, Jun 24, 2020 at 12:30:40PM +0200, Remi Bernon wrote:
> > > 
> > > SNIP
> > > 
> > > > +
> > > > +        symbols__insert(&dso->symbols, symbol);
> > > > +    }
> > > > +#ifdef bfd_get_section
> > > > +#undef bfd_asymbol_section
> > > > +#endif
> > > > +
> > > > +    symbols__fixup_end(&dso->symbols);
> > > > +    symbols__fixup_duplicate(&dso->symbols);
> > > > +    dso->adjust_symbols = 1;
> > > > +
> > > > +    err = 0;
> > > > +out_free:
> > > > +    free(symbols);
> > > > +out_close:
> > > > +    bfd_close(abfd);
> > > > +    return err;
> > > > +}
> > > > +#endif
> > > > +
> > > >   static bool dso__is_compatible_symtab_type(struct dso *dso, bool kmod,
> > > >                          enum dso_binary_type type)
> > > >   {
> > > > @@ -1695,6 +1816,7 @@ int dso__load(struct dso *dso, struct map *map)
> > > >           bool next_slot = false;
> > > >           bool is_reg;
> > > >           bool nsexit;
> > > > +        int bfdrc = -1;
> > > >           int sirc = -1;
> > > >           enum dso_binary_type symtab_type = binary_type_symtab[i];
> > > > @@ -1713,12 +1835,19 @@ int dso__load(struct dso *dso, struct map *map)
> > > >               nsinfo__mountns_exit(&nsc);
> > > >           is_reg = is_regular_file(name);
> > > > +#ifdef HAVE_LIBBFD_SUPPORT
> > > >           if (is_reg)
> > > > +            bfdrc = dso__load_bfd_symbols(dso, name);
> > > > +#endif
> > > 
> > > so the comment says that we try bfd when the libelf fails,
> > > but the code seems to do the oposite.. starts with bfd
> > > and skip libelf if it's ok
> > > 
> > > jirka
> > > 
> > 
> > Ah, sorry I forgot to reword the commit comment to match the changes,
> > thanks for noticing. It should probably be something like that:
> > 
> >    Trying to read symbols on non-ELF binaries with libbfd, when
> >    supported, makes it possible for perf to report symbols and
> >    annotations for Windows applications running under Wine.
> > 
> > As you suggested, the libbfd code now tries to detect the binary format,
> >  and early rejects if ELF is detected, so it then falls back to the
> > libelf code path.
> 
> Hi, it doesn't seem there's more comments, should I just resend the series
> with the updated commit message?

yes, please fix the changelog..  patches look good to me, but
I'd like somebody else with better bfd knowledge to review them ;-)

jirka