[PATCH -tip v2 1/2] [CLEANUP] perf-probe: Expand given path to absolute path

[PATCH -tip v2 1/2] [CLEANUP] perf-probe: Expand given path to absolute path

[Masami Hiramatsu]

Expand given path to absolute path in the option parser,
except for a module name.

Since realpath at later stage in processing several probe
point, can be called several times(even if currently doesn't,
it can happen when we expands the feature), it is waste of the
performance.
Processing it once at the early stage can avoid that.

Changes from previous one:
 - Fix not to print null string.
 - Allocate memory for given path/module name everytime.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/builtin-probe.c    |   16 +++++++++++++++-
 tools/perf/util/probe-event.c |   11 ++---------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 6ea9e85..5681266 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -169,6 +169,7 @@ static int opt_set_target(const struct option *opt, const char *str,
 			int unset __maybe_unused)
 {
 	int ret = -ENOENT;
+	char *tmp;
 
 	if  (str && !params.target) {
 		if (!strcmp(opt->long_name, "exec"))
@@ -180,7 +181,20 @@ static int opt_set_target(const struct option *opt, const char *str,
 		else
 			return ret;
 
-		params.target = str;
+		/* Expand given path to absolute path, except for modulename */
+		if (params.uprobes || strchr(str, '/')) {
+			tmp = realpath(str, NULL);
+			if (!tmp) {
+				pr_warning("Failed to find the path of %s.\n",
+					   str);
+				return ret;
+			}
+		} else {
+			tmp = strdup(str);
+			if (!tmp)
+				return -ENOMEM;
+		}
+		params.target = tmp;
 		ret = 0;
 	}
 
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index d7cff57..05be5de 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2281,7 +2281,7 @@ static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec)
 	struct perf_probe_point *pp = &pev->point;
 	struct symbol *sym;
 	struct map *map = NULL;
-	char *function = NULL, *name = NULL;
+	char *function = NULL;
 	int ret = -EINVAL;
 	unsigned long long vaddr = 0;
 
@@ -2297,12 +2297,7 @@ static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec)
 		goto out;
 	}
 
-	name = realpath(exec, NULL);
-	if (!name) {
-		pr_warning("Cannot find realpath for %s.\n", exec);
-		goto out;
-	}
-	map = dso__new_map(name);
+	map = dso__new_map(exec);
 	if (!map) {
 		pr_warning("Cannot find appropriate DSO for %s.\n", exec);
 		goto out;
@@ -2367,7 +2362,5 @@ out:
 	}
 	if (function)
 		free(function);
-	if (name)
-		free(name);
 	return ret;
 }

[PATCH -tip v2 2/2] perf-probe: Support basic dwarf-based operations on uprobe events

[Masami Hiramatsu]

Support basic dwarf(debuginfo) based operations for uprobe events.
With this change, perf probe can analyze debuginfo of user
application binary to set up new uprobe event.
This allows perf-probe --add(with local variables, line numbers)
and --line works with -x option.
(Actually, --vars has already accepted -x option)

