[PATCHv2 0/8] Fix perf probe issues on powerpc

[PATCHv2 0/8] Fix perf probe issues on powerpc

[Naveen N. Rao]

This patchset fixes various issues with perf probe on powerpc across ABIv1 and
ABIv2:
- in the presence of DWARF debug-info,
- in the absence of DWARF, but with the symbol table, and
- in the absence of debug-info, but with kallsyms.

Applies cleanly on -tip. Tested on ppc64 BE and LE.

Changes from previous version:
Addressed various review comments from Mike Ellerman largely to generalize
changes. Some of the simpler patches have been retained in their previous form
to limit code churn, while others have been generalized by introducing arch
helpers. Individual patches have more details.


- Naveen

Naveen N. Rao (8):
  kprobes: Fix kallsyms lookup across powerpc ABIv1 and ABIv2
  perf probe: Improve detection of file/function name in the probe
    pattern
  perf probe powerpc: Fix symbol fixup issues due to ELF type
  perf probe powerpc: Handle powerpc dot symbols
  perf probe powerpc: Allow matching against dot symbols
  perf tools powerpc: Fix PPC64 ELF ABIv2 symbol decoding
  perf probe powerpc: Use DWARF info only if necessary
  perf probe powerpc: Fixup function entry if using kallsyms lookup

 arch/powerpc/include/asm/kprobes.h          | 63 +++++++++++++++++++---------
 tools/perf/arch/powerpc/Makefile            |  1 +
 tools/perf/arch/powerpc/util/sym-handling.c | 64 +++++++++++++++++++++++++++++
 tools/perf/config/Makefile                  |  2 +
 tools/perf/util/probe-event.c               | 48 ++++++++++++++++++----
 tools/perf/util/probe-event.h               | 21 ++++++++++
 tools/perf/util/symbol-elf.c                |  5 ++-
 tools/perf/util/symbol.c                    |  6 +++
 tools/perf/util/symbol.h                    |  6 +++
 9 files changed, 188 insertions(+), 28 deletions(-)
 create mode 100644 tools/perf/arch/powerpc/util/sym-handling.c

-- 
2.1.3

[PATCHv2 1/8] kprobes: Fix kallsyms lookup across powerpc ABIv1 and ABIv2

[Naveen N. Rao]

Currently, all non-dot symbols are being treated as function descriptors
in ABIv1. This is incorrect and is resulting in perf probe not working:

  # perf probe do_fork
  Added new event:
  Failed to write event: Invalid argument
    Error: Failed to add events.
  # dmesg | tail -1
  [192268.073063] Could not insert probe at _text+768432: -22

perf probe bases all kernel probes on _text and writes,
for example, "p:probe/do_fork _text+768432" to
/sys/kernel/debug/tracing/kprobe_events. In-kernel, _text is being
considered to be a function descriptor and is resulting in the above
error.

Fix this by changing how we lookup symbol addresses on ppc64. We first
check for the dot variant of a symbol and look at the non-dot variant
only if that fails. In this manner, we avoid having to look at the
function descriptor.

While at it, also separate out how this works on ABIv2 where
we don't have dot symbols, but need to use the local entry point.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Mike,
I have restricted all changes to just the kprobe_lookup_name() macro. It has
now been split into different implementations for ABIv1 and ABIv2, hopefully
addressing the concerns you raised previously.

- Naveen

 arch/powerpc/include/asm/kprobes.h | 63 ++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
index af15d4d..039b583 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -41,34 +41,59 @@ typedef ppc_opcode_t kprobe_opcode_t;
 #define MAX_INSN_SIZE 1
 
 #ifdef CONFIG_PPC64
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+/* PPC64 ABIv2 needs local entry point */
+#define kprobe_lookup_name(name, addr)					\
+{									\
+	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);		\
+	if (addr)							\
+		addr = (kprobe_opcode_t *)ppc_function_entry(addr);	\
+}
+#else
 /*
- * 64bit powerpc uses function descriptors.
- * Handle cases where:
- * 		- User passes a <.symbol> or <module:.symbol>
- * 		- User passes a <symbol> or <module:symbol>
- * 		- User passes a non-existent symbol, kallsyms_lookup_name
- * 		  returns 0. Don't deref the NULL pointer in that case
+ * 64bit powerpc ABIv1 uses function descriptors:
+ * - Check for the dot variant of the symbol first.
+ * - If that fails, try looking up the symbol provided.
+ *
+ * This ensures we always get to the actual symbol and not the descriptor.
+ * Also handle <module:symbol> format.
  */
 #define kprobe_lookup_name(name, addr)					\
 {									\
-	addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);		\
-	if (addr) {							\
-		char *colon;						\
-		if ((colon = strchr(name, ':')) != NULL) {		\
-			colon++;					\
-			if (*colon != '\0' && *colon != '.')		\
-				addr = (kprobe_opcode_t *)ppc_function_entry(addr); \
-		} else if (name[0] != '.')				\
-			addr = (kprobe_opcode_t *)ppc_function_entry(addr); \
-	} else {							\
-		char dot_name[KSYM_NAME_LEN];				\
+	char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];		\
+	char *modsym;							\
+	bool dot_appended = false;					\
+	if ((modsym = strchr(name, ':')) != NULL) {			\
+		modsym++;						\
+		if (*modsym != '\0' && *modsym != '.') {		\
+			/* Convert to <module:.symbol> */		\
+			strncpy(dot_name, name, modsym - name);		\
+			dot_name[modsym - name] = '.';			\
+			dot_name[modsym - name + 1] = '\0';		\
+			strncat(dot_name, modsym,			\
+				sizeof(dot_name) - (modsym - name) - 2);\
+			dot_appended = true;				\
+		} else {						\
+			dot_name[0] = '\0';				\
+			strncat(dot_name, name, sizeof(dot_name) - 1);	\
+		}							\
+	} else if (name[0] != '.') {					\
 		dot_name[0] = '.';					\
 		dot_name[1] = '\0';					\
 		strncat(dot_name, name, KSYM_NAME_LEN - 2);		\
-		addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name); \
+		dot_appended = true;					\
+	} else {							\
+		dot_name[0] = '\0';					\
+		strncat(dot_name, name, KSYM_NAME_LEN - 1);		\
+	}								\
+	addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);	\
+	if (!addr && dot_appended) {					\
+		/* Let's try the original non-dot symbol lookup	*/	\
+		addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);	\
 	}								\
 }
