Re: [PATCHv2 5/8] perf probe powerpc: Allow matching against dot symbols

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Mon, Dec 15, 2014 at 08:20:35PM +0530, Naveen N. Rao escreveu:
> Allow perf probe to work on powerpc ABIv1 without the need to specify
> the leading dot '.' for functions. 'perf probe do_fork' works with this
> patch.
> 
> Introduce HAVE_ARCH_SYMBOL_HANDLING to indicate need for special
> handling of symbols. In this patch, we override probe_function_filter()
> on powerpc to account for dot symbols.

This one looks better, does arch specific stuff in tools/perf/arch,
good, some nits below.
 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> Changes from the previous patchset:
> Introduced arch helper to override the way probe function filter works.
> 
>  tools/perf/arch/powerpc/Makefile            |  1 +
>  tools/perf/arch/powerpc/util/sym-handling.c | 28 ++++++++++++++++++++++++++++
>  tools/perf/config/Makefile                  |  1 +
>  tools/perf/util/probe-event.c               | 10 +++++-----
>  tools/perf/util/probe-event.h               |  5 +++++
>  5 files changed, 40 insertions(+), 5 deletions(-)
>  create mode 100644 tools/perf/arch/powerpc/util/sym-handling.c
> 
> diff --git a/tools/perf/arch/powerpc/Makefile b/tools/perf/arch/powerpc/Makefile
> index 6f7782b..1c3d435 100644
> --- a/tools/perf/arch/powerpc/Makefile
> +++ b/tools/perf/arch/powerpc/Makefile
> @@ -3,4 +3,5 @@ PERF_HAVE_DWARF_REGS := 1
>  LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/dwarf-regs.o
>  LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/skip-callchain-idx.o
>  endif
> +LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/sym-handling.o
>  LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/header.o
> diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
> new file mode 100644
> index 0000000..0a77825
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/sym-handling.c
> @@ -0,0 +1,28 @@
> +/*
> + * Special symbol handling for PowerPC:
> + * - Handle dot symbols on ABIv1
> + *
> + * Copyright (C) 2014 Naveen N Rao, IBM Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include "map.h"
> +#include "symbol.h"
> +#include "probe-event.h"
> +
> +int probe_function_filter(struct map *map __maybe_unused, struct symbol *sym)
> +{
> +	if (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) {
> +		if ((strcmp(looking_function_name, sym->name) == 0) ||
> +		    (sym->name[0] == '.' && looking_function_name[0] != '.' &&
> +		     strcmp(looking_function_name, sym->name+1) == 0)) {
> +			num_matched_functions++;
> +			return 0;
> +		}
> +	}
> +	return 1;
> +}
> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
> index 5d4b039..35cf934 100644
> --- a/tools/perf/config/Makefile
> +++ b/tools/perf/config/Makefile
> @@ -383,6 +383,7 @@ ifeq ($(ARCH),powerpc)
>    ifndef NO_DWARF
>      CFLAGS += -DHAVE_SKIP_CALLCHAIN_IDX
>    endif
> +  CFLAGS += -DHAVE_ARCH_SYMBOL_HANDLING


Dunno about this naming, looks too general: SYMBOL_HANDLING, but can't
come to some better one now, anyone?

>  endif
>  
>  ifndef NO_LIBUNWIND
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 74b7fef..7eb9b27 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -50,6 +50,8 @@
>  #define PERFPROBE_GROUP "probe"
>  
>  bool probe_event_dry_run;	/* Dry run flag */
> +char *looking_function_name;
> +int num_matched_functions;
>  
>  #define semantic_error(msg ...) pr_err("Semantic error :" msg)
>  
> @@ -2210,11 +2212,8 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>  	return ret;
>  }
>  
> -static char *looking_function_name;
> -static int num_matched_functions;
> -
> -static int probe_function_filter(struct map *map __maybe_unused,
> -				      struct symbol *sym)
> +#ifndef HAVE_ARCH_SYMBOL_HANDLING
> +int probe_function_filter(struct map *map __maybe_unused, struct symbol *sym)
>  {
>  	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
>  	    strcmp(looking_function_name, sym->name) == 0) {
> @@ -2223,6 +2222,7 @@ static int probe_function_filter(struct map *map __maybe_unused,
>  	}
>  	return 1;
>  }
> +#endif /* HAVE_ARCH_SYMBOL_HANDLING */

Can't we do something like providing a weak function and let the linked
to its work? I guess we have cases like this in tools/ already. I.e. not
using the ifndef block. Minor nit tho.

>  
>  #define strdup_or_goto(str, label)	\
>  	({ char *__p = strdup(str); if (!__p) goto label; __p; })
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index e01e994..8564451 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -7,6 +7,8 @@
>  #include "strfilter.h"
>  
>  extern bool probe_event_dry_run;
> +extern char *looking_function_name;
> +extern int num_matched_functions;

>  
>  /* kprobe-tracer and uprobe-tracer tracing point */
>  struct probe_trace_point {
> @@ -136,6 +138,9 @@ extern int show_available_vars(struct perf_probe_event *pevs, int npevs,
>  extern int show_available_funcs(const char *module, struct strfilter *filter,
>  				bool user);
>  
> +extern int probe_function_filter(struct map *map __maybe_unused,
> +					struct symbol *sym);
> +

Please do not prefix function declarations with 'extern', even when we
have one just before, its not needed, patches removing the existing ones
would be accepted.

>  /* Maximum index number of event-name postfix */
>  #define MAX_EVENT_INDEX	1024
>  
> -- 
> 2.1.3