Re: [PATCH v6 6/6] perf util: use correct IP mapping to find srcline for hist entry

From: Namhyung Kim
Date: Mon Oct 30 2017 - 19:35:32 EST


Hi Arnaldo,

On Mon, Oct 30, 2017 at 05:03:47PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Oct 25, 2017 at 10:46:00AM +0900, Namhyung Kim escreveu:
> > Hi Milian,
> >
> > On Tue, Oct 24, 2017 at 10:51:43AM +0200, Milian Wolff wrote:
> > > On Freitag, 20. Oktober 2017 07:15:33 CEST Namhyung Kim wrote:
> > > > I looked into it and found a bug handling cumulative (children)
> > > > entries. For chilren entries that has no self period, the al->addr
> > > > (so he->ip) ends up having an doubly-mapped address.
> > > >
> > > > It seems to be there from the beginning but only affects entries that
> > > > have no srclines - finding srcline itself is done using a different
> > > > address but it will show the invalid address if no srcline was found.
> > > > I think we should fix the commit c7405d85d7a3 ("perf tools: Update
> > > > cpumode for each cumulative entry").
> > > >
> > > > Could you please test the following patch works for you?
> > >
> > > Sorry for the delay, nearly forgot about this mail. The patch below does help
> > > in my situation, thanks! Can you commit it please?
> >
> > Sure, I'll add your Tested-by then.
>
> Namhyung, I couldn't find a submission from you for this one, so I
> tentatively added this to my perf/core branch, please let me know if you
> want to reword this somehow.

I already sent it:

https://lkml.org/lkml/2017/10/24/1130

But I'm also ok with yours (with small changes below).


>
> commit 0485954310bf3490cb73936164ca03a0c5916773
> Author: Namhyung Kim <namhyung@xxxxxxxxxx>
> Date: Fri Oct 20 14:15:33 2017 +0900
>
> perf callchain: Fix double mapping al->addr for children without self period
>
> Milian Wolff found a problem he described in [1] and that for him would

Where's the link for [1]?

> get fixed:
>
> "Note how most of the large offset values are now gone. Most notably, we
> get proper srcline resolution for the random.h and complex headers."
>
> Then Namhyung found the root cause:
>
> "I looked into it and found a bug handling cumulative (children)
> entries. For children entries that has no self period, the al->addr (so

s/has/have/

Thanks,
Namhyung


> he->ip) ends up having an doubly-mapped address.
>
> It seems to be there from the beginning but only affects entries that
> have no srclines - finding srcline itself is done using a different
> address but it will show the invalid address if no srcline was found. I
> think we should fix the commit c7405d85d7a3 ("perf tools: Update cpumode
> for each cumulative entry")."
>
> Reported-by: Milian Wolff <milian.wolff@xxxxxxxx>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> Tested-by: Milian Wolff <milian.wolff@xxxxxxxx>
> Cc: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: kernel-team@xxxxxxx
> Fixes: c7405d85d7a3 ("perf tools: Update cpumode for each cumulative entry")
> Link: http://lkml.kernel.org/r/20171020051533.GA2746@sejong
> Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 3a3916934a92..837012147c7b 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -1091,10 +1091,7 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
> al->map = node->map;
> al->sym = node->sym;
> al->srcline = node->srcline;
> - if (node->map)
> - al->addr = node->map->map_ip(node->map, node->ip);
> - else
> - al->addr = node->ip;
> + al->addr = node->ip;
>
> if (al->sym == NULL) {
> if (hide_unresolved)