-#endif
+#endif /* defined(_CALL_ELF) && _CALL_ELF == 2 */
+#endif /* CONFIG_PPC64 */
 
 #define flush_insn_slot(p)	do { } while (0)
 #define kretprobe_blacklist_size 0
-- 
2.1.3

[PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern

[Naveen N. Rao]

Currently, perf probe considers patterns including a '.' to be a file.
However, this causes problems on powerpc ABIv1 where all functions have
a leading '.':

  $ perf probe -F | grep schedule_timeout_interruptible
  .schedule_timeout_interruptible
  $ perf probe .schedule_timeout_interruptible
  Semantic error :File always requires line number or lazy pattern.
    Error: Command Parse Error.

Fix this by checking the probe pattern in more detail.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 28eb141..9943ff3 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
 		arg = tmp;
 	}
 
+	/*
+	 * Check arg is function or file name and copy it.
+	 *
+	 * We consider arg to be a file spec if and only if it satisfies
+	 * all of the below criteria::
+	 * - it does not include any of "+@%",
+	 * - it includes one of ":;", and
+	 * - it has a period '.' in the name.
+	 *
+	 * Otherwise, we consider arg to be a function specification.
+	 */
+	c = 0;
+	if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
+		/* This is a file spec if it includes a '.' before ; or : */
+		if (memchr(arg, '.', ptr-arg))
+			c = 1;
+	}
+
 	ptr = strpbrk(arg, ";:+@%");
 	if (ptr) {
 		nc = *ptr;
@@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
 	if (tmp == NULL)
 		return -ENOMEM;
 
-	/* Check arg is function or file and copy it */
-	if (strchr(tmp, '.'))	/* File */
+	if (c == 1)
 		pp->file = tmp;
-	else			/* Function */
+	else
 		pp->function = tmp;
 
 	/* Parse other options */
-- 
2.1.3

Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern

[Arnaldo Carvalho de Melo]

Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> Currently, perf probe considers patterns including a '.' to be a file.
> However, this causes problems on powerpc ABIv1 where all functions have
> a leading '.':
> 
>   $ perf probe -F | grep schedule_timeout_interruptible
>   .schedule_timeout_interruptible
>   $ perf probe .schedule_timeout_interruptible
>   Semantic error :File always requires line number or lazy pattern.
>     Error: Command Parse Error.
> 
> Fix this by checking the probe pattern in more detail.

Masami, can I have your Acked-by or Reviewed-by?


- Arnaldo
 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-event.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 28eb141..9943ff3 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>  		arg = tmp;
>  	}
>  
> +	/*
> +	 * Check arg is function or file name and copy it.
> +	 *
> +	 * We consider arg to be a file spec if and only if it satisfies
> +	 * all of the below criteria::
> +	 * - it does not include any of "+@%",
> +	 * - it includes one of ":;", and
> +	 * - it has a period '.' in the name.
> +	 *
> +	 * Otherwise, we consider arg to be a function specification.
> +	 */
> +	c = 0;
> +	if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
> +		/* This is a file spec if it includes a '.' before ; or : */
> +		if (memchr(arg, '.', ptr-arg))
> +			c = 1;
> +	}
> +
>  	ptr = strpbrk(arg, ";:+@%");
>  	if (ptr) {
>  		nc = *ptr;
> @@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>  	if (tmp == NULL)
>  		return -ENOMEM;
>  
> -	/* Check arg is function or file and copy it */
> -	if (strchr(tmp, '.'))	/* File */
> +	if (c == 1)
>  		pp->file = tmp;
> -	else			/* Function */
> +	else
>  		pp->function = tmp;
>  
>  	/* Parse other options */
> -- 
> 2.1.3

Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern

[Arnaldo Carvalho de Melo]

Em Thu, Mar 12, 2015 at 05:24:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> > Currently, perf probe considers patterns including a '.' to be a file.
> > However, this causes problems on powerpc ABIv1 where all functions have
> > a leading '.':
> > 
> >   $ perf probe -F | grep schedule_timeout_interruptible
> >   .schedule_timeout_interruptible
> >   $ perf probe .schedule_timeout_interruptible
> >   Semantic error :File always requires line number or lazy pattern.
> >     Error: Command Parse Error.
> > 
> > Fix this by checking the probe pattern in more detail.
> 
> Masami, can I have your Acked-by or Reviewed-by?

It is limited to powerpc, but I would even so be happy if you could look
at it,

Thanks,

- Arnaldo
 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  tools/perf/util/probe-event.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 28eb141..9943ff3 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> >  		arg = tmp;
> >  	}
> >  
> > +	/*
> > +	 * Check arg is function or file name and copy it.
> > +	 *
> > +	 * We consider arg to be a file spec if and only if it satisfies
> > +	 * all of the below criteria::
> > +	 * - it does not include any of "+@%",
> > +	 * - it includes one of ":;", and
> > +	 * - it has a period '.' in the name.
> > +	 *
> > +	 * Otherwise, we consider arg to be a function specification.
> > +	 */
> > +	c = 0;
> > +	if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
> > +		/* This is a file spec if it includes a '.' before ; or : */
> > +		if (memchr(arg, '.', ptr-arg))
> > +			c = 1;
> > +	}
> > +
> >  	ptr = strpbrk(arg, ";:+@%");
> >  	if (ptr) {
> >  		nc = *ptr;
> > @@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> >  	if (tmp == NULL)
> >  		return -ENOMEM;
> >  
> > -	/* Check arg is function or file and copy it */
> > -	if (strchr(tmp, '.'))	/* File */
> > +	if (c == 1)
> >  		pp->file = tmp;
> > -	else			/* Function */
> > +	else
> >  		pp->function = tmp;
> >  
> >  	/* Parse other options */
> > -- 
> > 2.1.3

Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern

[Ananth N Mavinakayanahalli]

On Thu, Mar 12, 2015 at 05:24:14PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> > Currently, perf probe considers patterns including a '.' to be a file.
> > However, this causes problems on powerpc ABIv1 where all functions have
> > a leading '.':
> > 
> >   $ perf probe -F | grep schedule_timeout_interruptible
> >   .schedule_timeout_interruptible
> >   $ perf probe .schedule_timeout_interruptible
> >   Semantic error :File always requires line number or lazy pattern.
> >     Error: Command Parse Error.
> > 
> > Fix this by checking the probe pattern in more detail.
> 
> Masami, can I have your Acked-by or Reviewed-by?

Arnaldo,

FWIW, I have reviewed this code...

Reviewed-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

> 
> 
> - Arnaldo
> 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  tools/perf/util/probe-event.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 28eb141..9943ff3 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> >  		arg = tmp;
> >  	}
> >  
> > +	/*
> > +	 * Check arg is function or file name and copy it.
> > +	 *
> > +	 * We consider arg to be a file spec if and only if it satisfies
> > +	 * all of the below criteria::
> > +	 * - it does not include any of "+@%",
> > +	 * - it includes one of ":;", and
> > +	 * - it has a period '.' in the name.
> > +	 *
> > +	 * Otherwise, we consider arg to be a function specification.
> > +	 */
> > +	c = 0;
> > +	if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
> > +		/* This is a file spec if it includes a '.' before ; or : */
> > +		if (memchr(arg, '.', ptr-arg))
> > +			c = 1;
> > +	}
> > +
> >  	ptr = strpbrk(arg, ";:+@%");
> >  	if (ptr) {
> >  		nc = *ptr;
> > @@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> >  	if (tmp == NULL)
> >  		return -ENOMEM;
> >  
> > -	/* Check arg is function or file and copy it */
> > -	if (strchr(tmp, '.'))	/* File */
> > +	if (c == 1)
> >  		pp->file = tmp;
> > -	else			/* Function */
> > +	else
> >  		pp->function = tmp;
> >  
> >  	/* Parse other options */
> > -- 
> > 2.1.3

Re: Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern

[Masami Hiramatsu]

(2015/03/13 5:24), Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
>> Currently, perf probe considers patterns including a '.' to be a file.
>> However, this causes problems on powerpc ABIv1 where all functions have
>> a leading '.':
>>
>>   $ perf probe -F | grep schedule_timeout_interruptible
>>   .schedule_timeout_interruptible
>>   $ perf probe .schedule_timeout_interruptible
>>   Semantic error :File always requires line number or lazy pattern.
>>     Error: Command Parse Error.
>>
>> Fix this by checking the probe pattern in more detail.
> 
> Masami, can I have your Acked-by or Reviewed-by?

As far as I can see, this is not enough for fixing that issue.
Could you fold the first half of [4/8] to this patch?
I also have some comments on it. See below.

>  
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>  tools/perf/util/probe-event.c | 23 ++++++++++++++++++++---
>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 28eb141..9943ff3 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>>  		arg = tmp;
>>  	}
>>  
>> +	/*
>> +	 * Check arg is function or file name and copy it.
>> +	 *
>> +	 * We consider arg to be a file spec if and only if it satisfies
>> +	 * all of the below criteria::
>> +	 * - it does not include any of "+@%",
>> +	 * - it includes one of ":;", and
>> +	 * - it has a period '.' in the name.
>> +	 *
>> +	 * Otherwise, we consider arg to be a function specification.
>> +	 */
>> +	c = 0;

