Date: Thu, 26 Feb 2015 12:17:51 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: kan.liang@intel.com
Cc: jolsa@redhat.com, namhyung@kernel.org, linux-kernel@vger.kernel.org,
        ak@linux.intel.com
Subject: Re: [PATCH V7 1/1] perf tool:perf diff support for different binaries
Message-ID: <20150226151751.GD13373@kernel.org>
References: <1423460384-11645-1-git-send-email-kan.liang@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1423460384-11645-1-git-send-email-kan.liang@intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3079
Lines: 63

Em Mon, Feb 09, 2015 at 05:39:44AM +0000, kan.liang@intel.com escreveu:
> From: Kan Liang <kan.liang@intel.com>
> Currently, the perf diff only works with same binaries. That's because
> it compares the symbol start address. It doesn't work if the perf.data
> comes from different binaries. This patch matches the symbol names.
 
> Actually, perf diff once intended to compare the symbol names.
> The commit as below can look for a pair by name.
> 604c5c92972d (perf diff: Change the default sort order to "dso,symbol")
> However, at that time, perf diff used a global list of dsos. That means
> the binaries which has same name can only be loaded once. That's a
> problem for different binaries compare. For example, we have an old
> binary and an updated binary. They very likely have same name and most
> of the function, so only dsos from old binary will be loaded. When
> processing the data from updated binary, perf still use the symbol
> information from old binary. That's wrong.
 
> Then the commit as below used IP to replace symbol name.
> 9c443dfdd31e ("perf diff: Fix support for all --sort combinations")
> >From that time, perf diff starts to compare the symbol address.
 
> The global dsos is discarded from a patch in 2010.
> a1645ce12adb ("perf: 'perf kvm' tool for monitoring guest performance
> from host")
> However, at that time, perf diff already compared by address. So perf
> diff cannot work for different binaries as well.
 
> This patch actually rolls back the perf diff to original design. The
> document is also changed, so everybody knows the original design is to
> compare the symbol names.
 
> Here is an examples.
> The only difference between example_v1.c and example_v2.c is the
> location of f2 and f3. There is no change in behavior, but the previous
> perf diff display the wrong differential profile.

Thanks for being persistent and addressing the comments.

Having a nice explanation of the problem helps, as my first reaction to
this patch was: "What? This is what this tool is supposed to do, to
compare two versions of a binary, one that is being developed from the
same source, the other with slight modifications, etc", while the
description of the patch made it look as this was a feature that was now
being introduced.

The problem was that over time new features made it stop working for the
initial purpose of the tool, gack!

It would be _really_ nice if you (or someone else :-) ) could get this
patch description and made it a script that would get the two versions
of the source code, build it, then called perf diff and checked that the
output was the expected one, then added it as an entry to 'perf test',
so that we would catch a problem like this quickly if we ever
reintroduce this problem or something else breaks 'perf diff' :-)

Applying the patch, thanks.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/