Re: Re: [PATCH v2] perf probe: Find compilation directory path for lazy matching

[Naohiro Aota <naota@elisp.net>]

On Thu, Mar 12, 2015 at 10:42 AM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> (2015/03/11 22:30), Arnaldo Carvalho de Melo wrote:
>> Em Wed, Mar 04, 2015 at 09:34:38PM +0900, Masami Hiramatsu escreveu:
>>> (2015/03/04 16:52), Naohiro Aota wrote:
>>>> If we use lazy matching, it failed to open a souce file if perf command
>>>> is invoked outside of compilation directory:
>>>>
>>>> $ perf probe -a '__schedule;clear_*'
>>>> Failed to open kernel/sched/core.c: No such file or directory
>>>>   Error: Failed to add events. (-2)
>>>>
>>>> OTOH, other commands like "probe -L" can solve the souce directory by
>>>> themselves. Let's make it possible for lazy matching too!
>>>>
>>>
>>> Looks good to me :)
>>>
>>> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>
>> This doesn't make sense... se below:
>>
>>> Thank you!
>>>
>>>> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
>>>> index 46f009a..e6c0262 100644
>>>> --- a/tools/perf/util/probe-finder.c
>>>> +++ b/tools/perf/util/probe-finder.c
>>>> @@ -849,11 +849,22 @@ static int probe_point_lazy_walker(const char *fname, int lineno,
>>>>  static int find_probe_point_lazy(Dwarf_Die *sp_die, struct probe_finder *pf)
>>>>  {
>>>>     int ret = 0;
>>>> +   char *fpath;
>>>>
>>>>     if (intlist__empty(pf->lcache)) {
>>>> +           const char *comp_dir;
>>>> +
>>>> +           comp_dir = cu_get_comp_dir(&pf->cu_die);
>>>> +           ret = get_real_path(pf->fname, comp_dir, &fpath);
>>>> +           if (ret < 0) {
>>>> +                   pr_warning("Failed to find source file path.\n");
>>>> +                   return ret;
>>>> +           }
>>>> +
>>>>             /* Matching lazy line pattern */
>>>> -           ret = find_lazy_match_lines(pf->lcache, pf->fname,
>>>> +           ret = find_lazy_match_lines(pf->lcache, fpath,
>>>>                                         pf->pev->point.lazy_line);
>>>> +           free(fpath);
>>>>             if (ret <= 0)
>>>>                     return ret;
>>>>     }
>>>> @@ -1616,3 +1627,61 @@ found:
>>>>     return (ret < 0) ? ret : lf.found;
>>>>  }
>>>>
>>>> +/*
>>>> + * Find a src file from a DWARF tag path. Prepend optional source path prefix
>>>> + * and chop off leading directories that do not exist. Result is passed back as
>>>> + * a newly allocated path on success.
>>>> + * Return 0 if file was found and readable, -errno otherwise.
>>>> + */
>>>> +static int get_real_path(const char *raw_path, const char *comp_dir,
>>>> +                    char **new_path)
>>
>> The function is marked "static"
>>
>>>> +{
>>>> +   const char *prefix = symbol_conf.source_prefix;
>>>> +
>>>> +   if (!prefix) {
>>>> +           if (raw_path[0] != '/' && comp_dir)
>>>> +                   /* If not an absolute path, try to use comp_dir */
>>>> +                   prefix = comp_dir;
>>>> +           else {
>>>> +                   if (access(raw_path, R_OK) == 0) {
>>>> +                           *new_path = strdup(raw_path);
>>>> +                           return *new_path ? 0 : -ENOMEM;
>>>> +                   } else
>>>> +                           return -errno;
>>>> +           }
>>>> +   }
>>>> +
>>>> +   *new_path = malloc((strlen(prefix) + strlen(raw_path) + 2));
>>>> +   if (!*new_path)
>>>> +           return -ENOMEM;
>>>> +
>>>> +   for (;;) {
>>>> +           sprintf(*new_path, "%s/%s", prefix, raw_path);
>>>> +
>>>> +           if (access(*new_path, R_OK) == 0)
>>>> +                   return 0;
>>>> +
>>>> +           if (!symbol_conf.source_prefix) {
>>>> +                   /* In case of searching comp_dir, don't retry */
>>>> +                   zfree(new_path);
>>>> +                   return -errno;
>>>> +           }
>>>> +
>>>> +           switch (errno) {
>>>> +           case ENAMETOOLONG:
>>>> +           case ENOENT:
>>>> +           case EROFS:
>>>> +           case EFAULT:
>>>> +                   raw_path = strchr(++raw_path, '/');
>>>> +                   if (!raw_path) {
>>>> +                           zfree(new_path);
>>>> +                           return -ENOENT;
>>>> +                   }
>>>> +                   continue;
>>>> +
>>>> +           default:
>>>> +                   zfree(new_path);
>>>> +                   return -errno;
>>>> +           }
>>>> +   }
>>>> +}
>>>> diff --git a/tools/perf/util/probe-finder.h b/tools/perf/util/probe-finder.h
>>>> index 92590b2..5ef82dd 100644
>>>> --- a/tools/perf/util/probe-finder.h
>>>> +++ b/tools/perf/util/probe-finder.h
>>>> @@ -55,6 +55,10 @@ extern int debuginfo__find_available_vars_at(struct debuginfo *dbg,
>>>>                                          struct variable_list **vls,
>>>>                                          int max_points, bool externs);
>>>>
>>>> +/* Find a src file from a DWARF tag path */
>>>> +extern int get_real_path(const char *raw_path, const char *comp_dir,
>>>> +                    char **new_path);
>>>> +
>>
>> And then you mark it "extern"? Have you tried to compile this? I tried:
>>
>>   CC       /tmp/build/perf/tests/task-exit.o
>> util/probe-finder.c:1636:12: error: static declaration of 'get_real_path' follows non-static declaration
>>  static int get_real_path(const char *raw_path, const char *comp_dir,
>>             ^
>> In file included from util/probe-finder.c:41:0:
>> util/probe-finder.h:59:12: note: previous declaration of 'get_real_path' was here
>>  extern int get_real_path(const char *raw_path, const char *comp_dir,
>>             ^
>> util/probe-finder.c:1636:12: error: 'get_real_path' defined but not used [-Werror=unused-function]
>>  static int get_real_path(const char *raw_path, const char *comp_dir,
>>             ^
>> cc1: all warnings being treated as errors
>> make[3]: *** [/tmp/build/perf/util/probe-finder.o] Error 1
>> make[3]: *** Waiting for unfinished jobs....
>>   CC       /tmp/build/perf/tests/sw-clock.o
>>
>> Also please refrain from using 'extern' in front of function prototypes, its just noise.1
>
> Ooops! Sorry, I missed this ...
>
> Naohiro, could you fix this?
>
> Thanks,

Ooops, I must have messed the things up during rebasing. I'll soon send fix.

>
>>
>> - Arnaldo
>>
>>>>  struct probe_finder {
>>>>     struct perf_probe_event *pev;           /* Target probe event */
>>>>
>>>>
>>>
>>>
>>> --
>>> Masami HIRAMATSU
>>> Software Platform Research Dept. Linux Technology Research Center
>>> Hitachi, Ltd., Yokohama Research Laboratory
>>> E-mail: masami.hiramatsu.pt@hitachi.com
>>>
>> --
>> 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
>
>