Oh please, don't reuse 'char c' for a boolean flag, you should
introduce new 'bool file_loc' etc.

>> +	if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
>> +		/* This is a file spec if it includes a '.' before ; or : */
>> +		if (memchr(arg, '.', ptr-arg))
                                        ^^ add spaces around '-'.

Thank you,


>> +			c = 1;
>> +	}
>> +
>>  	ptr = strpbrk(arg, ";:+@%");
>>  	if (ptr) {
>>  		nc = *ptr;
>> @@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>>  	if (tmp == NULL)
>>  		return -ENOMEM;
>>  
>> -	/* Check arg is function or file and copy it */
>> -	if (strchr(tmp, '.'))	/* File */
>> +	if (c == 1)
>>  		pp->file = tmp;
>> -	else			/* Function */
>> +	else
>>  		pp->function = tmp;
>>  
>>  	/* Parse other options */
>> -- 
>> 2.1.3
> --
> 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/
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern

[Naveen N. Rao]

On 2015/03/13 08:20PM, Masami Hiramatsu wrote:
> (2015/03/13 5:24), Arnaldo Carvalho de Melo wrote:
> > Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> >> Currently, perf probe considers patterns including a '.' to be a file.
> >> However, this causes problems on powerpc ABIv1 where all functions have
> >> a leading '.':
> >>
> >>   $ perf probe -F | grep schedule_timeout_interruptible
> >>   .schedule_timeout_interruptible
> >>   $ perf probe .schedule_timeout_interruptible
> >>   Semantic error :File always requires line number or lazy pattern.
> >>     Error: Command Parse Error.
> >>
> >> Fix this by checking the probe pattern in more detail.
> > 
> > Masami, can I have your Acked-by or Reviewed-by?
> 
> As far as I can see, this is not enough for fixing that issue.
> Could you fold the first half of [4/8] to this patch?

Sure.

> I also have some comments on it. See below.
> 
> >  
> >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> >> ---
> >>  tools/perf/util/probe-event.c | 23 ++++++++++++++++++++---
> >>  1 file changed, 20 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> >> index 28eb141..9943ff3 100644
> >> --- a/tools/perf/util/probe-event.c
> >> +++ b/tools/perf/util/probe-event.c
> >> @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> >>  		arg = tmp;
> >>  	}
> >>  
> >> +	/*
> >> +	 * Check arg is function or file name and copy it.
> >> +	 *
> >> +	 * We consider arg to be a file spec if and only if it satisfies
> >> +	 * all of the below criteria::
> >> +	 * - it does not include any of "+@%",
> >> +	 * - it includes one of ":;", and
> >> +	 * - it has a period '.' in the name.
> >> +	 *
> >> +	 * Otherwise, we consider arg to be a function specification.
> >> +	 */
> >> +	c = 0;
> 
> Oh please, don't reuse 'char c' for a boolean flag, you should
> introduce new 'bool file_loc' etc.
> 
> >> +	if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
> >> +		/* This is a file spec if it includes a '.' before ; or : */
> >> +		if (memchr(arg, '.', ptr-arg))
>                                         ^^ add spaces around '-'.

