[PATCH] perf symbols: Cannot disassemble some routines when debuginfo present

[PATCH] perf symbols: Cannot disassemble some routines when debuginfo present

[Eric Saint-Etienne]

When the kernel is compiled with -ffunction-sections and perf uses the
kernel debuginfo, perf fails the very first symbol lookup and ends up with
an hex offset inside [kernel.vmlinux]. It's due to how perf loads the maps.

Indeed only .text gets loaded by map_groups__find() into al->map.
Consequently al->map address range encompass the whole code.
But map__load() has just loaded many function maps by splitting al->map,
which reduced al->map range drastically. Very likely the target address is
now in one of those newly created function maps, so we need to lookup the
map again to find that new map.

This issue is not specific to the kernel but to how the image is linked.
For the kernel, when we're not using the kernel debuginfo, perf will
fallback to using kallsyms and then the first lookup will work.

This patch makes sure that the event address we're looking-up is indeed
within the map we've found, otherwise we lookup another map again.
Only one extra lookup at most is required for the proper map to be found,
if it exists.

Signed-off-by: Eric Saint-Etienne <eric.saint.etienne@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
---
 tools/perf/util/event.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index e9c108a..a69ef52 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -1571,7 +1571,28 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
 		 */
 		if (load_map)
 			map__load(al->map);
-		al->addr = al->map->map_ip(al->map, al->addr);
+
+		/*
+		 * When using -ffunction-sections, only .text gets loaded by
+		 * map_groups__find() into al->map. Consequently al->map address
+		 * range encompass the whole code.
+		 *
+		 * But map__load() has just loaded many function maps by
+		 * splitting al->map, which reduced al->map range drastically.
+		 * Very likely the target address is now in one of those newly
+		 * created function maps, so we need to lookup the map again
+		 * to find that new map.
+		 */
+		if (al->addr < al->map->start || al->addr >= al->map->end)
+			al->map = map_groups__find(mg, al->addr);
+
+		/*
+		 * The new map *ought* to exist because the initial al->map
+		 * contained that address and subsequently has been split into
+		 * many *contiguous* maps.
+		 */
+		if (al->map != NULL)
+			al->addr = al->map->map_ip(al->map, al->addr);
 	}
 
 	return al->map;
-- 
1.8.3.1

Re: [PATCH] perf symbols: Cannot disassemble some routines when debuginfo present

[Jiri Olsa]

On Fri, Nov 23, 2018 at 02:25:26AM -0800, Eric Saint-Etienne wrote:
> When the kernel is compiled with -ffunction-sections and perf uses the
> kernel debuginfo, perf fails the very first symbol lookup and ends up with
> an hex offset inside [kernel.vmlinux]. It's due to how perf loads the maps.
> 
> Indeed only .text gets loaded by map_groups__find() into al->map.
> Consequently al->map address range encompass the whole code.
> But map__load() has just loaded many function maps by splitting al->map,
> which reduced al->map range drastically. Very likely the target address is
> now in one of those newly created function maps, so we need to lookup the
> map again to find that new map.
> 
> This issue is not specific to the kernel but to how the image is linked.
> For the kernel, when we're not using the kernel debuginfo, perf will
> fallback to using kallsyms and then the first lookup will work.
> 
> This patch makes sure that the event address we're looking-up is indeed
> within the map we've found, otherwise we lookup another map again.
> Only one extra lookup at most is required for the proper map to be found,
> if it exists.
> 
> Signed-off-by: Eric Saint-Etienne <eric.saint.etienne@oracle.com>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
>  tools/perf/util/event.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index e9c108a..a69ef52 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -1571,7 +1571,28 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>  		 */
>  		if (load_map)
>  			map__load(al->map);
> -		al->addr = al->map->map_ip(al->map, al->addr);
> +
> +		/*
> +		 * When using -ffunction-sections, only .text gets loaded by
> +		 * map_groups__find() into al->map. Consequently al->map address
> +		 * range encompass the whole code.
> +		 *
> +		 * But map__load() has just loaded many function maps by
> +		 * splitting al->map, which reduced al->map range drastically.
> +		 * Very likely the target address is now in one of those newly
> +		 * created function maps, so we need to lookup the map again
> +		 * to find that new map.
> +		 */

hum, so map__load actualy can split the map to create new maps?

cold you please point me to that code? I haven't touch
this area for some time and I can't find it

thanks,
jirka

> +		if (al->addr < al->map->start || al->addr >= al->map->end)
> +			al->map = map_groups__find(mg, al->addr);
> +
> +		/*
> +		 * The new map *ought* to exist because the initial al->map
> +		 * contained that address and subsequently has been split into
> +		 * many *contiguous* maps.
> +		 */
> +		if (al->map != NULL)
> +			al->addr = al->map->map_ip(al->map, al->addr);
>  	}
>  
>  	return al->map;
> -- 
> 1.8.3.1
> 

RE: [PATCH] perf symbols: Cannot disassemble some routines when debuginfo present

[Eric Saint Etienne]

> > +		/*
> > +		 * When using -ffunction-sections, only .text gets loaded by
> > +		 * map_groups__find() into al->map. Consequently al->map address
> > +		 * range encompass the whole code.
> > +		 *
> > +		 * But map__load() has just loaded many function maps by
> > +		 * splitting al->map, which reduced al->map range drastically.
> > +		 * Very likely the target address is now in one of those newly
> > +		 * created function maps, so we need to lookup the map again
> > +		 * to find that new map.
> > +		 */
> 
> hum, so map__load actualy can split the map to create new maps?
> 
> cold you please point me to that code? I haven't touch this area for some
> time and I can't find it

