[PATCH 0/4] perf-probe: Fix GNU IFUNC probe issue etc.

[PATCH 0/4] perf-probe: Fix GNU IFUNC probe issue etc.

[Masami Hiramatsu]

Hi,

Here are patches to fix some issues of probing on GNU IFUNC, duplicated
symbols, and memory leak, which were reported by Andi.

Andi reported that some issues on probing memcpy function in glibc,
which was related to GNU IFUNC (indirect function). As I described
in the patch [4/4], it is hard to support probing on the functions
which are selected by GNU indirect function because those are chosen
at runtime. I think we need a user-mode helper in uprobes to find which
one is chosen at runtime. (Oleg, Srikar, would you have any idea?)

While cleaning up the patches, I also found a memory leak problem
so fixed it ([3/4]).

Thank you,

---

Masami Hiramatsu (4):
      perf-probe: Avoid setting probes on same address on same event
      perf-probe: Fix wrong variable warning when the probe point is not found
      perf-probe: Fix memory leakage when the probe point is not found
      perf-probe: Warn if the target function is GNU Indirect function


 tools/perf/util/probe-event.c  |   14 ++++++++++++++
 tools/perf/util/probe-finder.c |    5 ++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH 1/4] perf-probe: Avoid setting probes on same address on same event

[Masami Hiramatsu]

There is a case that the several same-name symbols points
same address. In that case, perf probe returns an error.

E.g.

  perf probe -x /lib64/libc-2.30.so -v -a "memcpy arg1=%di"
  probe-definition(0): memcpy arg1=%di
  symbol:memcpy file:(null) line:0 offset:0 return:0 lazy:(null)
  parsing arg: arg1=%di into name:arg1 %di
  1 arguments
  symbol:setjmp file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:longjmp file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:longjmp_target file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:lll_lock_wait_private file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt_arena_max file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt_arena_test file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_tunable_tcache_max_bytes file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_tunable_tcache_count file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_tunable_tcache_unsorted_limit file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt_trim_threshold file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt_top_pad file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt_mmap_threshold file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt_mmap_max file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt_perturb file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt_mxfast file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_heap_new file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_arena_reuse_free_list file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_arena_reuse file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_arena_reuse_wait file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_arena_new file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_arena_retry file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_sbrk_less file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_heap_free file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_heap_less file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_tcache_double_free file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_heap_more file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_sbrk_more file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_malloc_retry file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_memalign_retry file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt_free_dyn_thresholds file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_realloc_retry file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_calloc_retry file:(null) line:0 offset:0 return:0 lazy:(null)
  symbol:memory_mallopt file:(null) line:0 offset:0 return:0 lazy:(null)
  Open Debuginfo file: /usr/lib/debug/usr/lib64/libc-2.30.so.debug
  Try to find probe point from debuginfo.
  Opening /sys/kernel/debug/tracing//README write=0
  Failed to find the location of the '%di' variable at this address.
   Perhaps it has been optimized out.
   Use -V with the --range option to show '%di' location range.
  An error occurred in debuginfo analysis (-2).
  Trying to use symbols.
  Opening /sys/kernel/debug/tracing//uprobe_events write=1
  Writing event: p:probe_libc/memcpy /usr/lib64/libc-2.30.so:0x914c0 arg1=%di
  Writing event: p:probe_libc/memcpy /usr/lib64/libc-2.30.so:0x914c0 arg1=%di
  Failed to write event: File exists
    Error: Failed to add events. Reason: File exists (Code: -17)

You can see the perf tried to write completely same probe definition
twice, which caused an error.

To fix this issue, check the symbol list and drop duplicated
symbols (which has same symbol name and address) from it.

With this patch;

  # perf probe -x /lib64/libc-2.30.so -a "memcpy arg1=%di"
  Failed to find the location of the '%di' variable at this address.
   Perhaps it has been optimized out.
   Use -V with the --range option to show '%di' location range.
  Added new events:
    probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
    probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)

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

  	perf record -e probe_libc:memcpy -aR sleep 1