Sure.

- Naveen
 

Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern

[Naveen N. Rao]

On 2015/03/13 08:20PM, Masami Hiramatsu wrote:
> (2015/03/13 5:24), Arnaldo Carvalho de Melo wrote:
> > Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> >> Currently, perf probe considers patterns including a '.' to be a file.
> >> However, this causes problems on powerpc ABIv1 where all functions have
> >> a leading '.':
> >>
> >>   $ perf probe -F | grep schedule_timeout_interruptible
> >>   .schedule_timeout_interruptible
> >>   $ perf probe .schedule_timeout_interruptible
> >>   Semantic error :File always requires line number or lazy pattern.
> >>     Error: Command Parse Error.
> >>
> >> Fix this by checking the probe pattern in more detail.
> > 
> > Masami, can I have your Acked-by or Reviewed-by?
> 
> As far as I can see, this is not enough for fixing that issue.
> Could you fold the first half of [4/8] to this patch?
> I also have some comments on it. See below.

Masami, Arnaldo,
Thanks for the review. v3 patches forthcoming...

- Naveen

[PATCHv2 3/8] perf probe powerpc: Fix symbol fixup issues due to ELF type

[Naveen N. Rao]

If using the symbol table, symbol addresses are not being fixed up
properly, resulting in probes being placed at wrong addresses:

  # perf probe do_fork
  Added new event:
    probe:do_fork        (on do_fork)

  You can now use it in all perf tools, such as:

	  perf record -e probe:do_fork -aR sleep 1

  # cat /sys/kernel/debug/tracing/kprobe_events
  p:probe/do_fork _text+635952
  # printf "%x" 635952
  9b430
  # grep do_fork /boot/System.map
  c0000000000ab430 T .do_fork

Fix by checking for ELF type ET_DYN used by ppc64 kernels.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/symbol-elf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 06fcd1b..7ac4e4c 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -683,7 +683,8 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 						     NULL) != NULL);
 	} else {
 		ss->adjust_symbols = ehdr.e_type == ET_EXEC ||
-				     ehdr.e_type == ET_REL;
+				     ehdr.e_type == ET_REL ||
+				     ehdr.e_type == ET_DYN;
 	}
 
 	ss->name   = strdup(name);
-- 
2.1.3

Re: [PATCHv2 3/8] perf probe powerpc: Fix symbol fixup issues due to ELF type

[Arnaldo Carvalho de Melo]

Em Mon, Dec 15, 2014 at 08:20:33PM +0530, Naveen N. Rao escreveu:
> If using the symbol table, symbol addresses are not being fixed up
> properly, resulting in probes being placed at wrong addresses:
> 
>   # perf probe do_fork
>   Added new event:
>     probe:do_fork        (on do_fork)
> 
>   You can now use it in all perf tools, such as:
> 
> 	  perf record -e probe:do_fork -aR sleep 1
> 
>   # cat /sys/kernel/debug/tracing/kprobe_events
>   p:probe/do_fork _text+635952
>   # printf "%x" 635952
>   9b430
>   # grep do_fork /boot/System.map
>   c0000000000ab430 T .do_fork
> 
> Fix by checking for ELF type ET_DYN used by ppc64 kernels.

Sorry if this was answered already, its been a while since this was
posted/discussed... Are you completely sure this is not a problem on
!ppc?

Woudln't it be more conservative to somehow only adjust symbols for this
ET_DYN type if the arch is ppc? I.e. something like moving that
adjust_symbols logic to be arch_adjust_symbols(), provide a default and
then override it for ppc to include also ET_DYN?

Looking at the other patches...

- Arnaldo
 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/symbol-elf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 06fcd1b..7ac4e4c 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -683,7 +683,8 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
>  						     NULL) != NULL);
>  	} else {
>  		ss->adjust_symbols = ehdr.e_type == ET_EXEC ||
> -				     ehdr.e_type == ET_REL;
> +				     ehdr.e_type == ET_REL ||
> +				     ehdr.e_type == ET_DYN;
>  	}
>  
>  	ss->name   = strdup(name);
> -- 
> 2.1.3

Re: [PATCHv2 3/8] perf probe powerpc: Fix symbol fixup issues due to ELF type

[Naveen N. Rao]

On 2015/03/12 05:23PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 15, 2014 at 08:20:33PM +0530, Naveen N. Rao escreveu:
> > If using the symbol table, symbol addresses are not being fixed up
> > properly, resulting in probes being placed at wrong addresses:
> > 
> >   # perf probe do_fork
> >   Added new event:
> >     probe:do_fork        (on do_fork)
> > 
> >   You can now use it in all perf tools, such as:
> > 
> > 	  perf record -e probe:do_fork -aR sleep 1
> > 
> >   # cat /sys/kernel/debug/tracing/kprobe_events
> >   p:probe/do_fork _text+635952
> >   # printf "%x" 635952
> >   9b430
> >   # grep do_fork /boot/System.map
> >   c0000000000ab430 T .do_fork
> > 
> > Fix by checking for ELF type ET_DYN used by ppc64 kernels.
> 
> Sorry if this was answered already, its been a while since this was
> posted/discussed... Are you completely sure this is not a problem on
> !ppc?
> 
> Woudln't it be more conservative to somehow only adjust symbols for this
> ET_DYN type if the arch is ppc? I.e. something like moving that
> adjust_symbols logic to be arch_adjust_symbols(), provide a default and
> then override it for ppc to include also ET_DYN?