The split happens in dso_process_kernel_symbol() in symbol-elf.c where we
call map_groups__find_by_name() to find an existing map, but with
-ffunction-sections and a symbol belonging to new (function) map, such map
doesn't exist yet so we end up creating one and adjusting existing maps
accordingly because adjust_kernel_syms is set. Makes sense?

As of 4.20-rc3 the call chain is as follows:
event:c:1573	   map__load()
map.c:315	   dso__load()
symobl.c:1528	   dso__load_kernel_sym()
symbol.c:1896	   dso__load_vmlinux_path()
		   (or we directly call dso__load_vmlinux() at line 1892)
symbol.c:1744	   dso__load_vmlinux()
symbol.c:1719	   dso__load_sym()
symbol-elf.c:1090  dso_process_kernel_symbol()

-eric

Re: [PATCH] perf symbols: Cannot disassemble some routines when debuginfo present

[Jiri Olsa]

On Fri, Nov 23, 2018 at 10:24:21AM -0800, Eric Saint Etienne wrote:
> > > +		/*
> > > +		 * When using -ffunction-sections, only .text gets loaded by
> > > +		 * map_groups__find() into al->map. Consequently al->map address
> > > +		 * range encompass the whole code.
> > > +		 *
> > > +		 * But map__load() has just loaded many function maps by
> > > +		 * splitting al->map, which reduced al->map range drastically.
> > > +		 * Very likely the target address is now in one of those newly
> > > +		 * created function maps, so we need to lookup the map again
> > > +		 * to find that new map.
> > > +		 */
> > 
> > hum, so map__load actualy can split the map to create new maps?
> > 
> > cold you please point me to that code? I haven't touch this area for some
> > time and I can't find it
> 
> The split happens in dso_process_kernel_symbol() in symbol-elf.c where we
> call map_groups__find_by_name() to find an existing map, but with
> -ffunction-sections and a symbol belonging to new (function) map, such map
> doesn't exist yet so we end up creating one and adjusting existing maps
> accordingly because adjust_kernel_syms is set. Makes sense?
> 
> As of 4.20-rc3 the call chain is as follows:
> event:c:1573	   map__load()
> map.c:315	   dso__load()
> symobl.c:1528	   dso__load_kernel_sym()
> symbol.c:1896	   dso__load_vmlinux_path()
> 		   (or we directly call dso__load_vmlinux() at line 1892)
> symbol.c:1744	   dso__load_vmlinux()
> symbol.c:1719	   dso__load_sym()
> symbol-elf.c:1090  dso_process_kernel_symbol()

i see, thank for the pointers.. could you please mention
this in your comment and changelog as well?

also to document in the code that it's related to map__load
could you put it to the if contidion, like:

+               if (load_map && (al->addr < al->map->start || al->addr >= al->map->end))


thanks,
jirka

Re: [PATCH] perf symbols: Cannot disassemble some routines when debuginfo present

[Arnaldo Carvalho de Melo]

Em Mon, Nov 26, 2018 at 09:39:17AM +0100, Jiri Olsa escreveu:
> On Fri, Nov 23, 2018 at 10:24:21AM -0800, Eric Saint Etienne wrote:
> > > > +		/*
> > > > +		 * When using -ffunction-sections, only .text gets loaded by
> > > > +		 * map_groups__find() into al->map. Consequently al->map address
> > > > +		 * range encompass the whole code.
> > > > +		 *
> > > > +		 * But map__load() has just loaded many function maps by
> > > > +		 * splitting al->map, which reduced al->map range drastically.
> > > > +		 * Very likely the target address is now in one of those newly
> > > > +		 * created function maps, so we need to lookup the map again
> > > > +		 * to find that new map.
> > > > +		 */
> > > 
> > > hum, so map__load actualy can split the map to create new maps?
> > > 
> > > cold you please point me to that code? I haven't touch this area for some
> > > time and I can't find it
> > 
> > The split happens in dso_process_kernel_symbol() in symbol-elf.c where we
> > call map_groups__find_by_name() to find an existing map, but with
> > -ffunction-sections and a symbol belonging to new (function) map, such map
> > doesn't exist yet so we end up creating one and adjusting existing maps
> > accordingly because adjust_kernel_syms is set. Makes sense?
> > 
> > As of 4.20-rc3 the call chain is as follows:
> > event:c:1573	   map__load()
> > map.c:315	   dso__load()
> > symobl.c:1528	   dso__load_kernel_sym()
> > symbol.c:1896	   dso__load_vmlinux_path()
> > 		   (or we directly call dso__load_vmlinux() at line 1892)
> > symbol.c:1744	   dso__load_vmlinux()
> > symbol.c:1719	   dso__load_sym()
> > symbol-elf.c:1090  dso_process_kernel_symbol()
> 
> i see, thank for the pointers.. could you please mention
> this in your comment and changelog as well?
> 
> also to document in the code that it's related to map__load
> could you put it to the if contidion, like:
> 
> +               if (load_map && (al->addr < al->map->start || al->addr >= al->map->end))
> 

Eric, please address Jiri's comments and send a v2, ok?

- Arnaldo