This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: Re: [PATCH -tip v2 2/2] perf-probe: Support basic dwarf-based operations on uprobe events
- From: Masami Hiramatsu <masami dot hiramatsu dot pt at hitachi dot com>
- To: David Ahern <dsahern at gmail dot com>
- Cc: Ingo Molnar <mingo at kernel dot org>, Arnaldo Carvalho de Melo <acme at ghostprotocols dot net>, Srikar Dronamraju <srikar at linux dot vnet dot ibm dot com>, lkml <linux-kernel at vger dot kernel dot org>, "Steven Rostedt (Red Hat)" <rostedt at goodmis dot org>, Oleg Nesterov <oleg at redhat dot com>, "David A. Long" <dave dot long at linaro dot org>, systemtap at sourceware dot org, yrl dot pp-manager dot tt at hitachi dot com, Namhyung Kim <namhyung at kernel dot org>
- Date: Fri, 27 Dec 2013 15:52:30 +0900
- Subject: Re: Re: [PATCH -tip v2 2/2] perf-probe: Support basic dwarf-based operations on uprobe events
- Authentication-results: sourceware.org; auth=none
- References: <20131226054148 dot 22364 dot 25224 dot stgit at kbuild-fedora dot novalocal> <20131226054152 dot 22364 dot 47021 dot stgit at kbuild-fedora dot novalocal> <52BC3F4A dot 1030203 at gmail dot com>
(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