I did check and I don't think any other arch kernel uses ET_DYN, but 
yes, being conservative, I could just make this a arch helper. Will do.

- Naveen

> 
> Looking at the other patches...
> 
> - Arnaldo
> 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  tools/perf/util/symbol-elf.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > index 06fcd1b..7ac4e4c 100644
> > --- a/tools/perf/util/symbol-elf.c
> > +++ b/tools/perf/util/symbol-elf.c
> > @@ -683,7 +683,8 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
> >  						     NULL) != NULL);
> >  	} else {
> >  		ss->adjust_symbols = ehdr.e_type == ET_EXEC ||
> > -				     ehdr.e_type == ET_REL;
> > +				     ehdr.e_type == ET_REL ||
> > +				     ehdr.e_type == ET_DYN;
> >  	}
> >  
> >  	ss->name   = strdup(name);
> > -- 
> > 2.1.3
> 

[PATCHv2 4/8] perf probe powerpc: Handle powerpc dot symbols

[Naveen N. Rao]

Fix up various perf aspects related to ppc64's usage of dot functions:
- ignore leading '.' when generating event names and when looking for
  existing events.
- use the proper prefix when ignoring SyS symbol lookups.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c | 8 ++++++++
 tools/perf/util/symbol.c      | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 9943ff3..74b7fef 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2080,6 +2080,10 @@ static int get_new_event_name(char *buf, size_t len, const char *base,
 {
 	int i, ret;
 
+	/* Skip the leading dot on powerpc */
+	if (*base == '.')
+		base++;
+
 	/* Try no suffix */
 	ret = e_snprintf(buf, len, "%s", base);
 	if (ret < 0) {
@@ -2538,6 +2542,10 @@ int del_perf_probe_events(struct strlist *dellist)
 			event = str;
 		}
 
+		/* Skip the leading dot on powerpc */
+		if (event && *event == '.')
+			event++;
+
 		ret = e_snprintf(buf, 128, "%s:%s", group, event);
 		if (ret < 0) {
 			pr_err("Failed to copy event.");
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index c24c5b8..e7b9bae 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -139,6 +139,12 @@ static int choose_best_symbol(struct symbol *syma, struct symbol *symb)
 	if (na >= 10 && !strncmp(syma->name, "compat_SyS", 10))
 		return SYMBOL_B;
 
+	/* On powerpc, ignore the dot variants */
+	if (na >= 4 && !strncmp(syma->name, ".SyS", 4))
+		return SYMBOL_B;
+	if (na >= 11 && !strncmp(syma->name, ".compat_SyS", 11))
+		return SYMBOL_B;
+
 	return SYMBOL_A;
 }
 
-- 
2.1.3

Re: [PATCHv2 4/8] perf probe powerpc: Handle powerpc dot symbols

[Arnaldo Carvalho de Melo]

Em Mon, Dec 15, 2014 at 08:20:34PM +0530, Naveen N. Rao escreveu:
> Fix up various perf aspects related to ppc64's usage of dot functions:
> - ignore leading '.' when generating event names and when looking for
>   existing events.
> - use the proper prefix when ignoring SyS symbol lookups.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-event.c | 8 ++++++++
>  tools/perf/util/symbol.c      | 6 ++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 9943ff3..74b7fef 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2080,6 +2080,10 @@ static int get_new_event_name(char *buf, size_t len, const char *base,
>  {
>  	int i, ret;
>  
> +	/* Skip the leading dot on powerpc */
> +	if (*base == '.')
> +		base++;
> +
>  	/* Try no suffix */
>  	ret = e_snprintf(buf, len, "%s", base);
>  	if (ret < 0) {
> @@ -2538,6 +2542,10 @@ int del_perf_probe_events(struct strlist *dellist)
>  			event = str;
>  		}
>  
> +		/* Skip the leading dot on powerpc */

Ok, but this is not powerpc specific code, is it?

> +		if (event && *event == '.')
> +			event++;
> +
>  		ret = e_snprintf(buf, 128, "%s:%s", group, event);
>  		if (ret < 0) {
>  			pr_err("Failed to copy event.");
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index c24c5b8..e7b9bae 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -139,6 +139,12 @@ static int choose_best_symbol(struct symbol *syma, struct symbol *symb)
>  	if (na >= 10 && !strncmp(syma->name, "compat_SyS", 10))
>  		return SYMBOL_B;
>  
> +	/* On powerpc, ignore the dot variants */
> +	if (na >= 4 && !strncmp(syma->name, ".SyS", 4))
> +		return SYMBOL_B;
> +	if (na >= 11 && !strncmp(syma->name, ".compat_SyS", 11))
> +		return SYMBOL_B;
> +
>  	return SYMBOL_A;
>  }
>  
> -- 
> 2.1.3

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

[Naveen N. Rao]

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.

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
 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 */
 
 #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);
+
 /* Maximum index number of event-name postfix */
 #define MAX_EVENT_INDEX	1024
 
-- 
2.1.3

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

[Arnaldo Carvalho de Melo]

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

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

[Naveen N. Rao]

On 2015/03/12 05:30PM, Arnaldo Carvalho de Melo wrote:
> 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.

That sounds like a good idea. I will move these over to use __weak 
functions.

> 
> >  
> >  #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.

Sure.

- Naveen

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

[PATCHv2 6/8] perf tools powerpc: Fix PPC64 ELF ABIv2 symbol decoding

[Naveen N. Rao]

PPC64 ELF ABIv2 has a Global Entry Point (GEP) and a Local Entry Point
(LEP). For purposes of probing, we need the LEP. Offset to the LEP is
encoded in st_other.

Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Changes from previous patchset:
Simplified logic by adding dependancy on HAVE_ARCH_SYMBOL_HANDLING. Also
generalized arch_elf_sym_decode() to be suitable for other architectures in
future.

 tools/perf/arch/powerpc/util/sym-handling.c | 10 ++++++++++
 tools/perf/util/symbol-elf.c                |  2 ++
 tools/perf/util/symbol.h                    |  6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index 0a77825..a27bfaf 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -1,8 +1,10 @@
 /*
  * Special symbol handling for PowerPC:
  * - Handle dot symbols on ABIv1
+ * - Decode offset from Global entry point to Local entry point on ABIv2
  *
  * Copyright (C) 2014 Naveen N Rao, IBM Corporation.
+ * Copyright (C) 2014 Ananth N Mavinakayanahalli, IBM Corporation.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -26,3 +28,11 @@ int probe_function_filter(struct map *map __maybe_unused, struct symbol *sym)
 	}
 	return 1;
 }
+
+inline void arch_elf_sym_decode(GElf_Sym *sym __maybe_unused)
+{
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+	if (sym && sym->st_other)
+		sym->st_value += PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
+#endif
+}
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 7ac4e4c..557b63b 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -902,6 +902,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
 		    (sym.st_value & 1))
 			--sym.st_value;
 
+		arch_elf_sym_decode(&sym);
+
 		if (dso->kernel || kmodule) {
 			char dso_name[PATH_MAX];
 
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 9d602e9..9e03cdf 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -294,4 +294,10 @@ int compare_proc_modules(const char *from, const char *to);
 int setup_list(struct strlist **list, const char *list_str,
 	       const char *list_name);
 
+#ifdef HAVE_ARCH_SYMBOL_HANDLING
+extern void arch_elf_sym_decode(GElf_Sym *sym __maybe_unused);
+#else
+static inline void arch_elf_sym_decode(GElf_Sym *sym __maybe_unused) { }
+#endif
+
 #endif /* __PERF_SYMBOL */
-- 
2.1.3

Re: [PATCHv2 6/8] perf tools powerpc: Fix PPC64 ELF ABIv2 symbol decoding

[Arnaldo Carvalho de Melo]

Em Mon, Dec 15, 2014 at 08:20:36PM +0530, Naveen N. Rao escreveu:
> PPC64 ELF ABIv2 has a Global Entry Point (GEP) and a Local Entry Point
> (LEP). For purposes of probing, we need the LEP. Offset to the LEP is
> encoded in st_other.
> 
> Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> Changes from previous patchset:
> Simplified logic by adding dependancy on HAVE_ARCH_SYMBOL_HANDLING. Also
> generalized arch_elf_sym_decode() to be suitable for other architectures in
> future.
> 
>  tools/perf/arch/powerpc/util/sym-handling.c | 10 ++++++++++
>  tools/perf/util/symbol-elf.c                |  2 ++
>  tools/perf/util/symbol.h                    |  6 ++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
> index 0a77825..a27bfaf 100644
> --- a/tools/perf/arch/powerpc/util/sym-handling.c
> +++ b/tools/perf/arch/powerpc/util/sym-handling.c
> @@ -1,8 +1,10 @@
>  /*
>   * Special symbol handling for PowerPC:
>   * - Handle dot symbols on ABIv1
> + * - Decode offset from Global entry point to Local entry point on ABIv2
>   *
>   * Copyright (C) 2014 Naveen N Rao, IBM Corporation.
> + * Copyright (C) 2014 Ananth N Mavinakayanahalli, IBM Corporation.
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -26,3 +28,11 @@ int probe_function_filter(struct map *map __maybe_unused, struct symbol *sym)
>  	}
>  	return 1;
>  }
> +
> +inline void arch_elf_sym_decode(GElf_Sym *sym __maybe_unused)
> +{
> +#if defined(_CALL_ELF) && _CALL_ELF == 2
> +	if (sym && sym->st_other)
> +		sym->st_value += PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
> +#endif
> +}
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 7ac4e4c..557b63b 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -902,6 +902,8 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  		    (sym.st_value & 1))
>  			--sym.st_value;
>  
> +		arch_elf_sym_decode(&sym);
> +

arch_elf_sym_adjust()? 

But this is done the right way, i.e. provide a way for arches to do
adjustments specific to their ABIs, thanks!

- Arnaldo

>  		if (dso->kernel || kmodule) {
>  			char dso_name[PATH_MAX];
>  
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 9d602e9..9e03cdf 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -294,4 +294,10 @@ int compare_proc_modules(const char *from, const char *to);
>  int setup_list(struct strlist **list, const char *list_str,
>  	       const char *list_name);
>  
> +#ifdef HAVE_ARCH_SYMBOL_HANDLING
> +extern void arch_elf_sym_decode(GElf_Sym *sym __maybe_unused);
> +#else
> +static inline void arch_elf_sym_decode(GElf_Sym *sym __maybe_unused) { }
> +#endif
> +
>  #endif /* __PERF_SYMBOL */
> -- 
> 2.1.3

[PATCHv2 7/8] perf probe powerpc: Use DWARF info only if necessary

[Naveen N. Rao]

Use symbol table lookups by default if DWARF is not necessary, since
powerpc ABIv2 encodes local entry points in the symbol table and the
function entry address in DWARF may not be appropriate for kprobes, as
described here:
https://sourceware.org/bugzilla/show_bug.cgi?id=17638

"The DWARF address ranges deliberately include the *whole* function,
both global and local entry points."
...
"If you want to set probes on a local entry point, you should look up
the symbol in the main symbol table (not DWARF), and check the st_other
bits; they will indicate whether the function has a local entry point,
and what its offset from the global entry point is.  Note that GDB does
the same when setting a breakpoint on a function entry."

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Changes from previous patchset:
Generalize and introduce helper to prefer symbol table over DWARF.

 tools/perf/arch/powerpc/util/sym-handling.c | 9 +++++++++
 tools/perf/config/Makefile                  | 1 +
 tools/perf/util/probe-event.c               | 6 ++++++
 tools/perf/util/probe-event.h               | 6 ++++++
 4 files changed, 22 insertions(+)

diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index a27bfaf..22fc2e6 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -36,3 +36,12 @@ inline void arch_elf_sym_decode(GElf_Sym *sym __maybe_unused)
 		sym->st_value += PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
 #endif
 }
+
+inline bool prefer_symtab(void)
+{
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+	return true;
+#else
+	return false;
+#endif
+}
diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index 35cf934..849f686 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -384,6 +384,7 @@ ifeq ($(ARCH),powerpc)
     CFLAGS += -DHAVE_SKIP_CALLCHAIN_IDX
   endif
   CFLAGS += -DHAVE_ARCH_SYMBOL_HANDLING
+  CFLAGS += -DARCH_PREFER_SYMTAB
 endif
 
 ifndef NO_LIBUNWIND
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 7eb9b27..dfc3151 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2373,6 +2373,12 @@ static int convert_to_probe_trace_events(struct perf_probe_event *pev,
 		}
 	}
 
+	if (prefer_symtab() && !perf_probe_event_need_dwarf(pev)) {
+		ret = find_probe_trace_events_from_map(pev, tevs, max_tevs, target);
+		if (ret > 0)
+			return ret; /* Found in symbol table */
+	}
+
 	/* Convert perf_probe_event with debuginfo */
 	ret = try_to_find_probe_trace_events(pev, tevs, max_tevs, target);
 	if (ret != 0)
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index 8564451..5f92906 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -141,6 +141,12 @@ extern int show_available_funcs(const char *module, struct strfilter *filter,
 extern int probe_function_filter(struct map *map __maybe_unused,
 					struct symbol *sym);
 
+#ifdef ARCH_PREFER_SYMTAB
+extern bool prefer_symtab(void);
+#else
+static inline bool prefer_symtab(void) { return false; };
+#endif
+
 /* Maximum index number of event-name postfix */
 #define MAX_EVENT_INDEX	1024
 
-- 
2.1.3

[PATCHv2 8/8] perf probe powerpc: Fixup function entry if using kallsyms lookup

[Naveen N. Rao]

On powerpc ABIv2, if no debug-info is found and we use kallsyms, we need
to fixup the function entry to point to the local entry point. Use
offset of 8 since current toolchains always generate 2 instructions (8
bytes).

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Changes:
Generalize and introduce helper to post-process trace point.

 tools/perf/arch/powerpc/util/sym-handling.c | 17 +++++++++++++++++
 tools/perf/util/probe-event.c               |  1 +
 tools/perf/util/probe-event.h               | 10 ++++++++++
 3 files changed, 28 insertions(+)

diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index 22fc2e6..30c8797 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -45,3 +45,20 @@ inline bool prefer_symtab(void)
 	return false;
 #endif
 }
+
+void arch_fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
+			    struct probe_trace_event *tev __maybe_unused,
+			    struct map *map __maybe_unused)
+{
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+	/*
+	 * If we used kallsyms, we should fixup the function entry address here.
+	 * ppc64 ABIv2 local entry point is currently always 2 instructions (8 bytes)
+	 * after the global entry point. Fix this if it ever changes.
+	 */
+	if (!pev->uprobes && map->dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS) {
+		tev->point.address += 8;
+		tev->point.offset += 8;
+	}
+#endif
+}
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index dfc3151..85f37a4 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2340,6 +2340,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 					strdup_or_goto(pev->args[i].type,
 							nomem_out);
 		}