Reported-by: Andi Kleen <andi@firstfloor.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/probe-event.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index df713a5d1e26..1e95a336862c 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2968,6 +2968,15 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	for (j = 0; j < num_matched_functions; j++) {
 		sym = syms[j];
 
+		/* There can be duplicated symbols in the map */
+		for (i = 0; i < j; i++)
+			if (sym->start == syms[i]->start) {
+				pr_debug("find duplicated symbol %s @ %lx\n", sym->name, sym->start);
+				break;
+			}
+		if (i != j)
+			continue;
+
 		tev = (*tevs) + ret;
 		tp = &tev->point;
 		if (ret == num_matched_functions) {

[PATCH 2/4] perf-probe: Fix wrong variable warning when the probe point is not found

[Masami Hiramatsu]

Fix a wrong "variable not found" warning when the probe point is
not found in the debuginfo.
Since the debuginfo__find_probes() can return 0 even if it does not
find given probe point in the debuginfo, fill_empty_trace_arg() can
be called with tf.ntevs == 0 and it can warn a wrong warning.
To fix this, reject ntevs == 0 in fill_empty_trace_arg().

E.g. without this patch;

  # perf probe -x /lib64/libc-2.30.so -a "memcpy arg1=%di"
  Failed to find the location of the '%di' variable at this address.
   Perhaps it has been optimized out.
   Use -V with the --range option to show '%di' location range.
  Added new events:
    probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
    probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)

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

  	perf record -e probe_libc:memcpy -aR sleep 1

With this;

  # perf probe -x /lib64/libc-2.30.so -a "memcpy arg1=%di"
  Added new events:
    probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
    probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)

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

  	perf record -e probe_libc:memcpy -aR sleep 1


