Re: [PATCH perf/core v2 4/5] perf symbols: Allow symbol alias when loading map for symbol name

[Arnaldo Carvalho de Melo]

Em Fri, Mar 06, 2015 at 03:06:04PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Mar 06, 2015 at 04:31:27PM +0900, Masami Hiramatsu escreveu:
> > From: Namhyung Kim <namhyung@xxxxxxxxxx>
> > 
> > When perf probe tries to add a probe in a binary using symbol name, it
> > sometimes failed since some symbols were discard during loading dso.
> > When it resolves an address to symbol, it'd be better to have just one
> > symbol at given address.  But for finding address from symbol, it'd be
> > better to keep all names (including aliases).
> > 
> > Add and propagate a new allow_alias argument to dso (and map) load
> > functions so that it can keep those duplicate symbol aliases.
> 
> Isn't this a global knob, i.e. one that a tool, such as 'perf probe'
> flips at startup and leave it turned on while other tools don't need to
> care about it?
> 
> If so, we should use symbol_conf for that, no? I.e.
> symbol_conf.allow_aliases?
> 
> Looking at doing that now...

Ugh, I was looking at when was that knob to flipped on in this patch, it
is when one calls map__find_symbol_by_name(), when it assumes that the
map is not loaded yet and thus asks for duplicates not to be eliminated.

But what happens if the map was already loaded by, say,
map__find_symbol(), i.e. to find by address? Then that 'true' in the
__map__load() call from map__find_symbol_by_name will be a nop.

So I think it is really better for this to be a
symbol_conf.allow_aliases that tools set at startup, and perhaps even
this could even be governed by symbol_conf.sort_by_name, but I'll leave
alow_aliases for now, its more flexible.

- Arnaldo
 