For example, the following command shows the probe-able lines of
a given user space function. Something that so far was only
available in the 'perf probe' tool for kernel space functions:
----
# ./perf probe -x perf --line map__load
<map__load@/home/fedora/ksrc/linux-2.6/tools/perf/util/map.c:0>
      0  int map__load(struct map *map, symbol_filter_t filter)
      1  {
      2         const char *name = map->dso->long_name;
                int nr;

      5         if (dso__loaded(map->dso, map->type))
      6                 return 0;

      8         nr = dso__load(map->dso, map, filter);
      9         if (nr < 0) {
     10                 if (map->dso->has_build_id) {
----

And this shows the available variables at the given line of
the function.
----
# ./perf probe -x perf --vars map__load:8
Available variables at map__load:8
        @<map__load+96>
                char*   name
                struct map*     map
                symbol_filter_t filter
        @<map__find_symbol+112>
                char*   name
                symbol_filter_t filter
        @<map__find_symbol_by_name+136>
                char*   name
                symbol_filter_t filter
        @<map_groups__find_symbol_by_name+176>
                char*   name
                struct map*     map
                symbol_filter_t filter
----

At last, we can now define a probe(s) with all available
variables on the given line.
----
# ./perf probe -x perf --add 'map__load:8 $vars'

Added new events:
  probe_perf:map__load (on map__load:8 with $vars)
  probe_perf:map__load_1 (on map__load:8 with $vars)
  probe_perf:map__load_2 (on map__load:8 with $vars)
  probe_perf:map__load_3 (on map__load:8 with $vars)

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

        perf record -e probe_perf:map__load_3 -aR sleep 1
----

Changes from previous version:
 - Add examples in the patch description.
 - Use .text section start address and dwarf symbol address
   for calculating the offset of given symbol, instead of
   searching the symbol in symtab again.
   With this change, we can safely handle multiple local
   function instances (e.g. scnprintf in perf).

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 tools/perf/builtin-probe.c     |    2 -
 tools/perf/util/probe-event.c  |  151 ++++++++++++++++++++++++++++++++++++----
 tools/perf/util/probe-event.h  |    1 
 tools/perf/util/probe-finder.c |    1 
 4 files changed, 138 insertions(+), 17 deletions(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 5681266..20b3e15 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -425,7 +425,7 @@ int cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	}
 
 #ifdef HAVE_DWARF_SUPPORT
-	if (params.show_lines && !params.uprobes) {
+	if (params.show_lines) {
 		if (params.mod_events) {
 			pr_err("  Error: Don't use --line with"
 			       " --add/--del.\n");
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 05be5de..2f82267 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -172,6 +172,52 @@ const char *kernel_get_module_path(const char *module)
 	return (dso) ? dso->long_name : NULL;
 }
 
+/* Copied from unwind.c */
+static Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
+				    GElf_Shdr *shp, const char *name)
+{
+	Elf_Scn *sec = NULL;
+
+	while ((sec = elf_nextscn(elf, sec)) != NULL) {
+		char *str;
+
+		gelf_getshdr(sec, shp);
+		str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
+		if (!strcmp(name, str))
+			break;
+	}
+
+	return sec;
+}
+
+static int get_text_start_address(const char *exec, unsigned long *address)
+{
+	Elf *elf;
+	GElf_Ehdr ehdr;
+	GElf_Shdr shdr;
+	int fd, ret = -ENOENT;
+
+	fd = open(exec, O_RDONLY);
+	if (fd < 0)
+		return -errno;
+
+	elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
+	if (elf == NULL)
+		return -EINVAL;
+
+	if (gelf_getehdr(elf, &ehdr) == NULL)
+		goto out;
+
+	if (!elf_section_by_name(elf, &ehdr, &shdr, ".text"))
+		goto out;
+
+	*address = shdr.sh_addr - shdr.sh_offset;
+	ret = 0;
+out:
+	elf_end(elf);
+	return ret;
+}
+
 static int init_user_exec(void)
 {
 	int ret = 0;
@@ -186,6 +232,37 @@ static int init_user_exec(void)
 	return ret;
 }
 
+static int convert_exec_to_group(const char *exec, char **result)
+{
+	char *ptr1, *ptr2, *exec_copy;
+	char buf[64];
+	int ret;
+
+	exec_copy = strdup(exec);
+	if (!exec_copy)
+		return -ENOMEM;
+
+	ptr1 = basename(exec_copy);
+	if (!ptr1) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ptr2 = strpbrk(ptr1, "-._");
+	if (ptr2)
+		*ptr2 = '\0';
+	ret = e_snprintf(buf, 64, "%s_%s", PERFPROBE_GROUP, ptr1);
+	if (ret < 0)
+		goto out;
+
+	*result = strdup(buf);
+	ret = *result ? 0 : -ENOMEM;
+
+out:
+	free(exec_copy);
+	return ret;
+}
+
 static int convert_to_perf_probe_point(struct probe_trace_point *tp,
 					struct perf_probe_point *pp)
 {
@@ -261,6 +338,40 @@ static int kprobe_convert_to_perf_probe(struct probe_trace_point *tp,
 	return 0;
 }
 
+static int add_exec_to_probe_trace_events(struct probe_trace_event *tevs,
+					  int ntevs, const char *exec)
+{
+	int i, ret = 0;
+	unsigned long offset, stext = 0;
+	char buf[32];
+
+	if (!exec)
+		return 0;
+
+	ret = get_text_start_address(exec, &stext);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < ntevs && ret >= 0; i++) {
+		offset = tevs[i].point.address - stext;
+		offset += tevs[i].point.offset;
+		tevs[i].point.offset = 0;
+		free(tevs[i].point.symbol);
+		ret = e_snprintf(buf, 32, "0x%lx", offset);
+		if (ret < 0)
+			break;
+		tevs[i].point.module = strdup(exec);
+		tevs[i].point.symbol = strdup(buf);
+		if (!tevs[i].point.symbol || !tevs[i].point.module) {
+			ret = -ENOMEM;
+			break;
+		}
+		tevs[i].uprobes = true;
+	}
+
+	return ret;
+}
+
 static int add_module_to_probe_trace_events(struct probe_trace_event *tevs,
 					    int ntevs, const char *module)
 {
@@ -305,15 +416,6 @@ static int try_to_find_probe_trace_events(struct perf_probe_event *pev,
 	struct debuginfo *dinfo;
 	int ntevs, ret = 0;
 
-	if (pev->uprobes) {
-		if (need_dwarf) {
-			pr_warning("Debuginfo-analysis is not yet supported"
-					" with -x/--exec option.\n");
-			return -ENOSYS;
-		}
-		return convert_name_to_addr(pev, target);
-	}
-
 	dinfo = open_debuginfo(target);
 
 	if (!dinfo) {
@@ -332,9 +434,14 @@ static int try_to_find_probe_trace_events(struct perf_probe_event *pev,
 
 	if (ntevs > 0) {	/* Succeeded to find trace events */
 		pr_debug("find %d probe_trace_events.\n", ntevs);
-		if (target)
-			ret = add_module_to_probe_trace_events(*tevs, ntevs,
-							       target);
+		if (target) {
+			if (pev->uprobes)
+				ret = add_exec_to_probe_trace_events(*tevs,
+						 ntevs, target);
+			else
+				ret = add_module_to_probe_trace_events(*tevs,
+						 ntevs, target);
+		}
 		return ret < 0 ? ret : ntevs;
 	}
 
@@ -654,9 +761,6 @@ static int try_to_find_probe_trace_events(struct perf_probe_event *pev,
 		return -ENOSYS;
 	}
 
-	if (pev->uprobes)
-		return convert_name_to_addr(pev, target);
-
 	return 0;
 }
 
@@ -1913,14 +2017,29 @@ static int convert_to_probe_trace_events(struct perf_probe_event *pev,
 					  int max_tevs, const char *target)
 {
 	struct symbol *sym;
-	int ret = 0, i;
+	int ret, i;
 	struct probe_trace_event *tev;
 
+	if (pev->uprobes && !pev->group) {
+		/* Replace group name if not given */
+		ret = convert_exec_to_group(target, &pev->group);
+		if (ret != 0) {
+			pr_warning("Failed to make a group name.\n");
+			return ret;
+		}
+	}
+
 	/* Convert perf_probe_event with debuginfo */
 	ret = try_to_find_probe_trace_events(pev, tevs, max_tevs, target);
 	if (ret != 0)
 		return ret;	/* Found in debuginfo or got an error */
 
+	if (pev->uprobes) {
+		ret = convert_name_to_addr(pev, target);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Allocate trace event buffer */
 	tev = *tevs = zalloc(sizeof(struct probe_trace_event));
 	if (tev == NULL)
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index f9f3de8..d481c46 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -12,6 +12,7 @@ struct probe_trace_point {
 	char		*symbol;	/* Base symbol */
 	char		*module;	/* Module name */
 	unsigned long	offset;		/* Offset from symbol */
+	unsigned long	address;	/* Actual address of the trace point */
 	bool		retprobe;	/* Return probe flag */
 };
 
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index ffb657f..7db7e05 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -729,6 +729,7 @@ static int convert_to_trace_point(Dwarf_Die *sp_die, Dwfl_Module *mod,
 		return -ENOENT;
 	}
 	tp->offset = (unsigned long)(paddr - sym.st_value);
+	tp->address = (unsigned long)paddr;
 	tp->symbol = strdup(symbol);
 	if (!tp->symbol)
 		return -ENOMEM;

[PATCH -tip v2 0/2] perf-probe: Dwarf support for uprobes

[Masami Hiramatsu]

Hi,

Here is the 2nd version of the series of perf-probe patches
which adds basic dwarf(debuginfo) support for uprobes.

Currently perf-probe doesn't support debuginfo for uprobes.
The lack of the debuginfo support for uprobes sometimes
confuses users (or makes them cry) because they can't use
perf-probe as for kprobes case, and that is not what I hope.

So I tried to add debuginfo support for uprobes on
perf-probe. Actually, this is not completely done yet.
We still need to add some features for make it perfect.
However, this series can provide minimum debuginfo
support for uprobes.

For example, the following command shows the probe-able lines of
a given user space function. Something that so far was only
available in the 'perf probe' tool for kernel space functions:
----
# ./perf probe -x perf --line map__load
<map__load@/home/fedora/ksrc/linux-2.6/tools/perf/util/map.c:0>
      0  int map__load(struct map *map, symbol_filter_t filter)
      1  {
      2         const char *name = map->dso->long_name;
                int nr;

      5         if (dso__loaded(map->dso, map->type))
      6                 return 0;

      8         nr = dso__load(map->dso, map, filter);
      9         if (nr < 0) {
     10                 if (map->dso->has_build_id) {
----

And this shows the available variables at the given line of
the function.
----
# ./perf probe -x perf --vars map__load:8
Available variables at map__load:8
        @<map__load+96>
                char*   name
                struct map*     map
                symbol_filter_t filter
        @<map__find_symbol+112>
                char*   name
                symbol_filter_t filter
        @<map__find_symbol_by_name+136>
                char*   name
                symbol_filter_t filter
        @<map_groups__find_symbol_by_name+176>
                char*   name
                struct map*     map
                symbol_filter_t filter
----

At last, we can now define a probe(s) with all available
variables on the given line.
----
# ./perf probe -x perf --add 'map__load:8 $vars'

Added new events:
  probe_perf:map__load (on map__load:8 with $vars)
  probe_perf:map__load_1 (on map__load:8 with $vars)
  probe_perf:map__load_2 (on map__load:8 with $vars)
  probe_perf:map__load_3 (on map__load:8 with $vars)

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

        perf record -e probe_perf:map__load_3 -aR sleep 1
----

To complete this requires Namhyung's uprobe
fetch-method updates which is almost done on LKML.

TODO:
 - Convert data symbol name in arguments to address
   offset value.
 - Support distro style debuginfo path (/usr/lib/debug/...)
 - Support --list to show actual lines and executable names.

Changes from previous one:
 - Fix not to print null string.
 - Allocate memory for given path/module name everytime.
 - Fix patch descriptions.
 - Add examples in the patch description.
 - Use .text section start address and dwarf symbol address
   for calculating the offset of given symbol, instead of
   searching the symbol in symtab again.
   With this change, we can safely handle multiple local
   function instances (e.g. scnprintf in perf).

---

Masami Hiramatsu (2):
      [CLEANUP] perf-probe: Expand given path to absolute path
      perf-probe: Support basic dwarf-based operations on uprobe events


 tools/perf/builtin-probe.c     |   18 ++++
 tools/perf/util/probe-event.c  |  162 ++++++++++++++++++++++++++++++++++------
 tools/perf/util/probe-event.h  |    1 
 tools/perf/util/probe-finder.c |    1 
 4 files changed, 155 insertions(+), 27 deletions(-)

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

Re: [PATCH -tip v2 1/2] [CLEANUP] perf-probe: Expand given path to absolute path

[David Ahern]

On 12/26/13, 12:41 AM, Masami Hiramatsu wrote:
> @@ -180,7 +181,20 @@ static int opt_set_target(const struct option *opt, const char *str,
>   		else
>   			return ret;
>
> -		params.target = str;
> +		/* Expand given path to absolute path, except for modulename */
> +		if (params.uprobes || strchr(str, '/')) {
> +			tmp = realpath(str, NULL);
> +			if (!tmp) {
> +				pr_warning("Failed to find the path of %s.\n",
> +					   str);

That error message will be misleading if it is generated. How about:
     Failed to get the absolute path of %s: %d\n", str, errno.

> +				return ret;
> +			}
> +		} else {
> +			tmp = strdup(str);
> +			if (!tmp)
> +				return -ENOMEM;
> +		}
> +		params.target = tmp;

When is params.target freed?

David

Re: [PATCH -tip v2 1/2] [CLEANUP] perf-probe: Expand given path to absolute path

[Arnaldo Carvalho de Melo]

Em Thu, Dec 26, 2013 at 09:14:46AM -0500, David Ahern escreveu:
> On 12/26/13, 12:41 AM, Masami Hiramatsu wrote:
> >@@ -180,7 +181,20 @@ static int opt_set_target(const struct option *opt, const char *str,
> >  		else
> >  			return ret;
> >
> >-		params.target = str;
> >+		/* Expand given path to absolute path, except for modulename */
> >+		if (params.uprobes || strchr(str, '/')) {
> >+			tmp = realpath(str, NULL);
> >+			if (!tmp) {
> >+				pr_warning("Failed to find the path of %s.\n",
> >+					   str);
> 
> That error message will be misleading if it is generated. How about:
>     Failed to get the absolute path of %s: %d\n", str, errno.

Changed it to:

 pr_warning("Failed to get the absolute path of %s: %m\n", str);

 
> >+				return ret;
> >+			}
> >+		} else {
> >+			tmp = strdup(str);
> >+			if (!tmp)
> >+				return -ENOMEM;
> >+		}
> >+		params.target = tmp;
> 
> When is params.target freed?

> David

Re: [PATCH -tip v2 2/2] perf-probe: Support basic dwarf-based operations on uprobe events

[David Ahern]

On 12/26/13, 12:41 AM, Masami Hiramatsu wrote:
> And this shows the available variables at the given line of
> the function.
> ----
> # ./perf probe -x perf --vars map__load:8
> Available variables at map__load:8
>          @<map__load+96>
>                  char*   name
>                  struct map*     map
>                  symbol_filter_t filter
>          @<map__find_symbol+112>
>                  char*   name
>                  symbol_filter_t filter
>          @<map__find_symbol_by_name+136>
>                  char*   name
>                  symbol_filter_t filter
>          @<map_groups__find_symbol_by_name+176>
>                  char*   name
>                  struct map*     map
>                  symbol_filter_t filter

Still limitations. This is Fedora 18:

# rpm -qa | grep debug
glibc-debuginfo-common-2.16-34.fc18.x86_64
glibc-debuginfo-2.16-34.fc18.x86_64

# /tmp/perf/perf probe -V malloc -x /lib64/libc-2.16.so
Failed to find variables at malloc (0)

So probing on system libraries does not benefit from this patch.

> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> index 5681266..20b3e15 100644
> --- a/tools/perf/builtin-probe.c
> +++ b/tools/perf/builtin-probe.c
> @@ -425,7 +425,7 @@ int cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
>   	}
>
>   #ifdef HAVE_DWARF_SUPPORT
> -	if (params.show_lines && !params.uprobes) {
> +	if (params.show_lines) {
>   		if (params.mod_events) {
>   			pr_err("  Error: Don't use --line with"
>   			       " --add/--del.\n");

Unrelated change.


> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 05be5de..2f82267 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -172,6 +172,52 @@ const char *kernel_get_module_path(const char *module)
>   	return (dso) ? dso->long_name : NULL;
>   }
>
> +/* Copied from unwind.c */
> +static Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
> +				    GElf_Shdr *shp, const char *name)
> +{
> +	Elf_Scn *sec = NULL;
> +
> +	while ((sec = elf_nextscn(elf, sec)) != NULL) {
> +		char *str;
> +
> +		gelf_getshdr(sec, shp);
> +		str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
> +		if (!strcmp(name, str))
> +			break;
> +	}
> +
> +	return sec;
> +}

Why copy it? With unwind.c and util/symbol-elf.c we now have 2 copies. 
How about exporting one of those?

> +
> +static int get_text_start_address(const char *exec, unsigned long *address)
> +{
> +	Elf *elf;
> +	GElf_Ehdr ehdr;
> +	GElf_Shdr shdr;
> +	int fd, ret = -ENOENT;
> +
> +	fd = open(exec, O_RDONLY);
> +	if (fd < 0)
> +		return -errno;
> +
> +	elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
> +	if (elf == NULL)
> +		return -EINVAL;
> +
> +	if (gelf_getehdr(elf, &ehdr) == NULL)
> +		goto out;
> +
> +	if (!elf_section_by_name(elf, &ehdr, &shdr, ".text"))
> +		goto out;
> +
> +	*address = shdr.sh_addr - shdr.sh_offset;
> +	ret = 0;
> +out:
> +	elf_end(elf);
> +	return ret;
> +}
> +
>   static int init_user_exec(void)
>   {
>   	int ret = 0;
> @@ -186,6 +232,37 @@ static int init_user_exec(void)
>   	return ret;
>   }
>
> +static int convert_exec_to_group(const char *exec, char **result)
> +{
> +	char *ptr1, *ptr2, *exec_copy;
> +	char buf[64];
> +	int ret;
> +
> +	exec_copy = strdup(exec);
> +	if (!exec_copy)
> +		return -ENOMEM;
> +
> +	ptr1 = basename(exec_copy);
> +	if (!ptr1) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ptr2 = strpbrk(ptr1, "-._");
> +	if (ptr2)
> +		*ptr2 = '\0';
> +	ret = e_snprintf(buf, 64, "%s_%s", PERFPROBE_GROUP, ptr1);
> +	if (ret < 0)
> +		goto out;
> +
> +	*result = strdup(buf);
> +	ret = *result ? 0 : -ENOMEM;
> +
> +out:
> +	free(exec_copy);
> +	return ret;
> +}
> +
>   static int convert_to_perf_probe_point(struct probe_trace_point *tp,
>   					struct perf_probe_point *pp)
>   {
> @@ -261,6 +338,40 @@ static int kprobe_convert_to_perf_probe(struct probe_trace_point *tp,
>   	return 0;
>   }
>
> +static int add_exec_to_probe_trace_events(struct probe_trace_event *tevs,
> +					  int ntevs, const char *exec)
> +{
> +	int i, ret = 0;
> +	unsigned long offset, stext = 0;
> +	char buf[32];
> +
> +	if (!exec)
> +		return 0;
> +
> +	ret = get_text_start_address(exec, &stext);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ntevs && ret >= 0; i++) {
> +		offset = tevs[i].point.address - stext;
> +		offset += tevs[i].point.offset;
> +		tevs[i].point.offset = 0;
> +		free(tevs[i].point.symbol);
> +		ret = e_snprintf(buf, 32, "0x%lx", offset);
> +		if (ret < 0)
> +			break;
> +		tevs[i].point.module = strdup(exec);
> +		tevs[i].point.symbol = strdup(buf);
> +		if (!tevs[i].point.symbol || !tevs[i].point.module) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +		tevs[i].uprobes = true;
> +	}
> +
> +	return ret;
> +}
> +

More strdup's. This is library code and we need methods to free that 
memory as well.

David

Re: [PATCH -tip v2 2/2] perf-probe: Support basic dwarf-based operations on uprobe events

[Arnaldo Carvalho de Melo]

Em Thu, Dec 26, 2013 at 09:38:02AM -0500, David Ahern escreveu:
> On 12/26/13, 12:41 AM, Masami Hiramatsu wrote:
> >And this shows the available variables at the given line of
> >the function.
> >----
> ># ./perf probe -x perf --vars map__load:8
> >Available variables at map__load:8
> >         @<map__load+96>
> >                 char*   name
> >                 struct map*     map
> >                 symbol_filter_t filter
> >         @<map__find_symbol+112>
> >                 char*   name
> >                 symbol_filter_t filter
> >         @<map__find_symbol_by_name+136>
> >                 char*   name
> >                 symbol_filter_t filter
> >         @<map_groups__find_symbol_by_name+176>
> >                 char*   name
> >                 struct map*     map
> >                 symbol_filter_t filter
> 
> Still limitations. This is Fedora 18:
> 
> # rpm -qa | grep debug
> glibc-debuginfo-common-2.16-34.fc18.x86_64
> glibc-debuginfo-2.16-34.fc18.x86_64
> 
> # /tmp/perf/perf probe -V malloc -x /lib64/libc-2.16.so
> Failed to find variables at malloc (0)
> 
> So probing on system libraries does not benefit from this patch.

Yeah, it needs to use symbol.c functions to find the right file to get
the detached DWARF info, just like annotate, etc does.

- Arnaldo
 
> >diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> >index 5681266..20b3e15 100644
> >--- a/tools/perf/builtin-probe.c
> >+++ b/tools/perf/builtin-probe.c
> >@@ -425,7 +425,7 @@ int cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> >  	}
> >
> >  #ifdef HAVE_DWARF_SUPPORT
> >-	if (params.show_lines && !params.uprobes) {
> >+	if (params.show_lines) {
> >  		if (params.mod_events) {
> >  			pr_err("  Error: Don't use --line with"
> >  			       " --add/--del.\n");
> 
> Unrelated change.
> 
> 
> >diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> >index 05be5de..2f82267 100644
> >--- a/tools/perf/util/probe-event.c
> >+++ b/tools/perf/util/probe-event.c
> >@@ -172,6 +172,52 @@ const char *kernel_get_module_path(const char *module)
> >  	return (dso) ? dso->long_name : NULL;
> >  }
> >
> >+/* Copied from unwind.c */
> >+static Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
> >+				    GElf_Shdr *shp, const char *name)
> >+{
> >+	Elf_Scn *sec = NULL;
> >+
> >+	while ((sec = elf_nextscn(elf, sec)) != NULL) {
> >+		char *str;
> >+
> >+		gelf_getshdr(sec, shp);
> >+		str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
> >+		if (!strcmp(name, str))
> >+			break;
> >+	}
> >+
> >+	return sec;
> >+}
> 
> Why copy it? With unwind.c and util/symbol-elf.c we now have 2
> copies. How about exporting one of those?
> 
> >+
> >+static int get_text_start_address(const char *exec, unsigned long *address)
> >+{
> >+	Elf *elf;
> >+	GElf_Ehdr ehdr;
> >+	GElf_Shdr shdr;
> >+	int fd, ret = -ENOENT;
> >+
> >+	fd = open(exec, O_RDONLY);
> >+	if (fd < 0)
> >+		return -errno;
> >+
> >+	elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
> >+	if (elf == NULL)
> >+		return -EINVAL;
> >+
> >+	if (gelf_getehdr(elf, &ehdr) == NULL)
> >+		goto out;
> >+
> >+	if (!elf_section_by_name(elf, &ehdr, &shdr, ".text"))
> >+		goto out;
> >+
> >+	*address = shdr.sh_addr - shdr.sh_offset;
> >+	ret = 0;
> >+out:
> >+	elf_end(elf);
> >+	return ret;
> >+}
> >+
> >  static int init_user_exec(void)
> >  {
> >  	int ret = 0;
> >@@ -186,6 +232,37 @@ static int init_user_exec(void)
> >  	return ret;
> >  }
> >
> >+static int convert_exec_to_group(const char *exec, char **result)
> >+{
> >+	char *ptr1, *ptr2, *exec_copy;
> >+	char buf[64];
> >+	int ret;
> >+
> >+	exec_copy = strdup(exec);
> >+	if (!exec_copy)
> >+		return -ENOMEM;
> >+
> >+	ptr1 = basename(exec_copy);
> >+	if (!ptr1) {
> >+		ret = -EINVAL;
> >+		goto out;
> >+	}
> >+
> >+	ptr2 = strpbrk(ptr1, "-._");
> >+	if (ptr2)
> >+		*ptr2 = '\0';
> >+	ret = e_snprintf(buf, 64, "%s_%s", PERFPROBE_GROUP, ptr1);
> >+	if (ret < 0)
> >+		goto out;
> >+
> >+	*result = strdup(buf);
> >+	ret = *result ? 0 : -ENOMEM;
> >+
> >+out:
> >+	free(exec_copy);
> >+	return ret;
> >+}
> >+
> >  static int convert_to_perf_probe_point(struct probe_trace_point *tp,
> >  					struct perf_probe_point *pp)
> >  {
> >@@ -261,6 +338,40 @@ static int kprobe_convert_to_perf_probe(struct probe_trace_point *tp,
> >  	return 0;
> >  }
> >
> >+static int add_exec_to_probe_trace_events(struct probe_trace_event *tevs,
> >+					  int ntevs, const char *exec)
> >+{
> >+	int i, ret = 0;
> >+	unsigned long offset, stext = 0;
> >+	char buf[32];
> >+
> >+	if (!exec)
> >+		return 0;
> >+
> >+	ret = get_text_start_address(exec, &stext);
> >+	if (ret < 0)
> >+		return ret;
> >+
> >+	for (i = 0; i < ntevs && ret >= 0; i++) {
> >+		offset = tevs[i].point.address - stext;
> >+		offset += tevs[i].point.offset;
> >+		tevs[i].point.offset = 0;
> >+		free(tevs[i].point.symbol);
> >+		ret = e_snprintf(buf, 32, "0x%lx", offset);
> >+		if (ret < 0)
> >+			break;
> >+		tevs[i].point.module = strdup(exec);
> >+		tevs[i].point.symbol = strdup(buf);
> >+		if (!tevs[i].point.symbol || !tevs[i].point.module) {
> >+			ret = -ENOMEM;
> >+			break;
> >+		}
> >+		tevs[i].uprobes = true;
> >+	}
> >+
> >+	return ret;
> >+}
> >+
> 
> More strdup's. This is library code and we need methods to free that
> memory as well.
> 
> David

Re: [PATCH -tip v2 1/2] [CLEANUP] perf-probe: Expand given path to absolute path

[Masami Hiramatsu]

(2013/12/26 23:22), Arnaldo Carvalho de Melo wrote:
> Em Thu, Dec 26, 2013 at 09:14:46AM -0500, David Ahern escreveu:
>> On 12/26/13, 12:41 AM, Masami Hiramatsu wrote:
>>> @@ -180,7 +181,20 @@ static int opt_set_target(const struct option *opt, const char *str,
>>>  		else
>>>  			return ret;
>>>
>>> -		params.target = str;
>>> +		/* Expand given path to absolute path, except for modulename */
>>> +		if (params.uprobes || strchr(str, '/')) {
>>> +			tmp = realpath(str, NULL);
>>> +			if (!tmp) {
>>> +				pr_warning("Failed to find the path of %s.\n",
>>> +					   str);
>>
>> That error message will be misleading if it is generated. How about:
>>     Failed to get the absolute path of %s: %d\n", str, errno.
> 
> Changed it to:
> 
>  pr_warning("Failed to get the absolute path of %s: %m\n", str);

Thanks, that looks good to me.

>>> +				return ret;
>>> +			}
>>> +		} else {
>>> +			tmp = strdup(str);
>>> +			if (!tmp)
>>> +				return -ENOMEM;
>>> +		}
>>> +		params.target = tmp;
>>
>> When is params.target freed?

Nowhere, since there is no terminal code for user
command interface.

Those memories are released when the program terminated.
I think it is just a waste of the time to free the memory
pieces which are not used(and allocated) repeatedly.
Or, is there any chance to call this part directly from
other command?


Thank you,

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

Re: Re: [PATCH -tip v2 2/2] perf-probe: Support basic dwarf-based operations on uprobe events

[Masami Hiramatsu]

(2013/12/26 23:38), David Ahern wrote:
> On 12/26/13, 12:41 AM, Masami Hiramatsu wrote:
>> And this shows the available variables at the given line of
>> the function.
>> ----
>> # ./perf probe -x perf --vars map__load:8
>> Available variables at map__load:8
>>          @<map__load+96>
>>                  char*   name
>>                  struct map*     map
>>                  symbol_filter_t filter
>>          @<map__find_symbol+112>
>>                  char*   name
>>                  symbol_filter_t filter
>>          @<map__find_symbol_by_name+136>
>>                  char*   name
>>                  symbol_filter_t filter
>>          @<map_groups__find_symbol_by_name+176>
>>                  char*   name
>>                  struct map*     map
>>                  symbol_filter_t filter
> 
> Still limitations. This is Fedora 18:
> 
> # rpm -qa | grep debug
> glibc-debuginfo-common-2.16-34.fc18.x86_64
> glibc-debuginfo-2.16-34.fc18.x86_64
> 
> # /tmp/perf/perf probe -V malloc -x /lib64/libc-2.16.so
> Failed to find variables at malloc (0)
> 
> So probing on system libraries does not benefit from this patch.

Yes, please see the todo list in 0/2

>  - Support distro style debuginfo path (/usr/lib/debug/...)

Not only the libraries but also all distro packaged binaries have
debuginfo as other binaries. We should locate it by using dso.
But since this is just the first step series, I'd like to start with
the "homebrew" debuginfo.


>> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
>> index 5681266..20b3e15 100644
>> --- a/tools/perf/builtin-probe.c
>> +++ b/tools/perf/builtin-probe.c
>> @@ -425,7 +425,7 @@ int cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
>>   	}
>>
>>   #ifdef HAVE_DWARF_SUPPORT
>> -	if (params.show_lines && !params.uprobes) {
>> +	if (params.show_lines) {
>>   		if (params.mod_events) {
>>   			pr_err("  Error: Don't use --line with"
>>   			       " --add/--del.\n");
> 
> Unrelated change.

No, this is required for the --line dwarf support.


>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 05be5de..2f82267 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -172,6 +172,52 @@ const char *kernel_get_module_path(const char *module)
>>   	return (dso) ? dso->long_name : NULL;
>>   }
>>
>> +/* Copied from unwind.c */
>> +static Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
>> +				    GElf_Shdr *shp, const char *name)
>> +{
>> +	Elf_Scn *sec = NULL;
>> +
>> +	while ((sec = elf_nextscn(elf, sec)) != NULL) {
>> +		char *str;
>> +
>> +		gelf_getshdr(sec, shp);
>> +		str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
>> +		if (!strcmp(name, str))
>> +			break;
>> +	}
>> +
>> +	return sec;
>> +}
> 
> Why copy it? With unwind.c and util/symbol-elf.c we now have 2 copies. 
> How about exporting one of those?

Hm, OK, I missed the copy in symbol-elf.c. I'd like to use that, since
the unwind.c can be disabled by NO_LIBUNWIND.

>> @@ -261,6 +338,40 @@ static int kprobe_convert_to_perf_probe(struct probe_trace_point *tp,
>>   	return 0;
>>   }
>>
>> +static int add_exec_to_probe_trace_events(struct probe_trace_event *tevs,
>> +					  int ntevs, const char *exec)
>> +{
>> +	int i, ret = 0;
>> +	unsigned long offset, stext = 0;
>> +	char buf[32];
>> +
>> +	if (!exec)
>> +		return 0;
>> +
>> +	ret = get_text_start_address(exec, &stext);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	for (i = 0; i < ntevs && ret >= 0; i++) {
>> +		offset = tevs[i].point.address - stext;
>> +		offset += tevs[i].point.offset;
>> +		tevs[i].point.offset = 0;
>> +		free(tevs[i].point.symbol);
>> +		ret = e_snprintf(buf, 32, "0x%lx", offset);
>> +		if (ret < 0)
>> +			break;
>> +		tevs[i].point.module = strdup(exec);
>> +		tevs[i].point.symbol = strdup(buf);
>> +		if (!tevs[i].point.symbol || !tevs[i].point.module) {
>> +			ret = -ENOMEM;
>> +			break;
>> +		}
>> +		tevs[i].uprobes = true;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
> 
> More strdup's. This is library code and we need methods to free that 
> memory as well.

Ah, yes. Still some error handling routine doesn't clean it at all.
And it should be a separated bugfix patch, since not only
add_exec_to_probe_trace_events but also add_module_to_probe_trace_events
does the similar strdups.

Thank you!

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

Re: [PATCH -tip v2 1/2] [CLEANUP] perf-probe: Expand given path to absolute path

[David Ahern]

On 12/27/13, 1:14 AM, Masami Hiramatsu wrote:
> Nowhere, since there is no terminal code for user
> command interface.
>
> Those memories are released when the program terminated.
> I think it is just a waste of the time to free the memory
> pieces which are not used(and allocated) repeatedly.
> Or, is there any chance to call this part directly from
> other command?

Most of the functionality has a destructor to clean up memory 
allocations. probe code needs to follow suit.

e.g, from builtin-record.c:

     err = __cmd_record(&record, argc, argv);

     perf_evlist__munmap(evsel_list);
     perf_evlist__close(evsel_list);
out_free_fd:
     perf_evlist__delete_maps(evsel_list);
out_symbol_exit:
     symbol__exit();

and __cmd_record ends by cleaning up the session struct.

David

Re: [PATCH -tip v2 1/2] [CLEANUP] perf-probe: Expand given path to absolute path

[Arnaldo Carvalho de Melo]

Em Fri, Dec 27, 2013 at 09:20:11AM -0500, David Ahern escreveu:
> On 12/27/13, 1:14 AM, Masami Hiramatsu wrote:
> >Nowhere, since there is no terminal code for user
> >command interface.

> >Those memories are released when the program terminated.
> >I think it is just a waste of the time to free the memory
> >pieces which are not used(and allocated) repeatedly.
> >Or, is there any chance to call this part directly from
> >other command?

There is always the chance of parts of a tool to be librarized, I think
the rule is: allocator -> lifetime -> destructor, explicit.

We may want to explicitely disable some big destructor call (or lots of
destructors, like symbols + hists, etc) because it may make exit time to
be overly long, but at least we'll know what destructors to call when
such code gets librarized.

- Arnaldo

> Most of the functionality has a destructor to clean up memory
> allocations. probe code needs to follow suit.
> 
> e.g, from builtin-record.c:
> 
>     err = __cmd_record(&record, argc, argv);
> 
>     perf_evlist__munmap(evsel_list);
>     perf_evlist__close(evsel_list);
> out_free_fd:
>     perf_evlist__delete_maps(evsel_list);
> out_symbol_exit:
>     symbol__exit();
> 
> and __cmd_record ends by cleaning up the session struct.
> 
> David

Re: [PATCH -tip v2 1/2] [CLEANUP] perf-probe: Expand given path to absolute path

[Arnaldo Carvalho de Melo]

Em Fri, Dec 27, 2013 at 02:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Dec 27, 2013 at 09:20:11AM -0500, David Ahern escreveu:
> > On 12/27/13, 1:14 AM, Masami Hiramatsu wrote:
> > >Nowhere, since there is no terminal code for user
> > >command interface.
> 
> > >Those memories are released when the program terminated.
> > >I think it is just a waste of the time to free the memory
> > >pieces which are not used(and allocated) repeatedly.
> > >Or, is there any chance to call this part directly from
> > >other command?
> 
> There is always the chance of parts of a tool to be librarized, I think
> the rule is: allocator -> lifetime -> destructor, explicit.
> 
> We may want to explicitely disable some big destructor call (or lots of
> destructors, like symbols + hists, etc) because it may make exit time to
> be overly long, but at least we'll know what destructors to call when
> such code gets librarized.

Having said that, I applied the current patch, as it doesn't makes
things much worser than they already are, i.e. we need to audit that
code further, and these patches provided an useful new functionality.

- Arnaldo