Reported-by: Andi Kleen <andi@firstfloor.org>
Fixes: cb4027308570 ("perf probe: Trace a magic number if variable is not found")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/probe-finder.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 55924255c535..9963e4e8ea20 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1408,6 +1408,9 @@ static int fill_empty_trace_arg(struct perf_probe_event *pev,
 	char *type;
 	int i, j, ret;
 
+	if (!ntevs)
+		return -ENOENT;
+
 	for (i = 0; i < pev->nargs; i++) {
 		type = NULL;
 		for (j = 0; j < ntevs; j++) {

[PATCH 3/4] perf-probe: Fix memory leakage when the probe point is not found

[Masami Hiramatsu]

Fix the memory leakage in debuginfo__find_trace_events() when the probe
point is not found in the debuginfo. If there is no probe point found in
the debuginfo, debuginfo__find_probes() will NOT return -ENOENT, but 0.
Thus the caller of debuginfo__find_probes() must check the tf.ntevs and
release the allocated memory for the array of struct probe_trace_event.

The current code releases the memory only if the debuginfo__find_probes()
hits an error but not checks tf.ntevs. In the result, the memory allocated
on *tevs are not released if tf.ntevs == 0.

This fixes the memory leakage by checking tf.ntevs == 0 in addition to
ret < 0.

Fixes: ff741783506c ("perf probe: Introduce debuginfo to encapsulate dwarf information")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/probe-finder.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 9963e4e8ea20..659024342e9a 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1467,7 +1467,7 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
 	if (ret >= 0 && tf.pf.skip_empty_arg)
 		ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
 
-	if (ret < 0) {
+	if (ret < 0 || tf.ntevs == 0) {
 		for (i = 0; i < tf.ntevs; i++)
 			clear_probe_trace_event(&tf.tevs[i]);
 		zfree(tevs);

[PATCH 4/4] perf-probe: Warn if the target function is GNU Indirect function

[Masami Hiramatsu]

Warn if the probe target function is GNU indirect function (GNU_IFUNC)
because it may not what the user want to probe.

The GNU indirect function ( https://sourceware.org/glibc/wiki/GNU_IFUNC )
is the dynamic solved symbol at runtime. IFUNC function is a selector
which is invoked from the elf loader, but the symbol address of the
function which will be modified by the IFUNC is same as the IFUNC in
the symbol table. This can confuse users who is trying to probe on
such functions.

For example, the memcpy is one of IFUNC.

# perf probe -x /lib64/libc-2.30.so -a memcpy
# perf probe -l
  probe_libc:memcpy    (on __new_memcpy_ifunc@x86_64/multiarch/memcpy.c in /usr/lib64/libc-2.30.so)

the probe is put on a IFUNC.

# perf record -e probe_libc:memcpy --call-graph dwarf -aR ./perf
# perf script
perf  1742 [000] 26201.715632: probe_libc:memcpy: (7fdaa53824c0)
            7fdaa53824c0 __new_memcpy_ifunc+0x0 (inlined)
            7fdaa5d4a980 elf_machine_rela+0x6c0 (inlined)
            7fdaa5d4a980 elf_dynamic_do_Rela+0x6c0 (inlined)
            7fdaa5d4a980 _dl_relocate_object+0x6c0 (/usr/lib64/ld-2.30.so)
            7fdaa5d42155 dl_main+0x1cc5 (/usr/lib64/ld-2.30.so)
            7fdaa5d5831a _dl_sysdep_start+0x54a (/usr/lib64/ld-2.30.so)
            7fdaa5d3ffeb _dl_start_final+0x25b (inlined)
            7fdaa5d3ffeb _dl_start+0x25b (/usr/lib64/ld-2.30.so)
            7fdaa5d3f117 .annobin_rtld.c+0x7 (inlined)
...

And the event is invoked from the elf loader instead of the target
program's main code.


Moreover, at this moment, we can not probe on the function which will
be selected by the IFUNC, because it is determined at runtime. But
uprobe will be prepared before running the target binary.

Thus, I decided to warn user when the perf probe detects the probe point
is on the GNU IFUNC symbol. Someone who wants to probe an IFUNC symbol to
debug the IFUNC function, they can ignore this warning.


Reported-by: Andi Kleen <andi@firstfloor.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/probe-event.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 1e95a336862c..671176d39569 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -379,6 +379,11 @@ static int find_alternative_probe_point(struct debuginfo *dinfo,
 			address = sym->start;
 		else
 			address = map->unmap_ip(map, sym->start) - map->reloc;
+		if (sym->type == STT_GNU_IFUNC) {
+			pr_warning("Warning: The probe address (0x%lx) is in a GNU indirect function.\n"
+				"This may not work as you expected unless you intend to probe the indirect function.\n",
+				(unsigned long)address);
+		}
 		break;
 	}
 	if (!address) {

Re: [PATCH 4/4] perf-probe: Warn if the target function is GNU Indirect function

[Andi Kleen]

> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 1e95a336862c..671176d39569 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -379,6 +379,11 @@ static int find_alternative_probe_point(struct debuginfo *dinfo,
>  			address = sym->start;
>  		else
>  			address = map->unmap_ip(map, sym->start) - map->reloc;
> +		if (sym->type == STT_GNU_IFUNC) {
> +			pr_warning("Warning: The probe address (0x%lx) is in a GNU indirect function.\n"
> +				"This may not work as you expected unless you intend to probe the indirect function.\n",

I would say something like this.

Consider identifying the final function used at run time and set the
probe directly on that.

I think that's more useful to the user.

-Andi

Re: [PATCH 2/4] perf-probe: Fix wrong variable warning when the probe point is not found

[Andi Kleen]

On Thu, Jul 09, 2020 at 05:07:12PM +0900, Masami Hiramatsu wrote:
> Fix a wrong "variable not found" warning when the probe point is
> not found in the debuginfo.
> Since the debuginfo__find_probes() can return 0 even if it does not
> find given probe point in the debuginfo, fill_empty_trace_arg() can
> be called with tf.ntevs == 0 and it can warn a wrong warning.
> To fix this, reject ntevs == 0 in fill_empty_trace_arg().
> 
> E.g. without this patch;
> 
>   # perf probe -x /lib64/libc-2.30.so -a "memcpy arg1=%di"
>   Failed to find the location of the '%di' variable at this address.
>    Perhaps it has been optimized out.
>    Use -V with the --range option to show '%di' location range.
>   Added new events:
>     probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
>     probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
> 
>   You can now use it in all perf tools, such as:
> 
>   	perf record -e probe_libc:memcpy -aR sleep 1
> 
> With this;
> 
>   # perf probe -x /lib64/libc-2.30.so -a "memcpy arg1=%di"
>   Added new events:
>     probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
>     probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
> 
>   You can now use it in all perf tools, such as:
> 
>   	perf record -e probe_libc:memcpy -aR sleep 1
> 
> 
> Reported-by: Andi Kleen <andi@firstfloor.org>
> Fixes: cb4027308570 ("perf probe: Trace a magic number if variable is not found")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Tested-by: Andi Kleen <ak@linux.intel.com>

Except for the minor nit on the message all patches look good to me.

-Andi

Re: [PATCH 4/4] perf-probe: Warn if the target function is GNU Indirect function

[Masami Hiramatsu]

On Thu, 9 Jul 2020 07:36:54 -0700
Andi Kleen <andi@firstfloor.org> wrote:

> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 1e95a336862c..671176d39569 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -379,6 +379,11 @@ static int find_alternative_probe_point(struct debuginfo *dinfo,
> >  			address = sym->start;
> >  		else
> >  			address = map->unmap_ip(map, sym->start) - map->reloc;
> > +		if (sym->type == STT_GNU_IFUNC) {
> > +			pr_warning("Warning: The probe address (0x%lx) is in a GNU indirect function.\n"
> > +				"This may not work as you expected unless you intend to probe the indirect function.\n",
> 
> I would say something like this.
> 
> Consider identifying the final function used at run time and set the
> probe directly on that.
> 
> I think that's more useful to the user.

Hmm, would you mean the default function which may be used for the symbol?
Let me check how we can find it.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 4/4] perf-probe: Warn if the target function is GNU Indirect function

[Srikar Dronamraju]

* Masami Hiramatsu <mhiramat@kernel.org> [2020-07-09 17:07:31]:

> Reported-by: Andi Kleen <andi@firstfloor.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/perf/util/probe-event.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 1e95a336862c..671176d39569 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -379,6 +379,11 @@ static int find_alternative_probe_point(struct debuginfo *dinfo,
>  			address = sym->start;
>  		else
>  			address = map->unmap_ip(map, sym->start) - map->reloc;
> +		if (sym->type == STT_GNU_IFUNC) {
> +			pr_warning("Warning: The probe address (0x%lx) is in a GNU indirect function.\n"
> +				"This may not work as you expected unless you intend to probe the indirect function.\n",
> +				(unsigned long)address);
> +		}

Are these GNU indirect functions possible in kernel? If not we could move
this warning under if (uprobes)

Also instead of printing the address, can we print the pp->function?

>  		break;
>  	}
>  	if (!address) {
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 2/4] perf-probe: Fix wrong variable warning when the probe point is not found

[Srikar Dronamraju]

* Masami Hiramatsu <mhiramat@kernel.org> [2020-07-09 17:07:12]:

> Fix a wrong "variable not found" warning when the probe point is
> not found in the debuginfo.
> Since the debuginfo__find_probes() can return 0 even if it does not
> find given probe point in the debuginfo, fill_empty_trace_arg() can
> be called with tf.ntevs == 0 and it can warn a wrong warning.
> To fix this, reject ntevs == 0 in fill_empty_trace_arg().
> 
> 
> 
> Reported-by: Andi Kleen <andi@firstfloor.org>
> Fixes: cb4027308570 ("perf probe: Trace a magic number if variable is not found")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Looks good to me.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  tools/perf/util/probe-finder.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 55924255c535..9963e4e8ea20 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1408,6 +1408,9 @@ static int fill_empty_trace_arg(struct perf_probe_event *pev,
>  	char *type;
>  	int i, j, ret;
> 
> +	if (!ntevs)
> +		return -ENOENT;
> +
>  	for (i = 0; i < pev->nargs; i++) {
>  		type = NULL;
>  		for (j = 0; j < ntevs; j++) {
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 3/4] perf-probe: Fix memory leakage when the probe point is not found

[Srikar Dronamraju]

* Masami Hiramatsu <mhiramat@kernel.org> [2020-07-09 17:07:22]:

> Fix the memory leakage in debuginfo__find_trace_events() when the probe
> point is not found in the debuginfo. If there is no probe point found in
> the debuginfo, debuginfo__find_probes() will NOT return -ENOENT, but 0.
> Thus the caller of debuginfo__find_probes() must check the tf.ntevs and
> release the allocated memory for the array of struct probe_trace_event.
> 
> The current code releases the memory only if the debuginfo__find_probes()
> hits an error but not checks tf.ntevs. In the result, the memory allocated
> on *tevs are not released if tf.ntevs == 0.
> 
> This fixes the memory leakage by checking tf.ntevs == 0 in addition to
> ret < 0.
> 
> Fixes: ff741783506c ("perf probe: Introduce debuginfo to encapsulate dwarf information")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>


Looks good to me.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  tools/perf/util/probe-finder.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 9963e4e8ea20..659024342e9a 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1467,7 +1467,7 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
>  	if (ret >= 0 && tf.pf.skip_empty_arg)
>  		ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> 
> -	if (ret < 0) {
> +	if (ret < 0 || tf.ntevs == 0) {
>  		for (i = 0; i < tf.ntevs; i++)
>  			clear_probe_trace_event(&tf.tevs[i]);
>  		zfree(tevs);
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 1/4] perf-probe: Avoid setting probes on same address on same event

[Srikar Dronamraju]

* Masami Hiramatsu <mhiramat@kernel.org> [2020-07-09 17:07:01]:

> There is a case that the several same-name symbols points
> same address. In that case, perf probe returns an error.
> 
> E.g.
> 
> With this patch;
> 
>   # perf probe -x /lib64/libc-2.30.so -a "memcpy arg1=%di"
>   Failed to find the location of the '%di' variable at this address.
>    Perhaps it has been optimized out.
>    Use -V with the --range option to show '%di' location range.
>   Added new events:
>     probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
>     probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
> 
>   You can now use it in all perf tools, such as:
> 
>   	perf record -e probe_libc:memcpy -aR sleep 1
> 
> 
> Reported-by: Andi Kleen <andi@firstfloor.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

One small nit:

> ---
>  tools/perf/util/probe-event.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index df713a5d1e26..1e95a336862c 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2968,6 +2968,15 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	for (j = 0; j < num_matched_functions; j++) {
>  		sym = syms[j];
> 
> +		/* There can be duplicated symbols in the map */
> +		for (i = 0; i < j; i++)
> +			if (sym->start == syms[i]->start) {
> +				pr_debug("find duplicated symbol %s @ %lx\n", sym->name, sym->start);
	
Would "Found" sound better than "find"?

> +				break;
> +			}
> +		if (i != j)
> +			continue;
> +

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>


>  		tev = (*tevs) + ret;
>  		tp = &tev->point;
>  		if (ret == num_matched_functions) {
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 4/4] perf-probe: Warn if the target function is GNU Indirect function

[Arnaldo Carvalho de Melo]

Em Fri, Jul 10, 2020 at 12:30:08PM +0900, Masami Hiramatsu escreveu:
> On Thu, 9 Jul 2020 07:36:54 -0700
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > > index 1e95a336862c..671176d39569 100644
> > > --- a/tools/perf/util/probe-event.c
> > > +++ b/tools/perf/util/probe-event.c
> > > @@ -379,6 +379,11 @@ static int find_alternative_probe_point(struct debuginfo *dinfo,
> > >  			address = sym->start;
> > >  		else
> > >  			address = map->unmap_ip(map, sym->start) - map->reloc;
> > > +		if (sym->type == STT_GNU_IFUNC) {
> > > +			pr_warning("Warning: The probe address (0x%lx) is in a GNU indirect function.\n"
> > > +				"This may not work as you expected unless you intend to probe the indirect function.\n",
> > 
> > I would say something like this.
> > 
> > Consider identifying the final function used at run time and set the
> > probe directly on that.
> > 
> > I think that's more useful to the user.
> 
> Hmm, would you mean the default function which may be used for the symbol?

Humm, I think he means that the user must somehow, knowing details
involved in picking the final function, probe that one instead of the
IFUNC one, right Andi?

- Arnaldo

> Let me check how we can find it.

Re: [PATCH 4/4] perf-probe: Warn if the target function is GNU Indirect function

[Masami Hiramatsu]

On Fri, 10 Jul 2020 16:45:12 +0530
Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> * Masami Hiramatsu <mhiramat@kernel.org> [2020-07-09 17:07:31]:
> 
> > Reported-by: Andi Kleen <andi@firstfloor.org>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  tools/perf/util/probe-event.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 1e95a336862c..671176d39569 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -379,6 +379,11 @@ static int find_alternative_probe_point(struct debuginfo *dinfo,
> >  			address = sym->start;
> >  		else
> >  			address = map->unmap_ip(map, sym->start) - map->reloc;
> > +		if (sym->type == STT_GNU_IFUNC) {
> > +			pr_warning("Warning: The probe address (0x%lx) is in a GNU indirect function.\n"
> > +				"This may not work as you expected unless you intend to probe the indirect function.\n",
> > +				(unsigned long)address);
> > +		}
> 
> Are these GNU indirect functions possible in kernel? If not we could move
> this warning under if (uprobes)

OK, I'll move it under if (uprobes).

> 
> Also instead of printing the address, can we print the pp->function?

Hmm, OK. But it may not help user because the pp->function will the name
specified by user...

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/4] perf-probe: Avoid setting probes on same address on same event

[Masami Hiramatsu]

On Fri, 10 Jul 2020 16:48:47 +0530
Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> * Masami Hiramatsu <mhiramat@kernel.org> [2020-07-09 17:07:01]:
> 
> > There is a case that the several same-name symbols points
> > same address. In that case, perf probe returns an error.
> > 
> > E.g.
> > 
> > With this patch;
> > 
> >   # perf probe -x /lib64/libc-2.30.so -a "memcpy arg1=%di"
> >   Failed to find the location of the '%di' variable at this address.
> >    Perhaps it has been optimized out.
> >    Use -V with the --range option to show '%di' location range.
> >   Added new events:
> >     probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
> >     probe_libc:memcpy    (on memcpy in /usr/lib64/libc-2.30.so with arg1=%di)
> > 
> >   You can now use it in all perf tools, such as:
> > 
> >   	perf record -e probe_libc:memcpy -aR sleep 1
> > 
> > 
> > Reported-by: Andi Kleen <andi@firstfloor.org>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> One small nit:
> 
> > ---
> >  tools/perf/util/probe-event.c |    9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index df713a5d1e26..1e95a336862c 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -2968,6 +2968,15 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
> >  	for (j = 0; j < num_matched_functions; j++) {
> >  		sym = syms[j];
> > 
> > +		/* There can be duplicated symbols in the map */
> > +		for (i = 0; i < j; i++)
> > +			if (sym->start == syms[i]->start) {
> > +				pr_debug("find duplicated symbol %s @ %lx\n", sym->name, sym->start);
> 	
> Would "Found" sound better than "find"?

OK.

> 
> > +				break;
> > +			}
> > +		if (i != j)
> > +			continue;
> > +
> 
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thank you!

> 
> 
> >  		tev = (*tevs) + ret;
> >  		tp = &tev->point;
> >  		if (ret == num_matched_functions) {
> > 
> 
> -- 
> Thanks and Regards
> Srikar Dronamraju


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 4/4] perf-probe: Warn if the target function is GNU Indirect function

[Masami Hiramatsu]

On Fri, 10 Jul 2020 08:55:40 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Fri, Jul 10, 2020 at 12:30:08PM +0900, Masami Hiramatsu escreveu:
> > On Thu, 9 Jul 2020 07:36:54 -0700
> > Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > > > index 1e95a336862c..671176d39569 100644
> > > > --- a/tools/perf/util/probe-event.c
> > > > +++ b/tools/perf/util/probe-event.c
> > > > @@ -379,6 +379,11 @@ static int find_alternative_probe_point(struct debuginfo *dinfo,
> > > >  			address = sym->start;
> > > >  		else
> > > >  			address = map->unmap_ip(map, sym->start) - map->reloc;
> > > > +		if (sym->type == STT_GNU_IFUNC) {
> > > > +			pr_warning("Warning: The probe address (0x%lx) is in a GNU indirect function.\n"
> > > > +				"This may not work as you expected unless you intend to probe the indirect function.\n",
> > > 
> > > I would say something like this.
> > > 
> > > Consider identifying the final function used at run time and set the
> > > probe directly on that.
> > > 
> > > I think that's more useful to the user.
> > 
> > Hmm, would you mean the default function which may be used for the symbol?
> 
> Humm, I think he means that the user must somehow, knowing details
> involved in picking the final function, probe that one instead of the
> IFUNC one, right Andi?

Ah, I got it. OK, I'll update the message :)

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>