> - Arnaldo
>  
> > Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> >  tools/perf/util/machine.c        |    2 +-
> >  tools/perf/util/map.c            |    6 +++---
> >  tools/perf/util/map.h            |    8 +++++++-
> >  tools/perf/util/symbol-elf.c     |    5 +++--
> >  tools/perf/util/symbol-minimal.c |    2 +-
> >  tools/perf/util/symbol.c         |    8 +++++---
> >  tools/perf/util/symbol.h         |    5 +++--
> >  7 files changed, 23 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 24f8c97..01ba9b6 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1128,7 +1128,7 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
> >  			 * preload dso of guest kernel and modules
> >  			 */
> >  			dso__load(kernel, machine->vmlinux_maps[MAP__FUNCTION],
> > -				  NULL);
> > +				  NULL, false);
> >  		}
> >  	}
> >  	return 0;
> > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> > index 62ca9f2..711e072 100644
> > --- a/tools/perf/util/map.c
> > +++ b/tools/perf/util/map.c
> > @@ -248,7 +248,7 @@ void map__fixup_end(struct map *map)
> >  
> >  #define DSO__DELETED "(deleted)"
> >  
> > -int map__load(struct map *map, symbol_filter_t filter)
> > +int __map__load(struct map *map, symbol_filter_t filter, bool allow_alias)
> >  {
> >  	const char *name = map->dso->long_name;
> >  	int nr;
> > @@ -256,7 +256,7 @@ int map__load(struct map *map, symbol_filter_t filter)
> >  	if (dso__loaded(map->dso, map->type))
> >  		return 0;
> >  
> > -	nr = dso__load(map->dso, map, filter);
> > +	nr = dso__load(map->dso, map, filter, allow_alias);
> >  	if (nr < 0) {
> >  		if (map->dso->has_build_id) {
> >  			char sbuild_id[BUILD_ID_SIZE * 2 + 1];
> > @@ -304,7 +304,7 @@ struct symbol *map__find_symbol(struct map *map, u64 addr,
> >  struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
> >  					symbol_filter_t filter)
> >  {
> > -	if (map__load(map, filter) < 0)
> > +	if (__map__load(map, filter, true) < 0)
> >  		return NULL;
> >  
> >  	if (!dso__sorted_by_name(map->dso, map->type))
> > diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> > index 0e42438..ba15607 100644
> > --- a/tools/perf/util/map.h
> > +++ b/tools/perf/util/map.h
> > @@ -149,7 +149,13 @@ size_t map__fprintf_dsoname(struct map *map, FILE *fp);
> >  int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
> >  			 FILE *fp);
> >  
> > -int map__load(struct map *map, symbol_filter_t filter);
> > +int __map__load(struct map *map, symbol_filter_t filter, bool allow_alias);
> > +
> > +static inline int map__load(struct map *map, symbol_filter_t filter)
> > +{
> > +	return __map__load(map, filter, false);
> > +}
> > +
> >  struct symbol *map__find_symbol(struct map *map,
> >  				u64 addr, symbol_filter_t filter);
> >  struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > index ada1676..fb630f8 100644
> > --- a/tools/perf/util/symbol-elf.c
> > +++ b/tools/perf/util/symbol-elf.c
> > @@ -754,7 +754,7 @@ static bool want_demangle(bool is_kernel_sym)
> >  
> >  int dso__load_sym(struct dso *dso, struct map *map,
> >  		  struct symsrc *syms_ss, struct symsrc *runtime_ss,
> > -		  symbol_filter_t filter, int kmodule)
> > +		  symbol_filter_t filter, int kmodule, bool allow_alias)
> >  {
> >  	struct kmap *kmap = dso->kernel ? map__kmap(map) : NULL;
> >  	struct map *curr_map = map;
> > @@ -1048,7 +1048,8 @@ new_symbol:
> >  	 * For misannotated, zeroed, ASM function sizes.
> >  	 */
> >  	if (nr > 0) {
> > -		symbols__fixup_duplicate(&dso->symbols[map->type]);
> > +		if (!allow_alias)
> > +			symbols__fixup_duplicate(&dso->symbols[map->type]);
> >  		symbols__fixup_end(&dso->symbols[map->type]);
> >  		if (kmap) {
> >  			/*
> > diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
> > index d7efb03..fefeeb3 100644
> > --- a/tools/perf/util/symbol-minimal.c
> > +++ b/tools/perf/util/symbol-minimal.c
> > @@ -334,7 +334,7 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
> >  		  struct symsrc *ss,
> >  		  struct symsrc *runtime_ss __maybe_unused,
> >  		  symbol_filter_t filter __maybe_unused,
> > -		  int kmodule __maybe_unused)
> > +		  int kmodule __maybe_unused, bool allow_alias __maybe_unused)
> >  {
> >  	unsigned char *build_id[BUILD_ID_SIZE];
> >  	int ret;
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index a690668..de72a16 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1345,7 +1345,8 @@ static bool dso__is_compatible_symtab_type(struct dso *dso, bool kmod,
> >  	}
> >  }
> >  
> > -int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter)
> > +int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter,
> > +	      bool allow_alias)
> >  {
> >  	char *name;
> >  	int ret = -1;
> > @@ -1458,7 +1459,8 @@ int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter)
> >  		runtime_ss = syms_ss;
> >  
> >  	if (syms_ss)
> > -		ret = dso__load_sym(dso, map, syms_ss, runtime_ss, filter, kmod);
> > +		ret = dso__load_sym(dso, map, syms_ss, runtime_ss, filter, kmod,
> > +				    allow_alias);
> >  	else
> >  		ret = -1;
> >  
> > @@ -1516,7 +1518,7 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
> >  	if (symsrc__init(&ss, dso, symfs_vmlinux, symtab_type))
> >  		return -1;
> >  
> > -	err = dso__load_sym(dso, map, &ss, &ss, filter, 0);
> > +	err = dso__load_sym(dso, map, &ss, &ss, filter, 0, false);
> >  	symsrc__destroy(&ss);
> >  
> >  	if (err > 0) {
> > diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> > index 1650dcb..8299038 100644
> > --- a/tools/perf/util/symbol.h
> > +++ b/tools/perf/util/symbol.h
> > @@ -218,7 +218,8 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
> >  bool symsrc__has_symtab(struct symsrc *ss);
> >  bool symsrc__possibly_runtime(struct symsrc *ss);
> >  
> > -int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter);
> > +int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter,
> > +	      bool allow_alias);
> >  int dso__load_vmlinux(struct dso *dso, struct map *map,
> >  		      const char *vmlinux, bool vmlinux_allocated,
> >  		      symbol_filter_t filter);
> > @@ -262,7 +263,7 @@ bool symbol__is_idle(struct symbol *sym);
> >  
> >  int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> >  		  struct symsrc *runtime_ss, symbol_filter_t filter,
> > -		  int kmodule);
> > +		  int kmodule, bool allow_alias);
> >  int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss,
> >  				struct map *map, symbol_filter_t filter);
> >  
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/