Re: [PATCH] perf dso: Fix dso comparison

[Jiri Olsa]

On Tue, Mar 24, 2020 at 10:00:52AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 24, 2020 at 11:48:43AM +0100, Jiri Olsa escreveu:
> > 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?
> 
> Jiri:
> 
> Humm, you mean sort__dso_cmp() -> _sort__dso_cmp() should consider the
> dso_id and not just its name? Humm, when "dso" sort key is used that
> means just the short_name (or long_name, if verbose), if we use the ID
> for "dso" then we need to somehow show the id in the output, otherwise
> we'd have multiple lines with the same DSO name, when multiple versions
> exist... Perhaps we should do a first pass, figure out if there are DSOs
> with the same name/different IDs and mark them for showing the ID to
> differentiate them on the output? But this is something that should be
> dealt with in a separece cset, I think.

true, also Ravi pointed out in the other answer,
it depends on what we want.. compare the name or
different binaries with same name

> 
> With that in mind, can I add your Acked-by for this patch, with my
> changes described below?

yep,

Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>

jirka

> 
> Ravi:
> 
> I'm applying it with the changes below, to keep namespacing consistency, ok?
> 
> - Arnaldo
> 
> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
> index 5c5bfa2538a9..939471731ea6 100644
> --- a/tools/perf/util/dsos.c
> +++ b/tools/perf/util/dsos.c
> @@ -26,7 +26,7 @@ static int __dso_id__cmp(struct dso_id *a, struct dso_id *b)
>  	return 0;
>  }
>  
> -static bool is_empty_dso_id(struct dso_id *id)
> +static bool dso_id__empty(struct dso_id *id)
>  {
>  	if (!id)
>  		return true;
> @@ -34,7 +34,7 @@ static bool is_empty_dso_id(struct dso_id *id)
>  	return !id->maj && !id->min && !id->ino && !id->ino_generation;
>  }
>  
> -static void inject_dso_id(struct dso *dso, struct dso_id *id)
> +static void dso__inject_id(struct dso *dso, struct dso_id *id)
>  {
>  	dso->id.maj = id->maj;
>  	dso->id.min = id->min;
> @@ -48,7 +48,7 @@ static int dso_id__cmp(struct dso_id *a, struct dso_id *b)
>  	 * The second is always dso->id, so zeroes if not set, assume passing
>  	 * NULL for a means a zeroed id
>  	 */
> -	if (is_empty_dso_id(a) || is_empty_dso_id(b))
> +	if (dso_id__empty(a) || dso_id__empty(b))
>  		return 0;
>  
>  	return __dso_id__cmp(a, b);
> @@ -266,8 +266,8 @@ static struct dso *__dsos__findnew_id(struct dsos *dsos, const char *name, struc
>  {
>  	struct dso *dso = __dsos__find_id(dsos, name, id, false);
>  
> -	if (dso && is_empty_dso_id(&dso->id) && !is_empty_dso_id(id))
> -		inject_dso_id(dso, id);
> +	if (dso && dso_id__empty(&dso->id) && !dso_id__empty(id))
> +		dso__inject_id(dso, id);
>  
>  	return dso ? dso : __dsos__addnew_id(dsos, name, id);
>  }
>