Re: [PATCH] perf dso: Fix dso comparison

[Arnaldo Carvalho de Melo]

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.

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

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);
 }