Re: [PATCH] perf dso: Fix dso comparison

[Ravi Bangoria]

On 3/24/20 4:18 PM, Jiri Olsa wrote:
> On Tue, Mar 24, 2020 at 09:54:24AM +0530, Ravi Bangoria wrote:
> > Perf gets dso details from two different sources. 1st, from builid
> > headers in perf.data and 2nd from MMAP2 samples. Dso from buildid
> > header does not have dso_id detail. And dso from MMAP2 samples does
> > not have buildid information. If detail of the same dso is present
> > at both the places, filename is common.
> >
> > Previously, __dsos__findnew_link_by_longname_id() used to compare only
> > long or short names, but Commit 0e3149f86b99 ("perf dso: Move dso_id
> > from 'struct map' to 'struct dso'") also added a dso_id comparison.
> > Because of that, now perf is creating two different dso objects of the
> > same file, one from buildid header (with dso_id but without buildid)
> > and second from MMAP2 sample (with buildid but without dso_id).
> >
> > This is causing issues with archive, buildid-list etc subcommands. Fix
> > this by comparing dso_id only when it's present. And incase dso is
> > present in 'dsos' list without dso_id, inject dso_id detail as well.
> >
> > Before:
> >
> >    $ sudo ./perf buildid-list -H
> >    0000000000000000000000000000000000000000 /usr/bin/ls
> >    0000000000000000000000000000000000000000 /usr/lib64/ld-2.30.so
> >    0000000000000000000000000000000000000000 /usr/lib64/libc-2.30.so
> >
> >    $ ./perf archive
> >    perf archive: no build-ids found
> >
> > After:
> >
> >    $ ./perf buildid-list -H
> >    b6b1291d0cead046ed0fa5734037fa87a579adee /usr/bin/ls
> >    641f0c90cfa15779352f12c0ec3c7a2b2b6f41e8 /usr/lib64/ld-2.30.so
> >    675ace3ca07a0b863df01f461a7b0984c65c8b37 /usr/lib64/libc-2.30.so
> >
> >    $ ./perf archive
> >    Now please run:
> >
> >    $ tar xvf perf.data.tar.bz2 -C ~/.debug
> >
> >    wherever you need to run 'perf report' on.
> >
> > Reported-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>
>
> looks good, do we need to add the dso_id check to sort__dso_cmp?

I guess with different filename there is no need to compare dso_id.
But for same filename, adding dso_id cmp will separate out the
samples:

Ex, Without dso_id compare:

  $ ./perf report -s dso,dso_size -v
    66.63%  /home/ravi/a.out                                  4096
    33.36%  /home/ravi/Workspace/linux/tools/perf/a.out       4096

  $ ./perf report -s dso,dso_size
    99.99%  a.out                 4096


With below diff:

  -       return strcmp(dso_name_l, dso_name_r);
  +       ret = strcmp(dso_name_l, dso_name_r);
  +       if (ret)
  +               return ret;
  +       else
  +               return dso__cmp_id(dso_l, dso_r);


  $ ./perf report -s dso,dso_size
    99.99%  a.out                 4096
    33.36%  a.out                 4096

though, the o/p also depends which other sort keys are used along
with dso key. Do you think this change makes sense?

Ravi