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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Wed, 8 Jul 2020 20:33:51 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Rémi Bernon <rbernon@...eweavers.com>
Cc:     linux-kernel@...r.kernel.org,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Namhyung Kim <namhyung@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Jacek Caban <jacek@...eweavers.com>
Subject: Re: [PATCH 2/3] perf symbols: Try reading the symbol table with
 libbfd

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