+		arch_fix_tev_from_maps(pev, tev, map);
 	}
 
 out:
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index 5f92906..335a6a4 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -147,6 +147,16 @@ extern bool prefer_symtab(void);
 static inline bool prefer_symtab(void) { return false; };
 #endif
 
+#ifdef HAVE_ARCH_SYMBOL_HANDLING
+extern void arch_fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
+				   struct probe_trace_event *tev __maybe_unused,
+				   struct map *map __maybe_unused);
+#else
+static inline void arch_fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
+					  struct probe_trace_event *tev __maybe_unused,
+					  struct map *map __maybe_unused) { }
+#endif
+
 /* Maximum index number of event-name postfix */
 #define MAX_EVENT_INDEX	1024
 
-- 
2.1.3

Re: [PATCHv2 8/8] perf probe powerpc: Fixup function entry if using kallsyms lookup

[Arnaldo Carvalho de Melo]

Em Mon, Dec 15, 2014 at 08:20:38PM +0530, Naveen N. Rao escreveu:
> On powerpc ABIv2, if no debug-info is found and we use kallsyms, we need
> to fixup the function entry to point to the local entry point. Use
> offset of 8 since current toolchains always generate 2 instructions (8
> bytes).
 

So perhaps we should rename ARCH_SYMBOL_HANDLING to ARCH_ABI_SYMBOL_FIXUPS?

> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> Changes:
> Generalize and introduce helper to post-process trace point.
> 
>  tools/perf/arch/powerpc/util/sym-handling.c | 17 +++++++++++++++++
>  tools/perf/util/probe-event.c               |  1 +
>  tools/perf/util/probe-event.h               | 10 ++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
> index 22fc2e6..30c8797 100644
> --- a/tools/perf/arch/powerpc/util/sym-handling.c
> +++ b/tools/perf/arch/powerpc/util/sym-handling.c
> @@ -45,3 +45,20 @@ inline bool prefer_symtab(void)
>  	return false;
>  #endif
>  }
> +
> +void arch_fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
> +			    struct probe_trace_event *tev __maybe_unused,
> +			    struct map *map __maybe_unused)
> +{
> +#if defined(_CALL_ELF) && _CALL_ELF == 2
> +	/*
> +	 * If we used kallsyms, we should fixup the function entry address here.
> +	 * ppc64 ABIv2 local entry point is currently always 2 instructions (8 bytes)
> +	 * after the global entry point. Fix this if it ever changes.
> +	 */
> +	if (!pev->uprobes && map->dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS) {
> +		tev->point.address += 8;
> +		tev->point.offset += 8;
> +	}
> +#endif
> +}
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index dfc3151..85f37a4 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2340,6 +2340,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  					strdup_or_goto(pev->args[i].type,
>  							nomem_out);
>  		}
> +		arch_fix_tev_from_maps(pev, tev, map);
>  	}
>  
>  out:
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index 5f92906..335a6a4 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -147,6 +147,16 @@ extern bool prefer_symtab(void);
>  static inline bool prefer_symtab(void) { return false; };
>  #endif
>  
> +#ifdef HAVE_ARCH_SYMBOL_HANDLING
> +extern void arch_fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
> +				   struct probe_trace_event *tev __maybe_unused,
> +				   struct map *map __maybe_unused);
> +#else
> +static inline void arch_fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
> +					  struct probe_trace_event *tev __maybe_unused,
> +					  struct map *map __maybe_unused) { }
> +#endif
> +
>  /* Maximum index number of event-name postfix */
>  #define MAX_EVENT_INDEX	1024
>  
> -- 
> 2.1.3

Re: [PATCHv2 0/8] Fix perf probe issues on powerpc

[Naveen N. Rao]

On 2014/12/15 08:20PM, Naveen N Rao wrote:
> This patchset fixes various issues with perf probe on powerpc across ABIv1 and
> ABIv2:
> - in the presence of DWARF debug-info,
> - in the absence of DWARF, but with the symbol table, and
> - in the absence of debug-info, but with kallsyms.
> 
> Applies cleanly on -tip. Tested on ppc64 BE and LE.
> 
> Changes from previous version:
> Addressed various review comments from Mike Ellerman largely to generalize
> changes. Some of the simpler patches have been retained in their previous form
> to limit code churn, while others have been generalized by introducing arch
> helpers. Individual patches have more details.

Michael,
Can you please take a quick look at this?

- Naveen
 

Re: [PATCHv2 0/8] Fix perf probe issues on powerpc

[Michael Ellerman]

On Wed, 2015-01-28 at 11:12 +0530, Naveen N. Rao wrote:
> On 2014/12/15 08:20PM, Naveen N Rao wrote:
> > This patchset fixes various issues with perf probe on powerpc across ABIv1 and
> > ABIv2:
> > - in the presence of DWARF debug-info,
> > - in the absence of DWARF, but with the symbol table, and
> > - in the absence of debug-info, but with kallsyms.
> > 
> > Applies cleanly on -tip. Tested on ppc64 BE and LE.
> > 
> > Changes from previous version:
> > Addressed various review comments from Mike Ellerman largely to generalize
> > changes. Some of the simpler patches have been retained in their previous form
> > to limit code churn, while others have been generalized by introducing arch
> > helpers. Individual patches have more details.
> 
> Michael,
> Can you please take a quick look at this?

I merged patch 1.

https://git.kernel.org/cgit/linux/kernel/git/mpe/linux.git/commit/?h=next&id=bf794bf52a80c6278a028f0af2ca32d7c3508c9b

The rest are not for me, they're perf tools, so you need to convince acme
they're good.

cheers

Re: [PATCHv2 0/8] Fix perf probe issues on powerpc

[Naveen N. Rao]

On 2015/01/28 05:14PM, Michael Ellerman wrote:
> On Wed, 2015-01-28 at 11:12 +0530, Naveen N. Rao wrote:
> > On 2014/12/15 08:20PM, Naveen N Rao wrote:
> > > This patchset fixes various issues with perf probe on powerpc across ABIv1 and
> > > ABIv2:
> > > - in the presence of DWARF debug-info,
> > > - in the absence of DWARF, but with the symbol table, and
> > > - in the absence of debug-info, but with kallsyms.
> > > 
> > > Applies cleanly on -tip. Tested on ppc64 BE and LE.
> > > 
> > > Changes from previous version:
> > > Addressed various review comments from Mike Ellerman largely to generalize
> > > changes. Some of the simpler patches have been retained in their previous form
> > > to limit code churn, while others have been generalized by introducing arch
> > > helpers. Individual patches have more details.
> > 
> > Michael,
> > Can you please take a quick look at this?
> 
> I merged patch 1.
> 
> https://git.kernel.org/cgit/linux/kernel/git/mpe/linux.git/commit/?h=next&id=bf794bf52a80c6278a028f0af2ca32d7c3508c9b
> 
> The rest are not for me, they're perf tools, so you need to convince acme
> they're good.

Oh, thanks! Sorry, I didn't realize you had already merged it.
I assume you are ok with my changes in v2 w.r.t your previous review 
comments.

Arnaldo,
Please have a look at this revision and let me know if you have any 
other concerns. Also, I have missed including Ananth's "Reviewed-by" in 
this patchset.  I'm guessing Ananth is ok with this revision as well.


- Naveen

Re: [PATCHv2 0/8] Fix perf probe issues on powerpc

[Michael Ellerman]

On Wed, 2015-01-28 at 12:13 +0530, Naveen N. Rao wrote:
> On 2015/01/28 05:14PM, Michael Ellerman wrote:
> > On Wed, 2015-01-28 at 11:12 +0530, Naveen N. Rao wrote:
> > > On 2014/12/15 08:20PM, Naveen N Rao wrote:
> > > > This patchset fixes various issues with perf probe on powerpc across ABIv1 and
> > > > ABIv2:
> > > > - in the presence of DWARF debug-info,
> > > > - in the absence of DWARF, but with the symbol table, and
> > > > - in the absence of debug-info, but with kallsyms.
> > > > 
> > > > Applies cleanly on -tip. Tested on ppc64 BE and LE.
> > > > 
> > > > Changes from previous version:
> > > > Addressed various review comments from Mike Ellerman largely to generalize
> > > > changes. Some of the simpler patches have been retained in their previous form
> > > > to limit code churn, while others have been generalized by introducing arch
> > > > helpers. Individual patches have more details.
> > > 
> > > Michael,
> > > Can you please take a quick look at this?
> > 
> > I merged patch 1.
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/mpe/linux.git/commit/?h=next&id=bf794bf52a80c6278a028f0af2ca32d7c3508c9b
> > 
> > The rest are not for me, they're perf tools, so you need to convince acme
> > they're good.
> 
> Oh, thanks! Sorry, I didn't realize you had already merged it.
> I assume you are ok with my changes in v2 w.r.t your previous review 
> comments.

Yeah it looks like you addressed most of my comments.

cheers